Message-ID: <d0aa3c51-291b-40fd-82b4-15024dc4404b@I-love.SAKURA.ne.jp>
Date: Thu, 29 Jan 2026 19:16:30 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: Leon Romanovsky <leon@...nel.org>
Cc: Sabrina Dubroca <sd@...asysnail.net>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
        Ilan Tayari <ilant@...lanox.com>, Guy Shapiro <guysh@...lanox.com>,
        Yossi Kuperman <yossiku@...lanox.com>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH net] xfrm: always flush state and policy upon
 NETDEV_DOWN/NETDEV_UNREGISTER events

On 2026/01/29 18:09, Leon Romanovsky wrote:
> On Thu, Jan 29, 2026 at 05:06:08PM +0900, Tetsuo Handa wrote:
>> On 2026/01/28 21:35, Leon Romanovsky wrote:
>>> On Wed, Jan 28, 2026 at 07:44:02PM +0900, Tetsuo Handa wrote:
>>>> On 2026/01/28 19:24, Leon Romanovsky wrote:
>>>>> I think this can work, but IMHO the more robust approach is to ensure that all
>>>>> states and policies are removed when the NETIF_F_HW_ESP feature bit is cleared.
>>>>
>>>> The transaction will become complicated, for dev->features manipulation
>>>> function can fail.
>>>
>>> Line above returning NOTIFY_OK, check that NETIF_F_HW_ESP is cleared,
>>> and remove everything.
>>
>> That answer needs more clarification. I came to get confused about what we should do.
>>
>> Question 1:
>>
>>   Since NETIF_F_HW_ESP is a hardware dependent flag, not all "struct net_device"
>>   support NETIF_F_HW_ESP flag. Is this interpretation correct?
> 
> Yes, however any device (SW or HW) should set this flag if they want to
> provide IPsec offload.

OK. There are "IPsec with offload" and "IPsec without offload".
Both cases use code in net/xfrm/ directory.

Users (not the kernel source but Linux administrator) can choose
"IPsec without offload" by clearing the NETIF_F_HW_ESP bit via
"ethtool -K $dev esp-hw-offload off" command even if $dev supports
both "IPsec with offload" and "IPsec without offload".

> 
>>
>> Question 2:
>>
>>   Sabrina Dubroca commented
>>
>>     But the current behavior ("ignore NETIF_F_HW_ESP and call
>>     xdo_dev_state_add for new states anyway") has been established for
>>     multiple years. Changing that now seems a bit risky.
>>
>>   at https://lkml.kernel.org/r/aXd3QjzwOVm0Q9LF@krikkit .
>>
>>   Is that comment saying that we have been permitting a "struct net_device"
>>   to be selected by xfrm_dev_state_add() even if that "struct net_device"
>>   does not support NETIF_F_HW_ESP flag. Is this interpretation correct?
> 
> I don't understand what does it mean "device doesn't support offload but
> state was offloaded anyway".

Users (not the kernel source but Linux administrator) who are using $dev
which supports only "IPsec without offload" can call xfrm_dev_state_add()
because xfrm_dev_state_add() does not check for the NETIF_F_HW_ESP bit.

Therefore such users can create "struct xfrm_state" with a reference to
"struct net_device" held at
https://elixir.bootlin.com/linux/v6.19-rc5/source/net/xfrm/xfrm_user.c#L986 .

> 
>>
>> Question 3:
>>
>>   Leon Romanovsky suggested that, as a more robust approach, remove all states
>>   and policies when the NETIF_F_HW_ESP feature bit is cleared.
>>
>>   But I consider that such approach will not work, for (according to Q2 above)
>>   xfrm_dev_state_add() can be called even if the NETIF_F_HW_ESP feature bit is not
>>   set. Also, I think that there is no guarantee that dev->features manipulation
>>   function is called after xfrm_dev_state_add() was called.
>>
>>   Therefore, we need an event that are guaranteed to be called.
>>   The NETDEV_UNREGISTER event is guaranteed to be called when unregistring
>>   a "struct net_device", and therefore a good place to remove all states and
>>   policies.
>>
>>   Is this interpretation correct?

Since we don't have a syzbot reproducer, I can't tell whether syzbot is manually
clearing the NETIF_F_HW_ESP bit or not. But as described above, a syzbot report

  unregister_netdevice: waiting for netdevsim0 to become free. Usage count = 2
  ref_tracker: netdev@...f888052f24618 has 1/1 users at
       __netdev_tracker_alloc include/linux/netdevice.h:4400 [inline]
       netdev_tracker_alloc include/linux/netdevice.h:4412 [inline]
       xfrm_dev_state_add+0x3a5/0x1080 net/xfrm/xfrm_device.c:316

indicates that "struct xfrm_state" with a reference to "struct net_device" held
is remaining because xfrm_dev_state_flush() is not called upon NETDEV_UNREGISTER
event.

>>
>> Question 4:
>>
>>   If Q1 is correct, Sabrina's comment
>>
>>     Changing that now seems a bit risky.
>>
>>   in Q2 might be applicable to xfrm_dev_down().
>>
>>   That is, someone who is using xfrm with a !NETIF_F_HW_ESP hardware might be
>>   expecting that state and policy are not flushed upon NETDEV_DOWN event.
> 
> Do we have such in-tree devices? If the answer is no, you shouldn't be
> worried about that case.

The "user" here is not "in-tree devices" but "Linux administrator" (such as Alice
and Bob).

We can't guess whether Alice is using $dev which supports only "IPsec without offload"
and is calling xfrm_dev_state_add(). If Alice is doing so, Alice might be expecting that
"struct xfrm_state" with a reference to "struct net_device" held is not released upon
NETDEV_DOWN event.

> 
>>
>>   If there is such possibility, I think we should avoid changing xfrm_dev_down()
>>   and instead re-introduce xfrm_dev_unregister(). What do you think?

Powered by blists - more mailing lists