ipv6: sr: fix errors during unregister

PATCH Applied Hangbin Liu 4 participants 12 messages May 9, 2024 – May 11, 2024

Subsystems (3)

Networking Network drivers Networking SRv6

lore

Share & tools

lore

Hangbin Liu

author

2024-05-09 13:18 UTC #0 lore raw

v3 cover ipv6: sr: fix errors during unregister

Fix some errors in seg6 unregister path, like missing unregister functions, incorrect unregister order, etc.

v3:
1) fix unreachable code when CONFIG_IPV6_SEG6_LWTUNNEL=n and CONFIG_IPV6_SEG6_HMAC=n (Sabrina Dubroca)
2) separate the patch into 3 small patches since they fix different commits (Simon Horman)

v2: define CONFIG_IPV6_SEG6_LWTUNNEL for label out_unregister_genl (Sabrina Dubroca)

Hangbin Liu (3):
ipv6: sr: add missing seg6_local_exit
ipv6: sr: fix incorrect unregister order
ipv6: sr: fix invalid unregister error path

 net/ipv6/seg6.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Signature · 1 line

2.43.0

Hangbin Liu

author

2024-05-09 13:18 UTC #1 lore raw

PATCH v3 1/3 ipv6: sr: add missing seg6_local_exit

Currently, we only call seg6_local_exit() in seg6_init() if seg6_local_init() failed. But forgot to call it in seg6_exit().

Fixes d1df6fd8a1d2 ("ipv6: sr: define core operations for seg6local lightweight tunnel")

Signed-off-by Hangbin Liu <[email protected]>

---
 net/ipv6/seg6.c | 1 +
 1 file changed, 1 insertion(+)

net/ipv6/seg6.c +1 -0

index 35508abd76f4..5423f1f2aa62 100644
@@ -564,6 +564,7 @@ void seg6_exit(void)
 	seg6_hmac_exit();
 #endif
 #ifdef CONFIG_IPV6_SEG6_LWTUNNEL
+	seg6_local_exit();
 	seg6_iptunnel_exit();
 #endif
 	unregister_pernet_subsys(&ip6_segments_ops);

Signature · 1 line

2.43.0

Sabrina Dubroca

Reviewed-by on v3 1/3

2024-05-10 08:05 UTC #2 lore raw

Re: [PATCHv3 net 1/3] ipv6: sr: add missing seg6_local_exit

Quoted 5 lines · Hangbin Liu

Currently, we only call seg6_local_exit() in seg6_init() if seg6_local_init() failed. But forgot to call it in seg6_exit().
Fixes d1df6fd8a1d2 ("ipv6: sr: define core operations for seg6local lightweight tunnel")
Signed-off-by Hangbin Liu <[email protected]>

Reviewed-by Sabrina Dubroca <[email protected]>

Signature · 1 line

Sabrina

David Ahern

Reviewed-by on v3 1/3

2024-05-11 01:48 UTC #3 lore raw

Re: [PATCHv3 net 1/3] ipv6: sr: add missing seg6_local_exit

In reply to Hangbin Liu

Quoted 9 lines · Hangbin Liu

Currently, we only call seg6_local_exit() in seg6_init() if seg6_local_init() failed. But forgot to call it in seg6_exit().
Fixes d1df6fd8a1d2 ("ipv6: sr: define core operations for seg6local lightweight tunnel")
Signed-off-by Hangbin Liu <[email protected]>
---
 net/ipv6/seg6.c | 1 +
 1 file changed, 1 insertion(+)

Reviewed-by David Ahern <[email protected]>

Hangbin Liu

author

2024-05-09 13:18 UTC #4 lore raw

PATCH v3 2/3 ipv6: sr: fix incorrect unregister order

In reply to Hangbin Liu

Commit 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref") changed the register order in seg6_init(). But the unregister order in seg6_exit() is not updated.

Fixes 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref")

