Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     Colin Foster <colin.foster@in-advantage.com>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        Jakub Kicinski <kuba@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        "UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Claudiu Manoil <claudiu.manoil@nxp.com>,
        Vladimir Oltean <olteanv@gmail.com>
Subject: Re: [PATCH v6 net-next 1/5] net: mscc: ocelot: fix mutex lock error
 during ethtool stats read
Thread-Topic: [PATCH v6 net-next 1/5] net: mscc: ocelot: fix mutex lock error
 during ethtool stats read
Thread-Index: AQHYHjTcMTiSBcyd20+aLf9EJELZxKyMivaA
Date:   Thu, 10 Feb 2022 09:50:52 +0000
Message-ID: <20220210095051.b7rgpy6muvqjnih2@skbuf>
References: <20220210041345.321216-1-colin.foster@in-advantage.com>
 <20220210041345.321216-2-colin.foster@in-advantage.com>
In-Reply-To: <20220210041345.321216-2-colin.foster@in-advantage.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?XRgYzNXsDK/UIqpS0czmVJW+cDWeXUEueFI1nQDB5td0VJthLeT4M5YKqZNW?=
 =?us-ascii?Q?OUNNy9Yvi0pHy7O1HCw2xA75eGqoXQEOfytBJl9h1+ht2PZ003zuw52sNYnA?=
 =?us-ascii?Q?qIXU+laeLnyEybRxBmx8FX4LDbi1dgn8SHw0KXjHzMXu5PqD5uHs7osb+0cf?=
 =?us-ascii?Q?ixN1EMvsTxoyDIOBbdJygwZW4gDqvqxCj5STWN9OCSGQbFgwTEN6x36pOkjI?=
 =?us-ascii?Q?tbLzWXk55lWXYxO+HLUKzgZR4hJfMeHNsLabMLxb2gEsDwQqVtFQPRYwpexz?=
 =?us-ascii?Q?en6ud0jyoz9aAgg1TkD4b+qPYwLW0POXIFdLQWqe8G6fVTPO8MNu66VTSf9/?=
 =?us-ascii?Q?Jif8VvXpUH864lDvi7uU7LX9ZA7oX1fJWE3L7twcXiRs8NdWuqyRQfyI7Mdz?=
 =?us-ascii?Q?6mkvF6UUa/ai+n5276hXlv90c0Y6PKAnosmIaRy94/cwXJdfEq+0QYGDaEyp?=
 =?us-ascii?Q?rpfQQ/dQQbiY7oR7zDdPrZT2uObgX1Jn8ekExcQ4Xq4N5U/6Hr/q9YxnB5VW?=
 =?us-ascii?Q?o2ICn+rYzXFPOfGnAjs+VpTYojMdg15Z8qhnsIwa2WeNBqtcpFyrwJD7hmad?=
 =?us-ascii?Q?HsUQQ91xmr3SeqSuh4cd3e6+iKocbrjFfdLDIoI2nBxE59UGeaBsH8Nb0kKr?=
 =?us-ascii?Q?Kp1n7D5lNNTlvVoy+BbZlfrNkMmtH+M1oYXqVt+44/PO89W90/6O43NWBe6O?=
 =?us-ascii?Q?Nu0Vs1g2i0Qh4HOTwpx5WmGUZ4z5BPvx1KvirhK3ErFJiLKbto3pyq9BEe/k?=
 =?us-ascii?Q?f2FfNtp/U7bMFi7/ZWnjy84AwvfyzsABmTc2i4dtHBz3nDyJoSNa3vShXSRR?=
 =?us-ascii?Q?ptnXlC/a/ztEm2Nax6W7bdRU9MDNphHYOFLIT/geC5QRkBxvDJv7SE0eDpDB?=
 =?us-ascii?Q?pbnYTRRu6JWQT6/yGhneOZp5JEetYKEa2PP7Q5vzEQK0N5SzsAil9tiQkjWj?=
 =?us-ascii?Q?f54GSmcKFV+saeK8PgiHRa/0S2sn0dw6YYTF/cKPYOoLcYjggurvtL1FClDB?=
 =?us-ascii?Q?0H7sx9aOv4nUdAtM+PgZDJRKgpAKJqiMXXXwaRQ2qqDQB/RRwvRaPYW7pFCO?=
 =?us-ascii?Q?EBqnjRoZi2D4Zq04y3JpVaz9kvK39lAimuIupQcrjDVnSwXKrakM2ei0rdWI?=
 =?us-ascii?Q?hMtxKkowpwKJvIeAKCLDR7ujL4BUQMw0LCp2gVypfSp9JDmXCJIX8G2YHiS1?=
 =?us-ascii?Q?qL9dWpNzdBjm1EDZUTFB4e6YuDSFtyCG6+74PXn+nJGk0V6H5L05OniEK86J?=
 =?us-ascii?Q?n0cv/DwA68yUDx7iFfcJ8Cx6PIRx/h+9TsxM/ZURju2zZTbDwck8QqqT+hUW?=
 =?us-ascii?Q?p7edByLJJ/zUieyp0pH/tHiibkVj3yxEfkpx1LTgEahPBTMUbiX3BrS9ZuoL?=
 =?us-ascii?Q?DrGiBQlXIKnL5L7FBsrdWVnyKY2iZK41JE1lUIckMKpWcY3ubdjMMwkhUWit?=
 =?us-ascii?Q?roqBHHWCdbh3nGuwvU3MMJZgwaPrtfBJAeI06TxMVmGFoCo/hly9ioFija2K?=
 =?us-ascii?Q?uHvJD0Mv2tAlg+Ib5DTs8BXboxyGOJaRZKDQ5xcrseZ5zw0NVnCvf52ANSml?=
 =?us-ascii?Q?+7Rc8IMa+J+vqkXRTgP2bVMr5Od7sHV2RxbhfpnKN56uLEbHY7QlmNuyInp/?=
 =?us-ascii?Q?sthvIBJd7EINYu9FL2ukTRQ=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <D1FE637A7AAACE42A902E1A61091E476@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: f6e567aa-a813-4823-c92c-08d9ec7ad367
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Feb 2022 09:50:52.5345
 (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: clJqmeVVOUbJ31CO+MHNsalOIx0eUUKncnYcFeyC9CVjkobBNeLfqEKMVBCf7osFOlOUQXx4FwR8ZgkMlNDg9w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB7117
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Feb 09, 2022 at 08:13:41PM -0800, Colin Foster wrote:
> An ongoing workqueue populates the stats buffer. At the same time, a user
> might query the statistics. While writing to the buffer is mutex-locked,
> reading from the buffer wasn't. This could lead to buggy reads by ethtool=
.
>=20
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> Fixes: a556c76adc052 ("net: mscc: Add initial Ocelot switch support")
> Reported-by: Vladimir Oltean <olteanv@gmail.com>

I reported this using vladimir.oltean@nxp.com btw.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

If you hurry and resend this against the "net" tree, you might catch
this week's pull request, since the last one was on Feb 3->4:
https://patchwork.kernel.org/project/netdevbpf/patch/20220204000428.2889873=
-1-kuba@kernel.org/
Then "net" will be merged into "net-next" probably the next day or so,
and you can resend patches 2-5 towards "net-next".

> ---
>  drivers/net/ethernet/mscc/ocelot.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>=20
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/ms=
cc/ocelot.c
> index 455293aa6343..6933dff1dd37 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -1737,12 +1737,11 @@ void ocelot_get_strings(struct ocelot *ocelot, in=
t port, u32 sset, u8 *data)
>  }
>  EXPORT_SYMBOL(ocelot_get_strings);
> =20
> +/* Caller must hold &ocelot->stats_lock */
>  static void ocelot_update_stats(struct ocelot *ocelot)
>  {
>  	int i, j;
> =20
> -	mutex_lock(&ocelot->stats_lock);
> -
>  	for (i =3D 0; i < ocelot->num_phys_ports; i++) {
>  		/* Configure the port to read the stats from */
>  		ocelot_write(ocelot, SYS_STAT_CFG_STAT_VIEW(i), SYS_STAT_CFG);
> @@ -1761,8 +1760,6 @@ static void ocelot_update_stats(struct ocelot *ocel=
ot)
>  					      ~(u64)U32_MAX) + val;
>  		}
>  	}
> -
> -	mutex_unlock(&ocelot->stats_lock);
>  }
> =20
>  static void ocelot_check_stats_work(struct work_struct *work)
> @@ -1771,7 +1768,9 @@ static void ocelot_check_stats_work(struct work_str=
uct *work)
>  	struct ocelot *ocelot =3D container_of(del_work, struct ocelot,
>  					     stats_work);
> =20
> +	mutex_lock(&ocelot->stats_lock);
>  	ocelot_update_stats(ocelot);
> +	mutex_unlock(&ocelot->stats_lock);
> =20
>  	queue_delayed_work(ocelot->stats_queue, &ocelot->stats_work,
>  			   OCELOT_STATS_CHECK_DELAY);
> @@ -1781,12 +1780,16 @@ void ocelot_get_ethtool_stats(struct ocelot *ocel=
ot, int port, u64 *data)
>  {
>  	int i;
> =20
> +	mutex_lock(&ocelot->stats_lock);
> +
>  	/* check and update now */
>  	ocelot_update_stats(ocelot);
> =20
>  	/* Copy all counters */
>  	for (i =3D 0; i < ocelot->num_stats; i++)
>  		*data++ =3D ocelot->stats[port * ocelot->num_stats + i];
> +
> +	mutex_unlock(&ocelot->stats_lock);
>  }
>  EXPORT_SYMBOL(ocelot_get_ethtool_stats);
> =20
> --=20
> 2.25.1
>=