Re: [PATCH net v1] tap: fix incorrect variable used for USO check in set_offload()

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
tap: fix incorrect variable used for USO check in set_offload()

This changes the RX_OFFLOADS decision in set_offload() so that the
TUN_F_USO4/TUN_F_USO6 test reads the ioctl argument instead of
feature_mask, since feature_mask only ever holds NETIF_F_* bits.

> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index fae115915c8ef..ce25ebeeccf0e 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -895,7 +895,7 @@ static int set_offload(struct tap_queue *q, unsigned long arg)
>  	 * user-space will not receive TSO frames.
>  	 */
>  	if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6) ||
> -	    (feature_mask & (TUN_F_USO4 | TUN_F_USO6)) == (TUN_F_USO4 | TUN_F_USO6))
> +	    (arg & (TUN_F_USO4 | TUN_F_USO6)) == (TUN_F_USO4 | TUN_F_USO6))
>  		features |= RX_OFFLOADS;
>  	else
>  		features &= ~RX_OFFLOADS;

[Severity: High]
This isn't a bug introduced by this patch, but there looks to be a second
wrong-variable defect from the same commit named in the Fixes: tag, a few
lines above in the same function:

drivers/net/tap.c:set_offload() {
	if (arg & TUN_F_CSUM) {
		feature_mask = NETIF_F_HW_CSUM;
		...
			if (arg & TUN_F_TSO4)
				feature_mask |= NETIF_F_TSO;
			if (arg & TUN_F_TSO6)
				feature_mask |= NETIF_F_TSO6;
		...
		/* TODO: for now USO4 and USO6 should work simultaneously */
		if ((arg & (TUN_F_USO4 | TUN_F_USO6)) == (TUN_F_USO4 | TUN_F_USO6))
			features |= NETIF_F_GSO_UDP_L4;
	}
	...
	tap->tap_features = feature_mask;
	if (tap->update_features)
		tap->update_features(tap, features);
}

Should NETIF_F_GSO_UDP_L4 be set in feature_mask rather than features here?
All the TSO bits go into feature_mask, and feature_mask is what becomes
tap->tap_features, i.e. the forward feature mask consulted in
tap_handle_frame():

	if (q->flags & IFF_VNET_HDR)
		features |= tap->tap_features;
	if (netif_needs_gso(skb, features)) {
		struct sk_buff *segs = __skb_gso_segment(skb, features, false);

With NETIF_F_GSO_UDP_L4 missing from tap->tap_features, net_gso_ok() fails
for SKB_GSO_UDP_L4 and every USO super-frame is split by
__skb_gso_segment() before it reaches userspace.

The write into the local features also looks like it cannot have a device
level effect, because macvlan_fix_features() filters vlan->set_features
through MACVLAN_FEATURES (which does not contain NETIF_F_GSO_UDP_L4) and
then does features |= ALWAYS_ON_FEATURES, where NETIF_F_GSO_SOFTWARE
already contains NETIF_F_GSO_UDP_L4.

For comparison, tun.c:set_offload() records NETIF_F_GSO_UDP_L4 in the mask
that becomes tun->set_features:

		if (arg & TUN_F_USO4 && arg & TUN_F_USO6) {
			features |= NETIF_F_GSO_UDP_L4;

Also, macvtap's TUN_OFFLOADS, used to initialise tap_features in
macvtap_newlink(), contains no NETIF_F_GSO_UDP_L4 either.

Would it make sense to fix both occurrences in one patch, so that USO on
tap/macvtap actually takes effect?  As it stands, this patch turns
GRO/LRO on for the USO case while the aggregated skbs are still
re-segmented, and a backporter reading the changelog would likely conclude
USO on tap works afterwards.

[Severity: Medium]
Is the new arg based term meant to be evaluated outside the
if (arg & TUN_F_CSUM) block?  The sibling USO test that sets
NETIF_F_GSO_UDP_L4 sits inside that block, so the two tests now disagree
on their precondition.

tap_ioctl()'s TUNSETOFFLOAD handler only rejects unknown bits:

		if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
			    TUN_F_TSO_ECN | TUN_F_UFO |
			    TUN_F_USO4 | TUN_F_USO6))
			return -EINVAL;

so arg == (TUN_F_USO4 | TUN_F_USO6) with TUN_F_CSUM clear is reachable from
userspace holding the tap fd.  In that case the whole
if (arg & TUN_F_CSUM) block is skipped, feature_mask stays 0, no offload is
negotiated, yet the new term is true and features |= RX_OFFLOADS runs.

This looks like a new state transition: the old expression
feature_mask & (TUN_F_USO4 | TUN_F_USO6) was always false, since
TUN_F_USO4/TUN_F_USO6 (0x20/0x40) map onto NETIF_F_HIGHDMA_BIT and
NETIF_F_FRAGLIST_BIT, which feature_mask never contains, so this arg value
previously took the features &= ~RX_OFFLOADS branch.

RX_OFFLOADS is (NETIF_F_GRO | NETIF_F_LRO), and both bits are inside
MACVLAN_FEATURES, so the value reaching macvtap_update_features() ->
vlan->set_features -> netdev_update_features() sticks and shows up in
ethtool -k.  Since tap->tap_features is 0 in that path, tap_handle_frame()
then re-segments everything GRO coalesced.

Would nesting the USO test under arg & TUN_F_CSUM, as tun.c:set_offload()
does, or deriving it from feature_mask once the NETIF_F_GSO_UDP_L4
assignment above is corrected, be preferable here?