Introduce SIOCGIFDNAME to get the interface device name

[allanjude]

Summary

Returns the if_dname and if_dunit set by if_initname()
This allows tracking the original name of the interface after it is renamed

Connect it to ifconfig, only display it if it is different from ifname

[allanjude]

Use strncmp

[rpokala]

Okay from me, but let @kp and @kevans weigh in as well.

[kp]

sbin/ifconfig/ifconfig.c

1417

I'd reverse the logic here. If the allocation fails break out. That moves the bulk of the code one level of indentation down.

1423

Is there a point in doing repeated allocations here? We know the maximum length of an interface name.

sys/net/if.c

2714

break;

Then the 'else' isn't needed and the bulk of the code moves one level of indentation down.

[allanjude]

Remove the loop, the buffer wont be larger than IFNAMSIZ

[allanjude]

Reduce indentation

[kp]

sbin/ifconfig/ifconfig.c

1417

That looks wrong. If allocation fails we do the ioctl(), otherwise we warn about an allocation failure.

[allanjude]

sbin/ifconfig/ifconfig.c

1417

Good catch, while refactoring I had looked at trying to just quit early if allocation failed, to reduce another level of indent, but it didn't work out, and this didn't get switched back.

[allanjude]

Fix logic reversal caught by kp

[kp]

So this doesn't actually build for me:

...
--- all_subdir_rescue ---
/usr/src/sbin/ifconfig/ifconfig.c:1373:16: error: unused variable 'ifs' [-Werror,-Wunused-variable]
        struct ifstat ifs;
                      ^
/usr/src/sbin/ifconfig/ifconfig.c:1371:18: error: unused variable 'ift' [-Werror,-Wunused-variable]
        struct ifaddrs *ift;
                        ^
