[PATCH net] inet: frag: make sure forced eviction removes all frags

[PATCH net] inet: frag: make sure forced eviction removes all frags

[Florian Westphal]

Quoting 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
  [..]
  Modules linked in: ipv6 [last unloaded: 6lowpan]
  Call Trace:
   [<c012af4c>] ? call_timer_fn+0x54/0xb3
   [<c012aef8>] ? process_timeout+0xa/0xa
   [<c012b66b>] run_timer_softirq+0x140/0x15f

Problem is that incomplete frags are still around after unload; when
their frag expire timer fires, we get crash.

When a netns is removed (also done when unloading module), inet_frag
calls the evictor with 'force' argument to purge remaining frags.

The evictor loop terminates when accounted memory ('work') drops to 0
or the lru-list becomes empty.  However, the mem accounting is done
via percpu counters and may not be accurate, i.e. loop may terminate
prematurely.

Alter evictor to only stop once the lru list is empty when force is
requested.

Reported-by: Phoebe Buckheister <phoebe.buckheister@itwm.fraunhofer.de>
Reported-by: Alexander Aring <alex.aring@gmail.com>
Tested-by: Alexander Aring <alex.aring@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---

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) {
 		spin_lock(&nf->lru_lock);
 
 		if (list_empty(&nf->lru_list)) {
-- 
1.8.1.5

Re: [PATCH net] inet: frag: make sure forced eviction removes all frags

[Eric Dumazet]

On Thu, 2014-03-06 at 18:06 +0100, Florian Westphal wrote:
> Quoting 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
>   [..]
>   Modules linked in: ipv6 [last unloaded: 6lowpan]
>   Call Trace:
>    [<c012af4c>] ? call_timer_fn+0x54/0xb3
>    [<c012aef8>] ? process_timeout+0xa/0xa
>    [<c012b66b>] run_timer_softirq+0x140/0x15f
> 
> Problem is that incomplete frags are still around after unload; when
> their frag expire timer fires, we get crash.
> 
> When a netns is removed (also done when unloading module), inet_frag
> calls the evictor with 'force' argument to purge remaining frags.
> 
> The evictor loop terminates when accounted memory ('work') drops to 0
> or the lru-list becomes empty.  However, the mem accounting is done
> via percpu counters and may not be accurate, i.e. loop may terminate
> prematurely.
> 
> Alter evictor to only stop once the lru list is empty when force is
> requested.
> 
> Reported-by: Phoebe Buckheister <phoebe.buckheister@itwm.fraunhofer.de>
> Reported-by: Alexander Aring <alex.aring@gmail.com>
> Tested-by: Alexander Aring <alex.aring@gmail.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
> 
> 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) {
>  		spin_lock(&nf->lru_lock);
>  
>  		if (list_empty(&nf->lru_list)) {

Fixes: 6d7b857d541e ("net: use lib/percpu_counter API for fragmentation mem accounting")
Cc: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net] inet: frag: make sure forced eviction removes all frags

[David Miller]

From: Florian Westphal <fw@strlen.de>
Date: Thu,  6 Mar 2014 18:06:41 +0100

> Quoting 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
>   [..]
>   Modules linked in: ipv6 [last unloaded: 6lowpan]
>   Call Trace:
>    [<c012af4c>] ? call_timer_fn+0x54/0xb3
>    [<c012aef8>] ? process_timeout+0xa/0xa
>    [<c012b66b>] run_timer_softirq+0x140/0x15f
> 
> Problem is that incomplete frags are still around after unload; when
> their frag expire timer fires, we get crash.
> 
> When a netns is removed (also done when unloading module), inet_frag
> calls the evictor with 'force' argument to purge remaining frags.
> 
> The evictor loop terminates when accounted memory ('work') drops to 0
> or the lru-list becomes empty.  However, the mem accounting is done
> via percpu counters and may not be accurate, i.e. loop may terminate
> prematurely.
> 
> Alter evictor to only stop once the lru list is empty when force is
> requested.
> 
> Reported-by: Phoebe Buckheister <phoebe.buckheister@itwm.fraunhofer.de>
> Reported-by: Alexander Aring <alex.aring@gmail.com>
> Tested-by: Alexander Aring <alex.aring@gmail.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>

Applied and queued up for -stable, thanks.

Re: [PATCH net] inet: frag: make sure forced eviction removes all frags

[Jesper Dangaard Brouer]

On Thu, 06 Mar 2014 09:36:22 -0800
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Thu, 2014-03-06 at 18:06 +0100, Florian Westphal wrote:
> > Quoting 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
> >   [..]
> >   Modules linked in: ipv6 [last unloaded: 6lowpan]
> >   Call Trace:
> >    [<c012af4c>] ? call_timer_fn+0x54/0xb3
> >    [<c012aef8>] ? process_timeout+0xa/0xa
> >    [<c012b66b>] run_timer_softirq+0x140/0x15f
> > 
> > Problem is that incomplete frags are still around after unload; when
> > their frag expire timer fires, we get crash.
> > 
> > When a netns is removed (also done when unloading module), inet_frag
> > calls the evictor with 'force' argument to purge remaining frags.
> > 
> > The evictor loop terminates when accounted memory ('work') drops to 0
> > or the lru-list becomes empty.  However, the mem accounting is done
> > via percpu counters and may not be accurate, i.e. loop may terminate
> > prematurely.
> > 
> > Alter evictor to only stop once the lru list is empty when force is
> > requested.
> > 
> > Reported-by: Phoebe Buckheister <phoebe.buckheister@itwm.fraunhofer.de>
> > Reported-by: Alexander Aring <alex.aring@gmail.com>
> > Tested-by: Alexander Aring <alex.aring@gmail.com>
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> > 
> > 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) {
> >  		spin_lock(&nf->lru_lock);
> >  
> >  		if (list_empty(&nf->lru_list)) {
> 
> Fixes: 6d7b857d541e ("net: use lib/percpu_counter API for fragmentation mem accounting")
> Cc: Jesper Dangaard Brouer <brouer@redhat.com>
> Acked-by: Eric Dumazet <edumazet@google.com>

Thanks for CC'ing me, and adding the "Fixes" tag (but which DaveM forgot
to pickup in the commit...)

Thanks for fixing this Florian. Using the empty LRU list is this case
is a good solution, in this case.

In other situations, people should look at using percpu_counter_sum(),
when wanting an accurate read via percpu_counters.  (Here frag_mem_limit()
uses percpu_counter_read() which caused the issue).

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH net] inet: frag: make sure forced eviction removes all frags

[David Miller]

From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Fri, 7 Mar 2014 09:56:05 +0100

> Thanks for CC'ing me, and adding the "Fixes" tag (but which DaveM forgot
> to pickup in the commit...)

Oops... we should probably ask patchwork to start recognizing it.