tuntap: add SIOCGIFCAP and SIOCSIFCAP ioctls

[timo.voelker_fh-muenster.de]

Summary

Add SIOCGIFCAP ioctl-command for tun/tap character device to be used by bhyve for offloading in the future.
Add SIOCSIFCAP for symmetry.

[kevans]

Adding Mark as a solid intersection between 'has touched tun/tap' and bhyve, though my recollection is that nothing would prevent a concurrent TAPSVNETHDR so we probably want SIOCGIFCAP to also take the lock and provide a consistent view of if_capabilities/if_capenable?

I'm also unsure that SIOCSIFCAP is sufficient; do we need to take into account the possible loss (or addition) of TAP_VNET_HDR_CAPS and reject that? Ditto that question for the interface ioctl handler, apparently.

[markj]

In D51289#1179139, @kevans wrote:

Adding Mark as a solid intersection between 'has touched tun/tap' and bhyve, though my recollection is that nothing would prevent a concurrent TAPSVNETHDR so we probably want SIOCGIFCAP to also take the lock and provide a consistent view of if_capabilities/if_capenable?

Yes, I agree.

These ioctls should be documented in tun.4 as well.

I'm also unsure that SIOCSIFCAP is sufficient; do we need to take into account the possible loss (or addition) of TAP_VNET_HDR_CAPS and reject that? Ditto that question for the interface ioctl handler, apparently.

tun_caps_changed() and tunwrite_l2(), for example, seem to handle the possibility that some of those caps are disabled. I'm not sure why we'd reject a request to clear something from TAP_VNET_HDR_CAPS?

[kevans]

In D51289#1179151, @markj wrote:

In D51289#1179139, @kevans wrote:

[... snip ...]

I'm also unsure that SIOCSIFCAP is sufficient; do we need to take into account the possible loss (or addition) of TAP_VNET_HDR_CAPS and reject that? Ditto that question for the interface ioctl handler, apparently.

tun_caps_changed() and tunwrite_l2(), for example, seem to handle the possibility that some of those caps are disabled. I'm not sure why we'd reject a request to clear something from TAP_VNET_HDR_CAPS?

Sorry, this was both poorly thought out and poorly communicated. My general concern here is that we could put ourselves into a weird position if we start disabling capabilities that we'd expect if tun_vhdrlen != 0 and that it'd be better to force TAPSVNETHDR to disable it instead, but I'm not sure that there's anything technically wrong with that and it's perhaps better to just assume userland knows what they're doing if it's relatively harmless.

[kevans]

Some more thoughts:

• The interface ioctl path is gated behind PRIV_NET_SETIFCAP; should we do the same here? We're issued through the cdev via an fd that would indicate we have some sort of ownership of it, so I would imagine not.
• The interface ioctl path also kicks back EINVAL if one would try to enable a capability not in if_capabilities; should we do the same here?
• Do we need to set if_lastchange here?

[kevans]

This comment was removed by kevans.

[timo.voelker_fh-muenster.de]

Thanks for your review @kevans

we probably want SIOCGIFCAP to also take the lock and provide a consistent view of if_capabilities/if_capenable?

You mean a lock that protects lines 1660 and 1661?

My general concern here is that we could put ourselves into a weird position if we start disabling capabilities that we'd expect if tun_vhdrlen != 0 and that it'd be better to force TAPSVNETHDR to disable it instead

Currently, I don't need SIOCSIFCAP. I can just remove the code if that resolves concerns.

The interface ioctl path is gated behind PRIV_NET_SETIFCAP; should we do the same here?

I don't know what that is. I can't find PRIV_NET_SETIFCAP in tuntap.c.

The interface ioctl path also kicks back EINVAL if one would try to enable a capability not in if_capabilities; should we do the same here?

I can't find the code that kicks back EINVAL in tuntap.c. Is it somewhere outside?

Do we need to set if_lastchange here?

I don't know what that is.

[timo.voelker_fh-muenster.de]

Thanks for your review @markj

These ioctls should be documented in tun.4 as well.

Since the existing SIOCGIFCAP for the interface is not documented in tun.4, my assumption was that such an ioctl is generic and does not need documentation in a specific interface man mage. Is that not correct?

[kevans]

In D51289#1179499, @timo.voelker_fh-muenster.de wrote:

Thanks for your review @kevans

we probably want SIOCGIFCAP to also take the lock and provide a consistent view of if_capabilities/if_capenable?

You mean a lock that protects lines 1660 and 1661?

Correct

My general concern here is that we could put ourselves into a weird position if we start disabling capabilities that we'd expect if tun_vhdrlen != 0 and that it'd be better to force TAPSVNETHDR to disable it instead

Currently, I don't need SIOCSIFCAP. I can just remove the code if that resolves concerns.

I think I'm OK either way- you're preserving an existing problem at most, and it's not even a major problem.

The interface ioctl path is gated behind PRIV_NET_SETIFCAP; should we do the same here?

I don't know what that is. I can't find PRIV_NET_SETIFCAP in tuntap.c.

