From: Robert Shearman <hidden> Date: 2017-01-18 15:33:52
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
From: Robert Shearman <hidden> Date: 2017-01-18 15:33:53
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 <redacted>
---
include/net/lwtunnel.h | 2 ++
net/core/lwtunnel.c | 11 +++++++++--
2 files changed, 11 insertions(+), 2 deletions(-)
@@ -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();returnret;
From: Robert Shearman <hidden> Date: 2017-01-18 15:33:55
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 <redacted>
---
net/ipv6/ila/ila_lwt.c | 1 +
1 file changed, 1 insertion(+)
From: David Miller <davem@davemloft.net> Date: 2017-01-20 17:03:26
From: Robert Shearman <redacted>
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.
From: Robert Shearman <hidden> Date: 2017-01-20 20:22:02
On 20/01/17 17:03, David Miller wrote:
From: Robert Shearman <redacted>
Date: Wed, 18 Jan 2017 15:32:01 +0000
quoted
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.
From: Robert Shearman <hidden> Date: 2017-01-21 00:23:17
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
From: Robert Shearman <hidden> Date: 2017-01-21 00:23:17
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 <redacted>
---
net/core/lwtunnel.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
From: Robert Shearman <hidden> Date: 2017-01-21 00:24:54
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 <redacted>
---
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(+)
From: David Miller <davem@davemloft.net> Date: 2017-01-23 20:42:50
From: Robert Shearman <redacted>
Date: Sat, 21 Jan 2017 00:21:26 +0000
quoted hunk
@@ -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.
From: Robert Shearman <hidden> Date: 2017-01-24 16:26:52
On 23/01/17 20:42, David Miller wrote:
From: Robert Shearman <redacted>
Date: Sat, 21 Jan 2017 00:21:26 +0000
quoted
@@ -115,8 +115,12 @@ int lwtunnel_build_state(struct net_device *dev, u16 encap_type, ret = -EOPNOTSUPP; rcu_read_lock();
Here 'ret' equals -EOPNOTSUPP
quoted
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
From: Robert Shearman <hidden> Date: 2017-01-24 16:27:23
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 <redacted>
---
net/core/lwtunnel.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
From: Robert Shearman <hidden> Date: 2017-01-24 16:27:41
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
From: Robert Shearman <hidden> Date: 2017-01-24 16:27:46
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 <redacted>
---
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(+)
From: David Miller <davem@davemloft.net> Date: 2017-01-24 21:23:08
From: Robert Shearman <redacted>
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