Skip to contentSkip to conversation

Couple of error unwind fixes to packet offload

PATCH Applied Leon Romanovsky 4 participants 11 messages Apr 19, 2023 – Apr 21, 2023

Subsystems (3)
Networking Network drivers Networking IPsec
lore
Share & tools
lore

Leon Romanovsky

author
#0 lore raw

cover Couple of error unwind fixes to packet offload

From: Leon Romanovsky <[email protected]>

Hi Steffen,

There are two straightforward fixes to XFRM.

Thanks

Leon Romanovsky (2):
xfrm: release all offloaded policy memory
xfrm: Fix leak of dev tracker

 net/xfrm/xfrm_device.c | 2 +-
 net/xfrm/xfrm_user.c   | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)
Signature · 1 line
2.40.0
author
#1 lore raw

PATCH 1/2 xfrm: release all offloaded policy memory

From: Leon Romanovsky <[email protected]>

Failure to add offloaded policy will cause to the following error once user will try to reload driver.

Unregister_netdevice: waiting for eth3 to become free. Usage count = 2

This was caused by xfrm_dev_policy_add() which increments reference to net_device. That reference was supposed to be decremented in xfrm_dev_policy_free(). However the latter wasn't called.

unregister_netdevice: waiting for eth3 to become free. Usage count = 2 leaked reference.

  xfrm_dev_policy_add+0xff/0x3d0
  xfrm_policy_construct+0x352/0x420
  xfrm_add_policy+0x179/0x320
  xfrm_user_rcv_msg+0x1d2/0x3d0
  netlink_rcv_skb+0xe0/0x210
  xfrm_netlink_rcv+0x45/0x50
  netlink_unicast+0x346/0x490
  netlink_sendmsg+0x3b0/0x6c0
  sock_sendmsg+0x73/0xc0
  sock_write_iter+0x13b/0x1f0
  vfs_write+0x528/0x5d0
  ksys_write+0x120/0x150
  do_syscall_64+0x3d/0x90
  entry_SYSCALL_64_after_hwframe+0x46/0xb0
Fixes 919e43fad516 ("xfrm: add an interface to offload policy")
Signed-off-by Leon Romanovsky <[email protected]>
---
 net/xfrm/xfrm_user.c | 1 +
 1 file changed, 1 insertion(+)
