netdev - Re: [PATCH net] xfrm: always flush state and policy upon NETDEV_DOWN/NETDEV

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: <b91432c3-5ed4-4669-8d7c-02cf7ea6f8fa@I-love.SAKURA.ne.jp>
Date: Thu, 29 Jan 2026 17:06:08 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: Leon Romanovsky <leon@...nel.org>, Sabrina Dubroca <sd@...asysnail.net>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>
Cc: 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/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;
 }