[PATCH net] xfrm: always flush state and policy upon NETDEV_DOWN/NETDEV_UNREGISTER events

[PATCH net] xfrm: always flush state and policy upon NETDEV_DOWN/NETDEV_UNREGISTER events

[Tetsuo Handa]

syzbot is reporting that "struct xfrm_state" refcount is leaking.

  unregister_netdevice: waiting for netdevsim0 to become free. Usage count = 2
  ref_tracker: netdev@ffff888052f24618 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
       xfrm_state_construct net/xfrm/xfrm_user.c:986 [inline]
       xfrm_add_sa+0x34ff/0x5fa0 net/xfrm/xfrm_user.c:1022
       xfrm_user_rcv_msg+0x58e/0xc00 net/xfrm/xfrm_user.c:3507
       netlink_rcv_skb+0x158/0x420 net/netlink/af_netlink.c:2550
       xfrm_netlink_rcv+0x71/0x90 net/xfrm/xfrm_user.c:3529
       netlink_unicast_kernel net/netlink/af_netlink.c:1318 [inline]
       netlink_unicast+0x5aa/0x870 net/netlink/af_netlink.c:1344
       netlink_sendmsg+0x8c8/0xdd0 net/netlink/af_netlink.c:1894
       sock_sendmsg_nosec net/socket.c:727 [inline]
       __sock_sendmsg net/socket.c:742 [inline]
       ____sys_sendmsg+0xa5d/0xc30 net/socket.c:2592
       ___sys_sendmsg+0x134/0x1d0 net/socket.c:2646
       __sys_sendmsg+0x16d/0x220 net/socket.c:2678
       do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
       do_syscall_64+0xcd/0xf80 arch/x86/entry/syscall_64.c:94
       entry_SYSCALL_64_after_hwframe+0x77/0x7f

This is because currently xfrm_dev_down() (which is called upon NETDEV_DOWN
event and NETDEV_UNREGISTER event) can release the reference to
"struct net_device" taken by xfrm_dev_state_add() only if
the NETIF_F_HW_ESP bit is set, but the NETIF_F_HW_ESP bit can be cleared
by "ethtool -K $dev esp-hw-offload off" command.
In other words, we cannot guess whether xfrm_dev_state_add() has taken a
reference to "struct net_device" based on whether the NETIF_F_HW_ESP bit
is currently set.

For recording why this patch does not re-introduce xfrm_dev_unregister(),
my guessed history about this module's NETDEV_UNREGISTER handler is shown
below.

Commit d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
introduced xfrm_dev_state_add(). That commit called xfrm_dev_state_add()
 from xfrm_state_construct(), and introduced the NETDEV_UNREGISTER case
to xfrm_dev_event(). But that commit implemented xfrm_dev_unregister() as
a no-op, and implemented xfrm_dev_down() to call xfrm_dev_state_flush()
only if (dev->features & NETIF_F_HW_ESP) != 0. I guess that that commit
expected that NETDEV_DOWN event is fired before NETDEV_UNREGISTER event
fires, and also assumed that xfrm_dev_state_add() is called only if
(dev->features & NETIF_F_HW_ESP) != 0.

Commit ec30d78c14a8 ("xfrm: add xdst pcpu cache") added
xfrm_policy_cache_flush() call to xfrm_dev_unregister(), but
commit e4db5b61c572 ("xfrm: policy: remove pcpu policy cache") removed
xfrm_policy_cache_flush() call from xfrm_dev_unregister() and also
removed the NETDEV_UNREGISTER case from xfrm_dev_event() because
xfrm_dev_unregister() again became no-op.

Commit 03891f820c21 ("xfrm: handle NETDEV_UNREGISTER for xfrm device")
re-introduced the NETDEV_UNREGISTER case to xfrm_dev_event(), but that
commit for unknown reason chose to share xfrm_dev_down() between the
NETDEV_DOWN case and the NETDEV_UNREGISTER case. Therefore, I assumed
that doing the same behavior for both cases is desirable. If something
is wrong with this choice, please re-introduce xfrm_dev_unregister().

