Re: [PATCH net v3 6/8] net: add a sysctl to opt out of virtio net hdr checksum validation

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

On Mon, Oct 19, 2026 at 04:19:16PM +0200, Björn Töpel wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > 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 | 19 ++++++++
> >  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               | 60 ++++++++++++++++++++++++
> >  6 files changed, 110 insertions(+)
> >
> > diff --git a/Documentation/admin-guide/sysctl/net.rst b/Documentation/admin-guide/sysctl/net.rst
> > index e586e17fc7a5..b1a1c4714b22 100644
> > --- a/Documentation/admin-guide/sysctl/net.rst
> > +++ b/Documentation/admin-guide/sysctl/net.rst
> > @@ -246,6 +246,25 @@ processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set.
> >  Default: 1 (on)
> >  
> >  
> > +virtio_net_hdr_csum_check
> > +-------------------------
> > +
> > +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.
> > +
> > +The setting is per network namespace, but only writable by the host
> > +root: root inside a network namespace created by an unprivileged user
> > +cannot disable the validation.
> > +
> > +Default: 1 (on)
> > +
> > +
> >  wmem_default
> >  ------------
> >  
> > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > index 97b554ef7123..25aa95c11e31 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.
> > + */
> > +DECLARE_STATIC_KEY_FALSE(virtio_net_hdr_csum_check_disabled_key);
> > +
> > +/* Failsafe: a namespace can disable the check with
> > + * net.core.virtio_net_hdr_csum_check.
> > + */
> > +static inline bool virtio_net_hdr_csum_check_disabled(struct sk_buff *skb)
> > +{
> > +	return static_branch_unlikely(&virtio_net_hdr_csum_check_disabled_key) &&
> > +	       !READ_ONCE(dev_net(skb->dev)->core.sysctl_virtio_net_hdr_csum_check);
> > +}
> >  
> >  /* Where does the L3 header start, and what protocol is it?
> >   * Handles Ethernet and VLAN headers; other devices use the
> > @@ -179,6 +193,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.
> > +	 */
> > +	if (virtio_net_hdr_csum_check_disabled(skb))
> > +		return true;
> > +
> >  	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
> >  		return false;
> >  
> > @@ -516,6 +536,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)))
> > diff --git a/include/net/netns/core.h b/include/net/netns/core.h
> > index 9ef3d70e5e9c..ae8110772c97 100644
> > --- a/include/net/netns/core.h
> > +++ b/include/net/netns/core.h
> > @@ -18,6 +18,7 @@ struct netns_core {
> >  	u8	sysctl_txrehash;
> >  	u8	sysctl_tstamp_allow_data;
> >  	u8	sysctl_bypass_prot_mem;
> > +	u8	sysctl_virtio_net_hdr_csum_check;
> >  
> >  #ifdef CONFIG_PROC_FS
> >  	struct prot_inuse __percpu *prot_inuse;
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index e76762e29360..d14950b1af3a 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -2389,6 +2389,12 @@ EXPORT_SYMBOL(tcf_sw_enabled_key);
> >  
> >  DEFINE_STATIC_KEY_FALSE(netstamp_needed_key);
> >  EXPORT_SYMBOL(netstamp_needed_key);
> > +
> > +/* Enabled while any network namespace has net.core.virtio_net_hdr_csum_check
> > + * set to 0.
> > + */
> > +DEFINE_STATIC_KEY_FALSE(virtio_net_hdr_csum_check_disabled_key);
> > +EXPORT_SYMBOL(virtio_net_hdr_csum_check_disabled_key);
> >  #ifdef CONFIG_JUMP_LABEL
> >  static atomic_t netstamp_needed_deferred;
> >  static atomic_t netstamp_wanted;
> > diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> > index da5f881fbd3b..2d340fafba85 100644
> > --- a/net/core/net_namespace.c
> > +++ b/net/core/net_namespace.c
> > @@ -396,6 +396,7 @@ static __net_init void preinit_net_sysctl(struct net *net)
> >  	net->core.sysctl_optmem_max = 128 * 1024;
> >  	net->core.sysctl_txrehash = SOCK_TXREHASH_ENABLED;
> >  	net->core.sysctl_tstamp_allow_data = 1;
> > +	net->core.sysctl_virtio_net_hdr_csum_check = 1;
> >  	net->core.sysctl_txq_reselection = msecs_to_jiffies(1000);
> >  }
> >  
> > diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
> > index eb35da3556f4..c65ff86cc5f9 100644
> > --- a/net/core/sysctl_net_core.c
> > +++ b/net/core/sysctl_net_core.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/sched/isolation.h>
> >  #include <linux/hex.h>
> > +#include <linux/jump_label.h>
> >  
> >  #include <net/ip.h>
> >  #include <net/sock.h>
> > @@ -678,6 +679,54 @@ static struct ctl_table net_core_table[] = {
> >  	},
> >  };
> >  
> > +/* 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.
> > + */
> 
> nit: OCD doesn't like mixing old netdev comments with vanilla style...
> one VVV as well.
> 
> > +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 */
> 
> nit: Redundant comment...
> 
> > +	struct ctl_table tmp = *table;
> 
> ...and here, can't we instead get rid of tmp, and do something like:
>     | u8 *value = table->data;
>     | u8 old, new;
> 
> > +	u8 old;
> > +	int ret;
> > +
> > +	/* Root in a user namespace must not be able to weaken the
> > +	 * validation for the network namespace it controls.
> > +	 */
> > +	if (write && !capable(CAP_NET_ADMIN))
> > +		return -EPERM;
> > +
> > +	/*
> > +	 * 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) {
> 
> and then:
>     | old = READ_ONCE(*value);
>     | ret = proc_dou8vec_minmax(table, write, buffer, lenp, ppos);
>     | new = READ_ONCE(*value);
>     | 
>     | if (write && !ret && old != new) {
> 
> at least that's easier to read for me. (No blocker!)
> 
> 
> Reviewed-by: Björn Töpel <bjorn@kernel.org>


Thanks, I'm addressing sashiko review points, will address this
in v4.