Thank you!

Would you reupload with context (example cmds on https://wiki.freebsd.org/Phabricator), to make it easier to review?

Hrmph, from https://github.com/DOCGroup/ACE_TAO/blob/25f5307a46b10ebe7f26f46c006892f0e249c365/ACE/ace/Monitor_Control/BSD_Network_Interface_Monitor.cpp#L81 it looks like OpenBSD and NetBSD implemented this differently:

#if defined (__OpenBSD__)
          struct if_data * const ifi = &if_data;
#else
          struct if_data * const ifi = &ifdr.ifdr_data;
#endif

In D26538#590881, @emaste wrote:

it looks like OpenBSD and NetBSD implemented this differently:

Correct. NetBSD is the odd man out here - the old ioctl used to behave the same way, I don't understand the reasons for changing it - probably compat related.
Interestingly, all define the ioctl as _IOWR but if i understand @markj correctly only NetBSD needs it.

So AFAICT NetBSD populates ifdr->ifdr_data and is properly _IOWR, OpenBSD matches this patch (and is unnecessarily _IOWR), and DragonFlyBSD has added ifru_data to struct ifreq and matches the NetBSD approach other than not having a separate struct ifdatareq

Oh, no I'm mistaken - dfly https://github.com/DragonFlyBSD/DragonFlyBSD/blob/master/sys/net/if.c#L1986

	case SIOCGIFDATA:
		error = copyout((caddr_t)&ifp->if_data, ifr->ifr_data,
				sizeof(ifp->if_data));
		break;

In D26538#590896, @emaste wrote:

So AFAICT NetBSD populates ifdr->ifdr_data and is properly _IOWR, OpenBSD matches this patch (and is unnecessarily _IOWR), and DragonFlyBSD has added ifru_data to struct ifreq and matches the NetBSD approach other than not having a separate struct ifdatareq

Dragonfly matches the approach in this patch, from my reading.

In D26538#590895, @roy_marples.name wrote:

In D26538#590881, @emaste wrote:

it looks like OpenBSD and NetBSD implemented this differently:

Correct. NetBSD is the odd man out here - the old ioctl used to behave the same way, I don't understand the reasons for changing it - probably compat related.

NetBSD's approach seems a bit cleaner. Old applications will fail closed if someone modifies sizeof(struct if_data) and forgets to update the ioctl number. On the other hand, I think compatibility with the OpenBSD interface would be useful, so I'm inclined to agree with adopting the interface in this patch.

Interestingly, all define the ioctl as _IOWR but if i understand @markj correctly only NetBSD needs it.

I think that's right.

Hrm, @markj pointed out on IRC https://people.freebsd.org/~emaste/patches/SIOCGIFDATA.diff

One downside of this approach (vs NetBSD) is that we'll have trouble if we want to grow if_data in the future, although we could deal with that if/when it happens.

In D26538#590924, @emaste wrote:

Hrm, @markj pointed out on IRC https://people.freebsd.org/~emaste/patches/SIOCGIFDATA.diff

One downside of this approach (vs NetBSD) is that we'll have trouble if we want to grow if_data in the future, although we could deal with that if/when it happens.

I'm not beholden to either approach as I have to deal with both.
I based this implementation on what existed in FreeBSD and what's currently in DragonFly as it's a similar code base and it's also the path of least resistance.
On the other hand I also agree the NetBSD approach is cleaner and should handle any potential changes better.

At the end of the day it's just another #ifdef I have to handle.
As it stands right now, BSD portable appliactions have to deal with it anyway.
FreeBSD should choose what's best for FreeBSD and I can adopt to that.

Would you like the NetBSD approach in another differential so both could be reviewed and then one selected?

Would you like the NetBSD approach in another differential so both could be reviewed and then one selected?

I don't want you to do that work if we're going to end up with this approach anyway, and after some IRC discussion with @markj this patch does seem like the probable choice, as the simpler way to go.

I'll give other folks a couple of days to have a look, and will commit if there are no objections.

For reference if_data was last changed in 2014 in rS263102 by @glebius , making all fields MI-types and increasing the size of many fields.

In D26538#590877, @markj wrote:

Looks ok to me, thanks. This seems like it would be useful for programs that run under Capsicum.

Note that pledge does not allow SIOCGIFDATA - at all.
Because it's more than just link state and Capsicum doens't allow getifaddrs (which also returns if_data for AF_LINK) then I expect SIOCGIFDATA to fail on FreeBSD in capsicum as well.

I don't know how to do that, so I've not done this here. Hopefully @melifaro can :)

In D26538#590975, @roy_marples.name wrote:

In D26538#590877, @markj wrote:

