Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-22 Thread Guillaume Nault 
On Thu, Oct 22, 2015 at 03:53:33AM +0300, Denys Fedoryshchenko wrote:
> On 2015-10-22 03:14, Matt Bennett wrote:
> >On Tue, 2015-10-13 at 05:13 +0300, Denys Fedoryshchenko wrote:
> >>On 2015-10-07 15:12, Guillaume Nault wrote:
> >>> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> >>> index 2ed7506..5e0b432 100644
> >>> --- a/drivers/net/ppp/pppoe.c
> >>> +++ b/drivers/net/ppp/pppoe.c
> >>> @@ -589,7 +589,7 @@ static int pppoe_release(struct socket *sock)
> >>>
> >>>   po = pppox_sk(sk);
> >>>
> >>> - if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
> >>> + if (po->pppoe_dev) {
> >>>   dev_put(po->pppoe_dev);
> >>>   po->pppoe_dev = NULL;
> >>>   }
> >>I just got OK to upgrade server yesterday, for now around 12 hours
> >>working fine. I need 1-2 more days, and maybe will upgrade few more
> >>servers to say for sure, if it is ok or not.
> >>Sorry for delay, just it is production servers and at current situation
> >>they cannot tolerate significant downtime.
> >>
> >Any update on whether this issue is fixed with the suggested patch?
> 
> As on server i am allowed to test - no crashed anymore, but i am unable to
> get permission yet to test
> on server where this crash was happening several times per day. But all i
> can say it is definitely better now.
> 
Good. It seems that more people are getting this problem, so I'm going
to submit the patch now.

I'm still interested in the result of your test on the second server
though.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-21 Thread Denys Fedoryshchenko 

On 2015-10-22 03:14, Matt Bennett wrote:

On Tue, 2015-10-13 at 05:13 +0300, Denys Fedoryshchenko wrote:

On 2015-10-07 15:12, Guillaume Nault wrote:
> On Mon, Oct 05, 2015 at 02:08:44PM +0200, Guillaume Nault wrote:
>>if (po) {
>>struct sock *sk = sk_pppox(po);
>>
>> -  bh_lock_sock(sk);
>> -
>> -  /* If the user has locked the socket, just ignore
>> -   * the packet.  With the way two rcv protocols hook into
>> -   * one socket family type, we cannot (easily) distinguish
>> -   * what kind of SKB it is during backlog rcv.
>> -   */
>> -  if (sock_owned_by_user(sk) == 0) {
>> -  /* We're no longer connect at the PPPOE layer,
>> -   * and must wait for ppp channel to disconnect us.
>> -   */
>> -  sk->sk_state = PPPOX_ZOMBIE;
>> -  }
>> -
>> -  bh_unlock_sock(sk);
>>if (!schedule_work(>proto.pppoe.padt_work))
>>sock_put(sk);
>>}
>>
> Finally, I think I'll keep this approach for net-next, to completely
> remove PPPOX_ZOMBIE.
> For now, let's just avoid any assumption about the relationship between
> the PPPOX_ZOMBIE state and the value of po->pppoe_dev, as suggested by
> Matt.
>
> Denys, can you let me know if your issue goes away with the following
> patch?
> ---
> diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> index 2ed7506..5e0b432 100644
> --- a/drivers/net/ppp/pppoe.c
> +++ b/drivers/net/ppp/pppoe.c
> @@ -589,7 +589,7 @@ static int pppoe_release(struct socket *sock)
>
>po = pppox_sk(sk);
>
> -  if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
> +  if (po->pppoe_dev) {
>dev_put(po->pppoe_dev);
>po->pppoe_dev = NULL;
>}
I just got OK to upgrade server yesterday, for now around 12 hours
working fine. I need 1-2 more days, and maybe will upgrade few more
servers to say for sure, if it is ok or not.
Sorry for delay, just it is production servers and at current 
situation

they cannot tolerate significant downtime.


Any update on whether this issue is fixed with the suggested patch?


As on server i am allowed to test - no crashed anymore, but i am unable 
to get permission yet to test
on server where this crash was happening several times per day. But all 
i can say it is definitely better now.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-21 Thread Matt Bennett 
On Tue, 2015-10-13 at 05:13 +0300, Denys Fedoryshchenko wrote:
> On 2015-10-07 15:12, Guillaume Nault wrote:
> > On Mon, Oct 05, 2015 at 02:08:44PM +0200, Guillaume Nault wrote:
> >>if (po) {
> >>struct sock *sk = sk_pppox(po);
> >> 
> >> -  bh_lock_sock(sk);
> >> -
> >> -  /* If the user has locked the socket, just ignore
> >> -   * the packet.  With the way two rcv protocols hook into
> >> -   * one socket family type, we cannot (easily) distinguish
> >> -   * what kind of SKB it is during backlog rcv.
> >> -   */
> >> -  if (sock_owned_by_user(sk) == 0) {
> >> -  /* We're no longer connect at the PPPOE layer,
> >> -   * and must wait for ppp channel to disconnect us.
> >> -   */
> >> -  sk->sk_state = PPPOX_ZOMBIE;
> >> -  }
> >> -
> >> -  bh_unlock_sock(sk);
> >>if (!schedule_work(>proto.pppoe.padt_work))
> >>sock_put(sk);
> >>}
> >> 
> > Finally, I think I'll keep this approach for net-next, to completely
> > remove PPPOX_ZOMBIE.
> > For now, let's just avoid any assumption about the relationship between
> > the PPPOX_ZOMBIE state and the value of po->pppoe_dev, as suggested by
> > Matt.
> > 
> > Denys, can you let me know if your issue goes away with the following
> > patch?
> > ---
> > diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> > index 2ed7506..5e0b432 100644
> > --- a/drivers/net/ppp/pppoe.c
> > +++ b/drivers/net/ppp/pppoe.c
> > @@ -589,7 +589,7 @@ static int pppoe_release(struct socket *sock)
> > 
> > po = pppox_sk(sk);
> > 
> > -   if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
> > +   if (po->pppoe_dev) {
> > dev_put(po->pppoe_dev);
> > po->pppoe_dev = NULL;
> > }
> I just got OK to upgrade server yesterday, for now around 12 hours 
> working fine. I need 1-2 more days, and maybe will upgrade few more 
> servers to say for sure, if it is ok or not.
> Sorry for delay, just it is production servers and at current situation 
> they cannot tolerate significant downtime.
> 
Any update on whether this issue is fixed with the suggested patch?
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

