From: roopa <roopa@cumulusnetworks•com>
To: David Miller <davem@davemloft•net>
Cc: nikolay@cumulusnetworks•com, stephen@networkplumber•org,
netdev@vger•kernel.org
Subject: Re: [PATCH net-next v2] bridge: set is_local and is_static before fdb entry is added to the fdb hashtable
Date: Tue, 27 Oct 2015 07:55:30 -0700 [thread overview]
Message-ID: <562F9062.2080100@cumulusnetworks.com> (raw)
In-Reply-To: <20151027.070255.918332804045095898.davem@davemloft.net>
On 10/27/15, 7:02 AM, David Miller wrote:
> From: Nikolay Aleksandrov <nikolay@cumulusnetworks•com>
> Date: Tue, 27 Oct 2015 14:31:48 +0100
>
>> On 10/27/2015 01:12 PM, Roopa Prabhu wrote:
>>> From: Roopa Prabhu <roopa@cumulusnetworks•com>
>>>
>>> Problem Description:
>>> We can add fdbs pointing to the bridge with NULL ->dst but that has a
>>> few race conditions because br_fdb_insert() is used which first creates
>>> the fdb and then, after the fdb has been published/linked, sets
>>> "is_local" to 1 and in that time frame if a packet arrives for that fdb
>>> it may see it as non-local and either do a NULL ptr dereference in
>>> br_forward() or attach the fdb to the port where it arrived, and later
>>> br_fdb_insert() will make it local thus getting a wrong fdb entry.
>>> Call chain br_handle_frame_finish() -> br_forward():
>>> But in br_handle_frame_finish() in order to call br_forward() the dst
>>> should not be local i.e. skb != NULL, whenever the dst is
>>> found to be local skb is set to NULL so we can't forward it,
>>> and here comes the problem since it's running only
>>> with RCU when forwarding packets it can see the entry before "is_local"
>>> is set to 1 and actually try to dereference NULL.
>>> The main issue is that if someone sends a packet to the switch while
>>> it's adding the entry which points to the bridge device, it may
>>> dereference NULL ptr. This is needed now after we can add fdbs
>>> pointing to the bridge. This poses a problem for
>>> br_fdb_update() as well, while someone's adding a bridge fdb, but
>>> before it has is_local == 1, it might get moved to a port if it comes
>>> as a source mac and then it may get its "is_local" set to 1
>>>
>>> This patch changes fdb_create to take is_local and is_static as
>>> arguments to set these values in the fdb entry before it is added to the
>>> hash. Also adds null check for port in br_forward.
>>>
>>> Reported-by: Nikolay Aleksandrov <nikolay@cumulusnetworks•com>
>>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks•com>
>>> ---
>>> v2 - fix compile error reported by kbuild test robot.
>>> Accidently i had posted an older version of the patch
>>>
>> Thanks for fixing this,
>>
>> Fixes: 3741873b4f73 ("bridge: allow adding of fdb entries pointing to the bridge device")
>> Reviewed-by: Nikolay Aleksandrov <nikolay@cumulusnetworks•com>
> This still doesn't compile:
>
> net/bridge/br_fdb.c: In function ‘br_fdb_external_learn_add’:
> net/bridge/br_fdb.c:1103:3: error: too few arguments to function ‘fdb_create’
> net/bridge/br_fdb.c:495:37: note: declared here
sorry david about the sloppiness. Resent. There was a problem with my patch generation.
prev parent reply other threads:[~2015-10-27 14:55 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-27 12:12 [PATCH net-next v2] bridge: set is_local and is_static before fdb entry is added to the fdb hashtable Roopa Prabhu
2015-10-27 13:31 ` Nikolay Aleksandrov
2015-10-27 14:02 ` David Miller
2015-10-27 14:55 ` roopa [this message]
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=562F9062.2080100@cumulusnetworks.com \
--to=roopa@cumulusnetworks$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nikolay@cumulusnetworks$(echo .)com \
--cc=stephen@networkplumber$(echo .)org \
/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