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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Oct 15, 2026 at 01:50:54PM -0400, Willem de Bruijn wrote:
> Michael S. Tsirkin wrote:
> > The ingress validation as implemented by the previous
> > patch is stricter than what it replaced: GSO
> > packets whose checksum metadata matches no transport header are
> > dropped, and non-GSO ones get their checksum completed in place.
> > Such packets used to pass with the metadata unvalidated,
> > and with no overhead.
> > 
> > I judge that this should not affect any deployments, but there is still
> > some risk that it does.  In case a deployment depends on them anyway,
> > provide a per-netns switch to get the old behavior back:
> > 
> > echo 0 > /proc/sys/net/core/virtio_net_hdr_csum_check
> > 
> > The default is validating.
> > 
> > Assisted-by: LLM
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  Documentation/admin-guide/sysctl/net.rst | 15 +++++++
> >  include/linux/virtio_net.h               | 23 ++++++++++
> >  include/net/netns/core.h                 |  1 +
> >  net/core/dev.c                           |  6 +++
> >  net/core/net_namespace.c                 |  1 +
> >  net/core/sysctl_net_core.c               | 54 ++++++++++++++++++++++++
> >  6 files changed, 100 insertions(+)
> > 
> > diff --git a/Documentation/admin-guide/sysctl/net.rst b/Documentation/admin-guide/sysctl/net.rst
> > index e586e17fc7a5..7f230929c37e 100644
> > --- a/Documentation/admin-guide/sysctl/net.rst
> > +++ b/Documentation/admin-guide/sysctl/net.rst
> > @@ -246,6 +246,21 @@ processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set.
> >  Default: 1 (on)
> >  
> >  
> > +virtio_net_hdr_csum_check
> > +-------------------------
> 
> Make more generic, virtio_net_hdr_validate?
> 
> In case we add more validation in the future (as we have in the past),
> without having to introduce multiple such knobs.

But it's uapi, are you sure we'll be able to change the meaning
of an existing knob, even if we keep the name vague?

> > +
> > +When processing packets with a virtio net header (through tun, tap, a
> > +packet socket or a virtio network device) that request checksum offload
> > +(VIRTIO_NET_HDR_F_NEEDS_CSUM), validate the packet: complete the checksum
> > +in software, or drop the packet where the header does not match it.
> > +
> > +When disabled, the header is trusted not to corrupt the packet, and the
> > +checksum is offloaded to hardware (or not verified if it never reaches
> > +hardware). This is only safe if no protocol is processing the packets.
> > +
> > +Default: 1 (on)
> > +
> > +
> >  wmem_default
> >  ------------
> >  
> > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > index 357115ff0ec2..aa2f8ac4657c 100644
> > --- a/include/linux/virtio_net.h
> > +++ b/include/linux/virtio_net.h
> > @@ -5,6 +5,7 @@
> >  #include <linux/if_vlan.h>
> >  #include <linux/ip.h>
> >  #include <linux/ipv6.h>
> > +#include <linux/jump_label.h>
> >  #include <linux/netdevice.h>
> >  #include <linux/udp.h>
> >  #include <net/gso.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_csum_check set to 0.
> > + */
> 
> Should this be a global sysctl, rather than per netns?


For customers I work with, it would be much less useful.


> Even if per-netns, most likely do not want it writable by netns root
> with user namespaces.
>
> That is the equivalent of ns_capable(CAP_NET_RAW), which is the cause
> of many crashes due to bad packets ingested by, e.g., a packet socket.

Hmm, yes i get that. Two knobs? a. verify b. the per netns value ro? Both
on by default?

> A global sysctl also allows using proc_do_static_key and avoids the
> custom logic in virtio_net_hdr_csum_check_sysctl.
> 
> >  /* Where does the L3 header start, and what protocol is it?
> >   * Handles Ethernet and VLAN headers; other devices use the
> > @@ -152,6 +166,12 @@ virtio_net_csum_partial_ok_at_offset(struct sk_buff *skb, u8 hdr_gso_type,
> >  	unsigned int thoff;
> >  	int ip_proto;
> >  
> > +	/* Failsafe: a namespace opted out of the validation,
> > +	 * trust the checksum metadata as before.
> > +	 */
> 
> nit: as before, imho the code here is clear and comment unnecessary.