Reported-by: syzbot+881d65229ca4f9ae8c84@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=881d65229ca4f9ae8c84
Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Previous discussion is https://lkml.kernel.org/r/924f9cf5-599a-48f0-b1e3-94cd971965b0@I-love.SAKURA.ne.jp

 net/xfrm/xfrm_device.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 52ae0e034d29..26e62b6a9db5 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -536,10 +536,8 @@ static int xfrm_api_check(struct net_device *dev)
 
 static int xfrm_dev_down(struct net_device *dev)
 {
-	if (dev->features & NETIF_F_HW_ESP) {
-		xfrm_dev_state_flush(dev_net(dev), dev, true);
-		xfrm_dev_policy_flush(dev_net(dev), dev, true);
-	}
+	xfrm_dev_state_flush(dev_net(dev), dev, true);
+	xfrm_dev_policy_flush(dev_net(dev), dev, true);
 
 	return NOTIFY_DONE;
 }
-- 
2.47.3

Re: [PATCH net] xfrm: always flush state and policy upon NETDEV_DOWN/NETDEV_UNREGISTER events

[Leon Romanovsky]

On Wed, Jan 28, 2026 at 12:32:08AM +0900, Tetsuo Handa wrote:
> syzbot is reporting that "struct xfrm_state" refcount is leaking.
> 
>   unregister_netdevice: waiting for netdevsim0 to become free. Usage count = 2
>   ref_tracker: netdev@ffff888052f24618 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
>        xfrm_state_construct net/xfrm/xfrm_user.c:986 [inline]
>        xfrm_add_sa+0x34ff/0x5fa0 net/xfrm/xfrm_user.c:1022
>        xfrm_user_rcv_msg+0x58e/0xc00 net/xfrm/xfrm_user.c:3507
>        netlink_rcv_skb+0x158/0x420 net/netlink/af_netlink.c:2550
>        xfrm_netlink_rcv+0x71/0x90 net/xfrm/xfrm_user.c:3529
>        netlink_unicast_kernel net/netlink/af_netlink.c:1318 [inline]
>        netlink_unicast+0x5aa/0x870 net/netlink/af_netlink.c:1344
>        netlink_sendmsg+0x8c8/0xdd0 net/netlink/af_netlink.c:1894
>        sock_sendmsg_nosec net/socket.c:727 [inline]
>        __sock_sendmsg net/socket.c:742 [inline]
>        ____sys_sendmsg+0xa5d/0xc30 net/socket.c:2592
>        ___sys_sendmsg+0x134/0x1d0 net/socket.c:2646
>        __sys_sendmsg+0x16d/0x220 net/socket.c:2678
>        do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
>        do_syscall_64+0xcd/0xf80 arch/x86/entry/syscall_64.c:94
>        entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> This is because currently xfrm_dev_down() (which is called upon NETDEV_DOWN
> event and NETDEV_UNREGISTER event) can release the reference to
> "struct net_device" taken by xfrm_dev_state_add() only if
> the NETIF_F_HW_ESP bit is set, but the NETIF_F_HW_ESP bit can be cleared
> by "ethtool -K $dev esp-hw-offload off" command.
> In other words, we cannot guess whether xfrm_dev_state_add() has taken a
> reference to "struct net_device" based on whether the NETIF_F_HW_ESP bit
> is currently set.
> 
> For recording why this patch does not re-introduce xfrm_dev_unregister(),
> my guessed history about this module's NETDEV_UNREGISTER handler is shown
> below.
> 
> Commit d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
> introduced xfrm_dev_state_add(). That commit called xfrm_dev_state_add()
>  from xfrm_state_construct(), and introduced the NETDEV_UNREGISTER case
> to xfrm_dev_event(). But that commit implemented xfrm_dev_unregister() as
> a no-op, and implemented xfrm_dev_down() to call xfrm_dev_state_flush()
> only if (dev->features & NETIF_F_HW_ESP) != 0. I guess that that commit
> expected that NETDEV_DOWN event is fired before NETDEV_UNREGISTER event
> fires, and also assumed that xfrm_dev_state_add() is called only if
> (dev->features & NETIF_F_HW_ESP) != 0.
> 
> Commit ec30d78c14a8 ("xfrm: add xdst pcpu cache") added
> xfrm_policy_cache_flush() call to xfrm_dev_unregister(), but
> commit e4db5b61c572 ("xfrm: policy: remove pcpu policy cache") removed
> xfrm_policy_cache_flush() call from xfrm_dev_unregister() and also
> removed the NETDEV_UNREGISTER case from xfrm_dev_event() because
> xfrm_dev_unregister() again became no-op.
> 
> Commit 03891f820c21 ("xfrm: handle NETDEV_UNREGISTER for xfrm device")
> re-introduced the NETDEV_UNREGISTER case to xfrm_dev_event(), but that
> commit for unknown reason chose to share xfrm_dev_down() between the
> NETDEV_DOWN case and the NETDEV_UNREGISTER case. Therefore, I assumed
> that doing the same behavior for both cases is desirable. If something
> is wrong with this choice, please re-introduce xfrm_dev_unregister().
> 
> Reported-by: syzbot+881d65229ca4f9ae8c84@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=881d65229ca4f9ae8c84
> Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> Previous discussion is https://lkml.kernel.org/r/924f9cf5-599a-48f0-b1e3-94cd971965b0@I-love.SAKURA.ne.jp
> 
>  net/xfrm/xfrm_device.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index 52ae0e034d29..26e62b6a9db5 100644
> --- a/net/xfrm/xfrm_device.c
> +++ b/net/xfrm/xfrm_device.c
> @@ -536,10 +536,8 @@ static int xfrm_api_check(struct net_device *dev)
>  
>  static int xfrm_dev_down(struct net_device *dev)
>  {
> -	if (dev->features & NETIF_F_HW_ESP) {
> -		xfrm_dev_state_flush(dev_net(dev), dev, true);
> -		xfrm_dev_policy_flush(dev_net(dev), dev, true);
> -	}
> +	xfrm_dev_state_flush(dev_net(dev), dev, true);
> +	xfrm_dev_policy_flush(dev_net(dev), dev, true);

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.

Would it be possible to handle this in NETDEV_FEAT_CHANGE?

Thanks.

>  
>  	return NOTIFY_DONE;
>  }
> -- 
> 2.47.3
> 

