netdev - Re: [PATCH net-next 2/2] geneve: break dependency to network drivers

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]

Message-ID: <CAEh+42gOhXgB=M-nxQcGN6yGwnk3vwFEOwkAKdfE3vpLQ=LZ5w@mail.gmail.com>
Date:	Wed, 6 Jan 2016 13:01:28 -0800
From:	Jesse Gross <jesse@...nel.org>
To:	Hannes Frederic Sowa <hannes@...essinduktion.org>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 2/2] geneve: break dependency to network drivers

On Wed, Jan 6, 2016 at 12:25 PM, Hannes Frederic Sowa
<hannes@...essinduktion.org> wrote:
> On 06.01.2016 20:52, Jesse Gross wrote:
>> On Wed, Jan 6, 2016 at 10:48 AM, Hannes Frederic Sowa
>> <hannes@...essinduktion.org> wrote:
>>> On 06.01.2016 19:00, Jesse Gross wrote:
>>>> Unfortunately, I don't think that we can assume that RTNL is held
>>>> here. It actually is for the drivers that implement Geneve at this
>>>> point but not in all cases for VXLAN. For example, ixgbe refreshes the
>>>> offloads in a service task in addition to when it is opened. There's
>>>> only a couple instances of things like this, so I guess it's probably
>>>> not too hard to through and make sure that we hold RTNL in those
>>>> cases.
>>>
>>>
>>>
>>> Hmm, I am tempted to switch over to the netevent atomic notifier chain
>>> and
>>> install those events there. It does not need rtnl lock at all, so we can
>>> preserve the current semantics. What do you think?
>>
>>
>> I think that holding RTNL while we do these updates is actually the
>> right thing to do. The current situation of having calls from
>> different protocols protected by different locks is not really a great
>> model given that at the driver level these are usually shared data
>> structures. RTNL is already held in the majority of cases already, so
>> I think it is better to just convert the rest.
>
>
> The refreshes from each module are completely synchronous and don't get
> interleaved, so as long as the driver is correctly handling the locking
> internally rtnl lock shouldn't be needed. But as I don't know too much about
> driver developing I can revisit this.
>
> As a advantage I see that the driver developers don't need to worry about
> the rtnl lock at all when adding new events. Is this realistic?

I don't think that there is much savings to be had by avoiding RTNL
since the majority of interactions that the driver has with the stack
involve holding it anyways.

In order to do this safely without RTNL we need to have a lock in each
driver. I don't think that this is safely handled in all cases today
and is likely to get worse in the future. I also noticed that Geneve
actually doesn't hold any special lock while calling into drivers from
geneve_get_rx_port() so it is de-facto relying on RTNL. All other
operations in the Geneve driver are protected by RTNL currently, so we
would need to introduce a new lock to handle this as well. In effect,
it seems like people are implicitly assuming that these operations are
covered by RTNL since most similar things are.
--
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