[PATCH] xfrm: fix xfrm_dev_event() missing when compile without CONFIG_XFRM_OFFLOAD

[PATCH] xfrm: fix xfrm_dev_event() missing when compile without CONFIG_XFRM_OFFLOAD

[Hangbin Liu]

In commit d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API") we
make xfrm_device.o only compiled when enable option CONFIG_XFRM_OFFLOAD.
But this will make xfrm_dev_event() missing if we only enable default XFRM
options.

So when we set down and unregister an interface with IPsec on it. there
will no xfrm_garbage_collect(), which will cause dev usage count hold and
get error like:

unregister_netdevice: waiting for <dev> to become free. Usage count = 4

Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/xfrm/Makefile      | 3 +--
 net/xfrm/xfrm_device.c | 2 ++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/Makefile b/net/xfrm/Makefile
index abf81b3..55b2ac3 100644
--- a/net/xfrm/Makefile
+++ b/net/xfrm/Makefile
@@ -4,8 +4,7 @@
 
 obj-$(CONFIG_XFRM) := xfrm_policy.o xfrm_state.o xfrm_hash.o \
 		      xfrm_input.o xfrm_output.o \
-		      xfrm_sysctl.o xfrm_replay.o
-obj-$(CONFIG_XFRM_OFFLOAD) += xfrm_device.o
+		      xfrm_sysctl.o xfrm_replay.o xfrm_device.o
 obj-$(CONFIG_XFRM_STATISTICS) += xfrm_proc.o
 obj-$(CONFIG_XFRM_ALGO) += xfrm_algo.o
 obj-$(CONFIG_XFRM_USER) += xfrm_user.o
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 574e6f3..5aba036 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -22,6 +22,7 @@
 #include <net/xfrm.h>
 #include <linux/notifier.h>
 
+#ifdef CONFIG_XFRM_OFFLOAD
 int validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t features)
 {
 	int err;
@@ -137,6 +138,7 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
 	return true;
 }
 EXPORT_SYMBOL_GPL(xfrm_dev_offload_ok);
+#endif
 
 int xfrm_dev_register(struct net_device *dev)
 {
-- 
2.5.5

Re: [PATCH] xfrm: fix xfrm_dev_event() missing when compile without CONFIG_XFRM_OFFLOAD

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2831 bytes --]

Hi Hangbin,

[auto build test ERROR on ipsec/master]
[also build test ERROR on v4.12-rc3 next-20170531]
[cannot apply to ipsec-next/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hangbin-Liu/xfrm-fix-xfrm_dev_event-missing-when-compile-without-CONFIG_XFRM_OFFLOAD/20170601-112559
base:   https://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git master
config: x86_64-randconfig-x014-201722 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> net//xfrm/xfrm_device.c:207:17: error: redefinition of 'xfrm_dev_init'
    void __net_init xfrm_dev_init(void)
                    ^~~~~~~~~~~~~
   In file included from net//xfrm/xfrm_device.c:22:0:
   include/net/xfrm.h:1880:31: note: previous definition of 'xfrm_dev_init' was here
    static inline void __net_init xfrm_dev_init(void)
                                  ^~~~~~~~~~~~~

vim +/xfrm_dev_init +207 net//xfrm/xfrm_device.c

d77e38e6 Steffen Klassert 2017-04-14  191  	case NETDEV_UNREGISTER:
d77e38e6 Steffen Klassert 2017-04-14  192  		return xfrm_dev_unregister(dev);
d77e38e6 Steffen Klassert 2017-04-14  193  
d77e38e6 Steffen Klassert 2017-04-14  194  	case NETDEV_FEAT_CHANGE:
d77e38e6 Steffen Klassert 2017-04-14  195  		return xfrm_dev_feat_change(dev);
d77e38e6 Steffen Klassert 2017-04-14  196  
21f42cc9 Steffen Klassert 2017-04-14  197  	case NETDEV_DOWN:
d77e38e6 Steffen Klassert 2017-04-14  198  		return xfrm_dev_down(dev);
21f42cc9 Steffen Klassert 2017-04-14  199  	}
21f42cc9 Steffen Klassert 2017-04-14  200  	return NOTIFY_DONE;
21f42cc9 Steffen Klassert 2017-04-14  201  }
21f42cc9 Steffen Klassert 2017-04-14  202  
21f42cc9 Steffen Klassert 2017-04-14  203  static struct notifier_block xfrm_dev_notifier = {
21f42cc9 Steffen Klassert 2017-04-14  204  	.notifier_call	= xfrm_dev_event,
21f42cc9 Steffen Klassert 2017-04-14  205  };
21f42cc9 Steffen Klassert 2017-04-14  206  
21f42cc9 Steffen Klassert 2017-04-14 @207  void __net_init xfrm_dev_init(void)
21f42cc9 Steffen Klassert 2017-04-14  208  {
21f42cc9 Steffen Klassert 2017-04-14  209  	register_netdevice_notifier(&xfrm_dev_notifier);
21f42cc9 Steffen Klassert 2017-04-14  210  }

