Re: [PATCH v3 net] packet: Do not call fanout_release from atomic contexts

[David Miller]

From: Anoob Soman 
Date: Wed, 15 Feb 2017 20:25:39 +

> Commit 6664498280cf ("packet: call fanout_release, while UNREGISTERING a
> netdev"), unfortunately, introduced the following issues.
 ...
> To fix the above problems, remove the call to fanout_release() under
> rcu_read_lock(). Instead, call __dev_remove_pack(>prot_hook) and
> netdev_run_todo will be happy that >ptype_specific list is empty. In 
> order
> to achieve this, I moved dev_{add,remove}_pack() out of fanout_{add,release} 
> to
> __fanout_{link,unlink}. So, call to {,__}unregister_prot_hook() will make sure
> fanout->prot_hook is removed as well.
> 
> Fixes: 6664498280cf ("packet: call fanout_release, while UNREGISTERING a 
> netdev")
> Reported-by: Eric Dumazet 
> Signed-off-by: Anoob Soman 
> ---
>  Changes in v3:
>  - Removed extra variable from fanout_release(), per Eric's suggestion.
>  Changes in v2:
>  - Incorporated Eric's suggestion to do fanout_release_data() and kfree() 
> after
>synchronize_net()

Applied and queued up for -stable, thanks.

Re: [PATCH v3 net] packet: Do not call fanout_release from atomic contexts

[Eric Dumazet]

On Wed, 2017-02-15 at 21:30 +, Anoob Soman wrote:

> If statement looks like this.
> if (atomic_dec_and_test(sk_ref))
>  list_del(f->list);
> else
> f = NULL;
> 
> there are no multiple lines under if.


Yes, I just noticed that.

Thanks.

Re: [PATCH v3 net] packet: Do not call fanout_release from atomic contexts

[Eric Dumazet]

On Wed, 2017-02-15 at 13:02 -0800, Eric Dumazet wrote:
> On Wed, 2017-02-15 at 20:25 +, Anoob Soman wrote:
> 
> > +static struct packet_fanout *fanout_release(struct sock *sk)
> >  {
> > struct packet_sock *po = pkt_sk(sk);
> > struct packet_fanout *f;
> > @@ -1728,17 +1736,17 @@ static void fanout_release(struct sock *sk)
> > if (f) {
> > po->fanout = NULL;
> >  
> > -   if (atomic_dec_and_test(>sk_ref)) {
> > +   if (atomic_dec_and_test(>sk_ref))
> > list_del(>list);
> > -   dev_remove_pack(>prot_hook);
> > -   fanout_release_data(f);
> > -   kfree(f);
> > -   }
> > +   else
> > +   f = NULL;
> 
> 
> Patch looks good, except this coding style issue.

Oh well, sorry , I missed we only had one left line ;)

Acked-by: Eric Dumazet 

Re: [PATCH v3 net] packet: Do not call fanout_release from atomic contexts

[Anoob Soman]

On 15/02/17 21:02, Eric Dumazet wrote:

On Wed, 2017-02-15 at 20:25 +, Anoob Soman wrote:


+static struct packet_fanout *fanout_release(struct sock *sk)
  {
struct packet_sock *po = pkt_sk(sk);
struct packet_fanout *f;
@@ -1728,17 +1736,17 @@ static void fanout_release(struct sock *sk)
if (f) {
po->fanout = NULL;
  
-		if (atomic_dec_and_test(>sk_ref)) {

+   if (atomic_dec_and_test(>sk_ref))
list_del(>list);
-   dev_remove_pack(>prot_hook);
-   fanout_release_data(f);
-   kfree(f);
-   }
+   else
+   f = NULL;


Patch looks good, except this coding style issue.

if (...) {
 multi lines;
 expressions;
} else {
 f = NULL;
}


Not :

if (...) {
 multi lines;
 expressions;
} else
 f = NULL;

If statement looks like this.
if (atomic_dec_and_test(sk_ref))
list_del(f->list);
else
   f = NULL;

there are no multiple lines under if.

Re: [PATCH v3 net] packet: Do not call fanout_release from atomic contexts

[Eric Dumazet]

On Wed, 2017-02-15 at 20:25 +, Anoob Soman wrote:

> +static struct packet_fanout *fanout_release(struct sock *sk)
>  {
>   struct packet_sock *po = pkt_sk(sk);
>   struct packet_fanout *f;
> @@ -1728,17 +1736,17 @@ static void fanout_release(struct sock *sk)
>   if (f) {
>   po->fanout = NULL;
>  
> - if (atomic_dec_and_test(>sk_ref)) {
> + if (atomic_dec_and_test(>sk_ref))
>   list_del(>list);
> - dev_remove_pack(>prot_hook);
> - fanout_release_data(f);
> - kfree(f);
> - }
> + else
> + f = NULL;


Patch looks good, except this coding style issue.

if (...) {
multi lines;
expressions;
} else {
f = NULL;
}


Not :

if (...) {
multi lines;
expressions;
} else
f = NULL;