Bug 235920 - ifconfig: unable to create another interface after renaming the previous one with the same name
Summary: ifconfig: unable to create another interface after renaming the previous one ...
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: freebsd-net (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-02-21 17:18 UTC by Oleg Ginzburg
Modified: 2023-04-20 01:40 UTC (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Oleg Ginzburg 2019-02-21 17:18:07 UTC
Tested on: 12.0-RELEASE, 13-CURRENT (r344398)

When I try to create an interface (via ifconfig), then rename it (via ifconfig name) and create another one with the same name, this causes an error. E.g:

1) Create and rename interface: 
ifconfig tap100 create name xxx
  or:
ifconfig tap100 create && ifconfig tap100 name xxx


2) check that the interface original interface is missing now:
ifconfig tap100
ifconfig: interface tap100 does not exist


3) great! we can create another one:
ifconfig tap100 create
ifconfig: SIOCIFCREATE2: File exists

As far as I can see, the problem is in the UNR(9) area (alloc_unr_specific)

The path to the error that I see:

a)
src/sbin/ifconfig/ifconfig.c:
  setifname() -> 
     ...
             if (ioctl(s, SIOCSIFNAME, (caddr_t)&ifr) < 0) {
                free(newname);
                err(1, "ioctl SIOCSIFNAME (set name)");
        }
     ...


b)
src/sys/net/if.c:
  ifioctl() ->
      ...
       case SIOCIFCREATE:
        case SIOCIFCREATE2:
                error = priv_check(td, PRIV_NET_IFCREATE);
                if (error == 0)
                        error = if_clone_create(ifr->ifr_name,
                            sizeof(ifr->ifr_name), cmd == SIOCIFCREATE2 ?
                            ifr_data_get_ptr(ifr) : NULL);
                goto out_noref;
      ...

c)
src/sys/net/if_clone.c:
  if_clone_create() ->
      ..
        return (if_clone_createif(ifc, name, len, params));
      ..

d)
src/sys/net/if_clone.c:
  if_clone_createif() ->
     ..
       err = ifc_simple_create(ifc, name, len, params);
     ..


e)
src/sys/net/if_clone.c:
  ifc_simple_create() ->
     ..
      err = ifc_alloc_unit(ifc, &unit);
     ..

f)
src/sys/net/if_clone.c:
  ifc_alloc_unit() ->
    ..
      if (alloc_unr_specific(ifc->ifc_unrhdr, *unit) == -1) {
                return (EEXIST);
        }
    ..


If we create an interface for the first time, this function ends with:
IF_CLONE_ADDREF(ifc);

When we create an interface a second time, we get:
return (EEXIST);
Comment 1 Oleg Ginzburg 2019-02-21 17:22:21 UTC
We can test that the interface is actually renamed via ioctl() via:

#include <stdio.h>
#include <string.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <net/if.h>
#include <sys/ioctl.h>

-------
int list_devices() {

    char data[4096];
    struct ifconf ifc;
    struct ifreq *ifr;
    int sk,length;
    unsigned int idx;

    sk = socket(AF_INET, SOCK_DGRAM, 0);
    if(sk < 0)
    {
        perror("socket");
        return 0;
    }

    ifc.ifc_len = sizeof(data);
    ifc.ifc_buf = (caddr_t)data;
    if(ioctl(sk, SIOCGIFCONF, &ifc) < 0)
    {
        perror("ioctl(SIOCGIFCONF)");
        return 0;
    }

    ifr = (struct ifreq*)data;
    for(int i=0;i<ifc.ifc_len;)
    {
        length=IFNAMSIZ + ifr->ifr_addr.sa_len;
        idx=if_nametoindex(ifr->ifr_name);
        printf("Found: %s [index: %d]\n", ifr->ifr_name,idx);
        ifr=(struct ifr*)((char*)ifr+length);
        i+=length;
    }

    return 0;
}
----------

ifr->ifr_name is changing, but the index is the same (and this is normal?).
However, some information remains from the old interface, and this affects http://man.freebsd.org/alloc_unr_specific/9
Comment 2 Andriy Voskoboinyk freebsd_committer freebsd_triage 2019-02-21 22:12:33 UTC
That's OK - not sure about ifnet(9) in general, but for ieee80211(9) net.wlan.0.* tunables are created (at least) and their names will not be changed after interface renaming.
Comment 3 Poul-Henning Kamp freebsd_committer freebsd_triage 2019-02-22 08:28:20 UTC
I don't think this has anything to do with the UNR(9) code, it seems to be only a matter of how interface names are managed.