net/xfrm/xfrm_user.c +1 -0
index d720e163ae6e..0e398a589536 100644
@@ -1980,6 +1980,7 @@ static int xfrm_add_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	if (err) {
 		xfrm_dev_policy_delete(xp);
+		xfrm_dev_policy_free(xp);
 		security_xfrm_policy_free(xp->security);
 		kfree(xp);
 		return err;
Signature · 1 line
2.40.0
Reviewed-by on v1 1/2
#2 lore raw

Re: [PATCH xfrm 1/2] xfrm: release all offloaded policy memory

Quoted 28 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Failure to add offloaded policy will cause to the following error once user will try to reload driver.

Unregister_netdevice: waiting for eth3 to become free. Usage count = 2

This was caused by xfrm_dev_policy_add() which increments reference to net_device. That reference was supposed to be decremented in xfrm_dev_policy_free(). However the latter wasn't called.

unregister_netdevice: waiting for eth3 to become free. Usage count = 2 leaked reference.

  xfrm_dev_policy_add+0xff/0x3d0
  xfrm_policy_construct+0x352/0x420
  xfrm_add_policy+0x179/0x320
  xfrm_user_rcv_msg+0x1d2/0x3d0
  netlink_rcv_skb+0xe0/0x210
  xfrm_netlink_rcv+0x45/0x50
  netlink_unicast+0x346/0x490
  netlink_sendmsg+0x3b0/0x6c0
  sock_sendmsg+0x73/0xc0
  sock_write_iter+0x13b/0x1f0
  vfs_write+0x528/0x5d0
  ksys_write+0x120/0x150
  do_syscall_64+0x3d/0x90
  entry_SYSCALL_64_after_hwframe+0x46/0xb0
Quoted 2 lines
Fixes 919e43fad516 ("xfrm: add an interface to offload policy")
Signed-off-by Leon Romanovsky <[email protected]>
Reviewed-by Simon Horman <[email protected]>
Reviewed-by on v1 1/2
#3 lore raw

Re: [PATCH xfrm 1/2] xfrm: release all offloaded policy memory

In reply to Leon Romanovsky
Quoted 29 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Failure to add offloaded policy will cause to the following error once user will try to reload driver.

Unregister_netdevice: waiting for eth3 to become free. Usage count = 2

This was caused by xfrm_dev_policy_add() which increments reference to net_device. That reference was supposed to be decremented in xfrm_dev_policy_free(). However the latter wasn't called.

unregister_netdevice: waiting for eth3 to become free. Usage count = 2 leaked reference.

  xfrm_dev_policy_add+0xff/0x3d0
  xfrm_policy_construct+0x352/0x420
  xfrm_add_policy+0x179/0x320
  xfrm_user_rcv_msg+0x1d2/0x3d0
  netlink_rcv_skb+0xe0/0x210
  xfrm_netlink_rcv+0x45/0x50
  netlink_unicast+0x346/0x490
  netlink_sendmsg+0x3b0/0x6c0
  sock_sendmsg+0x73/0xc0
  sock_write_iter+0x13b/0x1f0
  vfs_write+0x528/0x5d0
  ksys_write+0x120/0x150
  do_syscall_64+0x3d/0x90
  entry_SYSCALL_64_after_hwframe+0x46/0xb0
Quoted 3 lines
Fixes 919e43fad516 ("xfrm: add an interface to offload policy")
Signed-off-by Leon Romanovsky <[email protected]>

---

Reviewed-by Eric Dumazet <[email protected]>

While reviewing this patch, I also saw xfrm_dev_policy_add() could use GFP_KERNEL ?

net/xfrm/xfrm_device.c +0 -0
index bef28c6187ebdd0cfc34c8594aab96ac0b13dd24..508c96c90b3911eb88063ad680c77af2b317c95f

100644

net/xfrm/xfrm_device.c +0 -0
@@ -363,7 +363,7 @@ int xfrm_dev_policy_add(struct net *net, struct

xfrm_policy *xp,

        }

        xdo->dev = dev;
-       netdev_tracker_alloc(dev, &xdo->dev_tracker, GFP_ATOMIC);
+       netdev_tracker_alloc(dev, &xdo->dev_tracker, GFP_KERNEL);
        xdo->real_dev = dev;
        xdo->type = XFRM_DEV_OFFLOAD_PACKET;
        switch (dir) {
author
#4 lore raw

Re: [PATCH xfrm 1/2] xfrm: release all offloaded policy memory

Quoted 30 lines · Eric Dumazet
Quoted 29 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Failure to add offloaded policy will cause to the following error once user will try to reload driver.

Unregister_netdevice: waiting for eth3 to become free. Usage count = 2

This was caused by xfrm_dev_policy_add() which increments reference to net_device. That reference was supposed to be decremented in xfrm_dev_policy_free(). However the latter wasn't called.

unregister_netdevice: waiting for eth3 to become free. Usage count = 2 leaked reference.

  xfrm_dev_policy_add+0xff/0x3d0
  xfrm_policy_construct+0x352/0x420
  xfrm_add_policy+0x179/0x320
  xfrm_user_rcv_msg+0x1d2/0x3d0
  netlink_rcv_skb+0xe0/0x210
  xfrm_netlink_rcv+0x45/0x50
  netlink_unicast+0x346/0x490
  netlink_sendmsg+0x3b0/0x6c0
  sock_sendmsg+0x73/0xc0
  sock_write_iter+0x13b/0x1f0
  vfs_write+0x528/0x5d0
  ksys_write+0x120/0x150
  do_syscall_64+0x3d/0x90
  entry_SYSCALL_64_after_hwframe+0x46/0xb0
Quoted 8 lines
Quoted 3 lines
Fixes 919e43fad516 ("xfrm: add an interface to offload policy")
Signed-off-by Leon Romanovsky <[email protected]>

---

Reviewed-by Eric Dumazet <[email protected]>

While reviewing this patch, I also saw xfrm_dev_policy_add() could use GFP_KERNEL ?

netdev_tracker_alloc(...) line was copied from commit e1b539bd73a7 ("xfrm: add net device refcount tracker to struct xfrm_state_offload")

Thanks

Quoted 16 lines
net/xfrm/xfrm_device.c +0 -0
index bef28c6187ebdd0cfc34c8594aab96ac0b13dd24..508c96c90b3911eb88063ad680c77af2b317c95f

100644

net/xfrm/xfrm_device.c +1 -1
@@ -363,7 +363,7 @@ int xfrm_dev_policy_add(struct net *net, struct
 xfrm_policy *xp,
        }
 
        xdo->dev = dev;
-       netdev_tracker_alloc(dev, &xdo->dev_tracker, GFP_ATOMIC);
+       netdev_tracker_alloc(dev, &xdo->dev_tracker, GFP_KERNEL);
        xdo->real_dev = dev;
        xdo->type = XFRM_DEV_OFFLOAD_PACKET;

        switch (dir) {
#5 lore raw

Re: [PATCH xfrm 1/2] xfrm: release all offloaded policy memory

Quoted 38 lines · Leon Romanovsky
Quoted 36 lines · Eric Dumazet
Quoted 29 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Failure to add offloaded policy will cause to the following error once user will try to reload driver.

Unregister_netdevice: waiting for eth3 to become free. Usage count = 2

This was caused by xfrm_dev_policy_add() which increments reference to net_device. That reference was supposed to be decremented in xfrm_dev_policy_free(). However the latter wasn't called.

unregister_netdevice: waiting for eth3 to become free. Usage count = 2 leaked reference.

  xfrm_dev_policy_add+0xff/0x3d0
  xfrm_policy_construct+0x352/0x420
  xfrm_add_policy+0x179/0x320
  xfrm_user_rcv_msg+0x1d2/0x3d0
  netlink_rcv_skb+0xe0/0x210
  xfrm_netlink_rcv+0x45/0x50
  netlink_unicast+0x346/0x490
  netlink_sendmsg+0x3b0/0x6c0
  sock_sendmsg+0x73/0xc0
  sock_write_iter+0x13b/0x1f0
  vfs_write+0x528/0x5d0
  ksys_write+0x120/0x150
  do_syscall_64+0x3d/0x90
  entry_SYSCALL_64_after_hwframe+0x46/0xb0
Quoted 3 lines
Fixes 919e43fad516 ("xfrm: add an interface to offload policy")
Signed-off-by Leon Romanovsky <[email protected]>

---

Reviewed-by Eric Dumazet <[email protected]>
Quoted 7 lines
Quoted 2 lines

While reviewing this patch, I also saw xfrm_dev_policy_add() could use GFP_KERNEL ?

netdev_tracker_alloc(...) line was copied from commit e1b539bd73a7 ("xfrm: add net device refcount tracker to struct xfrm_state_offload")

Thanks

Then I guess Steffen can fix both call sites...

maintainer
#6 lore raw

Re: [PATCH xfrm 1/2] xfrm: release all offloaded policy memory

Quoted 42 lines · Eric Dumazet
Quoted 41 lines · Leon Romanovsky
Quoted 38 lines · Eric Dumazet
Quoted 29 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Failure to add offloaded policy will cause to the following error once user will try to reload driver.

Unregister_netdevice: waiting for eth3 to become free. Usage count = 2

This was caused by xfrm_dev_policy_add() which increments reference to net_device. That reference was supposed to be decremented in xfrm_dev_policy_free(). However the latter wasn't called.

unregister_netdevice: waiting for eth3 to become free. Usage count = 2 leaked reference.

  xfrm_dev_policy_add+0xff/0x3d0
  xfrm_policy_construct+0x352/0x420
  xfrm_add_policy+0x179/0x320
  xfrm_user_rcv_msg+0x1d2/0x3d0
  netlink_rcv_skb+0xe0/0x210
  xfrm_netlink_rcv+0x45/0x50
  netlink_unicast+0x346/0x490
  netlink_sendmsg+0x3b0/0x6c0
  sock_sendmsg+0x73/0xc0
  sock_write_iter+0x13b/0x1f0
  vfs_write+0x528/0x5d0
  ksys_write+0x120/0x150
  do_syscall_64+0x3d/0x90
  entry_SYSCALL_64_after_hwframe+0x46/0xb0
Quoted 3 lines
Fixes 919e43fad516 ("xfrm: add an interface to offload policy")
Signed-off-by Leon Romanovsky <[email protected]>

---

Reviewed-by Eric Dumazet <[email protected]>

While reviewing this patch, I also saw xfrm_dev_policy_add() could use GFP_KERNEL ?

Quoted 6 lines
Quoted 4 lines

netdev_tracker_alloc(...) line was copied from commit e1b539bd73a7 ("xfrm: add net device refcount tracker to struct xfrm_state_offload")

Thanks

Then I guess Steffen can fix both call sites...

Sure, will do a patch.

Thanks!

author
#7 lore raw

PATCH 2/2 xfrm: Fix leak of dev tracker

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

At the stage of direction checks, the netdev reference tracker is already initialized, but released with wrong *_put() call.

Fixes 919e43fad516 ("xfrm: add an interface to offload policy")
Signed-off-by Leon Romanovsky <[email protected]>
---
 net/xfrm/xfrm_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/xfrm/xfrm_device.c +1 -1
index bef28c6187eb..408f5e55744e 100644
@@ -378,7 +378,7 @@ int xfrm_dev_policy_add(struct net *net, struct xfrm_policy *xp,
 		break;
 	default:
 		xdo->dev = NULL;
-		dev_put(dev);
+		netdev_put(dev, &xdo->dev_tracker);
 		NL_SET_ERR_MSG(extack, "Unrecognized offload direction");
 		return -EINVAL;
 	}
Signature · 1 line
2.40.0
Reviewed-by on v1 2/2
#9 lore raw

Re: [PATCH xfrm 2/2] xfrm: Fix leak of dev tracker

In reply to Leon Romanovsky
Quoted 8 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

At the stage of direction checks, the netdev reference tracker is already initialized, but released with wrong *_put() call.

Fixes 919e43fad516 ("xfrm: add an interface to offload policy")
Signed-off-by Leon Romanovsky <[email protected]>
Reviewed-by Eric Dumazet <[email protected]>