:::::: The code at line 207 was first introduced by commit
:::::: 21f42cc95f07c1d7827b339c04442e147411e44b xfrm: Move device notifications to a sepatate file

:::::: TO: Steffen Klassert <steffen.klassert@secunet.com>
:::::: CC: Steffen Klassert <steffen.klassert@secunet.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28417 bytes --]

[PATCH v2] xfrm: fix xfrm_dev_event() missing when compile without CONFIG_XFRM_OFFLOAD

[Hangbin Liu]

In commit d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API") we
make xfrm_device.o only compiled when enable option CONFIG_XFRM_OFFLOAD.
But this will make xfrm_dev_event() missing if we only enable default XFRM
options.

Then if we set down and unregister an interface with IPsec on it. there
will no xfrm_garbage_collect(), which will cause dev usage count hold and
get error like:

unregister_netdevice: waiting for <dev> to become free. Usage count = 4

Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/net/xfrm.h     | 7 ++-----
 net/xfrm/Makefile      | 3 +--
 net/xfrm/xfrm_device.c | 2 ++
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 7e7e2b0..62f5a25 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1850,8 +1850,9 @@ static inline struct xfrm_offload *xfrm_offload(struct sk_buff *skb)
 }
 #endif
 
-#ifdef CONFIG_XFRM_OFFLOAD
 void __net_init xfrm_dev_init(void);
+
+#ifdef CONFIG_XFRM_OFFLOAD
 int validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t features);
 int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 		       struct xfrm_user_offload *xuo);
@@ -1877,10 +1878,6 @@ static inline void xfrm_dev_state_free(struct xfrm_state *x)
 	}
 }
 #else
-static inline void __net_init xfrm_dev_init(void)
-{
-}
-
 static inline int validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t features)
 {
 	return 0;
diff --git a/net/xfrm/Makefile b/net/xfrm/Makefile
index abf81b3..55b2ac3 100644
--- a/net/xfrm/Makefile
+++ b/net/xfrm/Makefile
@@ -4,8 +4,7 @@
 
 obj-$(CONFIG_XFRM) := xfrm_policy.o xfrm_state.o xfrm_hash.o \
 		      xfrm_input.o xfrm_output.o \
-		      xfrm_sysctl.o xfrm_replay.o
-obj-$(CONFIG_XFRM_OFFLOAD) += xfrm_device.o
+		      xfrm_sysctl.o xfrm_replay.o xfrm_device.o
 obj-$(CONFIG_XFRM_STATISTICS) += xfrm_proc.o
 obj-$(CONFIG_XFRM_ALGO) += xfrm_algo.o
 obj-$(CONFIG_XFRM_USER) += xfrm_user.o
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 574e6f3..5aba036 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -22,6 +22,7 @@
 #include <net/xfrm.h>
 #include <linux/notifier.h>
 
+#ifdef CONFIG_XFRM_OFFLOAD
 int validate_xmit_xfrm(struct sk_buff *skb, netdev_features_t features)
 {
 	int err;
@@ -137,6 +138,7 @@ bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x)
 	return true;
 }
 EXPORT_SYMBOL_GPL(xfrm_dev_offload_ok);
