[PATCH net 0/2] net: Fix oops on state free after lwt module unload

[PATCH net 0/2] net: Fix oops on state free after lwt module unload

[Robert Shearman]

This patchset fixes an oops in lwtstate_free and a memory leak that
would otherwise be exposed by ensuring that references are taken on
modules that need to stay around to clean up lwt state. To faciliate
this all ops that implement destroy_state and that can be configured
to build as a module are changed specify the owner module in the
ops. The intersection of those two sets is just ila at the moment.

Robert Shearman (2):
  lwtunnel: Fix oops on state free after encap module unload
  ila: Fix memory leak of lwt dst cache on module unload

 include/net/lwtunnel.h |  2 ++
 net/core/lwtunnel.c    | 11 +++++++++--
 net/ipv6/ila/ila_lwt.c |  1 +
 3 files changed, 12 insertions(+), 2 deletions(-)

-- 
2.1.4

[PATCH net 1/2] lwtunnel: Fix oops on state free after encap module unload

[Robert Shearman]

When attempting to free lwtunnel state after the module for the encap
has been unloaded an oops occurs:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: lwtstate_free+0x18/0x40
[..]
task: ffff88003e372380 task.stack: ffffc900001fc000
RIP: 0010:lwtstate_free+0x18/0x40
RSP: 0018:ffff88003fd83e88 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff88002bbb3380 RCX: ffff88000c91a300
[..]
Call Trace:
 <IRQ>
 free_fib_info_rcu+0x195/0x1a0
 ? rt_fibinfo_free+0x50/0x50
 rcu_process_callbacks+0x2d3/0x850
 ? rcu_process_callbacks+0x296/0x850
 __do_softirq+0xe4/0x4cb
 irq_exit+0xb0/0xc0
 smp_apic_timer_interrupt+0x3d/0x50
 apic_timer_interrupt+0x93/0xa0
[..]
Code: e8 6e c6 fc ff 89 d8 5b 5d c3 bb de ff ff ff eb f4 66 90 66 66 66 66 90 55 48 89 e5 53 0f b7 07 48 89 fb 48 8b 04 c5 00 81 d5 81 <48> 8b 40 08 48 85 c0 74 13 ff d0 48 8d 7b 20 be 20 00 00 00 e8

The problem is that we don't check for NULL ops in
lwtstate_free. Adding the check fixes the immediate problem but will
then won't properly clean up for ops that implement the
->destroy_state function if the implementing module has been unloaded,
resulting in memory leaks or other problems. So in addition, refcount
the module when the ops implements ->destroy_state so it can't be
unloaded while there is still state around.

Fixes: 1104d9ba443a ("lwtunnel: Add destroy state operation")
Signed-off-by: Robert Shearman <rshearma@brocade.com>
---
 include/net/lwtunnel.h |  2 ++
 net/core/lwtunnel.c    | 11 +++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/net/lwtunnel.h b/include/net/lwtunnel.h
index d4c1c75b8862..2b9993f33198 100644
--- a/include/net/lwtunnel.h
+++ b/include/net/lwtunnel.h
@@ -44,6 +44,8 @@ struct lwtunnel_encap_ops {
 	int (*get_encap_size)(struct lwtunnel_state *lwtstate);
 	int (*cmp_encap)(struct lwtunnel_state *a, struct lwtunnel_state *b);
 	int (*xmit)(struct sk_buff *skb);
+
+	struct module *owner;
 };
 
 #ifdef CONFIG_LWTUNNEL
diff --git a/net/core/lwtunnel.c b/net/core/lwtunnel.c
index a5d4e866ce88..3dc3cc3b38ec 100644
--- a/net/core/lwtunnel.c
+++ b/net/core/lwtunnel.c
@@ -126,8 +126,14 @@ int lwtunnel_build_state(struct net_device *dev, u16 encap_type,
 		}
 	}
 #endif
-	if (likely(ops && ops->build_state))
+	/* take module reference if destroy_state is in use */
+	if (unlikely(ops && ops->destroy_state && !try_module_get(ops->owner)))
+		ops = NULL;
+	if (likely(ops && ops->build_state)) {
 		ret = ops->build_state(dev, encap, family, cfg, lws);
+		if (ret && ops->destroy_state)
+			module_put(ops->owner);
+	}
 	rcu_read_unlock();
 
 	return ret;
