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 <[email protected]>
> ---
> Documentation/admin-guide/sysctl/net.rst | 15 +++++++
> 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 | 54 ++++++++++++++++++++++++
> 6 files changed, 100 insertions(+)
>
> diff --git a/Documentation/admin-guide/sysctl/net.rst
> b/Documentation/admin-guide/sysctl/net.rst
> index e586e17fc7a5..7f230929c37e 100644
> --- a/Documentation/admin-guide/sysctl/net.rst
> +++ b/Documentation/admin-guide/sysctl/net.rst
> @@ -246,6 +246,21 @@ processes are dropped unless socket option
> SOF_TIMESTAMPING_OPT_TSONLY is set.
> Default: 1 (on)
>
>
> +virtio_net_hdr_csum_check
> +-------------------------
Make more generic, virtio_net_hdr_validate?
In case we add more validation in the future (as we have in the past),
without having to introduce multiple such knobs.
> +
> +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.
> +
> +Default: 1 (on)
> +
> +
> wmem_default
> ------------
>
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 357115ff0ec2..aa2f8ac4657c 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.
> + */
Should this be a global sysctl, rather than per netns?
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.
A global sysctl also allows using proc_do_static_key and avoids the
custom logic in virtio_net_hdr_csum_check_sysctl.
> /* Where does the L3 header start, and what protocol is it?
> * Handles Ethernet and VLAN headers; other devices use the
> @@ -152,6 +166,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.
> + */
nit: as before, imho the code here is clear and comment unnecessary.
> + if (virtio_net_hdr_csum_check_disabled(skb))
> + return true;
> +
> if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
> return false;
>
> @@ -483,6 +503,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)))
> +/* 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.
> + */
> +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 */
> + struct ctl_table tmp = *table;
> + u8 old;
> + int ret;
> +
> + /*
> + * 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)
> {
> + if (net->core.sysctl_virtio_net_hdr_csum_check)
> +
> static_branch_dec(&virtio_net_hdr_csum_check_disabled_key);
> + else
> +
> static_branch_inc(&virtio_net_hdr_csum_check_disabled_key);
> + }
> +
> + mutex_unlock(&virtio_net_hdr_csum_check_mutex);
> +
> + return ret;
> +}
> +
> static const struct ctl_table netns_core_table[] = {
> #if IS_ENABLED(CONFIG_RPS)
> {
> @@ -737,6 +780,15 @@ static const struct ctl_table netns_core_table[] = {
> .extra1 = SYSCTL_ZERO,
> .extra2 = SYSCTL_ONE
> },
> + {
> + .procname = "virtio_net_hdr_csum_check",
> + .data = &init_net,
> + .maxlen = sizeof(u8),
> + .mode = 0644,
> + .proc_handler = virtio_net_hdr_csum_check_sysctl,
> + .extra1 = SYSCTL_ZERO,
> + .extra2 = SYSCTL_ONE
> + },
> /* sysctl_core_net_init() will set the values after this
> * to readonly in network namespaces
> */
> @@ -841,6 +893,8 @@ static __net_exit void sysctl_core_net_exit(struct net
> *net)
> tbl = net->core.sysctl_hdr->ctl_table_arg;
> unregister_net_sysctl_table(net->core.sysctl_hdr);
> BUG_ON(tbl == netns_core_table);
> + if (!net->core.sysctl_virtio_net_hdr_csum_check)
> + static_branch_dec(&virtio_net_hdr_csum_check_disabled_key);
> #if IS_ENABLED(CONFIG_RPS)
> kfree(net->core.rps_default_mask);
> #endif
> --
> MST
>