Re: [PATCH net] xfrm: always flush state and policy upon NETDEV_DOWN/NETDEV_UNREGISTER events

[Tetsuo Handa]

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.

> 
> Would it be possible to handle this in NETDEV_FEAT_CHANGE?
> 

Is there a guarantee that xfrm_dev_event(NETDEV_FEAT_CHANGE) is called
regardless of other callback functions, for it can return NOTIFY_BAD ?

Re: [PATCH net] xfrm: always flush state and policy upon NETDEV_DOWN/NETDEV_UNREGISTER events

[Leon Romanovsky]

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.

> 
> > 
> > Would it be possible to handle this in NETDEV_FEAT_CHANGE?
> > 
> 
> Is there a guarantee that xfrm_dev_event(NETDEV_FEAT_CHANGE) is called
> regardless of other callback functions, for it can return NOTIFY_BAD ?

I don't know.

Thanks

> 
> 

Re: [PATCH net] xfrm: always flush state and policy upon NETDEV_DOWN/NETDEV_UNREGISTER events

[Tetsuo Handa]

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?

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?

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?

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.

  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?

 net/xfrm/xfrm_device.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 52ae0e034d29..550457e4c4f0 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -544,6 +544,14 @@ static int xfrm_dev_down(struct net_device *dev)
 	return NOTIFY_DONE;
 }
 
