Re: [PATCH net-next 1/2] macvtap: Let TUNSETOFFLOAD actually controll offload features.

[Vlad Yasevich <vyasevic@redhat.com>]

On 06/19/2013 11:46 AM, Eric Dumazet wrote:
> On Wed, 2013-06-19 at 11:26 -0400, Vlad Yasevich wrote:
>
>> I think I do since vlan pointer may change even when I am holding
>> rtnl.  rtnl is needed to change features.  rcu is needed to get
>> the vlan pointer.
>>
>>> (A BH handler will not change q->vlan )
>>
>> No, but the _bh rcu calls seem to be used when dereferencing q->vlan.
>> I am not sure the reason for that...
>
> You mix the reader/fast path, properly using RCU,
> and the writer path, using macvtap_lock ( a spinlock ).
>
> That's clear sign you missed something.
>

I don't think I need macvtap_lock as I am not modifying the relationship
between q and vlan.  I am attempting to modify the macvlan device 
features.  So macvtap_lock does not apply, and _rcu is used.

Looking at the entire macvtap driver, only the _bh variants of rcu
are used throughout the driver,  including in the ioctl() function.  I
am not sure why the driver requires BH to be disabled, but that
seems to be the case.

>>
>>>
>>> BTW, it looks like ->vlan is protected by macvtap_lock
>>
>> Right.  This is why I use rcu to get vlan.  rtnl is needed to avoid
>> asserts in the feature change code.
>
> The management should be allowed to sleep, and rcu_read_lock_bh()
> disallows that.
>
> Maybe some driver callback will really sleep and crash after your patch.
>
> vi +69 drivers/net/macvtap.c
>
> /*
>   * RCU usage:
>   * The macvtap_queue and the macvlan_dev are loosely coupled, the
>   * pointers from one to the other can only be read while rcu_read_lock
>   * or macvtap_lock is held.
>
> Your patch does not respect the rules of this driver.

Why not?  It uses rcu to acquire the pointer thus following the rules. 
The use of  the pointer is within the critical section so we are
guaranteed to have a valid pointer.

>
> macvtap_lock is always acquired from process context, without any need
> for _bh variant.
>

No, the lock is acquired only when modifying the relationship between 
the macvtap_queue and macvtap_dev.


> Quite frankly, I would switch this driver to use a mutex for
> macvtap_lock.
>
> And simply remove it, as RTNL is most probably already owned.
>

That's the issue.  RTNL is not owned in the ioctl case.  In fact
rtnl_lock was added to the patch because RTNL asserts were triggered
when changing device features.

-vlad


>
>
>
>