* Re: [PATCH net v1] tap: fix incorrect variable used for USO check in set_offload()
2026-08-05 7:21 [PATCH net v1] tap: fix incorrect variable used for USO check in set_offload() Rongguang Wei
2026-08-05 13:34 ` Willem de Bruijn
@ 2026-08-06 16:39 ` Jakub Kicinski
1 sibling, 0 replies; 3+ messages in thread
From: Jakub Kicinski @ 2026-08-06 16:39 UTC (permalink / raw)
To: clementwei90
Cc: Jakub Kicinski, netdev, willemdebruijn.kernel, jasowangio,
andrew+netdev, weirongguang
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?
^ permalink raw reply [flat|nested] 3+ messages in thread