From: Nikolay Aleksandrov <razor@blackwall•org>
To: Nikolay Aleksandrov <nikolay@cumulusnetworks•com>,
Ido Schimmel <idosch@mellanox•com>
Cc: netdev@vger•kernel.org, roopa@cumulusnetworks•com,
bridge@lists•linux-foundation.org, davem@davemloft•net,
vyasevich@gmail•com
Subject: Re: [PATCH net-next 4/5] bridge: vlan: fix possible null ptr derefs on port init and deinit
Date: Sun, 11 Oct 2015 14:43:46 +0200 [thread overview]
Message-ID: <561A5982.4070008@blackwall.org> (raw)
In-Reply-To: <561A5918.5050404@cumulusnetworks.com>
On 10/11/2015 02:42 PM, Nikolay Aleksandrov wrote:
> On 10/11/2015 02:21 PM, Ido Schimmel wrote:
>> Wed, Sep 30, 2015 at 09:16:54PM IDT, razor@blackwall•org wrote:
>>> From: Nikolay Aleksandrov <nikolay@cumulusnetworks•com>
>>>
>>> When a new port is being added we need to make vlgrp available after
>>> rhashtable has been initialized and when removing a port we need to
>>> flush the vlans and free the resources after we're sure noone can use
>>> the port, i.e. after it's removed from the port list and synchronize_rcu
>>> is executed.
>>
>> Hi Nikolay,
>>
>> Changing the order of port deinit breaks symmetry with the init
>> sequence. It also introduces a problem for switchdev drivers. Flushing
>> the VLANs clears HW VLAN filters and then, when port is unlinked from
>> bridge and CHANGEUPPER is received, port is configured to direct traffic
>> to CPU (as it's not offloaded anymore). Doing the reverse (like in this
>> patch) renders the port unusable.
>>
>> Regarding the reason for this change, are you afraid that vlgrp will be
>> accessed in bridge's rx handler or xmit function after it's freed? If
>> so, maybe we can access it using RCU primitives? That way, both the rx
>> handler and xmit function (executed under RCU lock) will either have a
>> valid copy or not. Looking at previous iterations of this code, I see
>> that was the case with the 'net_port_vlans' struct.
>>
>> I can start working on a fix if you agree with the proposed solution.
>>
>> Thanks.
>>
>
> Hi,
> Ah, I didn't know about this, I feared that something might rely on the
> particular order of the operations but didn't have a way to test this one in
> particular. Anyway, your proposed solution sounds good to me.
>
> Thank you,
> Nik
One thing to be careful about is the creation/destruction of the rhashtable itself
and the order of operations in regard to vlgrp visibility, so it's not only if
vlgrp is visible or not - it should be visible after rhashtable has been initialized
and should be removed before it's freed, the rest is pretty straight-forward.
next prev parent reply other threads:[~2015-10-11 12:43 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-30 18:16 [PATCH net-next 0/5] bridge: vlan: cleanups & fixes Nikolay Aleksandrov
2015-09-30 18:16 ` [PATCH net-next 1/5] bridge: vlan: adjust rhashtable initial size and hash locks size Nikolay Aleksandrov
2015-09-30 18:16 ` [PATCH net-next 2/5] bridge: vlan: fix possible null vlgrp deref while registering new port Nikolay Aleksandrov
2015-09-30 18:16 ` [PATCH net-next 3/5] bridge: vlan: move pvid inside net_bridge_vlan_group Nikolay Aleksandrov
2015-09-30 18:16 ` [PATCH net-next 4/5] bridge: vlan: fix possible null ptr derefs on port init and deinit Nikolay Aleksandrov
2015-10-11 12:21 ` Ido Schimmel
2015-10-11 12:42 ` Nikolay Aleksandrov
2015-10-11 12:43 ` Nikolay Aleksandrov [this message]
2015-09-30 18:16 ` [PATCH net-next 5/5] bridge: vlan: don't pass flags when creating context only Nikolay Aleksandrov
2015-10-02 1:07 ` [PATCH net-next 0/5] bridge: vlan: cleanups & fixes David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=561A5982.4070008@blackwall.org \
--to=razor@blackwall$(echo .)org \
--cc=bridge@lists$(echo .)linux-foundation.org \
--cc=davem@davemloft$(echo .)net \
--cc=idosch@mellanox$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nikolay@cumulusnetworks$(echo .)com \
--cc=roopa@cumulusnetworks$(echo .)com \
--cc=vyasevich@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox