[PATCH net-next 0/2] 6lowpan: fixes for new reassembly implementation

[PATCH net-next 0/2] 6lowpan: fixes for new reassembly implementation

[Alexander Aring]

Hi all,

I detected a embarrassing mistake of registering sysctl interface while
init fragmentation. This is fixed in Patch 1/2
(6lowpan: reassembly: fix return of init function).

The second issue occurs while reassembly and unloading 6lowpan module.
I am definitively sure about if I fixed it correctly in this case.
The problem is there are some running timers which are not deleted after
unloading the 6lowpan module. This issue is fixed in Patch 2/2
(6lowpan: reassembly: fix kernel oops while unloading).

- Alex

Alexander Aring (2):
  6lowpan: reassembly: fix return of init function
  6lowpan: reassembly: fix kernel oops while unloading

 net/ieee802154/reassembly.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

-- 
1.9.0

[PATCH net-next 1/2] 6lowpan: reassembly: fix return of init function

[Alexander Aring]

This patch adds a missing return after fragmentation init. Otherwise we
register a sysctl interface and deregister it afterwards which makes no
sense.

Signed-off-by: Alexander Aring <alex.aring-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 net/ieee802154/reassembly.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ieee802154/reassembly.c b/net/ieee802154/reassembly.c
index 4511fc2..59db7b5 100644
--- a/net/ieee802154/reassembly.c
+++ b/net/ieee802154/reassembly.c
@@ -550,6 +550,8 @@ int __init lowpan_net_frag_init(void)
 	lowpan_frags.frag_expire = lowpan_frag_expire;
 	lowpan_frags.secret_interval = 10 * 60 * HZ;
 	inet_frags_init(&lowpan_frags);
+
+	return 0;
 err_pernet:
 	lowpan_frags_sysctl_unregister();
 out:
-- 
1.9.0


------------------------------------------------------------------------------
Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce.
With Perforce, you get hassle-free workflows. Merge that actually works. 
Faster operations. Version large binaries.  Built-in WAN optimization and the
freedom to use Git, Perforce or both. Make the move to Perforce.
http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.clktrk

Re: [PATCH net-next 1/2] 6lowpan: reassembly: fix return of init function

[Sergei Shtylyov]

Hello.

On 03/05/2014 11:43 PM, Alexander Aring wrote:

> This patch adds a missing return after fragmentation init. Otherwise we
> register a sysctl interface and deregister it afterwards which makes no
> sense.

> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>   net/ieee802154/reassembly.c | 2 ++
>   1 file changed, 2 insertions(+)

