netdev - Re: [PATCHv2 net-next 5/5] selftests/net: add offload checking test for virtual interface

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: <aL6jlYPhsPfDKT8C@krikkit>
Date: Mon, 8 Sep 2025 11:36:21 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, Jay Vosburgh <jv@...sburgh.net>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Jiri Pirko <jiri@...nulli.us>, Simon Horman <horms@...nel.org>,
	Ido Schimmel <idosch@...dia.com>, Shuah Khan <shuah@...nel.org>,
	Stanislav Fomichev <sdf@...ichev.me>,
	Kuniyuki Iwashima <kuniyu@...gle.com>,
	Ahmed Zaki <ahmed.zaki@...el.com>,
	Alexander Lobakin <aleksander.lobakin@...el.com>,
	bridge@...ts.linux.dev, linux-kselftest@...r.kernel.org
Subject: Re: [PATCHv2 net-next 5/5] selftests/net: add offload checking test
 for virtual interface

2025-09-08, 04:15:38 +0000, Hangbin Liu wrote:
> On Sat, Sep 06, 2025 at 11:30:58PM +0200, Sabrina Dubroca wrote:
> > > +check_xfrm()
> > > +{
> > > +	local dev=$1
> > > +	local src=192.0.2.1
> > > +	local dst=192.0.2.2
> > > +	local key="0x3132333435363738393031323334353664636261"
> > > +
> > > +	RET=0
> > > +
> > > +	ip -n "$ns" xfrm state flush
> > > +	ip -n "$ns" xfrm state add proto esp src "$src" dst "$dst" spi 9 \
> > > +		mode transport reqid 42 aead "rfc4106(gcm(aes))" "$key" 128 \
> > > +		sel src "$src"/24 dst "$dst"/24 offload dev "$dev" dir out
> > 
> > It's maybe not something you would expect, but this codepath will not
> > check that NETIF_F_HW_ESP is set on $dev (you can verify that by
> > running "ip xfrm state add ... offload ..." on the same bond+netdevsim
> > combination before/after toggling esp-hw-offload on/off for the
> > bond). Why not use __check_offload again for this feature?
> 
> The esp-hw-offload is fixed on netdevsim
> 
> # ethtool -k eni0np1 | grep -i esp-hw-offload
> esp-hw-offload: on [fixed]
> 
> There is no way to disable it.

I don't think this is intentional. nsim_ipsec_init only adds
NSIM_ESP_FEATURES to ->features but not to ->hw_features, but I think
it was just forgotten. I added a few in 494bd83bb519 ("netdevsim: add
more hw_features"), extending nsim_ipsec_init (and nsim_macsec_init
since I made the same mistake) to also add features to ->hw_features
would make sense to me.


> After we add the netdevsim to bond,
> the bond also shows "esp-hw-offload off" as the flag is inherit
> in dev->hw_enc_features, not dev->features.

Did you mean dev->hw_features?

> It looks the only way to check if bond dev->hw_enc_features has NETIF_F_HW_ESP
> is try set xfrm offload. As

Was this test meant to check hw_enc_features?

To check hw_enc_features, I think the only way would be sending GSO
packets, since it's only used in those situations.


> static int xfrm_api_check(struct net_device *dev)
> {

But this doesn't get called when creating a new xfrm state. Trying to
create a new offloaded xfrm state doesn't look at any of the
netdev->*features (and we can't change that behavior anymore).

xfrm_api_check only gets called for NETDEV_REGISTER/NETDEV_FEAT_CHANGE
to validate whether the netdevice is set up correctly.

> #ifdef CONFIG_XFRM_OFFLOAD
>         if ((dev->features & NETIF_F_HW_ESP_TX_CSUM) &&
>             !(dev->features & NETIF_F_HW_ESP))
>                 return NOTIFY_BAD;
> 
>         if ((dev->features & NETIF_F_HW_ESP) &&
>             (!(dev->xfrmdev_ops &&
>                dev->xfrmdev_ops->xdo_dev_state_add &&
>                dev->xfrmdev_ops->xdo_dev_state_delete)))
>                 return NOTIFY_BAD;
> 
> Please correct me if I made any mistake.
> 
> Thanks
> Hangbin

-- 
Sabrina