Looks ok to me, thanks. This seems like it would be useful for programs that run under Capsicum.

Note that pledge does not allow SIOCGIFDATA - at all.

Do you know if this is a deliberate decision?

Because it's more than just link state and Capsicum doens't allow getifaddrs (which also returns if_data for AF_LINK) then I expect SIOCGIFDATA to fail on FreeBSD in capsicum as well.

The reason getifaddrs() doesn't work is that it reads a sysctl. Capsicum disallows access to global namespaces such as the sysctl tree. Individual sysctls can be annotated to permit access in capability mode using CTLFLAG_CAPRD and _CAPWR, but this is done sparingly. To use SIOCGIFDATA one only needs to create a socket and call ioctl(), both of which are allowed in capability mode, so I would expect it to work. cap_ioctls_limit(2) can be used to block ioctls on a given FD, but this doesn't help much since sandboxed code can simply create a new socket.

So, the interface provided by these ioctls doesn't interact nicely with Capsicum. Ideally I think you'd have some interface to "bind" a socket to a particular network interface, and disallow these SIOC(S|G)IF* ioctls on unbound sockets when in capability mode. Perhaps we should disallow SIOCGIFDATA in capability mode for now, though some of the information it returns is already accessible via other ioctls.

@jmg something like this makes sense, but please submit a new review for the change and cross-reference this one.

In D26538#632166, @jmg wrote:

I know this was closed and committed, but I want to get some more discussion on this.

I happen to be reviewing this for my own code (to get at _hwassist), and noticed that the code does not check how much space the program allocated for the if_data structure. This means that as the interface stands, it cannot be expanded, ever in the future, as it'll break old programs when they use this.

Why can't we change the ioctl number and keep the old one for compat?

I propose that before the if_data_copy call, the ifi_datalen value is saved, and that only the userland's count of bytes be copied out. This will emulate standard sysctl behavior, and allow a program to detect when there are additional bytes (or not enough bytes), and handle that properly...

How would they handle it if they don't know the new if_data layout?

yes, this would make the code incompatible w/ OpenBSD's version, but they also don't have the structure length embedded.

As this hasn't made it into a release yet, but 13 is coming up soon, we should make this change ASAP.

If this sounds good, I'll develop a patch and attach it that implements this behavior.

In D26538#632352, @markj wrote:

In D26538#632166, @jmg wrote:

I know this was closed and committed, but I want to get some more discussion on this.

I happen to be reviewing this for my own code (to get at _hwassist), and noticed that the code does not check how much space the program allocated for the if_data structure. This means that as the interface stands, it cannot be expanded, ever in the future, as it'll break old programs when they use this.

Why can't we change the ioctl number and keep the old one for compat?

Compat with what? There is no code in the FreeBSD source tree that uses it, and no releases have this ioctl. It hasn't even been merged to 12-stable yet.

I propose that before the if_data_copy call, the ifi_datalen value is saved, and that only the userland's count of bytes be copied out. This will emulate standard sysctl behavior, and allow a program to detect when there are additional bytes (or not enough bytes), and handle that properly...

How would they handle it if they don't know the new if_data layout?

If you only ever add members at the end, and don't reorder the members, then there will not be an issue, and the first part of the structure will always have the same data.

You can view this as a union:
union {

struct if_data_v1 id1;
struct if_data_v2 id2;

};

where v2 is larger than v1, but has ALL the same members at the beginin as v1. C99 explicitly allows this type of construction for unions.

Compat with what? There is no code in the FreeBSD source tree that uses it, and no releases have this ioctl. It hasn't even been merged to 12-stable yet.

Future backwards compat - i.e. introduce the new ioctl if and when we extend if_data.

(Regardless of what we do here we should have a compile-time assertion to make sure this doesn't silently, unexpectedly change.)

In D26538#632510, @emaste wrote:

Compat with what? There is no code in the FreeBSD source tree that uses it, and no releases have this ioctl. It hasn't even been merged to 12-stable yet.

Future backwards compat - i.e. introduce the new ioctl if and when we extend if_data.

If that method is adopted, then the structure name should be versioned, if_data_v1 or something like that, so it's clear what structure gets used w/ which ioctl, so that when the new version gets used, _v2 + SIOCGIFDATAV2.

I much prefer my proposed method, as it results in significantly less code. If we add a new ioctl and structure each time it changes, than each past structure code will have to be retained (under COMPAT_ defs). Tests [should] be added and maintained for each version of the structure, and more.

This ties back in w/ the recent discussion on -arch w/ ABI compatibility, and how to deal with it. It really does need to get addressed at some point.

(Regardless of what we do here we should have a compile-time assertion to make sure this doesn't silently, unexpectedly change.)

+1