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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 19 Jun 2013 14:59:07 -0400
From:	Vlad Yasevich <vyasevic@...hat.com>
To:	arnd@...db.de
CC:	Eric Dumazet <eric.dumazet@...il.com>, netdev@...r.kernel.org,
	davem@...emloft.net, mst@...hat.com, jasowang@...hat.com
Subject: Re: [PATCH net-next 1/2] macvtap: Let TUNSETOFFLOAD actually controll
 offload features.

Arnd

MST suggested I add you.  Do you remember the reason
why macvtap uses rcu_read_lock_bh() instead of plain
rcu_read_lock()?  Additionally it seems to use
synchronize_rcu(), not the _bh() version.

Thanks
-vlad

On 06/19/2013 12:20 PM, Vlad Yasevich wrote:
> 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
>
>
>>
>>
>>
>>
>

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