Page MenuHomeFreeBSD

Add SIOCGIFDATA
ClosedPublic

Authored by roy_marples.name on Sep 23 2020, 8:31 PM.

Details

Summary

For interfaces that do not support SIOCGIFMEDIA (for which there are quite a few) the only fallback is to query the interface for if_data->ifi_link_state.
While it's possible to get at if_data for an interface via getifaddrs(3) or sysctl, both are heavy weight mechanisms.

SIOCGIFDATA is a simple ioctl to retrieve this fast with very little resource use in comparison.
This implementation mirrors that of other similar ioctls in FreeBSD.

Other BSD's support SIOCGIFDATA and I would like FreeBSD to as well.

Test Plan

Adjust ifconfig(8) to call SIOCGIFDATA if the call to SIOCGIFMEDIA fails.
If ifi_link_state != LINK_STATE_UNKNOWN then report the state.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Thank you!

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

Hopefully this is now enough context.

sys/net/if.c
2525 ↗(On Diff #77455)

I think we should memset(&ifd, 0, sizeof(ifd)) first, to ensure that any uninitialized padding bytes don't get copied out.

sys/sys/sockio.h
86 ↗(On Diff #77455)

Can't it be plain _IOW? This ioctl does not modify the request structure.

Ensure no uninitialised padding is leaked.

share/man/man9/ifnet.9
1337 ↗(On Diff #77457)

There should be a more detailed description of the ioctl here.

roy_marples.name added inline comments.
share/man/man9/ifnet.9
1337 ↗(On Diff #77457)

This area describes setting, not getting. This new ioctl only gets.
What more would you like me to say that's not already said above?

roy_marples.name marked an inline comment as not done.

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

share/man/man9/ifnet.9
1337 ↗(On Diff #77457)

Ah, sorry. Reading through the man page I guess it's clear enough that "data" means if_data.

This revision is now accepted and ready to land.Sep 23 2020, 9:34 PM

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

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;

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.

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.

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.

sys/sys/sockio.h
86 ↗(On Diff #77459)

.

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.

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 :)

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.

This revision was automatically updated to reflect the committed changes.