N�r��yb�X��ǧv�^�)޺{.n�+���z�^�)w*jg����ݢj/���z�ޖ��2�ޙ&�)ߡ�a�����G���h��j:+v���w��٥

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-13 Thread Guillaume Nault 
On Tue, Oct 13, 2015 at 05:13:54AM +0300, Denys Fedoryshchenko wrote:
> On 2015-10-07 15:12, Guillaume Nault wrote:
> >On Mon, Oct 05, 2015 at 02:08:44PM +0200, Guillaume Nault wrote:
> >>if (po) {
> >>struct sock *sk = sk_pppox(po);
> >>
> >>-   bh_lock_sock(sk);
> >>-
> >>-   /* If the user has locked the socket, just ignore
> >>-* the packet.  With the way two rcv protocols hook into
> >>-* one socket family type, we cannot (easily) distinguish
> >>-* what kind of SKB it is during backlog rcv.
> >>-*/
> >>-   if (sock_owned_by_user(sk) == 0) {
> >>-   /* We're no longer connect at the PPPOE layer,
> >>-* and must wait for ppp channel to disconnect us.
> >>-*/
> >>-   sk->sk_state = PPPOX_ZOMBIE;
> >>-   }
> >>-
> >>-   bh_unlock_sock(sk);
> >>if (!schedule_work(>proto.pppoe.padt_work))
> >>sock_put(sk);
> >>}
> >>
> >Finally, I think I'll keep this approach for net-next, to completely
> >remove PPPOX_ZOMBIE.
> >For now, let's just avoid any assumption about the relationship between
> >the PPPOX_ZOMBIE state and the value of po->pppoe_dev, as suggested by
> >Matt.
> >
> >Denys, can you let me know if your issue goes away with the following
> >patch?
> >---
> >diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
> >index 2ed7506..5e0b432 100644
> >--- a/drivers/net/ppp/pppoe.c
> >+++ b/drivers/net/ppp/pppoe.c
> >@@ -589,7 +589,7 @@ static int pppoe_release(struct socket *sock)
> >
> > po = pppox_sk(sk);
> >
> >-if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
> >+if (po->pppoe_dev) {
> > dev_put(po->pppoe_dev);
> > po->pppoe_dev = NULL;
> > }
> I just got OK to upgrade server yesterday, for now around 12 hours working
> fine. I need 1-2 more days, and maybe will upgrade few more servers to say
> for sure, if it is ok or not.
> Sorry for delay, just it is production servers and at current situation they
> cannot tolerate significant downtime.
> 
That's ok. I'll send an official patch when you consider the issue to
be definitely fixed.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-12 Thread Denys Fedoryshchenko 

On 2015-10-07 15:12, Guillaume Nault wrote:

On Mon, Oct 05, 2015 at 02:08:44PM +0200, Guillaume Nault wrote:

if (po) {
struct sock *sk = sk_pppox(po);

-   bh_lock_sock(sk);
-
-   /* If the user has locked the socket, just ignore
-* the packet.  With the way two rcv protocols hook into
-* one socket family type, we cannot (easily) distinguish
-* what kind of SKB it is during backlog rcv.
-*/
-   if (sock_owned_by_user(sk) == 0) {
-   /* We're no longer connect at the PPPOE layer,
-* and must wait for ppp channel to disconnect us.
-*/
-   sk->sk_state = PPPOX_ZOMBIE;
-   }
-
-   bh_unlock_sock(sk);
if (!schedule_work(>proto.pppoe.padt_work))
sock_put(sk);
}


Finally, I think I'll keep this approach for net-next, to completely
remove PPPOX_ZOMBIE.
For now, let's just avoid any assumption about the relationship between
the PPPOX_ZOMBIE state and the value of po->pppoe_dev, as suggested by
Matt.

Denys, can you let me know if your issue goes away with the following
patch?
---
diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 2ed7506..5e0b432 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -589,7 +589,7 @@ static int pppoe_release(struct socket *sock)

po = pppox_sk(sk);

-   if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
+   if (po->pppoe_dev) {
dev_put(po->pppoe_dev);
po->pppoe_dev = NULL;
}
I just got OK to upgrade server yesterday, for now around 12 hours 
working fine. I need 1-2 more days, and maybe will upgrade few more 
servers to say for sure, if it is ok or not.
Sorry for delay, just it is production servers and at current situation 
they cannot tolerate significant downtime.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-07 Thread Guillaume Nault 
On Tue, Oct 06, 2015 at 09:12:18PM +, Matt Bennett wrote:
> On Tue, 2015-10-06 at 11:46 +0200, Guillaume Nault wrote:
> > On Tue, Oct 06, 2015 at 04:46:04AM +, Matt Bennett wrote:
> > > I don't know why the code isn't like the following anyway.
> > > 
> > > -if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
> > > +if (po->pppoe_dev) {
> > >   dev_put(po->pppoe_dev);
> > >   po->pppoe_dev = NULL;
> > > }
> > I was thinking about that same approach. pppoe_release() is the only
> > function making that assumption. Other parts of the code seem to only
> > require that PPPOX_CONNECTED => pppoe_dev != NULL.
> > 
> > But I think the original condition was valid. Adding PPPOX_ZOMBIE into
> > the test and resetting pppoe_dev upon reception of PADT have changed the
> > relationship between sk_state and pppoe_dev, which is where the problem
> > stands.
> Yes originally the condition was valid. But I think the issue is plain
> to see when you look at the comment beside PPPOX_ZOMBIE declared in the
> enum.
> 
>  PPPOX_ZOMBIE = 8,  /* dead, but still bound to ppp device */
> 
> We have seen in the situation we have described previously that we can
> be in this state without being bound to the ppp device.
> 
> In my opinion the entire logic around
> pppoe_disc_rcv()/pppoe_unbind_sock_work() looks wrong and I agree we
> should do what you suggested a few emails back.
> 
> i.e in pppoe_disc_rcv():
> 
> if (po) {
>   struct sock *sk = sk_pppox(po);
> 
> - bh_lock_sock(sk);
> -
> - /* If the user has locked the socket, just ignore
> -  * the packet.  With the way two rcv protocols hook into
> -  * one socket family type, we cannot (easily) distinguish
> -  * what kind of SKB it is during backlog rcv.
> -  */
> - if (sock_owned_by_user(sk) == 0) {
> - /* We're no longer connect at the PPPOE layer,
> -  * and must wait for ppp channel to disconnect us.
> -  */
> - sk->sk_state = PPPOX_ZOMBIE;
> - }
> -
> - bh_unlock_sock(sk);
>   if (!schedule_work(>proto.pppoe.padt_work))
>   sock_put(sk);
>   }
> 
Yes, with the introduction of pppoe_unbind_sock_work(), setting
PPPOX_ZOMBIE shouldn't be required anymore.

