On 2019/5/7 下午10:41, Cong Wang wrote:
On Mon, May 6, 2019 at 11:19 PM Jason Wang <jasow...@redhat.com> wrote:
On 2019/5/7 下午12:54, Cong Wang wrote:
On Mon, May 6, 2019 at 9:03 PM Jason Wang <jasow...@redhat.com> wrote:
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index e9ca1c0..32a0b23 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -700,6 +700,8 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
tun->tfiles[tun->numqueues - 1]);
ntfile = rtnl_dereference(tun->tfiles[index]);
ntfile->queue_index = index;
+ rcu_assign_pointer(tun->tfiles[tun->numqueues - 1],
+ NULL);
How does this work? Existing readers could still read this
tun->tfiles[tun->numqueues - 1] before you NULL it. And,
_if_ the following sock_put() is the one frees it, you still miss
a RCU grace period.
if (clean) {
RCU_INIT_POINTER(tfile->tun, NULL);
sock_put(&tfile->sk);
Thanks.
My understanding is the socket will never be freed for this sock_put().
We just drop an extra reference count we held when the socket was
attached to the netdevice (there's a sock_hold() in tun_attach()). The
real free should happen at another sock_put() in the end of this function.
So you are saying readers will never read this sock after free, then
what are you fixing with this patch? Nothing, right?
It's the issue of the second sock_put() in tun_detach() not the first
one. Without proper synchronization for tun->numqueues, tun_net_xmit()
can actually dereference a tfile which has been freed by second
sock_put(). The synchornize_net() doesn't help since we're trying to
synchronize through tun->numqueues.
As I said, reading a stale tun->numqueues is fine, you just keep
believing it is a problem.
This is only true if you can make sure tfile[tun->numqueues] is not
freed. Either my patch or SOCK_RCU_FREE can solve this, but for
SOCK_RCU_FREE we need do extra careful audit to make sure it doesn't
break someting. So synchronize through pointers in tfiles[] which is
already protected by RCU is much more easier. It can make sure no
dereference from xmit path after synchornize_net(). And this matches the
assumptions of the codes after synchronize_net().
Thanks
