"Michael S. Tsirkin" <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