> Subsequently the PPPOX_ZOMBIE state can be completely removed?
> 
Yes, this is the last place where PPPOX_ZOMBIE can be set.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-07 Thread Guillaume Nault 
On Mon, Oct 05, 2015 at 02:08:44PM +0200, Guillaume Nault wrote:
>   if (po) {
>   struct sock *sk = sk_pppox(po);
>  
> - bh_lock_sock(sk);
> -
> - /* If the user has locked the socket, just ignore
> -  * the packet.  With the way two rcv protocols hook into
> -  * one socket family type, we cannot (easily) distinguish
> -  * what kind of SKB it is during backlog rcv.
> -  */
> - if (sock_owned_by_user(sk) == 0) {
> - /* We're no longer connect at the PPPOE layer,
> -  * and must wait for ppp channel to disconnect us.
> -  */
> - sk->sk_state = PPPOX_ZOMBIE;
> - }
> -
> - bh_unlock_sock(sk);
>   if (!schedule_work(>proto.pppoe.padt_work))
>   sock_put(sk);
>   }
> 
Finally, I think I'll keep this approach for net-next, to completely
remove PPPOX_ZOMBIE.
For now, let's just avoid any assumption about the relationship between
the PPPOX_ZOMBIE state and the value of po->pppoe_dev, as suggested by
Matt.

Denys, can you let me know if your issue goes away with the following
patch?
---
diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 2ed7506..5e0b432 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -589,7 +589,7 @@ static int pppoe_release(struct socket *sock)
 
po = pppox_sk(sk);
 
-   if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
+   if (po->pppoe_dev) {
dev_put(po->pppoe_dev);
po->pppoe_dev = NULL;
}
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-06 Thread Matt Bennett 
On Tue, 2015-10-06 at 11:46 +0200, Guillaume Nault wrote:
> On Tue, Oct 06, 2015 at 04:46:04AM +, Matt Bennett wrote:
> > > > The second one seems to be trickier. It looks like a race wrt. PADT
> > > > message reception. Reproducing the bug will probably require to
> > > > generate some PADT flooding to a host that creates and releases PPPoE
> > > > connections.
> > 
> > Ok I think I can see the potential race here, specifically the PADT
> > frame is received while the pppoe interface is being deleted. (I will
> > have a go inducing this with msleep() in the code tomorrow)
> > 
> > 1. pppoe_flush_dev() - sk->sk_state = PPPOX_DEAD, po->pppoe_dev = NULL
> > 
> > 2. pppoe_connect() - sk->sk_state = PPPOX_NONE, po->pppoe_dev = NULL
> > 
> > 3. pppoe_disc_rcv() - sk->sk_state = PPPOX_ZOMBIE po->pppoe_dev = NULL
> > 
> > 4. pppoe_release() - dev_put(po->pppoe_dev) > Oops
> > 
> Again, I don't know why you introduce pppoe_connect() into the mix.
Sorry, I'm just going off the function calls I can see happening with my
kernel (3.16).
> But anyway, you got the point. Note that pppoe_flush_dev() could be
> replaced by other calls since we just need to reset po->pppoe_dev
> (another pppoe_unbind_sock_work() call, due to duplicated PADT, would
> also trigger the bug). Note also that pppoe_release() needs to be run
> before pppoe_unbind_sock_work() gets scheduled (or at least before it
> locks the socket).
> 
> > Either in pppoe_disc_rcv() we add the condition:
> > 
> > @@ -496,7 +499,8 @@ static int pppoe_disc_rcv(struct sk_buff *skb,
> > struct net_device *dev,
> > /* We're no longer connect at the PPPOE layer,
> >  * and must wait for ppp channel to disconnect
> > us.
> >  */
> > -   sk->sk_state = PPPOX_ZOMBIE;
> > +   if (sk->sk_state & PPPOX_CONNECTED)
> > +   sk->sk_state = PPPOX_ZOMBIE;
> > }
> > 
> > Or perhaps we remove the assumption that the state PPPOX_ZOMBIE has a
> > non-null pppoe_dev on it.
> > 
> I don't think adding complexity in the socket state management would be
> a good think. Actually I event think about dropping the PPPOX_ZOMBIE
> state altogether. But that's probably something for net-next.
> 
> > I don't know why the code isn't like the following anyway.
> > 
> > -if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
> > +if (po->pppoe_dev) {
> > dev_put(po->pppoe_dev);
> > po->pppoe_dev = NULL;
> > }
> I was thinking about that same approach. pppoe_release() is the only
> function making that assumption. Other parts of the code seem to only
> require that PPPOX_CONNECTED => pppoe_dev != NULL.
> 
> But I think the original condition was valid. Adding PPPOX_ZOMBIE into
> the test and resetting pppoe_dev upon reception of PADT have changed the
> relationship between sk_state and pppoe_dev, which is where the problem
> stands.
Yes originally the condition was valid. But I think the issue is plain
to see when you look at the comment beside PPPOX_ZOMBIE declared in the
enum.

 PPPOX_ZOMBIE   = 8,  /* dead, but still bound to ppp device */

We have seen in the situation we have described previously that we can
be in this state without being bound to the ppp device.

In my opinion the entire logic around
pppoe_disc_rcv()/pppoe_unbind_sock_work() looks wrong and I agree we
should do what you suggested a few emails back.

i.e in pppoe_disc_rcv():

if (po) {
struct sock *sk = sk_pppox(po);

-   bh_lock_sock(sk);
-
-   /* If the user has locked the socket, just ignore
-* the packet.  With the way two rcv protocols hook into
-* one socket family type, we cannot (easily) distinguish
-* what kind of SKB it is during backlog rcv.
-*/
-   if (sock_owned_by_user(sk) == 0) {
-   /* We're no longer connect at the PPPOE layer,
-* and must wait for ppp channel to disconnect us.
-*/
-   sk->sk_state = PPPOX_ZOMBIE;
-   }
-
-   bh_unlock_sock(sk);
if (!schedule_work(>proto.pppoe.padt_work))
sock_put(sk);
}

Subsequently the PPPOX_ZOMBIE state can be completely removed?

