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:   Sai Krishna Gajula <saikrishnag@marvell.com>
To:     Dan Carpenter <dan.carpenter@linaro.org>
CC:     "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Sunil Kovvuri Goutham <sgoutham@marvell.com>,
        "maciej.fijalkowski@intel.com" <maciej.fijalkowski@intel.com>,
        Naveen Mamindlapalli <naveenm@marvell.com>
Subject: Re: [net PATCH v2] octeontx2-af: Move validation of ptp pointer
 before its usage
Thread-Topic: [net PATCH v2] octeontx2-af: Move validation of ptp pointer
 before its usage
Thread-Index: AQHZpcXPjEIdzNE8GUWRy5B3kUqU5A==
Date:   Fri, 23 Jun 2023 11:28:19 +0000
Message-ID: <BY3PR18MB470788B4096D586DEB9A3B22A023A@BY3PR18MB4707.namprd18.prod.outlook.com>
References: <20230609115806.2625564-1-saikrishnag@marvell.com>
 <880d628e-18bf-44a1-a55f-ffbe1777dd2b@kadam.mountain>
In-Reply-To: <880d628e-18bf-44a1-a55f-ffbe1777dd2b@kadam.mountain>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?pEZoF/bWhDx2BAn9q9aJR0sXuohPkr93UJ/OfhAUagKQRgCDAjwt6RMXd3Zv?=
 =?us-ascii?Q?y5nDEgdIqCXKqbBBh3ov6nlskgoTgMQX2xSduISSrEsnikko9xSZwK6IZMsA?=
 =?us-ascii?Q?Wckyb9NTGrixDeT8VXRB+I6EzqWlPWZ7i8ftzA+iviiXX5VQboAMeYfymxY5?=
 =?us-ascii?Q?LDSCgWhUEa9w5SIAal7hZpqt/S0CLDjsx9Y0sXoqEpQptNLzDQGEDBiUUADG?=
 =?us-ascii?Q?TduMHoon4bL85N9qX+x7tdwqPpYgHLt+zAf/JVe3x+Xl3pefmVssw1695XO/?=
 =?us-ascii?Q?VPLL0ZHUnqEGk7tH9VIPVJcJip3L1f2ugAQOTxSzlyOkbp5kFuE3BQ4Q2UTA?=
 =?us-ascii?Q?tvHR/goAPnusjbIsfIRwB99milsPwvvXVNSXk6IGjNVP+jnoBOqnpxrOKPtX?=
 =?us-ascii?Q?8S0AglFeakKWQVjOCmyTJ3g7+lUgSQVmremGSc5PeppzSmZjb0S8YmAOOAD7?=
 =?us-ascii?Q?b0m/76mPq6Id7aHO/ow9GgOM5i+Bf7VFt6gprp9e+7086sSlgZCr5Bxsi70d?=
 =?us-ascii?Q?d1AiJ+hnb3iCUAxozMiDkEOJhv+1G9uOe+qOBA7pnYOnyEqHVVvRrUuWg17O?=
 =?us-ascii?Q?hd5PwVVahWROgkapUT02Rk2Z4JKdzwrNyQBUHl0ckafrwNkm4JKefg2RSMkz?=
 =?us-ascii?Q?AznmzsqOYLBYcdlLey7o+cMiuOsgbTO5h9K4JwByDyJ9U7cpR0zLi7lL8iXT?=
 =?us-ascii?Q?co43pDdK8D63Jjl8upZ+PICyMXQpE5+82yAjruERyTS4IxKV5K3V+j/5A9iL?=
 =?us-ascii?Q?zxQSBouXRIh7DjB2laLNeVvJfiAFOQH7oMJauJEsI4D/e+i11tbkNjJAclpP?=
 =?us-ascii?Q?HJAKpLSjrrLlsTnpAmWQwvx/6knUu7CNyaC3tzSmBVzbg+zqaHU0ksyouACU?=
 =?us-ascii?Q?4mZW9pZZSN4K6R0y0Ln93RnpNmsCOXriqh2l9VX0SV5f3Z8IjzGBbGQBkjk+?=
 =?us-ascii?Q?Olv8FId/IWPrZwxAsuMW2Z55A3t4oWUO4lY9CjgB+lJ1YO80Sv8+EcjP2pvW?=
 =?us-ascii?Q?Tjb+h9NhfR1iliKDr50MDJcP25zG54xMii6fr3zNLtDYp4BzDe75ooa04XG9?=
 =?us-ascii?Q?Fbo26kfv1k10QXsqrJoexxWSN4Q4uHm80k5RMe2J3YAx3+MROOQCM1nAlJjc?=
 =?us-ascii?Q?ursYOAkDmEw5TO1PzGklB40pBz6zY0NqZ3my7BtqNXl1A69151r/+58/3f/r?=
 =?us-ascii?Q?7MyB8rL0v1EKtNlP446pdAzB/bMnXPXbR127NhuCrw/LpdDvmlZGVQecQqFr?=
 =?us-ascii?Q?4Tcs5HUzzotVMm3jWfJd47DVTEZoQvIwTpNEr3oqhYeVi5TZcCZDHjTibWYL?=
 =?us-ascii?Q?In5DrBXMhgFefgftQNVcJnF+enrf5uEQh/xBazewSx0xTjSRVixS5a8zVqCA?=
 =?us-ascii?Q?USNBNeG6jAICj1bVGGKI1KsVsOoRuS0WeU0k3b7kDcMP1rJAunPH3EtWfDS8?=
 =?us-ascii?Q?YXIMOiMjIiBt1E31+fYBoycYddfdJbRl8UXgUpdjCNRe8SQwCExIahzSShPW?=
 =?us-ascii?Q?t1yJxbIXHj7bH9WkPuvR842L0vLMdHNLOflxWA5HgmgHsML1BRTi6/Ml7e7e?=
 =?us-ascii?Q?nQye5eBQRhnr8hK/ThH3Z3vAPyV3HyrOY8AYguRy?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: marvell.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BY3PR18MB4707.namprd18.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c703c611-96d9-41f7-86cb-08db73dcf219
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Jun 2023 11:28:19.4244
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Hrxdydy2y6NymVKFLISFSo+jNrue9ltktqNAn8PdtBK7A2TTCMev5MpdHg0Tt2k6IndNAe9MzVJLNQnkJMd26w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR18MB4000
X-Proofpoint-ORIG-GUID: tf4bVxt_HHfx5nChSbNLPU0rE7MAJmf1
X-Proofpoint-GUID: tf4bVxt_HHfx5nChSbNLPU0rE7MAJmf1
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-06-23_05,2023-06-22_02,2023-05-22_02
X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,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


> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@linaro.org>
> Sent: Friday, June 9, 2023 6:48 PM
> To: Sai Krishna Gajula <saikrishnag@marvell.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> Sunil Kovvuri Goutham <sgoutham@marvell.com>;
> maciej.fijalkowski@intel.com; Naveen Mamindlapalli
> <naveenm@marvell.com>
> Subject: Re: [net PATCH v2] octeontx2-af: Move validation of ptp
> pointer before its usage
>=20
> On Fri, Jun 09, 2023 at 05:28:06PM +0530, Sai Krishna wrote:
> > @@ -428,7 +427,7 @@ static int ptp_probe(struct pci_dev *pdev,
> >  	return 0;
> >
> >  error_free:
> > -	devm_kfree(dev, ptp);
> > +	kfree(ptp);
>=20
> Yeah.  It's strange any time we call devm_kfree()...  So there is somethi=
ng
> here which I have not understood.
We moved from devm_kzalloc/devm_kfree  to kzalloc/kfree as per review comme=
nts from Maciej.
>=20
> >
> >  error:
> >  	/* For `ptp_get()` we need to differentiate between the case
>=20
> This probe function is super weird how it returns success on the failure =
path.
> One concern, I had initially was that if anything returns -EPROBE_DEFER t=
hen
> we cannot recover.  That's not possible in the current code, but it makes=
 me
> itch...  But here is a different crash.
>=20

In few circumstances, the PTP device is probed before the AF device in the =
driver. In such instance, -EPROBE_DEFER is used.
-- EDEFER_PROBE is useful when probe order changes. Ex: AF driver probes be=
fore PTP.

> drivers/net/ethernet/marvell/octeontx2/af/ptp.c
>    432  error:
>    433          /* For `ptp_get()` we need to differentiate between the c=
ase
>    434           * when the core has not tried to probe this device and t=
he case
> when
>    435           * the probe failed.  In the later case we pretend that t=
he
>    436           * initialization was successful and keep the error in
>    437           * `dev->driver_data`.
>    438           */
>    439          pci_set_drvdata(pdev, ERR_PTR(err));
>    440          if (!first_ptp_block)
>    441                  first_ptp_block =3D ERR_PTR(err);
>=20
> first_ptp_block is NULL for unprobed, an error pointer for probe failure,=
 or
> valid pointer.

This is correct.
>=20
>    442
>    443          return 0;
>    444  }
>=20
> drivers/net/ethernet/marvell/octeontx2/af/ptp.c
>    201  struct ptp *ptp_get(void)
>    202  {
>    203          struct ptp *ptp =3D first_ptp_block;
>                             ^^^^^^^^^^^^^^^^^^^^^^
>=20
>    204
>    205          /* Check PTP block is present in hardware */
>    206          if (!pci_dev_present(ptp_id_table))
>    207                  return ERR_PTR(-ENODEV);
>    208          /* Check driver is bound to PTP block */
>    209          if (!ptp)
>    210                  ptp =3D ERR_PTR(-EPROBE_DEFER);
>    211          else
>    212                  pci_dev_get(ptp->pdev);
>                                     ^^^^^^^^^ if first_ptp_block is an er=
ror pointer this will
> Oops.

Will fix this in v3 patch.

Thanks,
Sai
>=20
>    213
>    214          return ptp;
>    215  }
>=20
> regards,
> dan carpenter