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;
Date:   Wed, 7 Jun 2023 14:24:51 +0200
From:   Maciej Fijalkowski <maciej.fijalkowski@intel.com>
To:     Sai Krishna Gajula <saikrishnag@marvell.com>
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>,
        "dan.carpenter@linaro.org" <dan.carpenter@linaro.org>,
        Naveen Mamindlapalli <naveenm@marvell.com>
Subject: Re: [net PATCH] octeontx2-af: Fix pointer dereference before sanity
 check
Message-ID: <ZIB3E+nyJE6YIcge@boxer>
References: <20230607070255.2013980-1-saikrishnag@marvell.com>
 <ZIBuIXH2M1KbCg06@boxer>
 <BY3PR18MB47075B84527934A80F4B4C93A053A@BY3PR18MB4707.namprd18.prod.outlook.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <BY3PR18MB47075B84527934A80F4B4C93A053A@BY3PR18MB4707.namprd18.prod.outlook.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Ou/QJhWkTgoCOwBLsrUfJ2p3FmNNlBf5NBxrNZ81Lb0M/CUwTqqdVunKZq6c?=
 =?us-ascii?Q?sr0sCV8tJfQ3/3pGxHzorjPkit+TWf5P/PY7iPzg+uXRaPC+aeACjBBEPj8r?=
 =?us-ascii?Q?FeAwoHgSUZtVkhDYbXtxczWRbM3D9x99o5EqGW6RVOKtavw3PPuSn0KlDeKF?=
 =?us-ascii?Q?YFnGiifg3WvyS/5yMezBKo+B8rlFOQhX7hDXtv1ptJdJspWfBOK4gOhTf/F2?=
 =?us-ascii?Q?1wlCCLOJ1lf7wf/49EYoyDJGLWMG5hNmb4rh/Mit8xxY6jN3YGnlVU3juIL3?=
 =?us-ascii?Q?Fua5MpciAVyofUOLrKuFw9AOymMaiQb1hJnNelC5Qy+/O0mKXQ4kcIP7O5L+?=
 =?us-ascii?Q?iSPHh63cOcJlSjj1dfcbGf3hxt8JQ225dVj00L8CVYcYTmAJXiFYnvWrqRXi?=
 =?us-ascii?Q?CMQF9fFMJ4wNX30LEJevRIavqA7uM8grIkr/ZSdUdwC1NzEdO7NU3noaXNmo?=
 =?us-ascii?Q?3zJS0KGAXlOBfV/6Kb98aDgQM7a9QYcSn1Lu9+2pu5TlDglKb2jGbkvNgL2p?=
 =?us-ascii?Q?Pr1UhGp+ofZAILO/DNzGTihk4F2E+JkVayaETR1JvJaljwTMZMAFU5/jgjXf?=
 =?us-ascii?Q?KIGhKgUIfu3MNlHtXSGi2KMaUhVNzqUZsJ8UoDph5RZStqJkIbrX39iBKvdN?=
 =?us-ascii?Q?ATIzayR26iRIFEce+OO4xjMCSwL8s5je0bvawkoWOqaYmw9C7ymAxVzmxAOX?=
 =?us-ascii?Q?lYSK64m9B1Eprc2QLjX6zuqmsjWcr0MMYQ5MXNuYoFOZT2BTavyGxginiTiG?=
 =?us-ascii?Q?5/RxuWtN1wK08wD5tlNYqSHfsycqGI3DwDnY40LgW7qqM+Ss7yCv5j9e5vAD?=
 =?us-ascii?Q?5p4rnLOM98jpOZyeyViqX/ZkjbvTuoWgjLf0OFUM/eXhMExw1btVaoMd82Xe?=
 =?us-ascii?Q?mVBceax7KUAKlC6XJ31KmTa9PpAK/aVcFkjE4ZI/CaN/qOK6/S3n4QTiQ/MC?=
 =?us-ascii?Q?PEn1vCdSwgE+/T0JgrUhLofXZZzTfkaUhnpYlr223529bAH2wb7rHfehzrGx?=
 =?us-ascii?Q?ILtQokVsRHZWljCsho7EcLuYtu92DYle8MJvQa+T4ceRVS05YVxoB79TzxDc?=
 =?us-ascii?Q?uOzpTfGyFFKHpVsFhbGXiJcikCUaZZrsIkg9PbwihsrT423dP+BcR7GGr4eH?=
 =?us-ascii?Q?268nb/lL++xFNg9CquHGBnqZyK2fRZTd/CPN1/yyntxVHpdJXuoxu94ADexD?=
 =?us-ascii?Q?dCmikdKD9VDNM3UIq4RCmEHvJArxCejYBRMCqTjfuGpi7qDwi0HUK/LCiFvz?=
 =?us-ascii?Q?kN19ZyV0bf3vWyJEkGNc3WvjsIGKNqcd6blsfAydYeRZK+Kj3XZogOtdtxfM?=
 =?us-ascii?Q?LBn+KjwclDONVj0VMyLkV2ebQDJS4ntXKrYcNduA3Z6nMs3mncRKvmQIS3y6?=
 =?us-ascii?Q?0mugMWMDbvOi4JfOLBTQQlaBlYNgs5IqlK80dYxMCH50myN5h4cvP2QUS/pQ?=
 =?us-ascii?Q?9E6NeyZ2tYMXPzTO7nl1Rq2h1xiPwUyTf2dP6doQFsa8Z/OQ6OhuW8Sz5f+t?=
 =?us-ascii?Q?fXfgI/2sqbftwEw0Q+mVPduP/luDbn16OGCboArNnFchUjrjt0z2n3nv80Av?=
 =?us-ascii?Q?g4ApC67Xx9P6+nWAh5AQeNgy2W9q+ZIV0gO4Uu6OKh17b9mrh9X5dTBShbj+?=
 =?us-ascii?Q?zbk7Uim+101WMrVVUo72AW4=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 53ed8c31-e90b-4c3b-7eb9-08db6752391e
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6117.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Jun 2023 12:25:04.7380
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 779Per4Yd8emF1NDp7cLslkDUy85bHLV3kUT3mxTIuG4xr+XjBLeqqZrXUR6pow9xQhru1974ApnEGNB2ZXZAhyVClapLOBvtGsPBXkOeRY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR11MB6313
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED 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, Jun 07, 2023 at 12:04:40PM +0000, Sai Krishna Gajula wrote:
> 
> > -----Original Message-----
> > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > Sent: Wednesday, June 7, 2023 5:17 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>;
> > dan.carpenter@linaro.org; Naveen Mamindlapalli <naveenm@marvell.com>
> > Subject: Re: [net PATCH] octeontx2-af: Fix pointer dereference before
> > sanity check
> > 
> > On Wed, Jun 07, 2023 at 12:32:55PM +0530, Sai Krishna wrote:
> > > PTP pointer is being dereferenced before NULL, error check.
> > > Fixed the same to avoid NULL dereference and smatch checker warning.
> > 
> > please use imperative mood, you could say:
> > Move validation of ptp pointer before its usage
> > 
> I will change in V2 patch.
> 
> > >
> > > Fixes: 2ef4e45d99b1 ("octeontx2-af: Add PTP PPS Errata workaround on
> > CN10K silicon")
> > > Signed-off-by: Sai Krishna <saikrishnag@marvell.com>
> > > Signed-off-by: Naveen Mamindlapalli <naveenm@marvell.com>
> > 
> > Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > 
> > > ---
> > >  drivers/net/ethernet/marvell/octeontx2/af/ptp.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/ptp.c
> > b/drivers/net/ethernet/marvell/octeontx2/af/ptp.c
> > > index 3411e2e47d46..6a7dfb181fa8 100644
> > > --- a/drivers/net/ethernet/marvell/octeontx2/af/ptp.c
> > > +++ b/drivers/net/ethernet/marvell/octeontx2/af/ptp.c
> > > @@ -449,12 +449,12 @@ static void ptp_remove(struct pci_dev *pdev)
> > >  	struct ptp *ptp = pci_get_drvdata(pdev);
> > >  	u64 clock_cfg;
> > >
> > > -	if (cn10k_ptp_errata(ptp) && hrtimer_active(&ptp->hrtimer))
> > > -		hrtimer_cancel(&ptp->hrtimer);
> > > -
> > >  	if (IS_ERR_OR_NULL(ptp))
> > >  		return;
> > >
> > > +	if (cn10k_ptp_errata(ptp) && hrtimer_active(&ptp->hrtimer))
> > > +		hrtimer_cancel(&ptp->hrtimer);
> > > +
> > >  	/* Disable PTP clock */
> > >  	clock_cfg = readq(ptp->reg_base + PTP_CLOCK_CFG);
> > >  	clock_cfg &= ~PTP_CLOCK_CFG_PTP_EN;
> > 
> > i wonder if ptp_remove() would be able to free the struct ptp that
> > ptp_probe() allocated - then you wouldn't have to use devm_kzalloc().
> > 
> We intend to use devm_kzalloc() so that we do not need to call kfree in
> the remove function. Please let us know why you prefer to manually free
> the resource.

I just don't think this is really necessary as this object's lifetime
scope is clearly defined, i am in the rush now but i can try to come up
with further arguments later on if needed.

> 
> Thanks,
> Sai
> > > --
> > > 2.25.1
> > >
> > >