Ugh. One of these got through. Absolutely.

> > +	if (virtio_net_hdr_csum_check_disabled(skb))
> > +		return true;
> > +
> >  	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
> >  		return false;
> >  
> > @@ -483,6 +503,9 @@ virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
> >  	unsigned int thoff;
> >  	int ip_proto;
> >  
> > +	if (virtio_net_hdr_csum_check_disabled(skb))
> > +		return true;
> > +
> >  	if (!virtio_net_l3_start(skb, &nhoff, &proto))
> >  		return false;
> >  	if (outer_isv6 != (proto == htons(ETH_P_IPV6)))
> 
> > +/* Defined in net/core/dev.c, declared for the gate in
> > + * include/linux/virtio_net.h.
> > + */
> > +DECLARE_STATIC_KEY_FALSE(virtio_net_hdr_csum_check_disabled_key);
> > +
> > +/*
> > + * Protects changes to virtio_net_hdr_csum_check_disabled_key and each
> > + * net->core.sysctl_virtio_net_hdr_csum_check. Readers don't use a lock.
> > + */
> > +static DEFINE_MUTEX(virtio_net_hdr_csum_check_mutex);
> > +
> > +static int virtio_net_hdr_csum_check_sysctl(const struct ctl_table *table,
> > +					    int write, void *buffer,
> > +					    size_t *lenp, loff_t *ppos)
> > +{
> > +	struct net *net = (struct net *)table->data;
> > +	/* checkpatch warns but this can't be const: tmp.data is set below */
> > +	struct ctl_table tmp = *table;
> > +	u8 old;
> > +	int ret;
> > +
> > +	/*
> > +	 * Make sure virtio_net_hdr_csum_check_disabled_key counts zero values of
> > +	 * net->core.sysctl_virtio_net_hdr_csum_check.
> > +	 */
> > +	mutex_lock(&virtio_net_hdr_csum_check_mutex);
> > +
> > +	old = net->core.sysctl_virtio_net_hdr_csum_check;
> > +	tmp.data = &net->core.sysctl_virtio_net_hdr_csum_check;
> > +	ret = proc_dou8vec_minmax(&tmp, write, buffer, lenp, ppos);
> > +	if (write && !ret && old != net->core.sysctl_virtio_net_hdr_csum_check) {
> > +		if (net->core.sysctl_virtio_net_hdr_csum_check)
> > +			static_branch_dec(&virtio_net_hdr_csum_check_disabled_key);
> > +		else
> > +			static_branch_inc(&virtio_net_hdr_csum_check_disabled_key);
> > +	}
> > +
> > +	mutex_unlock(&virtio_net_hdr_csum_check_mutex);
> > +
> > +	return ret;
> > +}
> > +
> >  static const struct ctl_table netns_core_table[] = {
> >  #if IS_ENABLED(CONFIG_RPS)
> >  	{
> > @@ -737,6 +780,15 @@ static const struct ctl_table netns_core_table[] = {
> >  		.extra1		= SYSCTL_ZERO,
> >  		.extra2		= SYSCTL_ONE
> >  	},
> > +	{
> > +		.procname	= "virtio_net_hdr_csum_check",
> > +		.data		= &init_net,
> > +		.maxlen		= sizeof(u8),
> > +		.mode		= 0644,
> > +		.proc_handler	= virtio_net_hdr_csum_check_sysctl,
> > +		.extra1		= SYSCTL_ZERO,
> > +		.extra2		= SYSCTL_ONE
> > +	},
> >  	/* sysctl_core_net_init() will set the values after this
> >  	 * to readonly in network namespaces
> >  	 */
> > @@ -841,6 +893,8 @@ static __net_exit void sysctl_core_net_exit(struct net *net)
> >  	tbl = net->core.sysctl_hdr->ctl_table_arg;
> >  	unregister_net_sysctl_table(net->core.sysctl_hdr);
> >  	BUG_ON(tbl == netns_core_table);
> > +	if (!net->core.sysctl_virtio_net_hdr_csum_check)
> > +		static_branch_dec(&virtio_net_hdr_csum_check_disabled_key);
> >  #if IS_ENABLED(CONFIG_RPS)
> >  	kfree(net->core.rps_default_mask);
> >  #endif
> > -- 
> > MST
> > 
>