I have always assumed myself that naming an interface was an aliasing operation and that the interface retained its underlying "system-given" name - if nothing else in the sense that dev_printf() would still emit that name on the console.

Apart from those observations I have nothing further to add, and will take myself of the cc list.
Comment 4 Zhenlei Huang freebsd_committer freebsd_triage 2023-04-19 03:24:44 UTC
(In reply to Oleg Ginzburg from comment #0)
If `ifconfig create` with an unit number, then cloner will check whether the number is available or not ( via alloc_unr_specific() ).

```
static int
tun_clone_create(struct if_clone *ifc, char *name, size_t len,
    struct ifc_data *ifd, struct ifnet **ifpp)
{
        struct tuntap_driver *drv;
        struct cdev *dev;
        int err, i, tunflags, unit;

        tunflags = 0;
        /* The name here tells us exactly what we're creating */
        err = tuntap_name2info(name, &unit, &tunflags);
        if (err != 0)
                return (err);

        drv = tuntap_driver_from_flags(tunflags);
        if (drv == NULL)
                return (ENXIO);

        if (unit != -1) {
                /* If this unit number is still available that's okay. */
                if (alloc_unr_specific(drv->unrhdr, unit) == -1)
                        return (EEXIST);
        } else {
                unit = alloc_unr(drv->unrhdr);
        }

        snprintf(name, IFNAMSIZ, "%s%d", drv->cdevsw.d_name, unit);

        /* find any existing device, or allocate new unit number */
        dev = NULL;
        i = clone_create(&drv->clones, &drv->cdevsw, &unit, &dev, 0);
        /* No preexisting struct cdev *, create one */
        if (i != 0)
                i = tun_create_device(drv, unit, NULL, &dev, name);
        if (i == 0) {
                tuncreate(dev);
                struct tuntap_softc *tp = dev->si_drv1;
                *ifpp = tp->tun_ifp;
        }

        return (i);
}
```

When an interface is renamed, its unit number is not `freed` and thus lead this problem.

Other interfaces such as if_bridge are also affected.

```
# ifconfig bridge0 create name br0
# ifconfig bridge0 create
ifconfig: interface bridge0 already exists
```
Comment 5 Kyle Evans freebsd_committer freebsd_triage 2023-04-19 03:30:18 UTC
(In reply to Zhenlei Huang from comment #4)

> When an interface is renamed, its unit number is not `freed` and thus lead this problem.

Right, as soon as you start renaming the concept of a unit number can so easily fade away that it's not worth the complexity trying to manage it.

# ifconfig tun0 name sillytun

I suddenly have no unit number because there's not such a constraint on names, and imposing one would seem to be a bit weird. At some point, you converge on restricting renames to maintaining the common prefix so that, e.g., tun0 -> openvpn0 (bad example, but you can see where I'm going with this) wouldn't be be possible because you don't want to maintain the arbitrary "openvpn"  or whatever unr space as well.
Comment 6 Franco Fichtner 2023-04-19 06:51:40 UTC
Speaking for pf/OPNsense scope the management of this limitation is a non-issue since you have to manage manage the target device names as unique anyway so you never end up creating something with an intermediate name twice (or just omit the index and it's fine you will even get the device name back).

And tun/tap is a little tricky in its own regard here since the device nodes that may be required by the service are never being renamed (/dev/tunX etc.).
Comment 7 Zhenlei Huang freebsd_committer freebsd_triage 2023-04-20 01:40:51 UTC
(In reply to Franco Fichtner from comment #6)
> And tun/tap is a little tricky in its own regard here since the device nodes that
> may be required by the service are never being renamed (/dev/tunX etc.).

Yes, you are right.

For tap(4), even its unit number is freed the original controller device should still exist so that the applications (which have opened the /dev/tap) will not be disturbed.

I'm proposing UUID naming to fix that. Please see https://reviews.freebsd.org/D39689 .

For the design of tap(4), why not create `/dev/tap/some-uuid-name` and an alias `/dev/tap0 -> /dev/tap/some-uuid-name` ?