linux-kernel - Re: [PATCHv2 net] Bonding: Fix support for gso_partial

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: <40707a0ed22fa87dbe6b5e28d22fad586158675e.camel@nvidia.com>
Date: Thu, 23 Jan 2025 12:15:01 +0000
From: Cosmin Ratiu <cratiu@...dia.com>
To: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "liuhangbin@...il.com"
	<liuhangbin@...il.com>
CC: "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, "liali@...hat.com"
	<liali@...hat.com>, "razor@...ckwall.org" <razor@...ckwall.org>,
	"davem@...emloft.net" <davem@...emloft.net>, Tariq Toukan
	<tariqt@...dia.com>, "linux-kselftest@...r.kernel.org"
	<linux-kselftest@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "jv@...sburgh.net" <jv@...sburgh.net>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "horms@...nel.org"
	<horms@...nel.org>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, Boris Pismenny <borisp@...dia.com>,
	Jianbo Liu <jianbol@...dia.com>
Subject: Re: [PATCHv2 net] Bonding: Fix support for gso_partial_features

On Wed, 2025-01-22 at 13:52 +0000, Hangbin Liu wrote:
> The fixed commit adds NETIF_F_GSO_ESP bit for bonding
> gso_partial_features.
> However, if we don't set the dev NETIF_F_GSO_PARTIAL bit, the later
> netdev_change_features() -> netdev_fix_features() will remove the
> NETIF_F_GSO_ESP bit from the dev features. This causes ethtool to
> show
> that the bond does not support tx-esp-segmentation. For example
> 
>  # ethtool -k bond0 | grep esp
>  tx-esp-segmentation: off [requested on]
>  esp-hw-offload: on
>  esp-tx-csum-hw-offload: on
> 
> Add the NETIF_F_GSO_PARTIAL bit to bond dev features when set
> gso_partial_features to fix this issue.
> 
> Fixes: 4861333b4217 ("bonding: add ESP offload features when slaves
> support")
> Reported-by: Liang Li <liali@...hat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> ---
> v2: remove NETIF_F_GSO_PARTIAL bit if not set gso_partial_features.

I don't think this is needed, to avoid having bond_compute_features
modify bond_dev->features directly.
And in general, I think NETIF_F_GSO_PARTIAL should be set in bond_setup
once and left on.

NETIF_F_GSO_PARTIAL is used in __skb_gso_segment to invoke skb_gso_ok,
which checks if skb->gso_type is in (features & gso_partial_features).
If not, it locally disables NETIF_F_GSO_PARTIAL. Later, skb_segment
does another check for skb_gso_ok and skips segmentation if
NETIF_F_GSO_PARTIAL is locally disabled.
So a packet with SKB_GSO_ESP sent on a device with only
NETIF_F_GSO_PARTIAL but no NETIF_F_GSO_ESP with behave correctly:
__skb_gso_segment will locally remove NETIF_F_GSO_PARTIAL and
skb_segment will not do segmentation.


> The issue is reported internally, so there is no Closes tag.
> 
> BTW, I saw some drivers set NETIF_F_GSO_PARTIAL on dev->features.
> Some
> other drivers set NETIF_F_GSO_PARTIAL on dev->hw_enc_features. I
> haven't
> see a doc about where we should set. So I just set it on dev-
> >features.

It seems NETIF_F_GSO_PARTIAL is needed on both features and
hw_enc_features, otherwise traffic is not segmented and performance
suffers.
netif_skb_features returns the intersection of features &
hw_enc_features, and that is used to drive skb_gso_segment. The same
approach (features & hw_enc_features) is taken in a few .gso_segment
callbacks.

> ---
>  drivers/net/bonding/bond_main.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c
> b/drivers/net/bonding/bond_main.c
> index 7b78c2bada81..09d5a8433d86 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1598,10 +1598,13 @@ static void bond_compute_features(struct
> bonding *bond)
>  	}
>  	bond_dev->hard_header_len = max_hard_header_len;
>  
> -	if (gso_partial_features & NETIF_F_GSO_ESP)
> +	if (gso_partial_features & NETIF_F_GSO_ESP) {
>  		bond_dev->gso_partial_features |= NETIF_F_GSO_ESP;
> -	else
> +		bond_dev->features |= NETIF_F_GSO_PARTIAL;
> +	} else {
>  		bond_dev->gso_partial_features &= ~NETIF_F_GSO_ESP;
> +		bond_dev->features &= ~NETIF_F_GSO_PARTIAL;
> +	}
>  
>  done:
>  	bond_dev->vlan_features = vlan_features;