@@ -138,9 +144,10 @@ void lwtstate_free(struct lwtunnel_state *lws)
 {
 	const struct lwtunnel_encap_ops *ops = lwtun_encaps[lws->type];
 
-	if (ops->destroy_state) {
+	if (ops && ops->destroy_state) {
 		ops->destroy_state(lws);
 		kfree_rcu(lws, rcu);
+		module_put(ops->owner);
 	} else {
 		kfree(lws);
 	}
-- 
2.1.4

[PATCH net 2/2] ila: Fix memory leak of lwt dst cache on module unload

[Robert Shearman]

If routes with lwt state are present when the ila module is unloaded
and then subsequently deleted, the dst cache entry in the state will
be leaked.

Fix this by specifying the owning module in the lwt ops to allow lwt
to take a reference for each route and to keep the module around until
the last ila route is deleted.

Fixes: 79ff2fc31e0f ("ila: Cache a route to translated address")
Signed-off-by: Robert Shearman <rshearma@brocade.com>
---
 net/ipv6/ila/ila_lwt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv6/ila/ila_lwt.c b/net/ipv6/ila/ila_lwt.c
index a7bc54ab46e2..13b5e85fe0d5 100644
--- a/net/ipv6/ila/ila_lwt.c
+++ b/net/ipv6/ila/ila_lwt.c
@@ -238,6 +238,7 @@ static const struct lwtunnel_encap_ops ila_encap_ops = {
 	.fill_encap = ila_fill_encap_info,
 	.get_encap_size = ila_encap_nlsize,
 	.cmp_encap = ila_encap_cmp,
+	.owner = THIS_MODULE,
 };
 
 int ila_lwt_init(void)
-- 
2.1.4

Re: [PATCH net 0/2] net: Fix oops on state free after lwt module unload

[David Miller]

From: Robert Shearman <rshearma@brocade.com>
Date: Wed, 18 Jan 2017 15:32:01 +0000

> This patchset fixes an oops in lwtstate_free and a memory leak that
> would otherwise be exposed by ensuring that references are taken on
> modules that need to stay around to clean up lwt state. To faciliate
> this all ops that implement destroy_state and that can be configured
> to build as a module are changed specify the owner module in the
> ops. The intersection of those two sets is just ila at the moment.

Two things:

1) Under no circumstances should we allow a lwtunnel ops implementing
   module to unload while there is a rule using those ops which is
   alive.

   Therefore, we should not special case the destroy op.  We should
   unconditionally grab the module reference.

2) Please add the new 'owner' field and add an appropriate assignment
   for ops->owner to _every_ lwtunnel implementation, and do so in
   your first patch.  Please do not only do this for ILA.

Thanks.

Re: [PATCH net 0/2] net: Fix oops on state free after lwt module unload

[Robert Shearman]

On 20/01/17 17:03, David Miller wrote:
> From: Robert Shearman <rshearma@brocade.com>
> Date: Wed, 18 Jan 2017 15:32:01 +0000
>
>> This patchset fixes an oops in lwtstate_free and a memory leak that
>> would otherwise be exposed by ensuring that references are taken on
>> modules that need to stay around to clean up lwt state. To faciliate
>> this all ops that implement destroy_state and that can be configured
>> to build as a module are changed specify the owner module in the
>> ops. The intersection of those two sets is just ila at the moment.
>
> Two things:
>
> 1) Under no circumstances should we allow a lwtunnel ops implementing
>    module to unload while there is a rule using those ops which is
>    alive.
>
>    Therefore, we should not special case the destroy op.  We should
>    unconditionally grab the module reference.
>
> 2) Please add the new 'owner' field and add an appropriate assignment
>    for ops->owner to _every_ lwtunnel implementation, and do so in
>    your first patch.  Please do not only do this for ILA.
>
> Thanks.

Very clear, makes sense, will do.

Thanks,
Rob

[PATCH net v2 0/2] net: Fix oops on state free after lwt module unload

[Robert Shearman]

An oops is seen in lwtstate_free after an lwt ops module has been
unloaded. This patchset fixes this by preventing modules implementing
lwtunnel ops from being unloaded whilst there's state alive using
those ops. The first patch adds fills in a new owner field in all lwt
ops and the second patch makes use of this to reference count the
modules as state is built and destroyed using them.

