aoe-sancheck probes interfaces named eth* only

[cbiedl]

Debian bug report 986671 https://bugs.debian.org/986671:

The ethlist function in aoe-sancheck.c skips all network interfaces that do not start with "eth". This is a problem these days where people switch to different naming schemes like the so-called predictable interface names as mandated by udev/systemd.

The suggested fix was to probe for "e*" only; not too bad - bad perhaps it was wiser to refine that check by probing interface flags. After reading netdevice(7), I'd probe !IFF_NOARP, assuming ARP support means the same as having ethernet support. Alternativetly, try to align to the kernel behaviour which AFAICS happily probes everything, even IP-tunneling.

And a releated feature request: It was nice to aoe-sancheck could optionally indeed probe all interfaces, or a list of interfaces given on the command line regardless their name.

[ecashin]

Lately I've been appreciating command-line interfaces that are simple and explicit. Would you be willing to contribute a patch that causes the tool to probe user-identified interfaces?

…

On Fri, Apr 9, 2021, 12:17 PM cbiedl ***@***.***> wrote: Debian bug report 986671 https://bugs.debian.org/986671: The ethlist function in aoe-sancheck.c skips all network interfaces that do not start with "eth". This is a problem these days where people switch to different naming schemes like the so-called predictable interface names as mandated by udev/systemd. The suggested fix was to probe for "e*" only; not too bad - bad perhaps it was wiser to refine that check by probing interface flags. After reading netdevice(7), I'd probe !IFF_NOARP, assuming ARP support means the same as having ethernet support. Alternativetly, try to align to the kernel behaviour which AFAICS happily probes everything, even IP-tunneling. And a releated feature request: It was nice to aoe-sancheck could optionally indeed probe *all* interfaces, or a list of interfaces given on the command line regardless their name. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4FBPARV6FCATE6QZPLKLTH4SAZANCNFSM42VIRANA> .

[cbiedl]

This turned out to be somewhat bigger ...

The current implementation happily assumes there are no more than 16 interfaces with indexes from 0 to 15. That is far from reality. Luckily, the if_nameindex function provides the list of interfaces. Here's a patch that shouild fix both issues.

You might have seen the patch probposed in the Debian bug report; it also skips interfaces that are down, something that is probed later so I dropped that code.

A last observation: The "device summary" section is always empty. Possibly is something missing here.

--- a/aoe-sancheck.c
+++ b/aoe-sancheck.c
@@ -501,23 +501,38 @@ ethopen(Eth *e)     // get us a raw connection to an interface
 int
 ethlist(char **ifs, int nifs)
 {
-       int i, s, n;
+       int s, n;
        struct ifreq ifr;
+       struct if_nameindex *if_ni, *i;
 
        s = socket(AF_INET, SOCK_STREAM, 0);
        if (s < 0)
                return 0;
+
+       if_ni = if_nameindex();
+       if (if_ni == NULL)
+               return 0;
+
        n = 0;
-       for (i=0; i<nifs; i++) {
+       for (i = if_ni; ! (i->if_index == 0 && i->if_name == NULL); i++) {
                memset(&ifr, 0, sizeof ifr);
-               ifr.ifr_ifindex = i;
-               if (ioctl(s, SIOCGIFNAME, &ifr) < 0)
+               ifr.ifr_ifindex = i->if_index;
+               strcpy(ifr.ifr_name, i->if_name);
+               // get interface flags
+               if (ioctl(s, SIOCGIFFLAGS, &ifr) < 0)
                        continue;
-               if (strncmp(ifr.ifr_name, "eth", 3))
+               // only use interfaces that use arp protocol
+               if (ifr.ifr_flags & IFF_NOARP)
                        continue;
+               // skip loopback interfaces
+               if (ifr.ifr_flags & IFF_LOOPBACK)
+                       continue;
+               if (n == nifs)
+                       break;
                inserteth(ifs, nifs, ifr.ifr_name);
                n++;
        }
+       if_freenameindex(if_ni);
        close(s);
        return n;
 }

[cbiedl]

Amendment: For the time being I'd double the value of Neth to 32. In the long run, there should be no limit, allocate the memory based on the if_nameindex result instead.

[ecashin]

Thanks for following up on the details. Do you think you could create a pull request with some changes?

I did have a question, though: ARP is for getting the MAC addresses for IP addresses, so it has nothing to do with ATA over Ethernet in any real, essential sense. If I was making an ad hoc ethernet interface just for AoE, I'd be tempted to discourage ARP, just because mixing IP and AoE is often a sign of a poor network design. That's one reason I was attracted to the idea of probing a user-supplied list of interfaces.

[cbiedl]

You refer to probing for IFF_NOARP? The idea was to exempt interfaces that do transport IP instead of Ethernet, for example ppp and tun. The assumption was presence of ARP support indicates and ethernet layer, and possibly someone will prove me wrong for some corner cases. But it does the right thing for plain ethernet, tap, and gretap.

On the other hand, the kernel aoe module seems to probe any interface unless you restrict via aoe_iflist. Possibly it's better to reach feature parity - but I'd prefer to patch the kernel in that regard. Some day.

[ecashin]

Yes, that was one of the questions I asked. Thanks much.

…

On Tue, May 4, 2021 at 3:59 AM cbiedl ***@***.***> wrote: You refer to probing for IFF_NOARP? The idea was to exempt interfaces that do transport IP instead of Ethernet, for example ppp and tun. The assumption was presence of ARP support indicates and ethernet layer, and possibly someone will prove me wrong for some corner cases. But it does the right thing for plain ethernet, tap, and gretap. On the other hand, the kernel aoe module seems to probe *any* interface unless you restrict via aoe_iflist. Possibly it's better to reach feature parity - but I'd prefer to patch the kernel in that regard. Some day. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4FBJOH6I26MJGKTMOBR3TL6SO3ANCNFSM42VIRANA> .

[cbiedl]

Again, this took some time ...

I've checked with various tunneling implementations, so far I think using the (inverted) IFF_NOARP bit is a sound indicator for usable ethernet support. Fun part: A gre tunnel interface will transport the AoE discovery but setting up a running connection fails.

A pull request will follow shortly. Unreleated, #5 is not forgotten, will try to get that handled soon.