> diff --git a/net/ieee802154/reassembly.c b/net/ieee802154/reassembly.c
> index 4511fc2..59db7b5 100644
> --- a/net/ieee802154/reassembly.c
> +++ b/net/ieee802154/reassembly.c
> @@ -550,6 +550,8 @@ int __init lowpan_net_frag_init(void)
>   	lowpan_frags.frag_expire = lowpan_frag_expire;
>   	lowpan_frags.secret_interval = 10 * 60 * HZ;
>   	inet_frags_init(&lowpan_frags);
> +
> +	return 0;

    Perhaps 'goto out' for "consistency" with the code above? (I don't know 
why they used "goto out' in the first place.)

>   err_pernet:
>   	lowpan_frags_sysctl_unregister();
>   out:

WBR, Sergei

Re: [PATCH net-next 1/2] 6lowpan: reassembly: fix return of init function

[Alexander Aring]

Hi Sergei,

On Thu, Mar 06, 2014 at 01:04:20AM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 03/05/2014 11:43 PM, Alexander Aring wrote:
> 
> >This patch adds a missing return after fragmentation init. Otherwise we
> >register a sysctl interface and deregister it afterwards which makes no
> >sense.
> 
> >Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> >---
> >  net/ieee802154/reassembly.c | 2 ++
> >  1 file changed, 2 insertions(+)
> 
> >diff --git a/net/ieee802154/reassembly.c b/net/ieee802154/reassembly.c
> >index 4511fc2..59db7b5 100644
> >--- a/net/ieee802154/reassembly.c
> >+++ b/net/ieee802154/reassembly.c
> >@@ -550,6 +550,8 @@ int __init lowpan_net_frag_init(void)
> >  	lowpan_frags.frag_expire = lowpan_frag_expire;
> >  	lowpan_frags.secret_interval = 10 * 60 * HZ;
> >  	inet_frags_init(&lowpan_frags);
> >+
> >+	return 0;
> 
>    Perhaps 'goto out' for "consistency" with the code above? (I don't know
> why they used "goto out' in the first place.)
> 
ok, thanks, I will remove the "goto out" and will replace it with a
"return 0" if you are fine with that.

- Alex

Re: [PATCH net-next 1/2] 6lowpan: reassembly: fix return of init function

[Sergei Shtylyov]

Hello.

On 06-03-2014 9:44, Alexander Aring wrote:

>>> This patch adds a missing return after fragmentation init. Otherwise we
>>> register a sysctl interface and deregister it afterwards which makes no
>>> sense.

>>> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
>>> ---
>>>   net/ieee802154/reassembly.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>
>>> diff --git a/net/ieee802154/reassembly.c b/net/ieee802154/reassembly.c
>>> index 4511fc2..59db7b5 100644
>>> --- a/net/ieee802154/reassembly.c
>>> +++ b/net/ieee802154/reassembly.c
>>> @@ -550,6 +550,8 @@ int __init lowpan_net_frag_init(void)
>>>   	lowpan_frags.frag_expire = lowpan_frag_expire;
>>>   	lowpan_frags.secret_interval = 10 * 60 * HZ;
>>>   	inet_frags_init(&lowpan_frags);
>>> +
>>> +	return 0;

>>     Perhaps 'goto out' for "consistency" with the code above? (I don't know
>> why they used "goto out' in the first place.)

> ok, thanks, I will remove the "goto out" and will replace it with a
> "return 0" if you are fine with that.

    You can't replace it with "return 0", only with "return ret".

> - Alex

WBR, Sergei

[PATCH net-next 2/2] 6lowpan: reassembly: fix kernel oops while unloading

[Alexander Aring]

While fragmentation and unloading of 6lowpan module I got this kernel Oops
after few seconds:

BUG: unable to handle kernel paging request at f88bbc30
IP: [<f88bbc30>] 0xf88bbc30
*pde = 371ee067 *pte = 00000000
Oops: 0000 [#1] SMP
Modules linked in: ipv6 [last unloaded: 6lowpan]
CPU: 0 PID: 0 Comm: swapper/0 Not tainted
3.14.0-rc3-00831-g1f8ca2c-dirty #114
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
task: c0540870 ti: f700c000 task.ti: c0536000
EIP: 0060:[<f88bbc30>] EFLAGS: 00210286 CPU: 0
EIP is at 0xf88bbc30
EAX: f7096080 EBX: 00000100 ECX: 00000000 EDX: 00000000
ESI: f88bbc30 EDI: f700df8c EBP: f700df98 ESP: f700df60
 DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
CR0: 8005003b CR2: f88bbc30 CR3: 372cf000 CR4: 00000690
Stack:
 c012af4c 00000000 00000002 00000000 c012aef8 f7096080 f88bbc30 c0ac181c
 c0828200 00000000 c050ca53 c05d6340 f70960a0 f7096080 f700dfc4 c012b66b
 c05d6d70 c05d6b70 f700dfb0 f88bbc30 f71dadf8 f71dadf8 00000002 c053a204
Call Trace:
 [<c012af4c>] ? call_timer_fn+0x54/0xb3
 [<c012aef8>] ? process_timeout+0xa/0xa
 [<c012b66b>] run_timer_softirq+0x140/0x15f
 [<c0126ec1>] __do_softirq+0xd5/0x1bc
 [<c0126dec>] ? tasklet_hi_action+0xa8/0xa8
 <IRQ>
 [<c012714a>] ? irq_exit+0x39/0x82
 [<c0119ef1>] ? smp_apic_timer_interrupt+0x25/0x2f
 [<c03e7e1f>] ? apic_timer_interrupt+0x2f/0x40
 [<c014007b>] ? wake_up_new_task+0x5a/0x85
 [<c010743c>] ? default_idle+0xa/0xc
 [<c01078d8>] ? arch_cpu_idle+0x12/0x1c
 [<c0152afc>] ? cpu_startup_entry+0xb2/0x114
 [<c03ddd74>] ? rest_init+0x92/0x97
 [<c05728d5>] ? start_kernel+0x2b7/0x2bc
 [<c05722af>] ? i386_start_kernel+0x79/0x7d

It seems that the inet_frag_queue is deleted but the timer is running. This
patch adds a for loop to iterate over all frag_queue entries in the
frag_bucket and calling del_timer for each frag_queue entry while
unloading the 6lowpan module.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
Reported-by: Phoebe Buckheister <phoebe.buckheister@itwm.fraunhofer.de>
---
I am not sure about that I can do that in this simply way without hold
any lock of the inet_frag_queue or inet_frag_bucket. Please help there.
The kernel oops never occurs afterwards, but this isn't simple to test.
I can't test all cases.

 net/ieee802154/reassembly.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/ieee802154/reassembly.c b/net/ieee802154/reassembly.c
index 59db7b5..833b6ad 100644
--- a/net/ieee802154/reassembly.c
+++ b/net/ieee802154/reassembly.c
@@ -560,6 +560,18 @@ out:
 
 void lowpan_net_frag_exit(void)
 {
+	int i;
+
+	for (i = 0; i < INETFRAGS_HASHSZ; i++) {
+		struct inet_frag_bucket *hb;
+		struct inet_frag_queue *q;
+		struct hlist_node *n;
+
+		hb = &lowpan_frags.hash[i];
+		hlist_for_each_entry_safe(q, n, &hb->chain, list)
+			del_timer(&q->timer);
+	}
+
 	inet_frags_fini(&lowpan_frags);
 	lowpan_frags_sysctl_unregister();
 	unregister_pernet_subsys(&lowpan_frags_ops);
-- 
1.9.0

Re: [PATCH net-next 2/2] 6lowpan: reassembly: fix kernel oops while unloading

[Florian Westphal]

Alexander Aring <alex.aring@gmail.com> wrote:
> It seems that the inet_frag_queue is deleted but the timer is running. This
> patch adds a for loop to iterate over all frag_queue entries in the
> frag_bucket and calling del_timer for each frag_queue entry while
> unloading the 6lowpan module.
> 
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> Reported-by: Phoebe Buckheister <phoebe.buckheister@itwm.fraunhofer.de>
> ---
> I am not sure about that I can do that in this simply way without hold
> any lock of the inet_frag_queue or inet_frag_bucket. Please help there.
> The kernel oops never occurs afterwards, but this isn't simple to test.
> I can't test all cases.

I find it hard to believe that this is a 6lowpan specific problem,
most likely this needs a fix in inet_fragment code.

I am currently looking at that code for different reasons anyway and can
investigate tomorrow if you do not have time for it.

Re: [PATCH net-next 2/2] 6lowpan: reassembly: fix kernel oops while unloading

[Alexander Aring]

Hi Florian,

thanks for your reply.

On Wed, Mar 05, 2014 at 11:32:46PM +0100, Florian Westphal wrote:
> Alexander Aring <alex.aring@gmail.com> wrote:
> > It seems that the inet_frag_queue is deleted but the timer is running. This
> > patch adds a for loop to iterate over all frag_queue entries in the
> > frag_bucket and calling del_timer for each frag_queue entry while
> > unloading the 6lowpan module.
> > 
> > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > Reported-by: Phoebe Buckheister <phoebe.buckheister@itwm.fraunhofer.de>
> > ---
> > I am not sure about that I can do that in this simply way without hold
> > any lock of the inet_frag_queue or inet_frag_bucket. Please help there.
> > The kernel oops never occurs afterwards, but this isn't simple to test.
> > I can't test all cases.
> 
> I find it hard to believe that this is a 6lowpan specific problem,
> most likely this needs a fix in inet_fragment code.
> 
I thought that too, maybe it's a problem in the inet_fragment code.


There are two function which I call on exit:

inet_frags_fini(&lowpan_frags); - which deletes the secret_timer.
inet_frags_exit_net(&net->ieee802154_lowpan.frags, &lowpan_frags);
 - which runs a force inet_frag_evictor

maybe I forgot to call some other function to cleanup the fragmentation.

I don't saw any other exit function and I do a similar cleanup like ipv4/ipv6
and they don't have a module_exit function which is called for the
inet_fragment code. 

Example:

ipv6:

ipv6_frag_exit(); - which is only called in error branch of module_init
		    in net/ipv6/af_inet6.c.

ipv4:
I don't see that ipv4 call any of the inet_frag exit functions.


Maybe I have some special problem there because I can unload the 6lowpan
module which used the inet_fragment code.

If ipv4/ipv6 do a cleanup at shutdown, then maybe this never occurs because a
shutdown takes no longer than 60 seconds (in case of ipv6).

> I am currently looking at that code for different reasons anyway and can
> investigate tomorrow if you do not have time for it.

Ok, thanks you for that. I have time for that, but I don't believe that
I found a better solution for that issue. I will be grateful for any help!
Maybe we can find a proper solution together.

I wrote a small testscript with:

while true
do
	rmmod 6lowpan
	sleep 120
	modprobe 6lowpan
	ip link add link wpan0 name lowpan0 type lowpan
	ip link set lowpan0 up
	sleep 120
done

sleep 120 - to be sure we hit the 60 seconds timer arrivial.

I did a overnight test while run a fragmented ping from another node and
the kernel oops never occurs again. I can test some new patches again
this testscript, but this is not to be sure that it works 100%
correct.

- Alex

Re: [PATCH net-next 2/2] 6lowpan: reassembly: fix kernel oops while unloading

[Florian Westphal]

Alexander Aring <alex.aring@gmail.com> wrote:
> On Wed, Mar 05, 2014 at 11:32:46PM +0100, Florian Westphal wrote:
> > Alexander Aring <alex.aring@gmail.com> wrote:
> > > It seems that the inet_frag_queue is deleted but the timer is running. This
> > > patch adds a for loop to iterate over all frag_queue entries in the
> > > frag_bucket and calling del_timer for each frag_queue entry while
> > > unloading the 6lowpan module.
> > > 
> > > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > > Reported-by: Phoebe Buckheister <phoebe.buckheister@itwm.fraunhofer.de>
> > > ---
> > > I am not sure about that I can do that in this simply way without hold
> > > any lock of the inet_frag_queue or inet_frag_bucket. Please help there.
> > > The kernel oops never occurs afterwards, but this isn't simple to test.
> > > I can't test all cases.
> > 
> > I find it hard to believe that this is a 6lowpan specific problem,
> > most likely this needs a fix in inet_fragment code.
> > 
> I thought that too, maybe it's a problem in the inet_fragment code.
> 
> 
> There are two function which I call on exit:
> 
> inet_frags_fini(&lowpan_frags); - which deletes the secret_timer.
> inet_frags_exit_net(&net->ieee802154_lowpan.frags, &lowpan_frags);
>  - which runs a force inet_frag_evictor
> 
> maybe I forgot to call some other function to cleanup the fragmentation.

No, it looks correct.

> I don't saw any other exit function and I do a similar cleanup like ipv4/ipv6
> and they don't have a module_exit function which is called for the
> inet_fragment code. 

net/ipv6/netfilter/nf_defrag_ipv6_hooks.c has one (calls
nf_ct_frag6_cleanup).

I am currently testing this fix:

diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 322dceb..3b01959 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -208,7 +208,7 @@ int inet_frag_evictor(struct netns_frags *nf, struct
		inet_frags *f, bool force)
        }
 
        work = frag_mem_limit(nf) - nf->low_thresh;
-       while (work > 0) {
+       while (work > 0 || force) {


frag_mem_limit() may be inaccurate which causes evictor to terminate
earlier than it should.

Re: [PATCH net-next 2/2] 6lowpan: reassembly: fix kernel oops while unloading

[Alexander Aring]

On Thu, Mar 06, 2014 at 02:38:51PM +0100, Florian Westphal wrote:
> Alexander Aring <alex.aring@gmail.com> wrote:
> > On Wed, Mar 05, 2014 at 11:32:46PM +0100, Florian Westphal wrote:
> > > Alexander Aring <alex.aring@gmail.com> wrote:
> > > > It seems that the inet_frag_queue is deleted but the timer is running. This
> > > > patch adds a for loop to iterate over all frag_queue entries in the
> > > > frag_bucket and calling del_timer for each frag_queue entry while
> > > > unloading the 6lowpan module.
> > > > 
> > > > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > > > Reported-by: Phoebe Buckheister <phoebe.buckheister@itwm.fraunhofer.de>
> > > > ---
> > > > I am not sure about that I can do that in this simply way without hold
> > > > any lock of the inet_frag_queue or inet_frag_bucket. Please help there.
> > > > The kernel oops never occurs afterwards, but this isn't simple to test.
> > > > I can't test all cases.
> > > 
> > > I find it hard to believe that this is a 6lowpan specific problem,
> > > most likely this needs a fix in inet_fragment code.
> > > 
> > I thought that too, maybe it's a problem in the inet_fragment code.
> > 
> > 
> > There are two function which I call on exit:
> > 
> > inet_frags_fini(&lowpan_frags); - which deletes the secret_timer.
> > inet_frags_exit_net(&net->ieee802154_lowpan.frags, &lowpan_frags);
> >  - which runs a force inet_frag_evictor
> > 
> > maybe I forgot to call some other function to cleanup the fragmentation.
> 
> No, it looks correct.
> 
> > I don't saw any other exit function and I do a similar cleanup like ipv4/ipv6
> > and they don't have a module_exit function which is called for the
> > inet_fragment code. 
> 
> net/ipv6/netfilter/nf_defrag_ipv6_hooks.c has one (calls
> nf_ct_frag6_cleanup).
> 
ah, ok.

> I am currently testing this fix:
> 
> diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
> index 322dceb..3b01959 100644
> --- a/net/ipv4/inet_fragment.c
> +++ b/net/ipv4/inet_fragment.c
> @@ -208,7 +208,7 @@ int inet_frag_evictor(struct netns_frags *nf, struct
> 		inet_frags *f, bool force)
>         }
>  
>         work = frag_mem_limit(nf) - nf->low_thresh;
> -       while (work > 0) {
> +       while (work > 0 || force) {
> 
> 

I looked at this and try my little testscript and I don't get the kernel
oops also. What's the next step?

- Alex

Re: [PATCH net-next 2/2] 6lowpan: reassembly: fix kernel oops while unloading

[Florian Westphal]

Alexander Aring <alex.aring@gmail.com> wrote:
> ah, ok.
> 
> > I am currently testing this fix:
> > 
> > diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
> > index 322dceb..3b01959 100644
> > --- a/net/ipv4/inet_fragment.c
> > +++ b/net/ipv4/inet_fragment.c
> > @@ -208,7 +208,7 @@ int inet_frag_evictor(struct netns_frags *nf, struct
> > 		inet_frags *f, bool force)
> >         }
> >  
> >         work = frag_mem_limit(nf) - nf->low_thresh;
> > -       while (work > 0) {
> > +       while (work > 0 || force) {
> > 
> > 
> 
> I looked at this and try my little testscript and I don't get the kernel
> oops also. What's the next step?

Thanks for testing!

I'll make official submit soon, adding proper changelog and your
tested-by tag.