From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     Volodymyr Mytnyk <volodymyr.mytnyk@plvision.eu>
CC:     "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "mickeyr@marvell.com" <mickeyr@marvell.com>,
        "serhiy.pshyk@plvision.eu" <serhiy.pshyk@plvision.eu>,
        "taras.chornyi@plvision.eu" <taras.chornyi@plvision.eu>,
        Volodymyr Mytnyk <vmytnyk@marvell.com>,
        Taras Chornyi <tchornyi@marvell.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Ioana Ciornei <ioana.ciornei@nxp.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net] net: marvell: prestera: fix brige port operation
Thread-Topic: [PATCH net] net: marvell: prestera: fix brige port operation
Thread-Index: AQHX25fOH/y6p2zJhEGVbWfn0oBDyKwH9PgA
Date:   Wed, 17 Nov 2021 17:10:55 +0000
Message-ID: <20211117171054.cupmcwwi2ruxjxuh@skbuf>
References: <1637142232-19344-1-git-send-email-volodymyr.mytnyk@plvision.eu>
In-Reply-To: <1637142232-19344-1-git-send-email-volodymyr.mytnyk@plvision.eu>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?i1pvWwLvzC6KMWgPKizx0hFeixgw5/AYHKMo3wykVRiqnG8ZdyulCTZa8NAt?=
 =?us-ascii?Q?JsPhpIugmL5dPJuJKyQ5vvqWWOD6MgHoL6esJdzP4chRdLpQCmQUOwjer2uf?=
 =?us-ascii?Q?U/GoXcwvhxgzw7q/++Fc5uLM3DDyYG/2Crnt4XRqtd0NsBwM5kjlT3dAa7N4?=
 =?us-ascii?Q?U6MUrLQO1/OFMftxGGF7139resqNdDXkZZ38qvfuLY62bJK4JjdnyKJgzBLk?=
 =?us-ascii?Q?97l2Eppm4B3NgqXjk2dXghxg0LeWksgzHgK9zHhQ/W8wsSkeM4BOMAYUgPLb?=
 =?us-ascii?Q?rpxm9Pp1jcrY+CDsDKxlM6kkJEHPQI92LN4dS+/vdKR3+f0M2IITYUBR0EmV?=
 =?us-ascii?Q?uFFvYgHTsx6V9+a6wJZ6ClaMqJAqDah4SeG1ocBBxdTDCWcCdoJo0LkTkVIp?=
 =?us-ascii?Q?+TRtZOnENUPYvFV6n4D7U0E2tMHhJyfTg36fB52yKPeb2PYObVYUMh087eXo?=
 =?us-ascii?Q?FlNAQggG3AaftowrtTMgkT8Cj/4ypgPLOxThhg06gj2OIOcSMf35cdZFixE/?=
 =?us-ascii?Q?l6GAR5FpbDmGEFMyUZb2PB0VlnkS/yaaMIXBsyqnWqogtXSOzTgba5N5mmys?=
 =?us-ascii?Q?6BNGKNDYrrRfTdq6pl4DjrN++afGRd0+O34YHy5pFT9nezeSr7CbiXdZ44sn?=
 =?us-ascii?Q?LwV0/4Tr5QLbEFQkELmi7bGLQHsDn+GzufwN4EQoiAd9jvX+fQURKM9T6ROg?=
 =?us-ascii?Q?epahfwOC3r/Lv9jh0gjoaa8KKkJKW06LR7ZqGiSCD0Vp+hDj2SWZfgSxMf8f?=
 =?us-ascii?Q?oHTvBMTY/zFrYSEe/8pPTtllBILZ59hl42AfYRV2myIPuo58dWoBZOGZr/wf?=
 =?us-ascii?Q?tyVop8JlnBFldYwrGysnOZ5QN+O7uvzswbzUPEwOO4uv2VjbH7c9ujbYuiQD?=
 =?us-ascii?Q?T3rUdsBJqvDBUhGovVbp+ufimgdLJcAgE86tb/7oX9fzSvOd6I/gl+aFZo5i?=
 =?us-ascii?Q?1Q2NnHhD2qrAc3LPvJXtZG2e5VehepDxgQFtbjbIDrvw5TNSsBx0iB73skSq?=
 =?us-ascii?Q?31hLMPPwN9qeb/lRPj6rn4CPENZ9x29cG6IbV/lWNZY1oa2LOGQugkNYzaOy?=
 =?us-ascii?Q?LlxpxmyRfDiZbKTkc8HubKgYJoQT+PBEdcXDPjPY/WJ+NdA1i1e1lfLtZKhC?=
 =?us-ascii?Q?17CDFetEP+G42iFTDTpnbP7fjjEagJ9fGZUowdGvEMr5zcyKHR1WcoMfd/Ox?=
 =?us-ascii?Q?3AApSTR+2OQqqya5K3PPg81dT5bzizVBMn2j0R9Oa1RhCI+3ybkTga0IsBI3?=
 =?us-ascii?Q?5tL3zNaqNMEq9HgAlX6AeZqQl0vR4uJm3wPAmUe+LJhUiyCrH2kZjAjlSn1C?=
 =?us-ascii?Q?ZuSPNjOVCuUtJDxOPVidOaP3K1Ko+E8vB6uFqnw20LZtyLY7P94bW2dhuayM?=
 =?us-ascii?Q?k6UdUNRy3GTNHaQu0dw77A1TWYHkJBTfVkMtyv7zR9IbclVTr/ZtFsMCfqfL?=
 =?us-ascii?Q?lutCJM7nk/5wafpvSUaIaTvatV7cyZr8l3EuMDThiI0QnTRXXncQxjVSuooB?=
 =?us-ascii?Q?0v42lU0gQ49q+gGn5+j+UJ2bnX5ClMiVBHJGQGr5CKq65fsLqqwD2S2oBeX9?=
 =?us-ascii?Q?PLr1/MPjHLeG6K6kk1g1g13Bwgh+0sN/6dpSC4oqCYuA4If+zwazg2L5YdJw?=
 =?us-ascii?Q?BWvE54mmURMpk5zaKUGzi7M=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <352BE20F7438A640A431D9AF1FCE3F50@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: VI1PR04MB5136.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 12eb54dc-b00f-4ec0-9b13-08d9a9ed37e7
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Nov 2021 17:10:55.9045
 (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: UNtjvnoDpI5xGzCIJrBTU55933Aoc7y1yRmjybJl5JxaRryEmKEfBNQwee06f0WQUqvSNJ1MUGfOnHbODLpUyg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB6942
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Nov 17, 2021 at 11:43:51AM +0200, Volodymyr Mytnyk wrote:
> From: Volodymyr Mytnyk <vmytnyk@marvell.com>
>=20
> - handle SWITCHDEV_BRPORT_[UN]OFFLOADED events for
>   switchdev_bridge_port_offload to avoid fail return.
> - fix error path handling in prestera_bridge_port_join to
>   fix double free issue (see below).
>=20
> Trace:
>   Internal error: Oops: 96000044 [#1] SMP
>   Modules linked in: prestera_pci prestera uio_pdrv_genirq
>   CPU: 1 PID: 881 Comm: ip Not tainted 5.15.0 #1
>   pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=3D--)
>   pc : prestera_bridge_destroy+0x2c/0xb0 [prestera]
>   lr : prestera_bridge_port_join+0x2cc/0x350 [prestera]
>   sp : ffff800011a1b0f0
>   ...
>   x2 : ffff000109ca6c80 x1 : dead000000000100 x0 : dead000000000122
>    Call trace:
>   prestera_bridge_destroy+0x2c/0xb0 [prestera]
>   prestera_bridge_port_join+0x2cc/0x350 [prestera]
>   prestera_netdev_port_event.constprop.0+0x3c4/0x450 [prestera]
>   prestera_netdev_event_handler+0xf4/0x110 [prestera]
>   raw_notifier_call_chain+0x54/0x80
>   call_netdevice_notifiers_info+0x54/0xa0
>   __netdev_upper_dev_link+0x19c/0x380
>=20
> Fixes: 2f5dc00f7a3e ("net: bridge: switchdev: let drivers inform which br=
idge ports are offloaded")
> Signed-off-by: Volodymyr Mytnyk <vmytnyk@marvell.com>
> ---
>  drivers/net/ethernet/marvell/prestera/prestera_switchdev.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>=20
> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c b=
/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c
> index 3ce6ccd0f539..f1bc6699ec8b 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_switchdev.c
> @@ -497,8 +497,8 @@ int prestera_bridge_port_join(struct net_device *br_d=
ev,
> =20
>  	br_port =3D prestera_bridge_port_add(bridge, port->dev);
>  	if (IS_ERR(br_port)) {
> -		err =3D PTR_ERR(br_port);
> -		goto err_brport_create;
> +		prestera_bridge_put(bridge);
> +		return PTR_ERR(br_port);
>  	}

Here is how the function looked _before_ my patch:

int prestera_bridge_port_join(struct net_device *br_dev,
			      struct prestera_port *port)
{
	struct prestera_switchdev *swdev =3D port->sw->swdev;
	struct prestera_bridge_port *br_port;
	struct prestera_bridge *bridge;
	int err;

	bridge =3D prestera_bridge_by_dev(swdev, br_dev);
	if (!bridge) {
		bridge =3D prestera_bridge_create(swdev, br_dev);
		if (IS_ERR(bridge))
			return PTR_ERR(bridge);
	}

	br_port =3D prestera_bridge_port_add(bridge, port->dev);
	if (IS_ERR(br_port)) {
		err =3D PTR_ERR(br_port);
		goto err_brport_create;
	}

	if (bridge->vlan_enabled)
		return 0;

	err =3D prestera_bridge_1d_port_join(br_port);
	if (err)
		goto err_port_join;

	return 0;

err_port_join:
	prestera_bridge_port_put(br_port);
err_brport_create:
	prestera_bridge_put(bridge);
	return err;
}

The double free is due to the fact that prestera_bridge_port_put() calls
prestera_bridge_put() by itself too.

But the code was already buggy, for example the error path of
prestera_bridge_1d_port_join() would trigger this double free as well.
The change itself is ok (but is very poorly explained), if
prestera_bridge_port_add() fails, you want to undo just
prestera_bridge_create(), otherwise, prestera_bridge_port_put() will
undo both prestera_bridge_port_add() and prestera_bridge_create().

So the honest Fixes: tag should be:

Fixes: e1189d9a5fbe ("net: marvell: prestera: Add Switchdev driver implemen=
tation")

because you want this change to be backported even to stable kernels
where commit 2f5dc00f7a3e ("net: bridge: switchdev: let drivers inform
which bridge ports are offloaded") is not present.

> =20
>  	err =3D switchdev_bridge_port_offload(br_port->dev, port->dev, NULL,
> @@ -519,8 +519,6 @@ int prestera_bridge_port_join(struct net_device *br_d=
ev,
>  	switchdev_bridge_port_unoffload(br_port->dev, NULL, NULL, NULL);
>  err_switchdev_offload:
>  	prestera_bridge_port_put(br_port);
> -err_brport_create:
> -	prestera_bridge_put(bridge);
>  	return err;
>  }
> =20
> @@ -1123,6 +1121,9 @@ static int prestera_switchdev_blk_event(struct noti=
fier_block *unused,
>  						     prestera_netdev_check,
>  						     prestera_port_obj_attr_set);
>  		break;
> +	case SWITCHDEV_BRPORT_OFFLOADED:
> +	case SWITCHDEV_BRPORT_UNOFFLOADED:
> +		return NOTIFY_DONE;
>  	default:
>  		err =3D -EOPNOTSUPP;

May I suggest that the root cause of the problem is that you're
returning -EOPNOTSUPP here? The switchdev events may just as well not be
destined for your prestera switch. You should return NOTIFY_DONE ("don't
care") for event types you don't know how to handle.

And technically, this part of the patch should have:
Fixes: 957e2235e526 ("net: make switchdev_bridge_port_{,unoffload} loosely =
coupled with the bridge")

>  	}
> --=20
> 2.7.4
>=