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

quoted

quoted

quoted

quoted

quoted

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.
 

quoted

quoted

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

quoted

quoted

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;