netdev - Re: [PATCH v4 net-next 02/12] ipv6: add IFLA_GSO_IPV6_MAX

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e582432dbe85e743cc18d358d020711db5ddbf82.camel@gmail.com>
Date:   Fri, 06 May 2022 13:48:18 -0700
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Cc:     netdev <netdev@...r.kernel.org>, Coco Li <lixiaoyan@...gle.com>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH v4 net-next 02/12] ipv6: add IFLA_GSO_IPV6_MAX_SIZE

On Fri, 2022-05-06 at 08:30 -0700, Eric Dumazet wrote:
> From: Coco Li <lixiaoyan@...gle.com>
> 
> This enables ipv6/TCP stacks to build TSO packets bigger than
> 64KB if the driver is LSOv2 compatible.
> 
> This patch introduces new variable gso_ipv6_max_size
> that is modifiable through ip link.
> 
> ip link set dev eth0 gso_ipv6_max_size 185000
> 
> User input is capped by driver limit (tso_max_size)
> 
> Signed-off-by: Coco Li <lixiaoyan@...gle.com>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>

So I am still not a fan of adding all this extra tooling to make an
attribute that is just being applied to one protocol. Why not just
allow gso_max_size to extend beyond 64K and only limit it by
tso_max_size?

Doing that would make this patch much simpler as most of the code below
could be dropped.

> ---
>  include/linux/netdevice.h          |  2 ++
>  include/uapi/linux/if_link.h       |  1 +
>  net/core/dev.c                     |  2 ++
>  net/core/rtnetlink.c               | 23 +++++++++++++++++++++++
>  net/core/sock.c                    |  8 ++++++++
>  tools/include/uapi/linux/if_link.h |  1 +
>  6 files changed, 37 insertions(+)

<snip>

> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 512ed661204e0c66c8dcfaddc3001500d10f63ab..847cf80f81754451e5f220f846db734a7625695b 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c

<snip>

> @@ -2820,6 +2831,15 @@ static int do_setlink(const struct sk_buff *skb,
>  		}
>  	}
>  
> +	if (tb[IFLA_GSO_IPV6_MAX_SIZE]) {
> +		u32 max_size = nla_get_u32(tb[IFLA_GSO_IPV6_MAX_SIZE]);
> +
> +		if (dev->gso_ipv6_max_size ^ max_size) {
> +			netif_set_gso_ipv6_max_size(dev, max_size);
> +			status |= DO_SETLINK_MODIFIED;
> +		}
> +	}
> +
>  	if (tb[IFLA_GSO_MAX_SEGS]) {
>  		u32 max_segs = nla_get_u32(tb[IFLA_GSO_MAX_SEGS]);
>  

So the this code wouldn't be needed but the block above where we are
doing the check for max_size > GSO_MAX_SIZE could be removed.

> @@ -3283,6 +3303,9 @@ struct net_device *rtnl_create_link(struct net *net, const char *ifname,
>  		netif_set_gso_max_segs(dev, nla_get_u32(tb[IFLA_GSO_MAX_SEGS]));
>  	if (tb[IFLA_GRO_MAX_SIZE])
>  		netif_set_gro_max_size(dev, nla_get_u32(tb[IFLA_GRO_MAX_SIZE]));
> +	if (tb[IFLA_GSO_IPV6_MAX_SIZE])
> +		netif_set_gso_ipv6_max_size(dev,
> +			nla_get_u32(tb[IFLA_GSO_IPV6_MAX_SIZE]));
>  
>  	return dev;
>  }
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 6b287eb5427b32865d25fc22122fefeff3a4ccf5..4a29c3bf6b95f76280d8e32e903a0916322d5c4f 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2312,6 +2312,14 @@ void sk_setup_caps(struct sock *sk, struct dst_entry *dst)
>  			sk->sk_route_caps |= NETIF_F_SG | NETIF_F_HW_CSUM;
>  			/* pairs with the WRITE_ONCE() in netif_set_gso_max_size() */
>  			sk->sk_gso_max_size = READ_ONCE(dst->dev->gso_max_size);
> +#if IS_ENABLED(CONFIG_IPV6)
> +			if (sk->sk_family == AF_INET6 &&
> +			    sk_is_tcp(sk) &&
> +			    !ipv6_addr_v4mapped(&sk->sk_v6_rcv_saddr)) {
> +				/* Paired with WRITE_ONCE() in netif_set_gso_ipv6_max_size() */
> +				sk->sk_gso_max_size = READ_ONCE(dst->dev->gso_ipv6_max_size);
> +			}
> +#endif
>  			sk->sk_gso_max_size -= (MAX_TCP_HEADER + 1);
>  			/* pairs with the WRITE_ONCE() in netif_set_gso_max_segs() */
>  			max_segs = max_t(u32, READ_ONCE(dst->dev->gso_max_segs), 1);

This block here could then be rewritten as:
if (sk->sk_gso_max_size > GSO_MAX_SIZE &&
    (!IS_ENABLED(CONFIG_IPV6) || sk->sk_family != AF_INET6 || 
     !skb_is_tcp(sk) || ipv6_addr_v4mapped(&sk->sk_v6_rcf_saddr))
	sk->sk_gso_max_size = GSO_MAX_SIZE;

Then if we need protocol specific knobs in the future we could always
come back and make them act as caps instead of outright replacements
for the gso_max_size value.