+static int xfrm_dev_unregister(struct net_device *dev)
+{
+	xfrm_dev_state_flush(dev_net(dev), dev, true);
+	xfrm_dev_policy_flush(dev_net(dev), dev, true);
+
+	return NOTIFY_DONE;
+}
+
 static int xfrm_dev_event(struct notifier_block *this, unsigned long event, void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
@@ -556,8 +564,10 @@ static int xfrm_dev_event(struct notifier_block *this, unsigned long event, void
 		return xfrm_api_check(dev);
 
 	case NETDEV_DOWN:
-	case NETDEV_UNREGISTER:
 		return xfrm_dev_down(dev);
+
+	case NETDEV_UNREGISTER:
+		return xfrm_dev_unregister(dev);
 	}
 	return NOTIFY_DONE;
 }

Re: [PATCH net] xfrm: always flush state and policy upon NETDEV_DOWN/NETDEV_UNREGISTER events

[Leon Romanovsky]

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.

> 
> 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".

> 
> 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?
> 
> 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.

> 
>   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?
> 
>  net/xfrm/xfrm_device.c |   12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index 52ae0e034d29..550457e4c4f0 100644
> --- a/net/xfrm/xfrm_device.c
> +++ b/net/xfrm/xfrm_device.c
> @@ -544,6 +544,14 @@ static int xfrm_dev_down(struct net_device *dev)
>  	return NOTIFY_DONE;
>  }
>  
> +static int xfrm_dev_unregister(struct net_device *dev)
> +{
> +	xfrm_dev_state_flush(dev_net(dev), dev, true);
> +	xfrm_dev_policy_flush(dev_net(dev), dev, true);
> +
> +	return NOTIFY_DONE;
> +}
> +
>  static int xfrm_dev_event(struct notifier_block *this, unsigned long event, void *ptr)
>  {
>  	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> @@ -556,8 +564,10 @@ static int xfrm_dev_event(struct notifier_block *this, unsigned long event, void
>  		return xfrm_api_check(dev);
>  
>  	case NETDEV_DOWN:
> -	case NETDEV_UNREGISTER:
>  		return xfrm_dev_down(dev);
> +
> +	case NETDEV_UNREGISTER:
> +		return xfrm_dev_unregister(dev);
>  	}
>  	return NOTIFY_DONE;
>  }
> 
> 

Re: [PATCH net] xfrm: always flush state and policy upon NETDEV_DOWN/NETDEV_UNREGISTER events

[Tetsuo Handa]

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@ffff888052f24618 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?

Re: [PATCH net] xfrm: always flush state and policy upon NETDEV_DOWN/NETDEV_UNREGISTER events

[Tetsuo Handa]

On 2026/01/29 19:16, Tetsuo Handa wrote:
>> 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.

we can't guess whether Bob is using $dev which supports only "IPsec without offload".
Bob might be expecting that "struct xfrm_state" without a reference to "struct net_device"
held is not released upon NETDEV_DOWN event.

Therefore, if "[PATCH net] xfrm: always flush state and policy upon
NETDEV_DOWN/NETDEV_UNREGISTER events" is applied, Bob might be surprised...

That's why I consider re-introducing xfrm_dev_unregister() might be the better fix.

Re: [PATCH net] xfrm: always flush state and policy upon NETDEV_DOWN/NETDEV_UNREGISTER events

[Sabrina Dubroca]

2026-01-29, 19:16:30 +0900, Tetsuo Handa wrote:
> 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".

We should avoid talking about "IPsec with/without offload" when this
can mean multiple different things:

 - ip xfrm state add ... offload ...
   (and the offload request actually succeeded)
 - packet going through all the offload code and to the device
 - device with NETIF_F_HW_ESP set in dev->features
 - device with ->xdo_dev_state_add