The interface ioctl path also kicks back EINVAL if one would try to enable a capability not in if_capabilities; should we do the same here?

I can't find the code that kicks back EINVAL in tuntap.c. Is it somewhere outside?

Both of these are provided by the path that gets us into an ifnet's if_ioctl -- see ifioctl in if.c, SIOCSIFCAP.

Do we need to set if_lastchange here?

I don't know what that is.

Time of last administrative change (also done by ifioctl noted above), I'm unsure if it really matters much.

[timo.voelker_fh-muenster.de]

My general concern here is that we could put ourselves into a weird position if we start disabling capabilities that we'd expect if tun_vhdrlen != 0 and that it'd be better to force TAPSVNETHDR to disable it instead

Currently, I don't need SIOCSIFCAP. I can just remove the code if that resolves concerns.

I think I'm OK either way- you're preserving an existing problem at most, and it's not even a major problem.

I would like address your concerns, but I'm not sure if I fully understand them. Here is my understanding.

tap interface has initially these capabilities:

IFCAP_LINKSTATE, IFCAP_MEXTPG, IFCAP_RXCSUM, IFCAP_RXCSUM_IPV6, IFCAP_LRO

and these are enabled by default:

IFCAP_LINKSTATE, IFCAP_MEXTPG

Setting a vhdrlen > 0 with TAPSVNETHDR adds these capabilities without enabling them:

IFCAP_RXCSUM, IFCAP_TXCSUM, IFCAP_RXCSUM_IPV6, IFCAP_TXCSUM_IPV6, IFCAP_VLAN_HWCSUM, IFCAP_TSO, IFCAP_LRO, IFCAP_VLAN_HWTSO

Now, with the new SIOCSIFCAP one can enable or disable these capabilities. Is this your concern?

I assume the code never expects one of these capabilities enabled, otherwise setting vhdrlen > 0 with TAPSVNETHDR would have enabled them directly. Right?

[timo.voelker_fh-muenster.de]

Addressed @kevans's comment regarding using a lock for SIOCGIFCAP.

[p.mousavizadeh_protonmail.com]

In D51289#1179499, @timo.voelker_fh-muenster.de wrote:

I don't know what that is. I can't find PRIV_NET_SETIFCAP in tuntap.c.

AFAIK, tunioctl is not called after ifhwioctl, however, I don't think it's necessary, since other cases of tunioctl are also lack privilege check.

I can't find the code that kicks back EINVAL in tuntap.c. Is it somewhere outside?

It's in sys/net/if.c at 2614:

if (ifr->ifr_reqcap & ~ifp->if_capabilities)
    return (EINVAL);

I think it should be there too.

Do we need to set if_lastchange here?

I don't know what that is.

It's the time of last administrative change which is set by default in ifhwioctl for tunifioctl.
I don't think it should be updated here in tunioctl. It's not clear whether changes made by tunioctl (as opposed to tunifioctl) are considered administrative.
Also, if we're going to update if_lastchange here, we must update the other ioctl cases as well.

[timo.voelker_fh-muenster.de]

For SIOCSIFCAP, check if requested capabilities are supported before enabling them.

[timo.voelker_fh-muenster.de]

In D51289#1194063, @p.mousavizadeh_protonmail.com wrote:

In D51289#1179499, @timo.voelker_fh-muenster.de wrote:

I don't know what that is. I can't find PRIV_NET_SETIFCAP in tuntap.c.

AFAIK, tunioctl is not called after ifhwioctl, however, I don't think it's necessary, since other cases of tunioctl are also lack privilege check.

Agree.

I can't find the code that kicks back EINVAL in tuntap.c. Is it somewhere outside?

It's in sys/net/if.c at 2614:

if (ifr->ifr_reqcap & ~ifp->if_capabilities)
    return (EINVAL);

I think it should be there too.

I made a small test. Without this check it is possible to enable capabilities that are not supported. I just added this if statement.

Do we need to set if_lastchange here?

I don't know what that is.

It's the time of last administrative change which is set by default in ifhwioctl for tunifioctl.
I don't think it should be updated here in tunioctl. It's not clear whether changes made by tunioctl (as opposed to tunifioctl) are considered administrative.
Also, if we're going to update if_lastchange here, we must update the other ioctl cases as well.

Agree.

Thanks for your explanations.

[p.mousavizadeh_protonmail.com]

LGTM
Thank you

[p.mousavizadeh_protonmail.com]

In D51289#1194522, @timo.voelker_fh-muenster.de wrote:

In D51289#1194063, @p.mousavizadeh_protonmail.com wrote:

In D51289#1179499, @timo.voelker_fh-muenster.de wrote:

Do we need to set if_lastchange here?

I don't know what that is.

It's the time of last administrative change which is set by default in ifhwioctl for tunifioctl.
I don't think it should be updated here in tunioctl. It's not clear whether changes made by tunioctl (as opposed to tunifioctl) are considered administrative.
Also, if we're going to update if_lastchange here, we must update the other ioctl cases as well.

Agree.

Thanks for your explanations.

@ae @melifaro
could you take a look and share your thoughts on the if_lastchange here?