Signed-off-by Hangbin Liu <[email protected]>

---
 net/ipv6/seg6.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/ipv6/seg6.c +1 -1

index 5423f1f2aa62..c4ef96c8fdac 100644
@@ -567,6 +567,6 @@ void seg6_exit(void)
 	seg6_local_exit();
 	seg6_iptunnel_exit();
 #endif
-	unregister_pernet_subsys(&ip6_segments_ops);
 	genl_unregister_family(&seg6_genl_family);
+	unregister_pernet_subsys(&ip6_segments_ops);
 }

Signature · 1 line

2.43.0

Sabrina Dubroca

Reviewed-by on v3 2/3

2024-05-10 08:08 UTC #5 lore raw

Re: [PATCHv3 net 2/3] ipv6: sr: fix incorrect unregister order

Quoted 6 lines · Hangbin Liu

Commit 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref") changed the register order in seg6_init(). But the unregister order in seg6_exit() is not updated.
Fixes 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref")
Signed-off-by Hangbin Liu <[email protected]>

Reviewed-by Sabrina Dubroca <[email protected]>

Signature · 1 line

Sabrina

David Ahern

Reviewed-by on v3 2/3

2024-05-11 01:49 UTC #6 lore raw

Re: [PATCHv3 net 2/3] ipv6: sr: fix incorrect unregister order

In reply to Hangbin Liu

Quoted 10 lines · Hangbin Liu

Commit 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref") changed the register order in seg6_init(). But the unregister order in seg6_exit() is not updated.
Fixes 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref")
Signed-off-by Hangbin Liu <[email protected]>
---
 net/ipv6/seg6.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by David Ahern <[email protected]>

Hangbin Liu

author

2024-05-09 13:18 UTC #7 lore raw

PATCH v3 3/3 ipv6: sr: fix invalid unregister error path

In reply to Hangbin Liu

The error path of seg6_init() is wrong in case CONFIG_IPV6_SEG6_LWTUNNEL is not defined. In that case if seg6_hmac_init() fails, the genl_unregister_family() isn't called.

This issue exist since commit 46738b1317e1 ("ipv6: sr: add option to control lwtunnel support"), and commit 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref") replaced unregister_pernet_subsys() with genl_unregister_family() in this error path.

Fixes 46738b1317e1 ("ipv6: sr: add option to control lwtunnel support")

Reported-by Guillaume Nault <[email protected]>

Signed-off-by Hangbin Liu <[email protected]>

---
 net/ipv6/seg6.c | 2 ++
 1 file changed, 2 insertions(+)

net/ipv6/seg6.c +2 -0

index c4ef96c8fdac..a31521e270f7 100644
@@ -551,6 +551,8 @@ int __init seg6_init(void)
 #endif
 #ifdef CONFIG_IPV6_SEG6_LWTUNNEL
 out_unregister_genl:
+#endif
+#if IS_ENABLED(CONFIG_IPV6_SEG6_LWTUNNEL) || IS_ENABLED(CONFIG_IPV6_SEG6_HMAC)
 	genl_unregister_family(&seg6_genl_family);
 #endif
 out_unregister_pernet:

Signature · 1 line

2.43.0

Sabrina Dubroca

Reviewed-by on v3 3/3

2024-05-10 08:32 UTC #8 lore raw

Re: [PATCHv3 net 3/3] ipv6: sr: fix invalid unregister error path

Quoted 4 lines · Hangbin Liu

The error path of seg6_init() is wrong in case CONFIG_IPV6_SEG6_LWTUNNEL is not defined. In that case if seg6_hmac_init() fails, the genl_unregister_family() isn't called.

Quoted 8 lines

This issue exist since commit 46738b1317e1 ("ipv6: sr: add option to control lwtunnel support"), and commit 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref") replaced unregister_pernet_subsys() with genl_unregister_family() in this error path.
Fixes 46738b1317e1 ("ipv6: sr: add option to control lwtunnel support")
Reported-by Guillaume Nault <[email protected]>
Signed-off-by Hangbin Liu <[email protected]>