+#endif
 
 int xfrm_dev_register(struct net_device *dev)
 {
-- 
2.5.5

Re: [PATCH v2] xfrm: fix xfrm_dev_event() missing when compile without CONFIG_XFRM_OFFLOAD

[Steffen Klassert]

On Thu, Jun 01, 2017 at 02:57:56PM +0800, Hangbin Liu wrote:
> In commit d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API") we
> make xfrm_device.o only compiled when enable option CONFIG_XFRM_OFFLOAD.
> But this will make xfrm_dev_event() missing if we only enable default XFRM
> options.
> 
> Then if we set down and unregister an interface with IPsec on it.

You should not be able to register an interface with IPsec offload
without CONFIG_XFRM_OFFLOAD.

> there
> will no xfrm_garbage_collect(), which will cause dev usage count hold and
> get error like:
> 
> unregister_netdevice: waiting for <dev> to become free. Usage count = 4

Can you explain how to reproduce this?

Re: [PATCH v2] xfrm: fix xfrm_dev_event() missing when compile without CONFIG_XFRM_OFFLOAD

[Hangbin Liu]

On Tue, Jun 06, 2017 at 10:06:58AM +0200, Steffen Klassert wrote:
> On Thu, Jun 01, 2017 at 02:57:56PM +0800, Hangbin Liu wrote:
> > In commit d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API") we
> > make xfrm_device.o only compiled when enable option CONFIG_XFRM_OFFLOAD.
> > But this will make xfrm_dev_event() missing if we only enable default XFRM
> > options.
> > 
> > Then if we set down and unregister an interface with IPsec on it.
> 
> You should not be able to register an interface with IPsec offload
> without CONFIG_XFRM_OFFLOAD.

Yes, I mean when compile with default CONFIG_XFRM, the xfrm_dev_event() ->
xfrm_dev_down() -> xfrm_garbage_collect() will missing.

> 
> > there
> > will no xfrm_garbage_collect(), which will cause dev usage count hold and
> > get error like:
> > 
> > unregister_netdevice: waiting for <dev> to become free. Usage count = 4
> 
> Can you explain how to reproduce this?
> 

Sure, I didn't try physical drivers, just test latest net-next with
bridge/bonding and could reproduce it everytime.

```
# cat rep.sh
#!/bin/bash
iface=$1
src=$2
dst=$3
run=$4

brctl addbr br0
brctl addif br0 ${iface}
ip link set ${iface} up
ip link set br0 up
ip addr add ${src}/24 dev br0
ip xfrm state flush && ip xfrm policy flush
ip xfrm state add src ${src} dst ${dst} spi 1000 proto esp enc des3_ede _I_want_to_have_chicken_ auth sha1 beef_fish_pork_salad mode transport
ip xfrm state add src ${dst} dst ${src} spi 1000 proto esp enc des3_ede _I_want_to_have_chicken_ auth sha1 beef_fish_pork_salad mode transport
ip xfrm policy add src ${src} dst ${dst} dir out tmpl proto esp spi 1000 mode transport
ip xfrm policy add src ${dst} dst ${src} dir in tmpl proto esp spi 1000 mode transport

if [ "$run" ]; then
        sleep 1
        ping ${dst} -c 4
        ip link set br0 down
        ip link del br0
fi
```

On host A run : # ./rep.sh eth1 192.168.1.1 192.168.1.2
On host B run : # ./rep.sh eth1 192.168.1.2 192.168.1.1 run

Then we will see error like
kernel:unregister_netdevice: waiting for br0 to become free. Usage count = 3

Thanks
Hangbin

Re: [PATCH v2] xfrm: fix xfrm_dev_event() missing when compile without CONFIG_XFRM_OFFLOAD

[Steffen Klassert]

On Tue, Jun 06, 2017 at 05:26:01PM +0800, Hangbin Liu wrote:
> On Tue, Jun 06, 2017 at 10:06:58AM +0200, Steffen Klassert wrote:
> > On Thu, Jun 01, 2017 at 02:57:56PM +0800, Hangbin Liu wrote:
> > > In commit d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API") we
> > > make xfrm_device.o only compiled when enable option CONFIG_XFRM_OFFLOAD.
> > > But this will make xfrm_dev_event() missing if we only enable default XFRM
> > > options.
> > > 
> > > Then if we set down and unregister an interface with IPsec on it.
> > 
> > You should not be able to register an interface with IPsec offload
> > without CONFIG_XFRM_OFFLOAD.
> 
> Yes, I mean when compile with default CONFIG_XFRM, the xfrm_dev_event() ->
> xfrm_dev_down() -> xfrm_garbage_collect() will missing.

Ok, I see what you mean now. Thanks for the explanation!

Re: [PATCH v2] xfrm: fix xfrm_dev_event() missing when compile without CONFIG_XFRM_OFFLOAD

[Steffen Klassert]

On Thu, Jun 01, 2017 at 02:57:56PM +0800, Hangbin Liu wrote:
> In commit d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API") we
> make xfrm_device.o only compiled when enable option CONFIG_XFRM_OFFLOAD.
> But this will make xfrm_dev_event() missing if we only enable default XFRM
> options.
> 
> Then if we set down and unregister an interface with IPsec on it. there
> will no xfrm_garbage_collect(), which will cause dev usage count hold and
> get error like:
> 
> unregister_netdevice: waiting for <dev> to become free. Usage count = 4
> 
> Fixes: d77e38e612a0 ("xfrm: Add an IPsec hardware offloading API")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Applied, thanks Hangbin!