Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0 If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough). - [Medium] net: virtio: outer_l3min includes ETH_HLEN in virtio_net_hdr_tnl_to_skb() Pre-existing issues: - [Medium] This patch makes Ethernet framing an explicit argument (`has_eth`), but virtio_net_hdr_tnl_to_skb() in… -- net: virtio: do not use dev->type to find the outer L3 header virtio_net_l3_start() used skb->dev->type to decide whether a frame starts with an Ethernet header. tun can change that type with TUNSETLINK, so this patch adds a has_eth argument to virtio_net_hdr_tnl_to_skb() instead. Callers now pass true for virtio_net and tun_xdp_one(), and pass whether the device is IFF_TAP in tun_get_user(). Link: https://lore.kernel.org/netdev/[email protected]/ > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index dcec7311e6011..3920b3dff1e84 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1944,7 +1944,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, > struct tun_file *tfile, > break; > } > > - if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) { > + if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr, > + (tun->flags & TUN_TYPE_MASK) == IFF_TAP)) { > atomic_long_inc(&tun->rx_frame_errors); > err = -EINVAL; > goto free_skb; [ ... ] > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > index 357116c2a1489..b7b6b542f4763 100644 > --- a/include/linux/virtio_net.h > +++ b/include/linux/virtio_net.h [ ... ] > @@ -452,6 +450,7 @@ virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool > outer_isv6, > static inline int > virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, > const struct virtio_net_hdr_v1_hash_tunnel *vhdr, > + bool has_eth, > bool tnl_hdr_negotiated, > bool tnl_csum_negotiated, > bool little_endian) > @@ -505,7 +504,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, > if (ret) > return ret; > > - if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th)) > + if (!virtio_net_tnl_offsets_ok(skb, has_eth, outer_isv6, outer_th)) > return -EINVAL; [Severity: Medium] Now that the framing is passed in as has_eth, should the minimum outer transport offset check earlier in virtio_net_hdr_tnl_to_skb() depend on it as well? outer_l3min is still computed as ETH_HLEN + virtio_l3min(outer_isv6), so outer_th must be at least 34 for IPv4 or 54 for IPv6, whatever has_eth says. On an IFF_TUN device, tun_get_user() calls skb_reset_network_header() and passes has_eth=false. virtio_net_l3_start() then returns nhoff = 0, and virtio_net_l4_start() computes thoff = 20 for a plain IPv4 outer header, or 40 for a plain IPv6 one. virtio_net_tnl_offsets_ok() then requires outer_th == thoff, but the floor above has already rejected both values. As a result, the has_eth=false path only accepts frames whose outer L3 header is at least ETH_HLEN bytes longer than the minimum, such as IPv4 with options or IPv6 with extension headers. Would ordinary UDP tunnel GSO frames written to an IFF_TUN fd get -EINVAL and be counted in rx_frame_errors? The transmit side of the same ABI produces exactly those offsets: in TUN mode virtio_net_hdr_tnl_from_skb() fills outer_th_offset from skb->transport_header - skb_headroom(skb), which is 20 for an L3-first IPv4 frame. set_offload() also enables NETIF_F_GSO_UDP_TUNNEL without looking at IFF_TAP or IFF_TUN, so IFF_TUN devices reach this path. This isn't introduced by this patch, but since has_eth is now available here, would something like this work? outer_l3min = (has_eth ? ETH_HLEN : 0) + virtio_l3min(outer_isv6); [Severity: Medium] This isn't a bug introduced by this patch, but now that the framing is passed in as has_eth, should the minimum outer transport offset in virtio_net_hdr_tnl_to_skb() depend on it too? That check comes earlier in the same function and still assumes an Ethernet header: include/linux/virtio_net.h:virtio_net_hdr_tnl_to_skb() { ... outer_l3min = ETH_HLEN + virtio_l3min(outer_isv6); ... if (outer_th < outer_l3min || inner_nh < outer_th + sizeof(struct udphdr) || inner_th < inner_nh + inner_l3min) return -EINVAL; ... } On an IFF_TUN device, tun_get_user() resets the network header and passes has_eth=false. virtio_net_l3_start() then returns nhoff = 0, and virtio_net_tnl_offsets_ok() requires outer_th == thoff. For a plain outer header, thoff is 20 for IPv4 and 40 for IPv6. The floor above has already rejected both values, since it is 34 or 54. As a result, the has_eth=false path only accepts frames whose outer L3 header is at least ETH_HLEN bytes longer than the minimum, such as IPv4 with options or IPv6 with extension headers. Would ordinary UDP tunnel GSO frames written to an IFF_TUN fd get -EINVAL and be counted in rx_frame_errors? The transmit side of the same ABI seems to produce exactly those offsets. In TUN mode, tun_put_user() ends up in virtio_net_hdr_tnl_from_skb(), which writes: outer_th = skb->transport_header - skb_headroom(skb); vhdr->inner_nh_offset = cpu_to_le16(inner_nh); vhdr->outer_th_offset = cpu_to_le16(outer_th); For an L3-first frame, that gives 20 for IPv4. Also, set_offload() in tun.c enables NETIF_F_GSO_UDP_TUNNEL without checking IFF_TAP or IFF_TUN, so IFF_TUN devices can reach this receive path. The selftest commit in this series also mentions the limitation: "The outer IPv4 header carries options, as virtio_net_hdr_tnl_to_skb() expects room for an Ethernet header before the outer transport header." Would something like this work here? outer_l3min = (has_eth ? ETH_HLEN : 0) + virtio_l3min(outer_isv6); > > /* In case of USO, the inner protocol is still unknown and -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105115715.1466924-1-edumazet%40kernel.org