/usr/src/sbin/ifconfig/ifconfig.c:1438:2: error: expected identifier or '('
        if (ioctl(s, SIOCGIFCAP, (caddr_t)&ifr) == 0) {
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1449:2: error: type specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
        tunnel_status(s);
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1449:16: error: a parameter list without types is only allowed in a function definition
        tunnel_status(s);
                      ^
/usr/src/sbin/ifconfig/ifconfig.c:1451:2: error: expected identifier or '('
        for (ift = ifa; ift != NULL; ift = ift->ifa_next) {
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1481:2: error: expected identifier or '('
        if (allfamilies)
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1483:2: error: expected identifier or '('
        else if (afp->af_other_status != NULL)
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1486:10: error: unknown type name 'ifs'
        strlcpy(ifs.ifs_name, name, sizeof ifs.ifs_name);
                ^
/usr/src/sbin/ifconfig/ifconfig.c:1486:13: error: expected ')'
        strlcpy(ifs.ifs_name, name, sizeof ifs.ifs_name);
                   ^
/usr/src/sbin/ifconfig/ifconfig.c:1486:9: note: to match this '('
        strlcpy(ifs.ifs_name, name, sizeof ifs.ifs_name);
               ^
/usr/src/sbin/ifconfig/ifconfig.c:1486:2: error: type specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
        strlcpy(ifs.ifs_name, name, sizeof ifs.ifs_name);
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1487:2: error: expected identifier or '('
        if (ioctl(s, SIOCGIFSTATUS, &ifs) == 0)
        ^
...

It looks like ifconfig may be using the installed sockio.h header rather than the one from the source repo.

[allanjude]

In D28247#632082, @kp wrote:

So this doesn't actually build for me:

...
--- all_subdir_rescue ---
/usr/src/sbin/ifconfig/ifconfig.c:1373:16: error: unused variable 'ifs' [-Werror,-Wunused-variable]
        struct ifstat ifs;
                      ^
/usr/src/sbin/ifconfig/ifconfig.c:1371:18: error: unused variable 'ift' [-Werror,-Wunused-variable]
        struct ifaddrs *ift;
                        ^
/usr/src/sbin/ifconfig/ifconfig.c:1438:2: error: expected identifier or '('
        if (ioctl(s, SIOCGIFCAP, (caddr_t)&ifr) == 0) {
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1449:2: error: type specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
        tunnel_status(s);
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1449:16: error: a parameter list without types is only allowed in a function definition
        tunnel_status(s);
                      ^
/usr/src/sbin/ifconfig/ifconfig.c:1451:2: error: expected identifier or '('
        for (ift = ifa; ift != NULL; ift = ift->ifa_next) {
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1481:2: error: expected identifier or '('
        if (allfamilies)
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1483:2: error: expected identifier or '('
        else if (afp->af_other_status != NULL)
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1486:10: error: unknown type name 'ifs'
        strlcpy(ifs.ifs_name, name, sizeof ifs.ifs_name);
                ^
/usr/src/sbin/ifconfig/ifconfig.c:1486:13: error: expected ')'
        strlcpy(ifs.ifs_name, name, sizeof ifs.ifs_name);
                   ^
/usr/src/sbin/ifconfig/ifconfig.c:1486:9: note: to match this '('
        strlcpy(ifs.ifs_name, name, sizeof ifs.ifs_name);
               ^
/usr/src/sbin/ifconfig/ifconfig.c:1486:2: error: type specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
        strlcpy(ifs.ifs_name, name, sizeof ifs.ifs_name);
        ^
/usr/src/sbin/ifconfig/ifconfig.c:1487:2: error: expected identifier or '('
        if (ioctl(s, SIOCGIFSTATUS, &ifs) == 0)
        ^
...

It looks like ifconfig may be using the installed sockio.h header rather than the one from the source repo.

Was actually a missing { (or one too many }) on the final else in the new block in ifconfig.c

[allanjude]

Fix missing {

[rpokala]

LGTM

[kp]

It turns out there's an edge case we didn't consider:

epair0a: flags=8842<BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
        initname: epair0
        options=8<VLAN_MTU>
        ether 02:a5:17:a0:1e:0a
        groups: epair
        media: Ethernet 10Gbase-T (10Gbase-T <full-duplex>)
        status: active
        nd6 options=21<PERFORMNUD,AUTO_LINKLOCAL>

Also, this wants a test case or two.

[allanjude]

In D28247#632273, @kp wrote:

It turns out there's an edge case we didn't consider:

epair0a: flags=8842<BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
        initname: epair0
        options=8<VLAN_MTU>
        ether 02:a5:17:a0:1e:0a
        groups: epair
        media: Ethernet 10Gbase-T (10Gbase-T <full-duplex>)
        status: active
        nd6 options=21<PERFORMNUD,AUTO_LINKLOCAL>

Also, this wants a test case or two.

We could change the strncmp to use the length of 'name' instead, but what other corner cases might we run into with substrings? (vlan100 renamed to vlan1 or something)

[kp]

I don't think it's a string length issue. It's an issue of incorrect assumptions. This patch assumes that the original name of the interface is always the driver name ('if_dname') plus unit number. That's not the case for epair interfaces, and I wouldn't be surprised that this also fails for vlan interfaces (where the numeric suffix is not the unit number).

[allanjude]

In D28247#632482, @kp wrote:

I don't think it's a string length issue. It's an issue of incorrect assumptions. This patch assumes that the original name of the interface is always the driver name ('if_dname') plus unit number. That's not the case for epair interfaces, and I wouldn't be surprised that this also fails for vlan interfaces (where the numeric suffix is not the unit number).

It worked as expected when I tested creating 'bridge100' and 'vlan42'

even in the case of:
ifconfig vlan42 create vlandev igb0 vlan 42 name vlan45

but I am sure there is some driver that doesn't do this when it calls if_initname()

Right now, only epair seems to have the unexected behaviour, and the result is printing the extra line with the original interface name (I admit it isn't really the original interface name in the epair case).

Do you have a suggestion?

[allanjude]

In D28247#632482, @kp wrote:

I don't think it's a string length issue.

I didn't really mean string length, I meant limiting the strncmp to the shorter of the two strings, so as to avoid showing the extra line in the case where the original name is a substring of the current name, but I think that assumption is too broad.

[kp]

Oh. yeah, extending the ifconfig check to catch the epair case is indeed a bad idea.

The only way I can see to handle this is to allow if_epair the chance to construct the original name itself. Sadly we can't simply add SIOCGIFDNAME to epair_ioct(), that'd have been ideal. So that'd mean adding a new if_dname or whatever function pointer to struct ifnet just for this case.
Rather annoyingly the epair code doesn't even have a straightforward way of telling which one was the a interface and which one the b interface. I suppose it makes sense that it never had to care before. Shouldn't be too hard to fix either.

I think we really do want to handle epair correctly, because the primary use case I see for this is vnet related, and vnet jails very often are connected with epair interfaces.

[bz]

I have never understood why we never save the original in fields and be done with it; and then we could also check the kernel so you cannot create new ones with any original or currently used name/unit; but that's even trickier; some ifnet changes might make that easier but that's probably a longer-term project.

sbin/ifconfig/ifconfig.c

1413

That's unrelated white space; just commit that ;-) Usually we add the space to the end of line but I assume it's at width=80 already?

[kp]

In D28247#690380, @bz wrote:

I have never understood why we never save the original in fields and be done with it; and then we could also check the kernel so you cannot create new ones with any original or currently used name/unit; but that's even trickier; some ifnet changes might make that easier but that's probably a longer-term project.

There are some very "amusing" issues around interface naming already. Specifically there's confusion between ifnet name and ifgroup names, and the last time I looked at it the locking was either totally broken or I totally failed to understand it.
You're probably right about just storing the original name in ifnet for this, but if we want to do more than expose it to userspace the locking around it is going to hurt.

[melifaro]

I've created D39659 as an alternative wy to get this info from the kernel. If we still want this information in the ifconfig, I'll come up with the diff soon.

[zlei]

I do not think it is a good idea.

I'm going to fix https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=235920 and relax the if_dunit. So that

# ifconfig bridge0 create name br0
br0
# ifconfig bridge0 create
bridge0

will be possible.

If this change is applied then you will get the same original name of two different interfaces.

BTW, can IFDATA_DRIVERNAME in sys/net/if_mib.c serve the same purpose ?

case IFDATA_DRIVERNAME:
                /* 20 is enough for 64bit ints */
                dlen = strlen(ifp->if_dname) + 20 + 1;
                if ((dbuf = malloc(dlen, M_TEMP, M_NOWAIT)) == NULL) {
                        error = ENOMEM;
                        goto out;
                }
                if (ifp->if_dunit == IF_DUNIT_NONE)
                        strcpy(dbuf, ifp->if_dname);
                else
                        sprintf(dbuf, "%s%d", ifp->if_dname, ifp->if_dunit);

                error = SYSCTL_OUT(req, dbuf, strlen(dbuf) + 1);
                if (error == 0 && req->newptr != NULL)
                        error = EPERM;
                free(dbuf, M_TEMP);
                goto out;

[melifaro]

In D28247#903015, @zlei wrote:

I do not think it is a good idea.

Does it refer to the first statement (kernel part) or the second part (ifconfig part) ? :-)

I'm going to fix https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=235920 and relax the if_dunit. So that

# ifconfig bridge0 create name br0
br0
# ifconfig bridge0 create
bridge0

will be possible.

If this change is applied then you will get the same original name of two different interfaces.

BTW, can IFDATA_DRIVERNAME in sys/net/if_mib.c serve the same purpose ?

I'm actually going to deprecate this ( please see https://lists.freebsd.org/archives/dev-commits-src-main/2023-March/013718.html for more details).
Additionally, it provides wrong results (as noted by kp@ in D39659 for some interface cloners).

case IFDATA_DRIVERNAME:
                /* 20 is enough for 64bit ints */
                dlen = strlen(ifp->if_dname) + 20 + 1;
                if ((dbuf = malloc(dlen, M_TEMP, M_NOWAIT)) == NULL) {
                        error = ENOMEM;
                        goto out;
                }
                if (ifp->if_dunit == IF_DUNIT_NONE)
                        strcpy(dbuf, ifp->if_dname);
                else
                        sprintf(dbuf, "%s%d", ifp->if_dname, ifp->if_dunit);

                error = SYSCTL_OUT(req, dbuf, strlen(dbuf) + 1);
                if (error == 0 && req->newptr != NULL)
                        error = EPERM;
                free(dbuf, M_TEMP);
                goto out;

[zlei]

Sorry this is an excessive late response .

In D28247#903155, @melifaro wrote:

In D28247#903015, @zlei wrote:

I do not think it is a good idea.

Does it refer to the first statement (kernel part) or the second part (ifconfig part) ? :-)

I think I was referring to D39689 which stores original name of interface, and mainly the kernel part.
I do not familiar with NETLINK so no option on the second part.

I'm going to fix https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=235920 and relax the if_dunit. So that

# ifconfig bridge0 create name br0
br0
# ifconfig bridge0 create
bridge0

will be possible.

If this change is applied then you will get the same original name of two different interfaces.

So we will have same original name for two different interfaces.

BTW, can IFDATA_DRIVERNAME in sys/net/if_mib.c serve the same purpose ?

I'm actually going to deprecate this ( please see https://lists.freebsd.org/archives/dev-commits-src-main/2023-March/013718.html for more details).
Additionally, it provides wrong results (as noted by kp@ in D39659 for some interface cloners).

case IFDATA_DRIVERNAME:
                /* 20 is enough for 64bit ints */
                dlen = strlen(ifp->if_dname) + 20 + 1;
                if ((dbuf = malloc(dlen, M_TEMP, M_NOWAIT)) == NULL) {
                        error = ENOMEM;
                        goto out;
                }
                if (ifp->if_dunit == IF_DUNIT_NONE)
                        strcpy(dbuf, ifp->if_dname);
                else
                        sprintf(dbuf, "%s%d", ifp->if_dname, ifp->if_dunit);

                error = SYSCTL_OUT(req, dbuf, strlen(dbuf) + 1);
                if (error == 0 && req->newptr != NULL)
                        error = EPERM;
                free(dbuf, M_TEMP);
                goto out;

The above BTW is for this change, a new interface SIOCGIFDNAME . See also D42721 which try to utilize IFDATA_DRIVERNAME .