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

[Michael S. Tsirkin]

On Tue, Oct 20, 2026 at 07:12:57PM -0400, Willem de Bruijn wrote:
> Michael S. Tsirkin wrote:
> > On Tue, Oct 20, 2026 at 04:19:41PM -0400, Willem de Bruijn wrote:
> > > Michael S. Tsirkin wrote:
> > > > On Tue, Oct 20, 2026 at 03:54:23PM -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@xxxxxxxxxx>
> > > > > > ---
> > > > > >  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
> > > > > > +-------------------------
> > > > > 
> > > > > virtio_net_hdr_validate? In case we do have to add more validation
> > > > > later.
> > > > 
> > > > If you insist. My worry would be what if that validation is again
> > > > user-visible somehow and we need to distinguish users who want
> > > > to disable that future thing and users who want to disable
> > > > just this current thing.
> > > > 
> > > > Hard to predict the future. But I will go with your intuition.
> > > 
> > > Thanks, but definitely feel free to ignore my suggestions.
> > > 
> > > My intuition would be to define as
> > > 
> > > 0: off
> > > 1: everything one
> > > 2+: sadly needed fudge values because we keep having to tighten,
> > >     but some crazy workloads need to opt-out to avoid breakage.
> > 
> > yes you said i just do not get how it will work for userspace,
> > maybe u can explain?
> > 
> > and what specifically is needed right now, while we
> > just have 1.
> 
> Right now the sysctl is boolean.
> 
> If we define it as a narrow, very precisely defined sysctl, then its
> exact behavior is set in stone.
> 
> If we then add additional validation later, that can similarly have
> unintended breakage of weird users, we need to add another sysctl.
> 
> And if that validation is not entirely independent, i.e., builds on
> this, you get a weird set of interdependent sysctls.
> 
> While in general systems will run with all validation enabled, unless
> they do something weird that gets hit by validation.
> 
> A single broadly defined sysctl ("validate input at the source") can
> be refined over time, including to expand to validation of other
> virtio_net_hdr fields or protocol combinations.
> 
> Even if defining it more broadly we can still leave it a boolean:
> all validation on or off.
> 
> Or, optionally, we can also define additional validation levels,
> e.g, 0:off, 1:everything-on, 2:validate-csum-only.
> 
> 
> 

So I will just rename it to validate, we will see down the road
how it plays out?

-- 
MST