Changes in v2:
 - specify module owner for all modules as suggested by DaveM
 - reference count all modules building lwt state, not just those ops
   implementing destroy_state, as also suggested by DaveM.
 - rebased on top of David Ahern's lwtunnel changes

Robert Shearman (2):
  net: Specify the owning module for lwtunnel ops
  lwtunnel: Fix oops on state free after encap module unload

 include/net/lwtunnel.h    | 2 ++
 net/core/lwt_bpf.c        | 1 +
 net/core/lwtunnel.c       | 9 +++++++--
 net/ipv4/ip_tunnel_core.c | 2 ++
 net/ipv6/ila/ila_lwt.c    | 1 +
 net/ipv6/seg6_iptunnel.c  | 1 +
 net/mpls/mpls_iptunnel.c  | 1 +
 7 files changed, 15 insertions(+), 2 deletions(-)

-- 
2.1.4

[PATCH net v2 1/2] net: Specify the owning module for lwtunnel ops

[Robert Shearman]

Modules implementing lwtunnel ops should not be allowed to unload
while there is state alive using those ops, so specify the owning
module for all lwtunnel ops.

Signed-off-by: Robert Shearman <rshearma@brocade.com>
---
 include/net/lwtunnel.h    | 2 ++
 net/core/lwt_bpf.c        | 1 +
 net/ipv4/ip_tunnel_core.c | 2 ++
 net/ipv6/ila/ila_lwt.c    | 1 +
 net/ipv6/seg6_iptunnel.c  | 1 +
 net/mpls/mpls_iptunnel.c  | 1 +
 6 files changed, 8 insertions(+)

diff --git a/include/net/lwtunnel.h b/include/net/lwtunnel.h
index 0b585f1fd340..73dd87647460 100644
--- a/include/net/lwtunnel.h
+++ b/include/net/lwtunnel.h
@@ -44,6 +44,8 @@ struct lwtunnel_encap_ops {
 	int (*get_encap_size)(struct lwtunnel_state *lwtstate);
 	int (*cmp_encap)(struct lwtunnel_state *a, struct lwtunnel_state *b);
 	int (*xmit)(struct sk_buff *skb);
+
+	struct module *owner;
 };
 
 #ifdef CONFIG_LWTUNNEL
diff --git a/net/core/lwt_bpf.c b/net/core/lwt_bpf.c
index 71bb3e2eca08..b3eef90b2df9 100644
--- a/net/core/lwt_bpf.c
+++ b/net/core/lwt_bpf.c
@@ -386,6 +386,7 @@ static const struct lwtunnel_encap_ops bpf_encap_ops = {
 	.fill_encap	= bpf_fill_encap_info,
 	.get_encap_size = bpf_encap_nlsize,
 	.cmp_encap	= bpf_encap_cmp,
+	.owner		= THIS_MODULE,
 };
 
 static int __init bpf_lwt_init(void)
diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
index fed3d29f9eb3..0fd1976ab63b 100644
--- a/net/ipv4/ip_tunnel_core.c
+++ b/net/ipv4/ip_tunnel_core.c
@@ -313,6 +313,7 @@ static const struct lwtunnel_encap_ops ip_tun_lwt_ops = {
 	.fill_encap = ip_tun_fill_encap_info,
 	.get_encap_size = ip_tun_encap_nlsize,
 	.cmp_encap = ip_tun_cmp_encap,
+	.owner = THIS_MODULE,
 };
 
 static const struct nla_policy ip6_tun_policy[LWTUNNEL_IP6_MAX + 1] = {
@@ -403,6 +404,7 @@ static const struct lwtunnel_encap_ops ip6_tun_lwt_ops = {
 	.fill_encap = ip6_tun_fill_encap_info,
 	.get_encap_size = ip6_tun_encap_nlsize,
 	.cmp_encap = ip_tun_cmp_encap,
+	.owner = THIS_MODULE,
 };
 
 void __init ip_tunnel_core_init(void)
diff --git a/net/ipv6/ila/ila_lwt.c b/net/ipv6/ila/ila_lwt.c
index a7bc54ab46e2..13b5e85fe0d5 100644
--- a/net/ipv6/ila/ila_lwt.c
+++ b/net/ipv6/ila/ila_lwt.c
@@ -238,6 +238,7 @@ static const struct lwtunnel_encap_ops ila_encap_ops = {
 	.fill_encap = ila_fill_encap_info,
 	.get_encap_size = ila_encap_nlsize,
 	.cmp_encap = ila_encap_cmp,
+	.owner = THIS_MODULE,
 };
 
 int ila_lwt_init(void)
diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c
index 1d60cb132835..c46f8cbf5ab5 100644
--- a/net/ipv6/seg6_iptunnel.c
+++ b/net/ipv6/seg6_iptunnel.c
@@ -422,6 +422,7 @@ static const struct lwtunnel_encap_ops seg6_iptun_ops = {
 	.fill_encap = seg6_fill_encap_info,
 	.get_encap_size = seg6_encap_nlsize,
 	.cmp_encap = seg6_encap_cmp,
+	.owner = THIS_MODULE,
 };
 
 int __init seg6_iptunnel_init(void)
diff --git a/net/mpls/mpls_iptunnel.c b/net/mpls/mpls_iptunnel.c
index 2f7ccd934416..1d281c1ff7c1 100644
--- a/net/mpls/mpls_iptunnel.c
+++ b/net/mpls/mpls_iptunnel.c
@@ -215,6 +215,7 @@ static const struct lwtunnel_encap_ops mpls_iptun_ops = {
 	.fill_encap = mpls_fill_encap_info,
 	.get_encap_size = mpls_encap_nlsize,
 	.cmp_encap = mpls_encap_cmp,
+	.owner = THIS_MODULE,
 };
 
 static int __init mpls_iptunnel_init(void)
-- 
2.1.4

[PATCH net v2 2/2] lwtunnel: Fix oops on state free after encap module unload

[Robert Shearman]

When attempting to free lwtunnel state after the module for the encap
has been unloaded an oops occurs:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: lwtstate_free+0x18/0x40
[..]
task: ffff88003e372380 task.stack: ffffc900001fc000
RIP: 0010:lwtstate_free+0x18/0x40
RSP: 0018:ffff88003fd83e88 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff88002bbb3380 RCX: ffff88000c91a300
[..]
Call Trace:
 <IRQ>
 free_fib_info_rcu+0x195/0x1a0
 ? rt_fibinfo_free+0x50/0x50
 rcu_process_callbacks+0x2d3/0x850
 ? rcu_process_callbacks+0x296/0x850
 __do_softirq+0xe4/0x4cb
 irq_exit+0xb0/0xc0
 smp_apic_timer_interrupt+0x3d/0x50
 apic_timer_interrupt+0x93/0xa0
[..]
Code: e8 6e c6 fc ff 89 d8 5b 5d c3 bb de ff ff ff eb f4 66 90 66 66 66 66 90 55 48 89 e5 53 0f b7 07 48 89 fb 48 8b 04 c5 00 81 d5 81 <48> 8b 40 08 48 85 c0 74 13 ff d0 48 8d 7b 20 be 20 00 00 00 e8

The problem is after the module for the encap is unloaded the
corresponding ops is removed and thus is NULL here.

Modules implementing lwtunnel ops should not be allowed to unload
while there is state alive using those ops, so grab the module
reference for the ops on creating lwtunnel state and of course release
the reference when freeing the state.

Fixes: 1104d9ba443a ("lwtunnel: Add destroy state operation")
Signed-off-by: Robert Shearman <rshearma@brocade.com>
---
 net/core/lwtunnel.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/core/lwtunnel.c b/net/core/lwtunnel.c
index 47b1dd65947b..ebfaffaa777b 100644
--- a/net/core/lwtunnel.c
+++ b/net/core/lwtunnel.c
@@ -115,8 +115,12 @@ int lwtunnel_build_state(struct net_device *dev, u16 encap_type,
 	ret = -EOPNOTSUPP;
 	rcu_read_lock();
 	ops = rcu_dereference(lwtun_encaps[encap_type]);
-	if (likely(ops && ops->build_state))
-		ret = ops->build_state(dev, encap, family, cfg, lws);
+	if (likely(ops)) {
+		if (likely(try_module_get(ops->owner) && ops->build_state))
+			ret = ops->build_state(dev, encap, family, cfg, lws);
+		if (ret)
+			module_put(ops->owner);
+	}
 	rcu_read_unlock();
 
 	return ret;
@@ -194,6 +198,7 @@ void lwtstate_free(struct lwtunnel_state *lws)
 	} else {
 		kfree(lws);
 	}
+	module_put(ops->owner);
 }
 EXPORT_SYMBOL(lwtstate_free);
 