(I'm probably forgetting a few more)

> >> 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".

To Leon: this is not what Tetsuo wrote.
(but to be fair, "net_device does not support NETIF_F_HW_ESP flag" is
a bit confusing)

> 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@ffff888052f24618 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.

I don't know what syzbot did, but this triggers the same symptoms for
me:

echo 0 > /sys/bus/netdevsim/new_device
dev=$(ls -1 /sys/bus/netdevsim/devices/netdevsim0/net/)
ip x s a src 192.168.13.1 dst 192.168.13.2 proto esp spi 0x1000 mode tunnel aead 'rfc4106(gcm(aes))' $key 128 offload crypto dev $dev dir out
ethtool -K $dev esp-hw-offload off
echo 0 > /sys/bus/netdevsim/del_device

-- 
Sabrina

Re: [PATCH net] xfrm: always flush state and policy upon NETDEV_DOWN/NETDEV_UNREGISTER events

[Leon Romanovsky]

On Thu, Jan 29, 2026 at 05:05:32PM +0100, Sabrina Dubroca wrote:
> 2026-01-29, 19:16:30 +0900, Tetsuo Handa wrote:
> > 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".
> 
> We should avoid talking about "IPsec with/without offload" when this
> can mean multiple different things:
> 
>  - ip xfrm state add ... offload ...
>    (and the offload request actually succeeded)
>  - packet going through all the offload code and to the device
>  - device with NETIF_F_HW_ESP set in dev->features
>  - device with ->xdo_dev_state_add
> 
> (I'm probably forgetting a few more)

At least for me, "IPsec with offload" means all together:
device has ->xdo_dev_state_add + NETIF_F_HW_ESP bit + "ip xfrm state add ... offload
..."

I don't think that it is correct thing to adapt core code to something
specific to netdevsim which was introduced to emulate missing HW device.
Like in real HW device, the expectation is to have NETIF_F_HW_ESP bit,
we should have that bit in netdevsim too.

Thanks

Re: [PATCH net] xfrm: always flush state and policy upon NETDEV_DOWN/NETDEV_UNREGISTER events

[Tetsuo Handa]

On 2026/02/01 22:12, Leon Romanovsky wrote:
> I don't think that it is correct thing to adapt core code to something
> specific to netdevsim which was introduced to emulate missing HW device.
> Like in real HW device, the expectation is to have NETIF_F_HW_ESP bit,
> we should have that bit in netdevsim too.

I posted https://lkml.kernel.org/r/cea4b855-fe94-4b4e-9c2d-3cef7aac1be3@I-love.SAKURA.ne.jp
as what I think we should do regardless of netdevsim module.

syzbot is reporting "struct net_device" cannot be released because xfrm_dev_down()
cannot release reference to "struct net_device" unless NETIF_F_HW_ESP bit is set.

But I wonder when "struct xfrm_state" (without reference to "struct net_device") can be
released if xfrm_dev_down() becomes no-op due to checking for NETIF_F_HW_ESP bit;
I suspect there remains a resource leak bug unless xfrm_dev_unregister() is added.

Currently 50%+ reports of "unregister_netdevice: waiting for DEV to become free"
problem is hitting this xfrm path. Since there are other paths remaining, I want to
exclude xfrm path as quick as possible.

My question is, apart from how to fix netdevsim module, can my xfrm_dev_unregister()
patch be accepted?

Re: [PATCH net] xfrm: always flush state and policy upon NETDEV_DOWN/NETDEV_UNREGISTER events

[Sabrina Dubroca]

2026-01-29, 17:06:08 +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.

> 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?

Yes. You can verify this:

    echo 0 > /sys/bus/netdevsim/new_device
    dev=$(ls -1 /sys/bus/netdevsim/devices/netdevsim0/net/)
    ethtool -K $dev esp-hw-offload off  # clears NETIF_F_HW_ESP from dev->features
    ip xfrm state add src 192.168.13.1 dst 192.168.13.2 proto esp spi 0x1000 mode tunnel aead 'rfc4106(gcm(aes))' $key 128 offload crypto dev $dev dir out
    cat /sys/kernel/debug/netdevsim/netdevsim0/ports/0/ipsec
    ip xfrm state

(if you replace $dev in the "ip xfrm state add" command with some
device that can't do ipsec offload at all (no xfrmdev_ops), for
example a veth device, you'll see in the "ip xfrm state" output
something similar but without the "crypto offload" line)


> 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?
> 
> 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.
> 
>   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?

True. This is possible with

    ethtool -K $dev esp-hw-offload off ; ip link set $dev down

though not with mlx5 because dev->hw_features does not contain
NETIF_F_HW_ESP in mlx5 devices (this looks like a bug in the driver).

-- 
Sabrina