From: Ido Schimmel <idosch@mellanox•com>
To: Nikolay Aleksandrov <razor@blackwall•org>
Cc: <netdev@vger•kernel.org>, <roopa@cumulusnetworks•com>,
<vyasevich@gmail•com>, <stephen@networkplumber•org>,
<bridge@lists•linux-foundation.org>, <davem@davemloft•net>,
"Nikolay Aleksandrov" <nikolay@cumulusnetworks•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 15:21:18 +0300 [thread overview]
Message-ID: <20151011122118.GB2793@colbert.mtl.com> (raw)
In-Reply-To: <1443637015-4153-5-git-send-email-razor@blackwall.org>
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.
next prev parent reply other threads:[~2015-10-11 12:55 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 [this message]
2015-10-11 12:42 ` Nikolay Aleksandrov
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=20151011122118.GB2793@colbert.mtl.com \
--to=idosch@mellanox$(echo .)com \
--cc=bridge@lists$(echo .)linux-foundation.org \
--cc=davem@davemloft$(echo .)net \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nikolay@cumulusnetworks$(echo .)com \
--cc=razor@blackwall$(echo .)org \
--cc=roopa@cumulusnetworks$(echo .)com \
--cc=stephen@networkplumber$(echo .)org \
--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