[PATCH net V2] tuntap: synchronize through tfiles array instead of tun->numqueues

[PATCH net V2] tuntap: synchronize through tfiles array instead of tun->numqueues

[Jason Wang]

When a queue(tfile) is detached through __tun_detach(), we move the
last enabled tfile to the position where detached one sit but don't
NULL out last position. We expect to synchronize the datapath through
tun->numqueues. Unfortunately, this won't work since we're lacking
sufficient mechanism to order or synchronize the access to
tun->numqueues.

To fix this, NULL out the last position during detaching and check
RCU protected tfile against NULL instead of checking tun->numqueues in
datapath.

Cc: YueHaibing <yuehaibing@huawei.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: weiyongjun (A) <weiyongjun1@huawei.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Fixes: c8d68e6be1c3b ("tuntap: multiqueue support")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes from V1:
- keep the check in tun_xdp_xmit()
---
 drivers/net/tun.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

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);
 
 		--tun->numqueues;
 		if (clean) {
@@ -1082,7 +1084,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
 	tfile = rcu_dereference(tun->tfiles[txq]);
 
 	/* Drop packet if interface is not attached */
-	if (txq >= tun->numqueues)
+	if (!tfile)
 		goto drop;
 
 	if (!rcu_dereference(tun->steering_prog))
@@ -1313,6 +1315,10 @@ static int tun_xdp_xmit(struct net_device *dev, int n,
 
 	tfile = rcu_dereference(tun->tfiles[smp_processor_id() %
 					    numqueues]);
+	if (!tfile) {
+		rcu_read_unlock();
+		return -ENXIO; /* Caller will free/return all frames */
+	}
 
 	spin_lock(&tfile->tx_ring.producer_lock);
 	for (i = 0; i < n; i++) {
-- 
1.8.3.1

Re: [PATCH net V2] tuntap: synchronize through tfiles array instead of tun->numqueues

[Cong Wang]

On Mon, May 6, 2019 at 9:03 PM Jason Wang <jasowang@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.

Re: [PATCH net V2] tuntap: synchronize through tfiles array instead of tun->numqueues

[Jason Wang]

On 2019/5/7 下午12:54, Cong Wang wrote:
> On Mon, May 6, 2019 at 9:03 PM Jason Wang <jasowang@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.

Thanks

Re: [PATCH net V2] tuntap: synchronize through tfiles array instead of tun->numqueues

[Cong Wang]

On Mon, May 6, 2019 at 11:19 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2019/5/7 下午12:54, Cong Wang wrote:
> > On Mon, May 6, 2019 at 9:03 PM Jason Wang <jasowang@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?

As I said, reading a stale tun->numqueues is fine, you just keep
believing it is a problem.

Re: [PATCH net V2] tuntap: synchronize through tfiles array instead of tun->numqueues

[Jason Wang]

On 2019/5/7 下午10:41, Cong Wang wrote:
> On Mon, May 6, 2019 at 11:19 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2019/5/7 下午12:54, Cong Wang wrote:
>>> On Mon, May 6, 2019 at 9:03 PM Jason Wang <jasowang@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

Re: [PATCH net V2] tuntap: synchronize through tfiles array instead of tun->numqueues

[Michael S. Tsirkin]

On Tue, May 07, 2019 at 12:03:36AM -0400, Jason Wang wrote:
> When a queue(tfile) is detached through __tun_detach(), we move the
> last enabled tfile to the position where detached one sit but don't
> NULL out last position. We expect to synchronize the datapath through
> tun->numqueues. Unfortunately, this won't work since we're lacking
> sufficient mechanism to order or synchronize the access to
> tun->numqueues.
> 
> To fix this, NULL out the last position during detaching and check
> RCU protected tfile against NULL instead of checking tun->numqueues in
> datapath.
> 
> Cc: YueHaibing <yuehaibing@huawei.com>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: weiyongjun (A) <weiyongjun1@huawei.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Fixes: c8d68e6be1c3b ("tuntap: multiqueue support")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> Changes from V1:
> - keep the check in tun_xdp_xmit()
> ---
>  drivers/net/tun.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> 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);
>  
>  		--tun->numqueues;
>  		if (clean) {
> @@ -1082,7 +1084,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
>  	tfile = rcu_dereference(tun->tfiles[txq]);
>  
>  	/* Drop packet if interface is not attached */
> -	if (txq >= tun->numqueues)
> +	if (!tfile)
>  		goto drop;
>  
>  	if (!rcu_dereference(tun->steering_prog))

Hmm don't we need to range check txq?


> @@ -1313,6 +1315,10 @@ static int tun_xdp_xmit(struct net_device *dev, int n,
>  
>  	tfile = rcu_dereference(tun->tfiles[smp_processor_id() %
>  					    numqueues]);
> +	if (!tfile) {
> +		rcu_read_unlock();
> +		return -ENXIO; /* Caller will free/return all frames */
> +	}
>  
>  	spin_lock(&tfile->tx_ring.producer_lock);
>  	for (i = 0; i < n; i++) {
> -- 
> 1.8.3.1

Re: [PATCH net V2] tuntap: synchronize through tfiles array instead of tun->numqueues

[Jason Wang]

On 2019/5/8 下午12:16, Michael S. Tsirkin wrote:
> On Tue, May 07, 2019 at 12:03:36AM -0400, Jason Wang wrote:
>> When a queue(tfile) is detached through __tun_detach(), we move the
>> last enabled tfile to the position where detached one sit but don't
>> NULL out last position. We expect to synchronize the datapath through
>> tun->numqueues. Unfortunately, this won't work since we're lacking
>> sufficient mechanism to order or synchronize the access to
>> tun->numqueues.
>>
>> To fix this, NULL out the last position during detaching and check
>> RCU protected tfile against NULL instead of checking tun->numqueues in
>> datapath.
>>
>> Cc: YueHaibing <yuehaibing@huawei.com>
>> Cc: Cong Wang <xiyou.wangcong@gmail.com>
>> Cc: weiyongjun (A) <weiyongjun1@huawei.com>
>> Cc: Eric Dumazet <eric.dumazet@gmail.com>
>> Fixes: c8d68e6be1c3b ("tuntap: multiqueue support")
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> Changes from V1:
>> - keep the check in tun_xdp_xmit()
>> ---
>>   drivers/net/tun.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> 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);
>>   
>>   		--tun->numqueues;
>>   		if (clean) {
>> @@ -1082,7 +1084,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
>>   	tfile = rcu_dereference(tun->tfiles[txq]);
>>   
>>   	/* Drop packet if interface is not attached */
>> -	if (txq >= tun->numqueues)
>> +	if (!tfile)
>>   		goto drop;
>>   
>>   	if (!rcu_dereference(tun->steering_prog))
> Hmm don't we need to range check txq?


Looks not since tun_select_queue will always return a value which is 
less than MAX_TAP_QUEUES. And we NULL out the last enabled queue in 
tun_detach().

Thanks


>
>
>> @@ -1313,6 +1315,10 @@ static int tun_xdp_xmit(struct net_device *dev, int n,
>>   
>>   	tfile = rcu_dereference(tun->tfiles[smp_processor_id() %
>>   					    numqueues]);
>> +	if (!tfile) {
>> +		rcu_read_unlock();
>> +		return -ENXIO; /* Caller will free/return all frames */
>> +	}
>>   
>>   	spin_lock(&tfile->tx_ring.producer_lock);
>>   	for (i = 0; i < n; i++) {
>> -- 
>> 1.8.3.1

Re: [PATCH net V2] tuntap: synchronize through tfiles array instead of tun->numqueues

[David Miller]

From: Jason Wang <jasowang@redhat.com>
Date: Tue,  7 May 2019 00:03:36 -0400

> @@ -1313,6 +1315,10 @@ static int tun_xdp_xmit(struct net_device *dev, int n,
>  
>  	tfile = rcu_dereference(tun->tfiles[smp_processor_id() %
>  					    numqueues]);
> +	if (!tfile) {
> +		rcu_read_unlock();
> +		return -ENXIO; /* Caller will free/return all frames */
> +	}
>  
>  	spin_lock(&tfile->tx_ring.producer_lock);
>  	for (i = 0; i < n; i++) {

The only way we can see a NULL here is if a detach happened in parallel,
and if that happens we should retry the tfile[] indexing after resampling
numqueues rather than dropping the packet.

Re: [PATCH net V2] tuntap: synchronize through tfiles array instead of tun->numqueues

[Jason Wang]

On 2019/5/9 上午1:36, David Miller wrote:
> From: Jason Wang <jasowang@redhat.com>
> Date: Tue,  7 May 2019 00:03:36 -0400
>
>> @@ -1313,6 +1315,10 @@ static int tun_xdp_xmit(struct net_device *dev, int n,
>>   
>>   	tfile = rcu_dereference(tun->tfiles[smp_processor_id() %
>>   					    numqueues]);
>> +	if (!tfile) {
>> +		rcu_read_unlock();
>> +		return -ENXIO; /* Caller will free/return all frames */
>> +	}
>>   
>>   	spin_lock(&tfile->tx_ring.producer_lock);
>>   	for (i = 0; i < n; i++) {
> The only way we can see a NULL here is if a detach happened in parallel,
> and if that happens we should retry the tfile[] indexing after resampling
> numqueues rather than dropping the packet.


Ok, will fix this in V3.

Thanks

Re: [PATCH net V2] tuntap: synchronize through tfiles array instead of tun->numqueues

[Cong Wang]

On Tue, May 7, 2019 at 7:54 PM Jason Wang <jasowang@redhat.com> wrote:
> 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().
>

It is hard to tell which sock_put() matches with this synchronize_net()
given the call path is complicated.

With SOCK_RCU_FREE, no such a problem, all sock_put() will be safe.
So to me SOCK_RCU_FREE is much easier to understand and audit.

Thanks.

Re: [PATCH net V2] tuntap: synchronize through tfiles array instead of tun->numqueues

[Jason Wang]

On 2019/5/9 下午1:34, Cong Wang wrote:
> On Tue, May 7, 2019 at 7:54 PM Jason Wang <jasowang@redhat.com> wrote:
>> 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().
>>
> It is hard to tell which sock_put() matches with this synchronize_net()
> given the call path is complicated.
>
> With SOCK_RCU_FREE, no such a problem, all sock_put() will be safe.
> So to me SOCK_RCU_FREE is much easier to understand and audit.


The problem is not tfile itself but the data structure associated. As I 
mentioned earlier, the xdp_rxq_info_unreg() looks racy if tun_net_xmit() 
can read stale value of numqueues. It's just one example, we may meet 
similar issues in the future when adding more features.

Thanks


>
> Thanks.