-- 
2.1.4

Re: [PATCH net v2 2/2] lwtunnel: Fix oops on state free after encap module unload

[David Miller]

From: Robert Shearman <rshearma@brocade.com>
Date: Sat, 21 Jan 2017 00:21:26 +0000

> @@ -115,8 +115,12 @@ int lwtunnel_build_state(struct net_device *dev, u16 encap_type,
>  	ret = -EOPNOTSUPP;
>  	rcu_read_lock();

Here 'ret' equals -EOPNOTSUPP

>  	ops = rcu_dereference(lwtun_encaps[encap_type]);
> -	if (likely(ops && ops->build_state))
> -		ret = ops->build_state(dev, encap, family, cfg, lws);
> +	if (likely(ops)) {
> +		if (likely(try_module_get(ops->owner) && ops->build_state))
> +			ret = ops->build_state(dev, encap, family, cfg, lws);
> +		if (ret)
> +			module_put(ops->owner);

If try_module_get() fails, 'ret' will still be -EOPNOTSUPP and we will
module_put() on a module we did not grab a reference to.

I think you need to adjust the logic here.  You only want to 'put' if
try_module_get() succeeds and ->build_state() returns an error.

Re: [PATCH net v2 2/2] lwtunnel: Fix oops on state free after encap module unload

[Robert Shearman]

On 23/01/17 20:42, David Miller wrote:
> From: Robert Shearman <rshearma@brocade.com>
> Date: Sat, 21 Jan 2017 00:21:26 +0000
>
>> @@ -115,8 +115,12 @@ int lwtunnel_build_state(struct net_device *dev, u16 encap_type,
>>  	ret = -EOPNOTSUPP;
>>  	rcu_read_lock();
>
> Here 'ret' equals -EOPNOTSUPP
>
>>  	ops = rcu_dereference(lwtun_encaps[encap_type]);
>> -	if (likely(ops && ops->build_state))
>> -		ret = ops->build_state(dev, encap, family, cfg, lws);
>> +	if (likely(ops)) {
>> +		if (likely(try_module_get(ops->owner) && ops->build_state))
>> +			ret = ops->build_state(dev, encap, family, cfg, lws);
>> +		if (ret)
>> +			module_put(ops->owner);
>
> If try_module_get() fails, 'ret' will still be -EOPNOTSUPP and we will
> module_put() on a module we did not grab a reference to.
>
> I think you need to adjust the logic here.  You only want to 'put' if
> try_module_get() succeeds and ->build_state() returns an error.

Indeed, good point. Will address in a v3 shortly.

Thanks,
Rob

[PATCH net v3 0/2] net: Fix oops on state free after lwt module unload

[Robert Shearman]

An oops is seen in lwtstate_free after an lwt ops module has been
unloaded. This patchset fixes this by preventing modules implementing
lwtunnel ops from being unloaded whilst there's state alive using
those ops. The first patch adds fills in a new owner field in all lwt
ops and the second patch makes use of this to reference count the
modules as state is built and destroyed using them.

Changes in v3:
 - don't put module reference if try_module_get fails on building state

Changes in v2:
 - specify module owner for all modules as suggested by DaveM
 - reference count all modules building lwt state, not just those ops
   implementing destroy_state, as also suggested by DaveM.
 - rebased on top of David Ahern's lwtunnel changes

Robert Shearman (2):
  net: Specify the owning module for lwtunnel ops
  lwtunnel: Fix oops on state free after encap module unload

 include/net/lwtunnel.h    | 2 ++
 net/core/lwt_bpf.c        | 1 +
 net/core/lwtunnel.c       | 6 +++++-
 net/ipv4/ip_tunnel_core.c | 2 ++
 net/ipv6/ila/ila_lwt.c    | 1 +
 net/ipv6/seg6_iptunnel.c  | 1 +
 net/mpls/mpls_iptunnel.c  | 1 +
 7 files changed, 13 insertions(+), 1 deletion(-)

-- 
2.1.4

[PATCH net v3 1/2] net: Specify the owning module for lwtunnel ops

[Robert Shearman]

Modules implementing lwtunnel ops should not be allowed to unload
while there is state alive using those ops, so specify the owning
module for all lwtunnel ops.

Signed-off-by: Robert Shearman <rshearma@brocade.com>
---
 include/net/lwtunnel.h    | 2 ++
 net/core/lwt_bpf.c        | 1 +
 net/ipv4/ip_tunnel_core.c | 2 ++
 net/ipv6/ila/ila_lwt.c    | 1 +
 net/ipv6/seg6_iptunnel.c  | 1 +
 net/mpls/mpls_iptunnel.c  | 1 +
 6 files changed, 8 insertions(+)

diff --git a/include/net/lwtunnel.h b/include/net/lwtunnel.h
index 0b585f1fd340..73dd87647460 100644
--- a/include/net/lwtunnel.h
+++ b/include/net/lwtunnel.h
@@ -44,6 +44,8 @@ struct lwtunnel_encap_ops {
 	int (*get_encap_size)(struct lwtunnel_state *lwtstate);
 	int (*cmp_encap)(struct lwtunnel_state *a, struct lwtunnel_state *b);
 	int (*xmit)(struct sk_buff *skb);
+
+	struct module *owner;
 };
 
 #ifdef CONFIG_LWTUNNEL
diff --git a/net/core/lwt_bpf.c b/net/core/lwt_bpf.c
index 71bb3e2eca08..b3eef90b2df9 100644
--- a/net/core/lwt_bpf.c
+++ b/net/core/lwt_bpf.c
@@ -386,6 +386,7 @@ static const struct lwtunnel_encap_ops bpf_encap_ops = {
 	.fill_encap	= bpf_fill_encap_info,
 	.get_encap_size = bpf_encap_nlsize,
 	.cmp_encap	= bpf_encap_cmp,
+	.owner		= THIS_MODULE,
 };
 
 static int __init bpf_lwt_init(void)
diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
index fed3d29f9eb3..0fd1976ab63b 100644
--- a/net/ipv4/ip_tunnel_core.c
+++ b/net/ipv4/ip_tunnel_core.c
@@ -313,6 +313,7 @@ static const struct lwtunnel_encap_ops ip_tun_lwt_ops = {
 	.fill_encap = ip_tun_fill_encap_info,
 	.get_encap_size = ip_tun_encap_nlsize,
 	.cmp_encap = ip_tun_cmp_encap,
+	.owner = THIS_MODULE,
 };
 
 static const struct nla_policy ip6_tun_policy[LWTUNNEL_IP6_MAX + 1] = {
@@ -403,6 +404,7 @@ static const struct lwtunnel_encap_ops ip6_tun_lwt_ops = {
 	.fill_encap = ip6_tun_fill_encap_info,
 	.get_encap_size = ip6_tun_encap_nlsize,
 	.cmp_encap = ip_tun_cmp_encap,
+	.owner = THIS_MODULE,
 };
 
 void __init ip_tunnel_core_init(void)
diff --git a/net/ipv6/ila/ila_lwt.c b/net/ipv6/ila/ila_lwt.c
index a7bc54ab46e2..13b5e85fe0d5 100644
--- a/net/ipv6/ila/ila_lwt.c
+++ b/net/ipv6/ila/ila_lwt.c
@@ -238,6 +238,7 @@ static const struct lwtunnel_encap_ops ila_encap_ops = {
 	.fill_encap = ila_fill_encap_info,
 	.get_encap_size = ila_encap_nlsize,
 	.cmp_encap = ila_encap_cmp,
+	.owner = THIS_MODULE,
 };
 
 int ila_lwt_init(void)
diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c
index 1d60cb132835..c46f8cbf5ab5 100644
--- a/net/ipv6/seg6_iptunnel.c
+++ b/net/ipv6/seg6_iptunnel.c
@@ -422,6 +422,7 @@ static const struct lwtunnel_encap_ops seg6_iptun_ops = {
 	.fill_encap = seg6_fill_encap_info,
 	.get_encap_size = seg6_encap_nlsize,
 	.cmp_encap = seg6_encap_cmp,
+	.owner = THIS_MODULE,
 };
 
 int __init seg6_iptunnel_init(void)
diff --git a/net/mpls/mpls_iptunnel.c b/net/mpls/mpls_iptunnel.c
index 2f7ccd934416..1d281c1ff7c1 100644
--- a/net/mpls/mpls_iptunnel.c
+++ b/net/mpls/mpls_iptunnel.c
@@ -215,6 +215,7 @@ static const struct lwtunnel_encap_ops mpls_iptun_ops = {
 	.fill_encap = mpls_fill_encap_info,
 	.get_encap_size = mpls_encap_nlsize,
 	.cmp_encap = mpls_encap_cmp,
+	.owner = THIS_MODULE,
 };
 
 static int __init mpls_iptunnel_init(void)
-- 
2.1.4

[PATCH net v3 2/2] lwtunnel: Fix oops on state free after encap module unload

[Robert Shearman]

When attempting to free lwtunnel state after the module for the encap
has been unloaded an oops occurs:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: lwtstate_free+0x18/0x40
[..]
task: ffff88003e372380 task.stack: ffffc900001fc000
RIP: 0010:lwtstate_free+0x18/0x40
RSP: 0018:ffff88003fd83e88 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff88002bbb3380 RCX: ffff88000c91a300
[..]
Call Trace:
 <IRQ>
 free_fib_info_rcu+0x195/0x1a0
 ? rt_fibinfo_free+0x50/0x50
 rcu_process_callbacks+0x2d3/0x850
 ? rcu_process_callbacks+0x296/0x850
 __do_softirq+0xe4/0x4cb
 irq_exit+0xb0/0xc0
 smp_apic_timer_interrupt+0x3d/0x50
 apic_timer_interrupt+0x93/0xa0
[..]
Code: e8 6e c6 fc ff 89 d8 5b 5d c3 bb de ff ff ff eb f4 66 90 66 66 66 66 90 55 48 89 e5 53 0f b7 07 48 89 fb 48 8b 04 c5 00 81 d5 81 <48> 8b 40 08 48 85 c0 74 13 ff d0 48 8d 7b 20 be 20 00 00 00 e8

The problem is after the module for the encap can be unloaded the
corresponding ops is removed and is thus NULL here.

Modules implementing lwtunnel ops should not be allowed to unload
while there is state alive using those ops, so grab the module
reference for the ops on creating lwtunnel state and of course release
the reference when freeing the state.

Fixes: 1104d9ba443a ("lwtunnel: Add destroy state operation")
Signed-off-by: Robert Shearman <rshearma@brocade.com>
---
 net/core/lwtunnel.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/core/lwtunnel.c b/net/core/lwtunnel.c
index 47b1dd65947b..c23465005f2f 100644
--- a/net/core/lwtunnel.c
+++ b/net/core/lwtunnel.c
@@ -115,8 +115,11 @@ int lwtunnel_build_state(struct net_device *dev, u16 encap_type,
 	ret = -EOPNOTSUPP;
 	rcu_read_lock();
 	ops = rcu_dereference(lwtun_encaps[encap_type]);
-	if (likely(ops && ops->build_state))
+	if (likely(ops && ops->build_state && try_module_get(ops->owner))) {
 		ret = ops->build_state(dev, encap, family, cfg, lws);
+		if (ret)
+			module_put(ops->owner);
+	}
 	rcu_read_unlock();
 
 	return ret;
@@ -194,6 +197,7 @@ void lwtstate_free(struct lwtunnel_state *lws)
 	} else {
 		kfree(lws);
 	}
+	module_put(ops->owner);
 }
 EXPORT_SYMBOL(lwtstate_free);
 
-- 
2.1.4

Re: [PATCH net v3 0/2] net: Fix oops on state free after lwt module unload

[David Miller]

From: Robert Shearman <rshearma@brocade.com>
Date: Tue, 24 Jan 2017 16:26:46 +0000

> An oops is seen in lwtstate_free after an lwt ops module has been
> unloaded. This patchset fixes this by preventing modules implementing
> lwtunnel ops from being unloaded whilst there's state alive using
> those ops. The first patch adds fills in a new owner field in all lwt
> ops and the second patch makes use of this to reference count the
> modules as state is built and destroyed using them.
> 
> Changes in v3:
>  - don't put module reference if try_module_get fails on building state
> 
> Changes in v2:
>  - specify module owner for all modules as suggested by DaveM
>  - reference count all modules building lwt state, not just those ops
>    implementing destroy_state, as also suggested by DaveM.
>  - rebased on top of David Ahern's lwtunnel changes

Applied and queued up for -stable.

Thanks.