Reviewed-by Sabrina Dubroca <[email protected]>

seg6_hmac_init_algo also returns without cleaning up the previous allocations if one fails, so it's going to leak all that memory and the crypto tfms.

Signature · 1 line

Sabrina

David Ahern

Reviewed-by on v3 3/3

2024-05-11 01:52 UTC #9 lore raw

Re: [PATCHv3 net 3/3] ipv6: sr: fix invalid unregister error path

In reply to Hangbin Liu

Quoted 21 lines · Hangbin Liu

The error path of seg6_init() is wrong in case CONFIG_IPV6_SEG6_LWTUNNEL is not defined. In that case if seg6_hmac_init() fails, the genl_unregister_family() isn't called.
This issue exist since commit 46738b1317e1 ("ipv6: sr: add option to control lwtunnel support"), and commit 5559cea2d5aa ("ipv6: sr: fix possible use-after-free and null-ptr-deref") replaced unregister_pernet_subsys() with genl_unregister_family() in this error path.
Fixes 46738b1317e1 ("ipv6: sr: add option to control lwtunnel support")
Reported-by Guillaume Nault <[email protected]>
Signed-off-by Hangbin Liu <[email protected]>
---
 net/ipv6/seg6.c | 2 ++
 1 file changed, 2 insertions(+)
net/ipv6/seg6.c +0 -0
index c4ef96c8fdac..a31521e270f7 100644

net/ipv6/seg6.c int __init seg6_init(void) · line 558

Quoted 8 lines

net/ipv6/seg6.c +2 -0

@@ -551,6 +551,8 @@ int __init seg6_init(void)
 #endif
 #ifdef CONFIG_IPV6_SEG6_LWTUNNEL
 out_unregister_genl:
+#endif
+#if IS_ENABLED(CONFIG_IPV6_SEG6_LWTUNNEL) || IS_ENABLED(CONFIG_IPV6_SEG6_HMAC)
 	genl_unregister_family(&seg6_genl_family);
 #endif
 out_unregister_pernet:

a good example of why ifdef's create problems. It would have been simpler if all of those init functions were defined for both cases and this function does not need the '#if' spaghetti.

Reviewed-by David Ahern <[email protected]>

Hangbin Liu

author

2024-05-11 07:42 UTC #10 lore raw

Re: [PATCHv3 net 3/3] ipv6: sr: fix invalid unregister error path

Quoted 3 lines · David Ahern

a good example of why ifdef's create problems. It would have been simpler if all of those init functions were defined for both cases and this function does not need the '#if' spaghetti.

Yes, I will post a restruct patch for net-next.

Thanks
Hangbin

[email protected]

CI Applied

2024-05-11 02:30 UTC #11 lore raw

In reply to Hangbin Liu

This series was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:

Quoted 10 lines · On Thu, 9 May 2024 21:18:09 +0800 you wrote:

Fix some errors in seg6 unregister path, like missing unregister functions, incorrect unregister order, etc.
v3:
1) fix unreachable code when CONFIG_IPV6_SEG6_LWTUNNEL=n and CONFIG_IPV6_SEG6_HMAC=n (Sabrina Dubroca)
2) separate the patch into 3 small patches since they fix different commits (Simon Horman)
[...]

Here is the summary with links:
- [PATCHv3,net,1/3] ipv6: sr: add missing seg6_local_exit

    https://git.kernel.org/netdev/net/c/3321687e3213
  - [PATCHv3,net,2/3] ipv6: sr: fix incorrect unregister order
    https://git.kernel.org/netdev/net/c/6e370a771d29
  - [PATCHv3,net,3/3] ipv6: sr: fix invalid unregister error path
    https://git.kernel.org/netdev/net/c/160e9d275218

You are awesome, thank you!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html