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 <[email protected]>
Signed-off-by: Naveen Mamindlapalli <[email protected]>
---
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;
--
2.25.1
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 <[email protected]>
> Signed-off-by: Naveen Mamindlapalli <[email protected]>
Reviewed-by: Maciej Fijalkowski <[email protected]>
> ---
> 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 <[email protected]>
> Sent: Wednesday, June 7, 2023 5:17 PM
> To: Sai Krishna Gajula <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; Sunil Kovvuri Goutham <[email protected]>;
> [email protected]; Naveen Mamindlapalli <[email protected]>
> 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 <[email protected]>
> > Signed-off-by: Naveen Mamindlapalli <[email protected]>
>
> Reviewed-by: Maciej Fijalkowski <[email protected]>
>
> > ---
> > 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 <[email protected]>
> > Sent: Wednesday, June 7, 2023 5:17 PM
> > To: Sai Krishna Gajula <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; linux-
> > [email protected]; Sunil Kovvuri Goutham <[email protected]>;
> > [email protected]; Naveen Mamindlapalli <[email protected]>
> > 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 <[email protected]>
> > > Signed-off-by: Naveen Mamindlapalli <[email protected]>
> >
> > Reviewed-by: Maciej Fijalkowski <[email protected]>
> >
> > > ---
> > > 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
> > >
> > >