Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] net: RCU violation reading dev_net() in virtio_net_hdr_validate_disabled() --
commit 69b1f5f4a234fc132ca5965bd53c81fd86975566 Author: Michael S. Tsirkin <[email protected]> net: add a sysctl to opt out of virtio net hdr checksum validation This commit adds a per-netns sysctl, net.core.virtio_net_hdr_validate, guarded by a static key to allow opting out of virtio_net_hdr checksum offset validation. > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > index ec6f47ce1c974..b3bbecbd023cb 100644 > --- a/include/linux/virtio_net.h > +++ b/include/linux/virtio_net.h [ ... ] > @@ -78,6 +79,19 @@ static inline struct iphdr *virtio_net_ip4_parse(struct > sk_buff *skb, > *thoff = nhoff + iph->ihl * sizeof(u32); > return iph; > } > +/* Defined in net/core/dev.c, enabled while any network namespace > + * has net.core.virtio_net_hdr_validate set to 0. > + */ > +DECLARE_STATIC_KEY_FALSE(virtio_net_hdr_validate_disabled_key); > + > +/* Failsafe: a namespace can disable the check with > + * net.core.virtio_net_hdr_validate. > + */ > +static inline bool virtio_net_hdr_validate_disabled(struct sk_buff *skb) > +{ > + return static_branch_unlikely(&virtio_net_hdr_validate_disabled_key) && > + > !READ_ONCE(dev_net(skb->dev)->core.sysctl_virtio_net_hdr_validate); > +} [Severity: High] Does virtio_net_hdr_validate_disabled() need RCU read-side protection when fetching dev_net(skb->dev) and reading core.sysctl_virtio_net_hdr_validate? When net.core.virtio_net_hdr_validate is set to 0 in any network namespace (enabling virtio_net_hdr_validate_disabled_key), several callers of virtio_net_hdr_to_skb() and virtio_net_hdr_tnl_to_skb() invoke virtio_net_hdr_validate_disabled(skb) from preemptible process context while holding only dev_hold(skb->dev) and without holding rcu_read_lock(). Specifically, tun_get_user() calls tun_vnet_hdr_tnl_to_skb() -> virtio_net_hdr_tnl_to_skb() before acquiring rcu_read_lock(): drivers/net/tun.c:tun_get_user() { ... err = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr); if (err < 0) { atomic_long_inc(&tun->rx_frame_errors); err = -EINVAL; goto free_skb; } ... rcu_read_lock(); if (unlikely(!(tun->dev->flags & IFF_UP))) { ... } Similarly, tpacket_snd() and packet_snd() in net/packet/af_packet.c call virtio_net_hdr_to_skb() in process context without rcu_read_lock(): net/packet/af_packet.c:tpacket_snd() { ... if (has_vnet_hdr) { if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le()) < 0) { tp_len = -EINVAL; goto tpacket_error; } } ... } net/packet/af_packet.c:packet_snd() { ... if (vnet_hdr_sz) { err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le()); if (err < 0) goto out_free; len += vnet_hdr_sz; } ... } Holding dev_hold(skb->dev) does not prevent __dev_change_net_namespace() from moving skb->dev to another network namespace via dev_net_set(dev, net). If the old network namespace then exits and is freed via cleanup_net(), a task preempted after loading skb->dev->nd_net.net outside rcu_read_lock() could dereference a freed struct net. Should virtio_net_hdr_validate_disabled() use dev_net_rcu(skb->dev) inside rcu_read_lock() and rcu_read_unlock(), similar to how commit afec62cd0a4191 ("flow_dissector: use RCU protection to fetch dev_net()") fixed the same pattern in __skb_flow_dissect()? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
