:
looks like a race between a NIT socket (tcpdump, maybe?) that is closing,
and a standard tcp socket.. packet_release() takes the po->bind_lock
to remove the socket from the ptype_all NIT queue. but how does
that sync with the Tx path for other af_inet/af_inet6 sockets?
--Sowmini
:
looks like a race between a NIT socket (tcpdump, maybe?) that is closing,
and a standard tcp socket.. packet_release() takes the po->bind_lock
to remove the socket from the ptype_all NIT queue. but how does
that sync with the Tx path for other af_inet/af_inet6 sockets?
@@ -2924,6 +2924,8 @@ static int packet_release(struct socket *sock)sock_prot_inuse_add(net,sk->sk_prot,-1);preempt_enable();+synchronize_net();+spin_lock(&po->bind_lock);unregister_prot_hook(sk,false);packet_cached_dev_reset(po);
More likely the bug is in fanout_add(), with a buggy sequence in error
case, and not correct locking.
kfree(po->rollover);
po->rollover = NULL;
Two cpus entering fanout_add() (using the same af_packet socket,
syzkaller courtesy...) might both see po->fanout being NULL.
Then they grab the mutex. Too late...
From: Eric Dumazet <hidden> Date: 2017-02-10 03:24:51
On Thu, 2017-02-09 at 19:19 -0800, Eric Dumazet wrote:
More likely the bug is in fanout_add(), with a buggy sequence in error
case, and not correct locking.
kfree(po->rollover);
po->rollover = NULL;
Two cpus entering fanout_add() (using the same af_packet socket,
syzkaller courtesy...) might both see po->fanout being NULL.
Then they grab the mutex. Too late...
Patch could be :
net/packet/af_packet.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
More likely the bug is in fanout_add(), with a buggy sequence in error
case, and not correct locking.
kfree(po->rollover);
po->rollover = NULL;
Two cpus entering fanout_add() (using the same af_packet socket,
syzkaller courtesy...) might both see po->fanout being NULL.
Then they grab the mutex. Too late...
I'm not sure I follow- aiui the panic was in acceessing the
sk_receive_queue.lock in a socket that had been closed earlier. I think
the assumption is that rcu_read_lock_bh in __dev_queue_xmit (and
rcu_read_lock in dev_queue_xmit_nit?) should make sure that the nit
packet delivery can be done safely, and the synchronize_net in
packet_release() makes sure that the Tx paths are quiesced before freeing
the socket. What is the race-hole here? Does it have to do with the
_bh and softirq context, somehow?
--Sowmini
From: Eric Dumazet <edumazet@google.com> Date: 2017-02-10 04:27:17
On Thu, Feb 9, 2017 at 7:33 PM, Sowmini Varadhan
[off-list ref] wrote:
On (02/09/17 19:19), Eric Dumazet wrote:
quoted
More likely the bug is in fanout_add(), with a buggy sequence in error
case, and not correct locking.
kfree(po->rollover);
po->rollover = NULL;
Two cpus entering fanout_add() (using the same af_packet socket,
syzkaller courtesy...) might both see po->fanout being NULL.
Then they grab the mutex. Too late...
I'm not sure I follow- aiui the panic was in acceessing the
sk_receive_queue.lock in a socket that had been closed earlier. I think
the assumption is that rcu_read_lock_bh in __dev_queue_xmit (and
rcu_read_lock in dev_queue_xmit_nit?) should make sure that the nit
packet delivery can be done safely, and the synchronize_net in
packet_release() makes sure that the Tx paths are quiesced before freeing
the socket. What is the race-hole here? Does it have to do with the
_bh and softirq context, somehow?
We have probably a dozen of bugs to fix in af_packet.c
The race in fanout_add() is one ot theml.
I do not believe Anoob Soman sent his fixes btw ...
( Look for this thread : http://marc.info/?l=linux-netdev&m=148588680525648&w=2
From: Cong Wang <hidden> Date: 2017-02-10 17:59:06
On Thu, Feb 9, 2017 at 7:23 PM, Eric Dumazet [off-list ref] wrote:
On Thu, 2017-02-09 at 19:19 -0800, Eric Dumazet wrote:
quoted
More likely the bug is in fanout_add(), with a buggy sequence in error
case, and not correct locking.
kfree(po->rollover);
po->rollover = NULL;
Two cpus entering fanout_add() (using the same af_packet socket,
syzkaller courtesy...) might both see po->fanout being NULL.
Then they grab the mutex. Too late...
Patch could be :
For me, clearly the data structure that use-after-free'd is struct sock
rather than struct packet_rollover.
From: Eric Dumazet <hidden> Date: 2017-02-10 18:04:47
On Fri, 2017-02-10 at 09:49 -0800, Cong Wang wrote:
On Thu, Feb 9, 2017 at 7:23 PM, Eric Dumazet [off-list ref] wrote:
quoted
On Thu, 2017-02-09 at 19:19 -0800, Eric Dumazet wrote:
quoted
More likely the bug is in fanout_add(), with a buggy sequence in error
case, and not correct locking.
kfree(po->rollover);
po->rollover = NULL;
Two cpus entering fanout_add() (using the same af_packet socket,
syzkaller courtesy...) might both see po->fanout being NULL.
Then they grab the mutex. Too late...
Patch could be :
For me, clearly the data structure that use-after-free'd is struct sock
rather than struct packet_rollover.
From: Cong Wang <hidden> Date: 2017-02-10 18:08:40
On Thu, Feb 9, 2017 at 7:33 PM, Sowmini Varadhan
[off-list ref] wrote:
On (02/09/17 19:19), Eric Dumazet wrote:
quoted
More likely the bug is in fanout_add(), with a buggy sequence in error
case, and not correct locking.
kfree(po->rollover);
po->rollover = NULL;
Two cpus entering fanout_add() (using the same af_packet socket,
syzkaller courtesy...) might both see po->fanout being NULL.
Then they grab the mutex. Too late...
I'm not sure I follow- aiui the panic was in acceessing the
sk_receive_queue.lock in a socket that had been closed earlier. I think
the assumption is that rcu_read_lock_bh in __dev_queue_xmit (and
rcu_read_lock in dev_queue_xmit_nit?) should make sure that the nit
packet delivery can be done safely, and the synchronize_net in
packet_release() makes sure that the Tx paths are quiesced before freeing
the socket. What is the race-hole here? Does it have to do with the
_bh and softirq context, somehow?
My understanding about the race here is packet_release() doesn't
wait for flying packets correctly, which leads to a flying packet still
refers to the struct sock which is being released.
This could happen because struct packet_fanout is refcn'ted, it is
still there when this is not the last sock referring it, therefore, the
callback packet_rcv_fanout() is not removed yet. When packet_release()
tries to remove the pointer to struct sock from f->arr[i] in
__fanout_unlink(), a flying packet could race with f->arr[i]:
po = pkt_sk(f->arr[idx]);
Of course, the fix may not be as easy as just adding a synchronize_net(),
perhaps we need the spinlock too in fanout_demux_rollover().
At least I believe this explains the crash Dmitry reported.
From: Eric Dumazet <hidden> Date: 2017-02-10 18:10:19
On Fri, 2017-02-10 at 09:59 -0800, Eric Dumazet wrote:
On Fri, 2017-02-10 at 09:49 -0800, Cong Wang wrote:
quoted
On Thu, Feb 9, 2017 at 7:23 PM, Eric Dumazet [off-list ref] wrote:
quoted
On Thu, 2017-02-09 at 19:19 -0800, Eric Dumazet wrote:
quoted
More likely the bug is in fanout_add(), with a buggy sequence in error
case, and not correct locking.
kfree(po->rollover);
po->rollover = NULL;
Two cpus entering fanout_add() (using the same af_packet socket,
syzkaller courtesy...) might both see po->fanout being NULL.
Then they grab the mutex. Too late...
Patch could be :
For me, clearly the data structure that use-after-free'd is struct sock
rather than struct packet_rollover.
From: Cong Wang <hidden> Date: 2017-02-10 18:10:25
On Fri, Feb 10, 2017 at 9:59 AM, Eric Dumazet [off-list ref] wrote:
On Fri, 2017-02-10 at 09:49 -0800, Cong Wang wrote:
quoted
On Thu, Feb 9, 2017 at 7:23 PM, Eric Dumazet [off-list ref] wrote:
quoted
On Thu, 2017-02-09 at 19:19 -0800, Eric Dumazet wrote:
quoted
More likely the bug is in fanout_add(), with a buggy sequence in error
case, and not correct locking.
kfree(po->rollover);
po->rollover = NULL;
Two cpus entering fanout_add() (using the same af_packet socket,
syzkaller courtesy...) might both see po->fanout being NULL.
Then they grab the mutex. Too late...
Patch could be :
For me, clearly the data structure that use-after-free'd is struct sock
rather than struct packet_rollover.
Fine. But your patch makes absolutely no sense.
I don't have to give a 100% correct patch to prove my explanation
of the crash. At least it makes more sense than yours...
From: Cong Wang <hidden> Date: 2017-02-10 18:40:53
On Fri, Feb 10, 2017 at 10:02 AM, Eric Dumazet [off-list ref] wrote:
On Fri, 2017-02-10 at 09:59 -0800, Eric Dumazet wrote:
quoted
On Fri, 2017-02-10 at 09:49 -0800, Cong Wang wrote:
quoted
On Thu, Feb 9, 2017 at 7:23 PM, Eric Dumazet [off-list ref] wrote:
quoted
On Thu, 2017-02-09 at 19:19 -0800, Eric Dumazet wrote:
quoted
More likely the bug is in fanout_add(), with a buggy sequence in error
case, and not correct locking.
kfree(po->rollover);
po->rollover = NULL;
Two cpus entering fanout_add() (using the same af_packet socket,
syzkaller courtesy...) might both see po->fanout being NULL.
Then they grab the mutex. Too late...
Patch could be :
For me, clearly the data structure that use-after-free'd is struct sock
rather than struct packet_rollover.
My understanding about the race here is packet_release() doesn't
wait for flying packets correctly, which leads to a flying packet still
refers to the struct sock which is being released.
This could happen because struct packet_fanout is refcn'ted, it is
:
At least I believe this explains the crash Dmitry reported.
hmm, the proof of the pudding is in the eating- would be good to
be able to reliably reproduce this somewhere (thus proving that
root-cause analysis is rock-solid), maybe by introducing artificial
delays to slow down paths..
I'm travelling at the moment but may be able to give this (try
to reproduce it reliably) next week.
--Sowmini
I've not been able to reproduce Dmitry's panic (though I did not try
very hard either) but there's a call to fanout_release from packet_release
before the synchronize_net() - I wonder if this could end up kfree'ing f
when there are threads in the middle of dev_queue_xmit_nit().
--Sowmini
I've not been able to reproduce Dmitry's panic (though I did not try
very hard either) but there's a call to fanout_release from packet_release
before the synchronize_net() - I wonder if this could end up kfree'ing f
when there are threads in the middle of dev_queue_xmit_nit().
^
ffff8801cf083e00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff8801cf083e80: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
==================================================================
From: Cong Wang <hidden> Date: 2017-02-17 19:27:37
On Mon, Feb 13, 2017 at 7:17 AM, Dmitry Vyukov [off-list ref] wrote:
Another similar one:
The other possibility is: __fanout_link() is called twice on the same
packet sock
for some reason, but __fanout_unlink() only unlinks the first one, which led to
this use-after-free. However, the po->running and po->fanout seem enough
to guarantee this should not happen. I still want to point this out in case I
miss anything here so that other people could figure it out.