Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     Nikolay Aleksandrov <nikolay@nvidia.com>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "bridge@lists.linux-foundation.org" 
        <bridge@lists.linux-foundation.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        Roopa Prabhu <roopa@nvidia.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "kuba@kernel.org" <kuba@kernel.org>
Subject: Re: [RFC PATCH] net: bridge: call br_multicast_del_port before the
 port leaves
Thread-Topic: [RFC PATCH] net: bridge: call br_multicast_del_port before the
 port leaves
Thread-Index: AQHWoxlk3CgJjcYqVUG5/aECg5LB0qmaPloAgAA0UYA=
Date:   Fri, 16 Oct 2020 16:50:22 +0000
Message-ID: <20201016165021.fjrxiwofwfqespei@skbuf>
References: <20201015173355.564934-1-vladimir.oltean@nxp.com>
 <ed19387091755aee165c074983776f37f2b87892.camel@nvidia.com>
In-Reply-To: <ed19387091755aee165c074983776f37f2b87892.camel@nvidia.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <01BB5C912E1F944C867A03D3C92DBCC5@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 6b901356-18ec-4e6d-4aec-08d871f39315
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Oct 2020 16:50:23.0573
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: iz9ByR2avXDSVJUV+fGRB/q/i9BH0Xfpic8Fb1dSYkFlS5+ZAq2QFoBWy+narTM/3jddh7riHbb1IHfHp46Gzg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR04MB7343
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Oct 16, 2020 at 01:43:06PM +0000, Nikolay Aleksandrov wrote:
> It can potentially use after free, multicast resources (per-cpu stats) ar=
e freed
> in br_multicast_del_port() and can be used due to a race with port state
> sync on other CPUs since the handler can still process packets. That has =
a
> chance of happening if vlans are not used.

Interesting, thanks for pointing this out, I haven't observed
use-after-free in my limited testing of this patch.

> Interesting that br_stp_disable_port() calls br_multicast_disable_port() =
which
> flushes all non-permanent mdb entries, so I'm guessing you have problem o=
nly
> with permanent ones?

Indeed, I'm testing out your L2 multicast patch.

> Perhaps we can flush them all before. Either by passing an argument to
> br_stp_disable_port() that we're deleting the port which will be
> passed down to br_multicast_disable_port() or by calling an additional
> helper to flush all which can be re-used by both disable_port() and
> stop_multicast() calls. Adding an argument to br_stp_disable_port() to
> be passed down sounds cleaner to me. What do you think?

That sounds a bit complicated, to be honest.
In fact, the reason why I submitted this as RFC only is because it isn't
solving all my problems. You know that saying "- it hurts when I do that
- then don't do that"? I think I can just change the ocelot driver to
stop remapping the untagged MDB entries to its pvid, and then I can drop
all my charges to the bridge driver.=