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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

> > > > > > Should this be a global sysctl, rather than per netns?
> > > > > 
> > > > > 
> > > > > For customers I work with, it would be much less useful.
> > > > 
> > > > Third option: make it an integer rather than a boolean.
> > > > 
> > > > 0: no validation
> > > > 1: current level of validation
> > > > 2 and beyond: if we have to extend it later
> > > > 
> > > > Not pretty, I'll admit. So fine to keep as is too if you prefer.
> > > > But I do foresee more changes in this area, if history is a guide.
> > > >  
> > > > > 
> > > > > > 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 single system wide field is simplest.
> > > > 
> > > > Is there a benefit to making this configurable on a per netns basis?
> > > 
> > > As I tried to explain: a specific container breaks, you decide to trust it,
> > > you do not need to trust all.
> > 
> > Ok
> > 
> > > > If so, that's fine too. As long as root-in-namespace cannot modify it,
> > > > so readonly indeed.
> > > 
> > > 
> > > Right. So I envision along the lines of:
> > > 
> > > net.core.virtio_net_hdr_validate_all - validate packets from any netns,
> > > netns root can not change it, only host root
> > > 
> > > net.core.virtio_net_hdr_validate - per netns
> > 
> > Then skip the global option, as it's just a convenience: the same can be
> > achieved by iterating over all the per-netns settings.
> > 
> > Per netns validation, but only settable by root, not by root-in-namespace.
> 
> Hmm I don't know how to achieve that. Are there examples of such
> options?

Two examples, both basically make the sysctl writable by root-in-ns,
but then add an extra capable() check in the write handler.

ipvs max_conn: in proc_do_conn_max()

        if (write && !rc && val != unset) {
                struct netns_ipvs *ipvs = table->extra2;
                bool priv = capable(CAP_NET_ADMIN);
                int max;

                mutex_lock(&ipvs->service_mutex);
                /* Unprivileged admins can not go above the hard limit */
                max = priv ? IP_VS_CONN_MAX : ipvs->conn_max_limit;
                if (val < 0 || val > max) {
                        rc = -EINVAL;

ns_last_pid, which in its original commit b8f566b04d3c ("sysctl: add
the kernel.ns_last_pid control")had a direct capable check (it was
later relaxed):

        static int pid_ns_ctl_handler(struct ctl_table *table, int write,
                       void __user *buffer, size_t *lenp, loff_t *ppos)
        {
               struct ctl_table tmp = *table;
        
               if (write && !capable(CAP_SYS_ADMIN))
                       return -EPERM;