[PATCH net-next] ipv6: sr: restruct ifdefines

[PATCH net-next] ipv6: sr: restruct ifdefines

[Hangbin Liu]

There are too many ifdef in IPv6 segment routing code that may cause logic
problems. like commit 160e9d275218 ("ipv6: sr: fix invalid unregister error
path"). To avoid this, the init functions are redefined for both cases. The
code could be more clear after all fidefs are removed.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/net/seg6.h      |  7 +++++++
 include/net/seg6_hmac.h |  7 +++++++
 net/ipv6/seg6.c         | 22 ----------------------
 3 files changed, 14 insertions(+), 22 deletions(-)

diff --git a/include/net/seg6.h b/include/net/seg6.h
index af668f17b398..82b3fbbcbb93 100644
--- a/include/net/seg6.h
+++ b/include/net/seg6.h
@@ -52,10 +52,17 @@ static inline struct seg6_pernet_data *seg6_pernet(struct net *net)
 
 extern int seg6_init(void);
 extern void seg6_exit(void);
+#ifdef CONFIG_IPV6_SEG6_LWTUNNEL
 extern int seg6_iptunnel_init(void);
 extern void seg6_iptunnel_exit(void);
 extern int seg6_local_init(void);
 extern void seg6_local_exit(void);
+#else
+static inline int seg6_iptunnel_init(void) { return 0; }
+static inline void seg6_iptunnel_exit(void) {}
+static inline int seg6_local_init(void) { return 0; }
+static inline void seg6_local_exit(void) {}
+#endif
 
 extern bool seg6_validate_srh(struct ipv6_sr_hdr *srh, int len, bool reduced);
 extern struct ipv6_sr_hdr *seg6_get_srh(struct sk_buff *skb, int flags);
diff --git a/include/net/seg6_hmac.h b/include/net/seg6_hmac.h
index 2b5d2ee5613e..24f733b3e3fe 100644
--- a/include/net/seg6_hmac.h
+++ b/include/net/seg6_hmac.h
@@ -49,9 +49,16 @@ extern int seg6_hmac_info_del(struct net *net, u32 key);
 extern int seg6_push_hmac(struct net *net, struct in6_addr *saddr,
 			  struct ipv6_sr_hdr *srh);
 extern bool seg6_hmac_validate_skb(struct sk_buff *skb);
+#ifdef CONFIG_IPV6_SEG6_HMAC
 extern int seg6_hmac_init(void);
 extern void seg6_hmac_exit(void);
 extern int seg6_hmac_net_init(struct net *net);
 extern void seg6_hmac_net_exit(struct net *net);
+#else
+static inline int seg6_hmac_init(void) { return 0; }
+static inline void seg6_hmac_exit(void) {}
+static inline int seg6_hmac_net_init(struct net *net) { return 0; }
+static inline void seg6_hmac_net_exit(struct net *net) {}
+#endif
 
 #endif
diff --git a/net/ipv6/seg6.c b/net/ipv6/seg6.c
index a31521e270f7..671aa1706d04 100644
--- a/net/ipv6/seg6.c
+++ b/net/ipv6/seg6.c
@@ -21,9 +21,7 @@
 #include <net/genetlink.h>
 #include <linux/seg6.h>
 #include <linux/seg6_genl.h>
-#ifdef CONFIG_IPV6_SEG6_HMAC
 #include <net/seg6_hmac.h>
-#endif
 
 bool seg6_validate_srh(struct ipv6_sr_hdr *srh, int len, bool reduced)
 {
@@ -437,13 +435,11 @@ static int __net_init seg6_net_init(struct net *net)
 
 	net->ipv6.seg6_data = sdata;
 
-#ifdef CONFIG_IPV6_SEG6_HMAC
 	if (seg6_hmac_net_init(net)) {
 		kfree(rcu_dereference_raw(sdata->tun_src));
 		kfree(sdata);
 		return -ENOMEM;
 	}
-#endif
 
 	return 0;
 }
@@ -452,9 +448,7 @@ static void __net_exit seg6_net_exit(struct net *net)
 {
 	struct seg6_pernet_data *sdata = seg6_pernet(net);
 
-#ifdef CONFIG_IPV6_SEG6_HMAC
 	seg6_hmac_net_exit(net);
-#endif
 
 	kfree(rcu_dereference_raw(sdata->tun_src));
 	kfree(sdata);
@@ -520,7 +514,6 @@ int __init seg6_init(void)
 	if (err)
 		goto out_unregister_pernet;
 
-#ifdef CONFIG_IPV6_SEG6_LWTUNNEL
 	err = seg6_iptunnel_init();
 	if (err)
 		goto out_unregister_genl;
@@ -530,31 +523,20 @@ int __init seg6_init(void)
 		seg6_iptunnel_exit();
 		goto out_unregister_genl;
 	}
-#endif
 
-#ifdef CONFIG_IPV6_SEG6_HMAC
 	err = seg6_hmac_init();
 	if (err)
 		goto out_unregister_iptun;
-#endif
 
 	pr_info("Segment Routing with IPv6\n");
 
 out:
 	return err;
-#ifdef CONFIG_IPV6_SEG6_HMAC
 out_unregister_iptun:
-#ifdef CONFIG_IPV6_SEG6_LWTUNNEL
 	seg6_local_exit();
 	seg6_iptunnel_exit();
-#endif
-#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:
 	unregister_pernet_subsys(&ip6_segments_ops);
 	goto out;
@@ -562,13 +544,9 @@ int __init seg6_init(void)
 
 void seg6_exit(void)
 {
-#ifdef CONFIG_IPV6_SEG6_HMAC
 	seg6_hmac_exit();
-#endif
-#ifdef CONFIG_IPV6_SEG6_LWTUNNEL
 	seg6_local_exit();
 	seg6_iptunnel_exit();
-#endif
 	genl_unregister_family(&seg6_genl_family);
 	unregister_pernet_subsys(&ip6_segments_ops);
 }
-- 
2.43.0

Re: [PATCH net-next] ipv6: sr: restruct ifdefines

[Sabrina Dubroca]

Hi Hangbin,

2024-05-28, 11:25:30 +0800, Hangbin Liu wrote:
> There are too many ifdef in IPv6 segment routing code that may cause logic
> problems. like commit 160e9d275218 ("ipv6: sr: fix invalid unregister error
> path"). To avoid this, the init functions are redefined for both cases. The
> code could be more clear after all fidefs are removed.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

I think this was suggested by Simon?


> @@ -520,7 +514,6 @@ int __init seg6_init(void)
>  	if (err)
>  		goto out_unregister_pernet;
>  

(With a bit more context around this:)

> -#ifdef CONFIG_IPV6_SEG6_LWTUNNEL
>  	err = seg6_iptunnel_init();
>  	if (err)
>  		goto out_unregister_genl;
>  
>  	err = seg6_local_init();
>  	if (err) {
>  		seg6_iptunnel_exit();

With those changes, we don't need this weird partial uninit anymore,
we can just create a new label above the other seg6_iptunnel_exit()
call and jump there directly.

>  		goto out_unregister_genl;
>  	}
> -#endif
>  
> -#ifdef CONFIG_IPV6_SEG6_HMAC
>  	err = seg6_hmac_init();
>  	if (err)
>  		goto out_unregister_iptun;
> -#endif
>  
>  	pr_info("Segment Routing with IPv6\n");
>  
>  out:
>  	return err;
> -#ifdef CONFIG_IPV6_SEG6_HMAC
>  out_unregister_iptun:
> -#ifdef CONFIG_IPV6_SEG6_LWTUNNEL
>  	seg6_local_exit();

[jump here]

>  	seg6_iptunnel_exit();
> -#endif
> -#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:
>  	unregister_pernet_subsys(&ip6_segments_ops);
>  	goto out;

-- 
Sabrina

Re: [PATCH net-next] ipv6: sr: restruct ifdefines

[Hangbin Liu]

On Tue, May 28, 2024 at 12:05:12PM +0200, Sabrina Dubroca wrote:
> Hi Hangbin,
> 
> 2024-05-28, 11:25:30 +0800, Hangbin Liu wrote:
> > There are too many ifdef in IPv6 segment routing code that may cause logic
> > problems. like commit 160e9d275218 ("ipv6: sr: fix invalid unregister error
> > path"). To avoid this, the init functions are redefined for both cases. The
> > code could be more clear after all fidefs are removed.
> > 
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> 
> I think this was suggested by Simon?

Yes, and David Ahern also suggested this. And I thought you also mentioned it?
I was afraid there are too many suggested-by tags here :) I can add them
in next version patch.

> 
> 
> > @@ -520,7 +514,6 @@ int __init seg6_init(void)
> >  	if (err)
> >  		goto out_unregister_pernet;
> >  
> 
> (With a bit more context around this:)
> 
> > -#ifdef CONFIG_IPV6_SEG6_LWTUNNEL
> >  	err = seg6_iptunnel_init();
> >  	if (err)
> >  		goto out_unregister_genl;
> >  
> >  	err = seg6_local_init();
> >  	if (err) {
> >  		seg6_iptunnel_exit();
> 
> With those changes, we don't need this weird partial uninit anymore,
> we can just create a new label above the other seg6_iptunnel_exit()
> call and jump there directly.

Yes.

Thanks
Hangbin

Re: [PATCH net-next] ipv6: sr: restruct ifdefines

[Sabrina Dubroca]

2024-05-28, 21:00:13 +0800, Hangbin Liu wrote:
> On Tue, May 28, 2024 at 12:05:12PM +0200, Sabrina Dubroca wrote:
> > Hi Hangbin,
> > 
> > 2024-05-28, 11:25:30 +0800, Hangbin Liu wrote:
> > > There are too many ifdef in IPv6 segment routing code that may cause logic
> > > problems. like commit 160e9d275218 ("ipv6: sr: fix invalid unregister error
> > > path"). To avoid this, the init functions are redefined for both cases. The
> > > code could be more clear after all fidefs are removed.
> > > 
> > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > 
> > I think this was suggested by Simon?
> 
> Yes, and David Ahern also suggested this. And I thought you also mentioned it?

I don't think I did.

> I was afraid there are too many suggested-by tags here :) I can add them
> in next version patch.

Yes, please add tags for David and Simon.

Thanks!

-- 
Sabrina