Matt

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-06 Thread Guillaume Nault 
On Tue, Oct 06, 2015 at 12:26:20AM +, Matt Bennett wrote:
> On Mon, 2015-10-05 at 14:24 +0200, Guillaume Nault wrote:
> > On Mon, Oct 05, 2015 at 04:08:51AM +, Matt Bennett wrote:
> > > Hi, I am seeing this panic occur occasionally however I am unsure how to
> > > go about reproducing it. Is it enough to simply keep creating and
> > > tearing down the PPP interface? I can also test and/or investigate this
> > > issue if a suitable reproduction method is available.
> > > 
> > There are at least two issues resulting in similar Oops.
> > 
> > The first one goes with MTU/address/link state updates on the
> > underlying interface: any such update on an interface used by a
> > PPPoE connection will generally result in an Oops when releasing the
> > PPPoE connection. This is fixed by e6740165b8f7 ("ppp: don't override
> > sk->sk_state in pppoe_flush_dev()").
> 
> Without your patch ("ppp: don't override sk->sk_state in
> pppoe_flush_dev()") I can see the following function calls being made
> when changing the mtu on the underlying ethernet interface for the PPPoE
> connection:
> 
> 1. pppoe_flush_dev() - setting PPPOX_ZOMBIE
> 
> 2. pppoe_connect - setting PPPOX_NONE (shown below)
> 
> /* Delete the old binding */
>   if (stage_session(po->pppoe_pa.sid)) {
>   pppox_unbind_sock(sk);
>   pn = pppoe_pernet(sock_net(sk));
>   delete_item(pn, po->pppoe_pa.sid,
>   po->pppoe_pa.remote, po->pppoe_ifindex);
>   if (po->pppoe_dev) {
>   dev_put(po->pppoe_dev);
>   po->pppoe_dev = NULL;
>   }
> 
>   memset(sk_pppox(po) + 1, 0,
>  sizeof(struct pppox_sock) - sizeof(struct sock));
>   sk->sk_state = PPPOX_NONE;
>   }
> 
> 3. pppoe_release - No oops (since sk->sk_state is no longer in
> {PPPOX_CONNECTED,PPPOX_BOUND,PPPOX_ZOMBIE})
> 
> It doesn't look to me like the above functions can execute
> asynchronously but I'd have to look harder. I am using 3.16 by the way.
> 
Just drop the pppoe_connect() call. Right after the pppoe_flush_dev()
call, sk_state is PPPOX_ZOMBIE and pppoe_dev is NULL. This is enouhg to
make pppoe_release() crash.

The typical scenario e6740165b8f7 ("ppp: don't override sk->sk_state in
pppoe_flush_dev()") fixes is:

  Userspace process #1:   Userspace process #2:
  -   -
fd = socket(AF_PPPOX, PX_PROTO_OE, 0);
connect(fd, {AF_PPPOX, PX_PROTO_EO,
$sid, $mac_addr, $ifname},
sizeof(struct sockaddr_pppox));

... process_packets() ...   # ip link set $ifname mtu $mtu

close(fd); --> Kernel Oops
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-06 Thread Guillaume Nault 
On Tue, Oct 06, 2015 at 04:46:04AM +, Matt Bennett wrote:
> > > The second one seems to be trickier. It looks like a race wrt. PADT
> > > message reception. Reproducing the bug will probably require to
> > > generate some PADT flooding to a host that creates and releases PPPoE
> > > connections.
> 
> Ok I think I can see the potential race here, specifically the PADT
> frame is received while the pppoe interface is being deleted. (I will
> have a go inducing this with msleep() in the code tomorrow)
> 
> 1. pppoe_flush_dev() - sk->sk_state = PPPOX_DEAD, po->pppoe_dev = NULL
> 
> 2. pppoe_connect() - sk->sk_state = PPPOX_NONE, po->pppoe_dev = NULL
> 
> 3. pppoe_disc_rcv() - sk->sk_state = PPPOX_ZOMBIE po->pppoe_dev = NULL
> 
> 4. pppoe_release() - dev_put(po->pppoe_dev) > Oops
> 
Again, I don't know why you introduce pppoe_connect() into the mix.
But anyway, you got the point. Note that pppoe_flush_dev() could be
replaced by other calls since we just need to reset po->pppoe_dev
(another pppoe_unbind_sock_work() call, due to duplicated PADT, would
also trigger the bug). Note also that pppoe_release() needs to be run
before pppoe_unbind_sock_work() gets scheduled (or at least before it
locks the socket).

> Either in pppoe_disc_rcv() we add the condition:
> 
> @@ -496,7 +499,8 @@ static int pppoe_disc_rcv(struct sk_buff *skb,
> struct net_device *dev,
> /* We're no longer connect at the PPPOE layer,
>  * and must wait for ppp channel to disconnect
> us.
>  */
> -   sk->sk_state = PPPOX_ZOMBIE;
> +   if (sk->sk_state & PPPOX_CONNECTED)
> +   sk->sk_state = PPPOX_ZOMBIE;
> }
> 
> Or perhaps we remove the assumption that the state PPPOX_ZOMBIE has a
> non-null pppoe_dev on it.
> 
I don't think adding complexity in the socket state management would be
a good think. Actually I event think about dropping the PPPOX_ZOMBIE
state altogether. But that's probably something for net-next.

> I don't know why the code isn't like the following anyway.
> 
> -if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
> +if (po->pppoe_dev) {
>   dev_put(po->pppoe_dev);
>   po->pppoe_dev = NULL;
> }
I was thinking about that same approach. pppoe_release() is the only
function making that assumption. Other parts of the code seem to only
require that PPPOX_CONNECTED => pppoe_dev != NULL.

But I think the original condition was valid. Adding PPPOX_ZOMBIE into
the test and resetting pppoe_dev upon reception of PADT have changed the
relationship between sk_state and pppoe_dev, which is where the problem
stands.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-05 Thread Matt Bennett 
> > The second one seems to be trickier. It looks like a race wrt. PADT
> > message reception. Reproducing the bug will probably require to
> > generate some PADT flooding to a host that creates and releases PPPoE
> > connections.

Ok I think I can see the potential race here, specifically the PADT
frame is received while the pppoe interface is being deleted. (I will
have a go inducing this with msleep() in the code tomorrow)

1. pppoe_flush_dev() - sk->sk_state = PPPOX_DEAD, po->pppoe_dev = NULL

2. pppoe_connect() - sk->sk_state = PPPOX_NONE, po->pppoe_dev = NULL

3. pppoe_disc_rcv() - sk->sk_state = PPPOX_ZOMBIE po->pppoe_dev = NULL

4. pppoe_release() - dev_put(po->pppoe_dev) > Oops


Either in pppoe_disc_rcv() we add the condition:

@@ -496,7 +499,8 @@ static int pppoe_disc_rcv(struct sk_buff *skb,
struct net_device *dev,
/* We're no longer connect at the PPPOE layer,
 * and must wait for ppp channel to disconnect
us.
 */
-   sk->sk_state = PPPOX_ZOMBIE;
+   if (sk->sk_state & PPPOX_CONNECTED)
+   sk->sk_state = PPPOX_ZOMBIE;
}

Or perhaps we remove the assumption that the state PPPOX_ZOMBIE has a
non-null pppoe_dev on it.

I don't know why the code isn't like the following anyway.

-if (sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | PPPOX_ZOMBIE)) {
+if (po->pppoe_dev) {
dev_put(po->pppoe_dev);
po->pppoe_dev = NULL;
}

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-05 Thread David Miller 
From: Guillaume Nault 
Date: Wed, 30 Sep 2015 11:45:33 +0200

> Since commit 2b018d57ff18 ("pppoe: drop PPPOX_ZOMBIEs in pppoe_release"),
> pppoe_release() calls dev_put(po->pppoe_dev) if sk is in the
> PPPOX_ZOMBIE state. But pppoe_flush_dev() can set sk->sk_state to
> PPPOX_ZOMBIE _and_ reset po->pppoe_dev to NULL. This leads to the
> following oops:
 ...
> pppoe_flush_dev() has no reason to override sk->sk_state with
> PPPOX_ZOMBIE. pppox_unbind_sock() already sets sk->sk_state to
> PPPOX_DEAD, which is the correct state given that sk is unbound and
> po->pppoe_dev is NULL.
> 
> Fixes: 2b018d57ff18 ("pppoe: drop PPPOX_ZOMBIEs in pppoe_release")
> Tested-by: Oleksii Berezhniak 
> Signed-off-by: Guillaume Nault 

Applied and queued up for -stable, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-05 Thread Matt Bennett 
On Mon, 2015-10-05 at 14:24 +0200, Guillaume Nault wrote:
> On Mon, Oct 05, 2015 at 04:08:51AM +, Matt Bennett wrote:
> > Hi, I am seeing this panic occur occasionally however I am unsure how to
> > go about reproducing it. Is it enough to simply keep creating and
> > tearing down the PPP interface? I can also test and/or investigate this
> > issue if a suitable reproduction method is available.
> > 
> There are at least two issues resulting in similar Oops.
> 
> The first one goes with MTU/address/link state updates on the
> underlying interface: any such update on an interface used by a
> PPPoE connection will generally result in an Oops when releasing the
> PPPoE connection. This is fixed by e6740165b8f7 ("ppp: don't override
> sk->sk_state in pppoe_flush_dev()").

Without your patch ("ppp: don't override sk->sk_state in
pppoe_flush_dev()") I can see the following function calls being made
when changing the mtu on the underlying ethernet interface for the PPPoE
connection:

1. pppoe_flush_dev() - setting PPPOX_ZOMBIE

2. pppoe_connect - setting PPPOX_NONE (shown below)

/* Delete the old binding */
if (stage_session(po->pppoe_pa.sid)) {
pppox_unbind_sock(sk);
pn = pppoe_pernet(sock_net(sk));
delete_item(pn, po->pppoe_pa.sid,
po->pppoe_pa.remote, po->pppoe_ifindex);
if (po->pppoe_dev) {
dev_put(po->pppoe_dev);
po->pppoe_dev = NULL;
}

memset(sk_pppox(po) + 1, 0,
   sizeof(struct pppox_sock) - sizeof(struct sock));
sk->sk_state = PPPOX_NONE;
}

3. pppoe_release - No oops (since sk->sk_state is no longer in
{PPPOX_CONNECTED,PPPOX_BOUND,PPPOX_ZOMBIE})

It doesn't look to me like the above functions can execute
asynchronously but I'd have to look harder. I am using 3.16 by the way.

> 
> The second one seems to be trickier. It looks like a race wrt. PADT
> message reception. Reproducing the bug will probably require to
> generate some PADT flooding to a host that creates and releases PPPoE
> connections.

I will investigate the PADT message reception however since I am on 3.16
I don't have the commits for "pppoe: Use workqueue to die properly when
a PADT is received" and "pppoe: drop pppoe device in
pppoe_unbind_sock_work". 

Matt
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-05 Thread Guillaume Nault 
On Mon, Oct 05, 2015 at 04:08:51AM +, Matt Bennett wrote:
> Hi, I am seeing this panic occur occasionally however I am unsure how to
> go about reproducing it. Is it enough to simply keep creating and
> tearing down the PPP interface? I can also test and/or investigate this
> issue if a suitable reproduction method is available.
> 
There are at least two issues resulting in similar Oops.

The first one goes with MTU/address/link state updates on the
underlying interface: any such update on an interface used by a
PPPoE connection will generally result in an Oops when releasing the
PPPoE connection. This is fixed by e6740165b8f7 ("ppp: don't override
sk->sk_state in pppoe_flush_dev()").

The second one seems to be trickier. It looks like a race wrt. PADT
message reception. Reproducing the bug will probably require to
generate some PADT flooding to a host that creates and releases PPPoE
connections.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-05 Thread Guillaume Nault 
> I am doing just "dirty" patch like this, i cannot certainly remember if i
> was doing git reversal, because
> it was a while when i spotted this bug. After that pppoe server is not
> rebooting.
> 
> diff -Naur linux-4.2.2-vanilla/drivers/net/ppp/pppoe.c
> linux-4.2.2-changed/drivers/net/ppp/pppoe.c
> --- linux-4.2.2-vanilla/drivers/net/ppp/pppoe.c 2015-09-29
> 20:38:27.0 +0300
> +++ linux-4.2.2-changed/drivers/net/ppp/pppoe.c 2015-10-04
> 19:05:55.697732991 +0300
> @@ -519,7 +519,7 @@
> }
> 
> bh_unlock_sock(sk);
> -   if (!schedule_work(>proto.pppoe.padt_work))
> +// if (!schedule_work(>proto.pppoe.padt_work))
> sock_put(sk);
> }
> 
> @@ -633,7 +633,7 @@
> 
> lock_sock(sk);
> 
> -   INIT_WORK(>proto.pppoe.padt_work, pppoe_unbind_sock_work);
> +// INIT_WORK(>proto.pppoe.padt_work, pppoe_unbind_sock_work);
> 
> error = -EINVAL;
> if (sp->sa_protocol != PX_PROTO_OE)
> 
Ok, so this is clearly related with PADT message handling. Setting
sk->sk_state to PPPOX_ZOMBIE in pppoe_disc_rcv() looks wrong to me.
Furthurmore, at a first glance, it doesn't look necessary. If you're
feeling lucky, you can try the following diff (WARNING: not even
compile-tested!):

if (po) {
struct sock *sk = sk_pppox(po);
 
-   bh_lock_sock(sk);
-
-   /* If the user has locked the socket, just ignore
-* the packet.  With the way two rcv protocols hook into
-* one socket family type, we cannot (easily) distinguish
-* what kind of SKB it is during backlog rcv.
-*/
-   if (sock_owned_by_user(sk) == 0) {
-   /* We're no longer connect at the PPPOE layer,
-* and must wait for ppp channel to disconnect us.
-*/
-   sk->sk_state = PPPOX_ZOMBIE;
-   }
-
-   bh_unlock_sock(sk);
if (!schedule_work(>proto.pppoe.padt_work))
sock_put(sk);
}

I'll take a closer look and do proper testing during the week.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-04 Thread Matt Bennett 
Hi, I am seeing this panic occur occasionally however I am unsure how to
go about reproducing it. Is it enough to simply keep creating and
tearing down the PPP interface? I can also test and/or investigate this
issue if a suitable reproduction method is available.

On Sun, 2015-10-04 at 19:08 +0300, Denys Fedoryshchenko wrote:
> On 2015-10-02 20:54, Guillaume Nault wrote:

N�r��yb�X��ǧv�^�)޺{.n�+���z�^�)w*jg����ݢj/���z�ޖ��2�ޙ&�)ߡ�a�����G���h��j:+v���w��٥

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-04 Thread Denys Fedoryshchenko 

On 2015-10-02 20:54, Guillaume Nault wrote:

On Fri, Oct 02, 2015 at 11:01:45AM +0300, Denys Fedoryshchenko wrote:
Here is similar panic after patch applied (it might be different bug), 
got

over netconsole:

 [126348.617115] CPU: 0 PID: 5254 Comm: accel-pppd Not tainted
4.2.2-build-0087 #2
 [126348.617632] Hardware name: Intel Corporation S2600GZ/S2600GZ, 
BIOS

SE5C600.86B.02.03.0003.041920141333 04/19/2014
 [126348.618193] task: 8817cfbe ti: 8817c635 task.ti:
8817c635
 [126348.618696] RIP: 0010:[]
 [] pppoe_release+0x56/0x142 [pppoe]
 [126348.619306] RSP: 0018:8817c6353e28  EFLAGS: 00010202
 [126348.619601] RAX:  RBX: 8817a92b0400 RCX:

 [126348.620152] RDX: 0001 RSI: fe01 RDI:
8180c18a
 [126348.620715] RBP: 8817c6353e68 R08:  R09:

 [126348.621254] R10: 88173c02b210 R11: 0293 R12:
8817b3c18000
 [126348.621784] R13: 8817b3c18030 R14: 8817967f1140 R15:
8817d226c920
 [126348.622330] FS:  7f9444db9700() GS:8817dee0()
knlGS:
 [126348.622876] CS:  0010 DS:  ES:  CR0: 80050033
 [126348.623202] CR2: 0428 CR3: 0017c70b2000 CR4:
001406f0
 [126348.623760] Stack:
 [126348.624056]  000100200018
 
 0001
 8817b3c18000

 [126348.624925]  a00ec280
 8817b3c18030
 8817967f1140
 8817d226c920

 [126348.625736]  8817c6353e88
 8180820a
 88173c02b200
 0008

 [126348.626533] Call Trace:
 [126348.626873]  [] sock_release+0x1a/0x70
 [126348.627183]  [] sock_close+0xd/0x11
 [126348.627512]  [] __fput+0xdf/0x193
 [126348.627845]  [] fput+0x9/0xb
 [126348.628169]  [] task_work_run+0x78/0x8f
 [126348.628517]  [] do_notify_resume+0x40/0x4e
 [126348.628837]  [] int_signal+0x12/0x17


Ok, so there's another possibility for pppoe_release() to be called 
while
sk->sk_state is PPPOX_{CONNECTED,BOUND,ZOMBIE} but po->pppoe_dev is 
NULL.


I'll check the code to see if I can find any race wrt. po->pppoe_dev
and sk->sk_state settings.

In a previous message, you said you'd try reverting 287f3a943fef
("pppoe: Use workqueue to die properly when a PADT is received") and
related patches. I guess "related patches" means 665a6cd809f4 ("pppoe:
drop pppoe device in pppoe_unbind_sock_work"), right?.
Did these reverts give any successful result?

BTW, please don't top-post.
I am doing just "dirty" patch like this, i cannot certainly remember if 
i was doing git reversal, because
it was a while when i spotted this bug. After that pppoe server is not 
rebooting.


diff -Naur linux-4.2.2-vanilla/drivers/net/ppp/pppoe.c 
linux-4.2.2-changed/drivers/net/ppp/pppoe.c
--- linux-4.2.2-vanilla/drivers/net/ppp/pppoe.c 2015-09-29 
20:38:27.0 +0300
+++ linux-4.2.2-changed/drivers/net/ppp/pppoe.c 2015-10-04 
19:05:55.697732991 +0300

@@ -519,7 +519,7 @@
}

bh_unlock_sock(sk);
-   if (!schedule_work(>proto.pppoe.padt_work))
+// if (!schedule_work(>proto.pppoe.padt_work))
sock_put(sk);
}

@@ -633,7 +633,7 @@

lock_sock(sk);

-   INIT_WORK(>proto.pppoe.padt_work, pppoe_unbind_sock_work);
+// INIT_WORK(>proto.pppoe.padt_work, pppoe_unbind_sock_work);

error = -EINVAL;
if (sp->sa_protocol != PX_PROTO_OE)




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-02 Thread Guillaume Nault 
On Fri, Oct 02, 2015 at 11:01:45AM +0300, Denys Fedoryshchenko wrote:
> Here is similar panic after patch applied (it might be different bug), got
> over netconsole:
> 
>  [126348.617115] CPU: 0 PID: 5254 Comm: accel-pppd Not tainted
> 4.2.2-build-0087 #2
>  [126348.617632] Hardware name: Intel Corporation S2600GZ/S2600GZ, BIOS
> SE5C600.86B.02.03.0003.041920141333 04/19/2014
>  [126348.618193] task: 8817cfbe ti: 8817c635 task.ti:
> 8817c635
>  [126348.618696] RIP: 0010:[]
>  [] pppoe_release+0x56/0x142 [pppoe]
>  [126348.619306] RSP: 0018:8817c6353e28  EFLAGS: 00010202
>  [126348.619601] RAX:  RBX: 8817a92b0400 RCX:
> 
>  [126348.620152] RDX: 0001 RSI: fe01 RDI:
> 8180c18a
>  [126348.620715] RBP: 8817c6353e68 R08:  R09:
> 
>  [126348.621254] R10: 88173c02b210 R11: 0293 R12:
> 8817b3c18000
>  [126348.621784] R13: 8817b3c18030 R14: 8817967f1140 R15:
> 8817d226c920
>  [126348.622330] FS:  7f9444db9700() GS:8817dee0()
> knlGS:
>  [126348.622876] CS:  0010 DS:  ES:  CR0: 80050033
>  [126348.623202] CR2: 0428 CR3: 0017c70b2000 CR4:
> 001406f0
>  [126348.623760] Stack:
>  [126348.624056]  000100200018
>  
>  0001
>  8817b3c18000
> 
>  [126348.624925]  a00ec280
>  8817b3c18030
>  8817967f1140
>  8817d226c920
> 
>  [126348.625736]  8817c6353e88
>  8180820a
>  88173c02b200
>  0008
> 
>  [126348.626533] Call Trace:
>  [126348.626873]  [] sock_release+0x1a/0x70
>  [126348.627183]  [] sock_close+0xd/0x11
>  [126348.627512]  [] __fput+0xdf/0x193
>  [126348.627845]  [] fput+0x9/0xb
>  [126348.628169]  [] task_work_run+0x78/0x8f
>  [126348.628517]  [] do_notify_resume+0x40/0x4e
>  [126348.628837]  [] int_signal+0x12/0x17

Ok, so there's another possibility for pppoe_release() to be called while
sk->sk_state is PPPOX_{CONNECTED,BOUND,ZOMBIE} but po->pppoe_dev is NULL.

I'll check the code to see if I can find any race wrt. po->pppoe_dev
and sk->sk_state settings.

In a previous message, you said you'd try reverting 287f3a943fef
("pppoe: Use workqueue to die properly when a PADT is received") and
related patches. I guess "related patches" means 665a6cd809f4 ("pppoe:
drop pppoe device in pppoe_unbind_sock_work"), right?.
Did these reverts give any successful result?

BTW, please don't top-post.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-10-02 Thread Denys Fedoryshchenko 
Here is similar panic after patch applied (it might be different bug), 
got over netconsole:


 [126348.610996] BUG: unable to handle kernel
 NULL pointer dereference
 at 0428
 [126348.611656] IP:
 [] pppoe_release+0x56/0x142 [pppoe]
 [126348.612033] PGD 17d0b03067
 PUD 17c721b067
 PMD 0

 [126348.612545] Oops:  [#1]
 SMP

 [126348.612981] Modules linked in:
 act_skbedit
 sch_fq
 cls_fw
 act_police
 cls_u32
 sch_ingress
 sch_sfq
 sch_htb
 pppoe
 pppox
 ppp_generic
 slhc
 netconsole
 configfs
 xt_nat
 ts_bm
 xt_string
 xt_connmark
 xt_TCPMSS
 xt_tcpudp
 xt_mark
 iptable_filter
 iptable_nat
 nf_conntrack_ipv4
 nf_defrag_ipv4
 nf_nat_ipv4
 nf_nat
 nf_conntrack
 iptable_mangle
 ip_tables
 x_tables
 8021q
 garp
 mrp
 stp
 llc
 bonding

 [126348.617115] CPU: 0 PID: 5254 Comm: accel-pppd Not tainted 
4.2.2-build-0087 #2
 [126348.617632] Hardware name: Intel Corporation S2600GZ/S2600GZ, BIOS 
SE5C600.86B.02.03.0003.041920141333 04/19/2014
 [126348.618193] task: 8817cfbe ti: 8817c635 task.ti: 
8817c635

 [126348.618696] RIP: 0010:[]
 [] pppoe_release+0x56/0x142 [pppoe]
 [126348.619306] RSP: 0018:8817c6353e28  EFLAGS: 00010202
 [126348.619601] RAX:  RBX: 8817a92b0400 RCX: 

 [126348.620152] RDX: 0001 RSI: fe01 RDI: 
8180c18a
 [126348.620715] RBP: 8817c6353e68 R08:  R09: 

 [126348.621254] R10: 88173c02b210 R11: 0293 R12: 
8817b3c18000
 [126348.621784] R13: 8817b3c18030 R14: 8817967f1140 R15: 
8817d226c920
 [126348.622330] FS:  7f9444db9700() GS:8817dee0() 
knlGS:

 [126348.622876] CS:  0010 DS:  ES:  CR0: 80050033
 [126348.623202] CR2: 0428 CR3: 0017c70b2000 CR4: 
001406f0

 [126348.623760] Stack:
 [126348.624056]  000100200018
 
 0001
 8817b3c18000

 [126348.624925]  a00ec280
 8817b3c18030
 8817967f1140
 8817d226c920

 [126348.625736]  8817c6353e88
 8180820a
 88173c02b200
 0008

 [126348.626533] Call Trace:
 [126348.626873]  [] sock_release+0x1a/0x70
 [126348.627183]  [] sock_close+0xd/0x11
 [126348.627512]  [] __fput+0xdf/0x193
 [126348.627845]  [] fput+0x9/0xb
 [126348.628169]  [] task_work_run+0x78/0x8f
 [126348.628517]  [] do_notify_resume+0x40/0x4e
 [126348.628837]  [] int_signal+0x12/0x17
 [126348.629131] Code:
 48
 8b
 83
 e0
 00
 00
 00
 a8
 01
 74
 12
 48
 89
 df
 e8
 0d
 24
 72
 e1
 b8
 f7
 ff
 ff
 ff
 e9
 eb
 00
 00
 00
 8a
 43
 12
 a8
 0b
 74
 1c
 48
 8b
 83
 a0
 02
 00
 00

 8b
 80
 28
 04
 00
 00
 65
 ff
 08
 48
 c7
 83
 a0
 02
 00
 00
 00
 00
 00
 00

 [126348.635060] RIP
 [] pppoe_release+0x56/0x142 [pppoe]
 [126348.635432]  RSP 
 [126348.635718] CR2: 0428
 [126348.641165] ---[ end trace 911ff90a1416e3d1 ]---
 [126348.653235] Kernel panic - not syncing: Fatal exception
 [126348.653538] Kernel Offset: disabled
 [126348.677177] Rebooting in 5 seconds..




On 2015-09-30 12:45, Guillaume Nault wrote:
Since commit 2b018d57ff18 ("pppoe: drop PPPOX_ZOMBIEs in 
pppoe_release"),

pppoe_release() calls dev_put(po->pppoe_dev) if sk is in the
PPPOX_ZOMBIE state. But pppoe_flush_dev() can set sk->sk_state to
PPPOX_ZOMBIE _and_ reset po->pppoe_dev to NULL. This leads to the
following oops:

[  570.140800] BUG: unable to handle kernel NULL pointer dereference
at 04e0
[  570.142931] IP: [] pppoe_release+0x50/0x101 
[pppoe]

[  570.144601] PGD 3d119067 PUD 3dbc1067 PMD 0
[  570.144601] Oops:  [#1] SMP
[  570.144601] Modules linked in: l2tp_ppp l2tp_netlink l2tp_core
ip6_udp_tunnel udp_tunnel pppoe pppox ppp_generic slhc loop
crc32c_intel ghash_clmulni_intel jitterentropy_rng sha256_generic hmac
drbg ansi_cprng aesni_intel aes_x86_64 ablk_helper cryptd lrw gf128mul
glue_helper acpi_cpufreq evdev serio_raw processor button ext4 crc16
mbcache jbd2 virtio_net virtio_blk virtio_pci virtio_ring virtio
[  570.144601] CPU: 1 PID: 15738 Comm: ppp-apitest Not tainted 4.2.0 #1
[  570.144601] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS Debian-1.8.2-1 04/01/2014
[  570.144601] task: 88003d30d600 ti: 880036b6 task.ti:
880036b6
[  570.144601] RIP: 0010:[]  []
pppoe_release+0x50/0x101 [pppoe]
[  570.144601] RSP: 0018:880036b63e08  EFLAGS: 00010202
[  570.144601] RAX:  RBX: 88003434 RCX: 
0206
[  570.144601] RDX: 0006 RSI: 88003d30dd20 RDI: 
88003d30dd20
[  570.144601] RBP: 880036b63e28 R08: 0001 R09: 

[  570.144601] R10: 7ffee9b50420 R11: 880034340078 R12: 
8800387ec780
[  570.144601] R13: 8800387ec7b0 R14: 88003e222aa0 R15: 
8800387ec7b0

[  570.144601] FS:  7f5672f48700() GS:88003fc8()
knlGS:
[  570.144601] CS:  0010 DS:  ES:  CR0: 80050033
[  570.144601] CR2:

[PATCH net] ppp: don't override sk->sk_state in pppoe_flush_dev()

2015-09-30 Thread Guillaume Nault 
Since commit 2b018d57ff18 ("pppoe: drop PPPOX_ZOMBIEs in pppoe_release"),
pppoe_release() calls dev_put(po->pppoe_dev) if sk is in the
PPPOX_ZOMBIE state. But pppoe_flush_dev() can set sk->sk_state to
PPPOX_ZOMBIE _and_ reset po->pppoe_dev to NULL. This leads to the
following oops:

[  570.140800] BUG: unable to handle kernel NULL pointer dereference at 
04e0
[  570.142931] IP: [] pppoe_release+0x50/0x101 [pppoe]
[  570.144601] PGD 3d119067 PUD 3dbc1067 PMD 0
[  570.144601] Oops:  [#1] SMP
[  570.144601] Modules linked in: l2tp_ppp l2tp_netlink l2tp_core 
ip6_udp_tunnel udp_tunnel pppoe pppox ppp_generic slhc loop crc32c_intel 
ghash_clmulni_intel jitterentropy_rng sha256_generic hmac drbg ansi_cprng 
aesni_intel aes_x86_64 ablk_helper cryptd lrw gf128mul glue_helper acpi_cpufreq 
evdev serio_raw processor button ext4 crc16 mbcache jbd2 virtio_net virtio_blk 
virtio_pci virtio_ring virtio
[  570.144601] CPU: 1 PID: 15738 Comm: ppp-apitest Not tainted 4.2.0 #1
[  570.144601] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
Debian-1.8.2-1 04/01/2014
[  570.144601] task: 88003d30d600 ti: 880036b6 task.ti: 
880036b6
[  570.144601] RIP: 0010:[]  [] 
pppoe_release+0x50/0x101 [pppoe]
[  570.144601] RSP: 0018:880036b63e08  EFLAGS: 00010202
[  570.144601] RAX:  RBX: 88003434 RCX: 0206
[  570.144601] RDX: 0006 RSI: 88003d30dd20 RDI: 88003d30dd20
[  570.144601] RBP: 880036b63e28 R08: 0001 R09: 
[  570.144601] R10: 7ffee9b50420 R11: 880034340078 R12: 8800387ec780
[  570.144601] R13: 8800387ec7b0 R14: 88003e222aa0 R15: 8800387ec7b0
[  570.144601] FS:  7f5672f48700() GS:88003fc8() 
knlGS:
[  570.144601] CS:  0010 DS:  ES:  CR0: 80050033
[  570.144601] CR2: 04e0 CR3: 37f7e000 CR4: 000406a0
[  570.144601] Stack:
[  570.144601]  a018f240 8800387ec780 a018f240 
8800387ec7b0
[  570.144601]  880036b63e48 812caabe 880039e4e000 
0008
[  570.144601]  880036b63e58 812cabad 880036b63ea8 
811347f5
[  570.144601] Call Trace:
[  570.144601]  [] sock_release+0x1a/0x75
[  570.144601]  [] sock_close+0xd/0x11
[  570.144601]  [] __fput+0xff/0x1a5
[  570.144601]  [] fput+0x9/0xb
[  570.144601]  [] task_work_run+0x66/0x90
[  570.144601]  [] prepare_exit_to_usermode+0x8c/0xa7
[  570.144601]  [] syscall_return_slowpath+0x16d/0x19b
[  570.144601]  [] int_ret_from_sys_call+0x25/0x9f
[  570.144601] Code: 48 8b 83 c8 01 00 00 a8 01 74 12 48 89 df e8 8b 27 14 e1 
b8 f7 ff ff ff e9 b7 00 00 00 8a 43 12 a8 0b 74 1c 48 8b 83 a8 04 00 00 <48> 8b 
80 e0 04 00 00 65 ff 08 48 c7 83 a8 04 00 00 00 00 00 00
[  570.144601] RIP  [] pppoe_release+0x50/0x101 [pppoe]
[  570.144601]  RSP 
[  570.144601] CR2: 04e0
[  570.200518] ---[ end trace 46956baf17349563 ]---

pppoe_flush_dev() has no reason to override sk->sk_state with
PPPOX_ZOMBIE. pppox_unbind_sock() already sets sk->sk_state to
PPPOX_DEAD, which is the correct state given that sk is unbound and
po->pppoe_dev is NULL.

Fixes: 2b018d57ff18 ("pppoe: drop PPPOX_ZOMBIEs in pppoe_release")
Tested-by: Oleksii Berezhniak 
Signed-off-by: Guillaume Nault 
---
 drivers/net/ppp/pppoe.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 3837ae3..2ed7506 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -313,7 +313,6 @@ static void pppoe_flush_dev(struct net_device *dev)
if (po->pppoe_dev == dev &&
sk->sk_state & (PPPOX_CONNECTED | PPPOX_BOUND | 
PPPOX_ZOMBIE)) {
pppox_unbind_sock(sk);
-   sk->sk_state = PPPOX_ZOMBIE;
sk->sk_state_change(sk);
po->pppoe_dev = NULL;
dev_put(dev);
-- 
2.5.3

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html