From: Nikolay Aleksandrov <nikolay@cumulusnetworks•com>
To: Ido Schimmel <idosch@mellanox•com>,
Nikolay Aleksandrov <razor@blackwall•org>
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:42:00 +0200 [thread overview]
Message-ID: <561A5918.5050404@cumulusnetworks.com> (raw)
In-Reply-To: <20151011122118.GB2793@colbert.mtl.com>
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
next prev parent reply other threads:[~2015-10-11 12:42 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 [this message]
2015-10-11 12:43 ` Nikolay Aleksandrov
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=561A5918.5050404@cumulusnetworks.com \
--to=nikolay@cumulusnetworks$(echo .)com \
--cc=bridge@lists$(echo .)linux-foundation.org \
--cc=davem@davemloft$(echo .)net \
--cc=idosch@mellanox$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=razor@blackwall$(echo .)org \
--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