| Message ID | 20230607070255.2013980-1-saikrishnag@marvell.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] octeontx2-af: Fix pointer dereference before sanity check | expand |
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 > > 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(). > -- > 2.25.1 > >
> -----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. Thanks, Sai > > -- > > 2.25.1 > > > >
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 > > > > > >
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;
PTP pointer is being dereferenced before NULL, error check. Fixed the same to avoid NULL dereference and smatch checker warning. 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> --- drivers/net/ethernet/marvell/octeontx2/af/ptp.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)