netdev - Re: [PATCH net-next] bnxt_en: add unlocked version of bnxt_refclk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CALs4sv10sWUyRfUDSQsU0N1jevDdenbMejyeKSwOM6uC=uVhYg@mail.gmail.com>
Date: Thu, 7 Nov 2024 18:41:57 +0530
From: Pavan Chebbi <pavan.chebbi@...adcom.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: Vadim Fedorenko <vadfed@...a.com>, Michael Chan <michael.chan@...adcom.com>, 
	Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew+netdev@...n.ch>, Paolo Abeni <pabeni@...hat.com>, 
	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org, 
	Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH net-next] bnxt_en: add unlocked version of bnxt_refclk_read

> >> -       bnxt_refclk_read(ptp->bp, NULL, &ns);
> >> +       __bnxt_refclk_read(ptp->bp, NULL, &ns);
> >
> > With this change, bnxt_cc_read() is executed without protection during
> > timecounter_init(), right?
> > I think we should hold the ptp_lock inside bnxt_ptp_timecounter_init()
> > just like we do in bnxt_ptp_init_rtc()
>
> Well, yes, that's correct. Technically we have to hold this lock (and I
> will add it in v2), but if think a bit wider, do we expect
> bnxt_fw_reset()/bnxt_force_fw_reset() to be called during device init
> phase? If yes, we have proper time frame between bnxt_ptp_cfg allocation
> in __bnxt_hwrm_ptp_qcfg() (which assigns it to bp->ptp) and spinlock
> initialization in bnxt_ptp_init(), during which spinlock must not be
> accessed. And if we imagine the situation when fw_reset request can be
> initiated during initialization, the next flow can happen:
>
> CPU0:                           CPU1:
> __bnxt_hwrm_ptp_qcfg()
>    ptp_cfg = kzalloc()
>    bp->ptp = ptp_cfg
>                                 bnxt_force_fw_reset()
>                                   if (bp->ptp)
>                                     spin_lock_irqsave(bp->ptp->ptp_lock)
>    bnxt_ptp_init()
>      spinlock_init(ptp_lock)
>
>
> So we either should not have an option of resetting FW during init
> phase (and then there will be no need to use lock), or we have to
> re-think FW reset serialization completley. WDYT?
>
I don't disagree. Since bnxt_ptp_init() is happening at the probe
time, absent async event queue and devlink infra, in my mind we can
safely assume we won't encounter a race with fw_reset.
The holding of lock is more for being theoretically correct, like you
pointed out.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4209 bytes)