Re: [PATCH net v5 5/7] net: add a sysctl to opt out of virtio net hdr checksum validation

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 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] virtio_net_hdr_validate_disabled() reads
  dev_net(skb->dev)->core.sysctl_virtio_net_hdr_validate with no RCU…

--

net: add a sysctl to opt out of virtio net hdr checksum validation

This adds a per-netns sysctl, net.core.virtio_net_hdr_validate. Host root
can use it to turn off the virtio net header checksum validation added by
the previous patch. A static key gates a lockless read of the per-netns
value in virtio_net_hdr_validate_disabled(), and that helper is checked
at the start of the csum and tunnel offset validators.

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 52852a33a2db7..cea70ccf0ed4b 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h

[ ... ]

> @@ -78,6 +79,20 @@ 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.
> + * The caller must keep the netns of skb->dev alive.
> + */
> +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: Medium]

Can this read hit a freed struct net? There is no rcu_read_lock() here
and no reference on the netns.

dev_net() is read_pnet(), which is just a plain load of dev->nd_net. The
comment asks the caller to keep the netns of skb->dev alive, but the
callers don't appear to do that.

For example, tun_get_user() gets here from preemptible process context,
before either of its rcu_read_lock() sections. The only thing it holds is
the device reference taken by tun_get():

tun_get_user()
  tun_vnet_hdr_tnl_to_skb()
    ...
      virtio_net_hdr_validate_disabled()

packet_snd() and tpacket_snd() reach virtio_net_hdr_to_skb() the same
way, also holding only a device reference.

Here is one possible sequence:

  - The writer loads dev->nd_net (net_B) and is preempted.
  - The device is moved from net_B to net_C. tun does not set
    netns_local, and dev_change_net_namespace() does not wait for
    device references.
  - The last reference to net_B is dropped and cleanup_net() runs.
    This reader is not in an RCU read-side section, so the
    synchronize_rcu() there does not wait for it, and net_B is freed.
  - The writer resumes and reads sysctl_virtio_net_hdr_validate from
    the freed net_B.

The window is small. It needs the static key enabled (some netns has
the sysctl set to 0) and a privileged netns move with teardown.

Would it be better to do the read under rcu_read_lock() and use
dev_net_rcu(), something like this?

	bool disabled;

	rcu_read_lock();
	disabled = !READ_ONCE(dev_net_rcu(skb->dev)->core.sysctl_virtio_net_hdr_validate);
	rcu_read_unlock();

> +}
>  
>  /* Where does the L3 header start, and what protocol is it?
>   * Handles Ethernet and VLAN headers; other devices use the

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.v5.git.mst%40redhat.com