| 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
| ||
|
Message-ID: <CAMArcTXnZ-T6nsSyEtBLj+_SzMJhEz8R4g5HnR-ToFx8JPLngw@mail.gmail.com>
Date: Thu, 1 May 2025 01:43:21 +0900
From: Taehee Yoo <ap420073@...il.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: Michael Chan <michael.chan@...adcom.com>, Pavan Chebbi <pavan.chebbi@...adcom.com>,
Jakub Kicinski <kuba@...nel.org>, Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net v4] bnxt_en: improve TX timestamping FIFO configuration
On Wed, Apr 30, 2025 at 11:55 PM Vadim Fedorenko
<vadim.fedorenko@...ux.dev> wrote:
>
> On 30/04/2025 13:59, Taehee Yoo wrote:
> > On Thu, Apr 24, 2025 at 10:11 PM Vadim Fedorenko <vadfed@...a.com> wrote:
> >>
> >
> > Hi Vadim,
> > Thanks for this work!
> >
> >> Reconfiguration of netdev may trigger close/open procedure which can
> >> break FIFO status by adjusting the amount of empty slots for TX
> >> timestamps. But it is not really needed because timestamps for the
> >> packets sent over the wire still can be retrieved. On the other side,
> >> during netdev close procedure any skbs waiting for TX timestamps can be
> >> leaked because there is no cleaning procedure called. Free skbs waiting
> >> for TX timestamps when closing netdev.
> >>
> >> Fixes: 8aa2a79e9b95 ("bnxt_en: Increase the max total outstanding PTP TX packets to 4")
> >> Reviewed-by: Michael Chan <michael.chan@...adcom.com>
> >> Reviewed-by: Pavan Chebbi <pavan.chebbi@...adcom.com>
> >> Signed-off-by: Vadim Fedorenko <vadfed@...a.com>
> >> ---
> >> v3 -> v4:
> >> * actually remove leftover unused variable in bnxt_ptp_clear()
> >> (v3 was not committed before preparing unfortunately)
> >> v2 -> v3:
> >> * remove leftover unused variable in bnxt_ptp_clear()
> >> v1 -> v2:
> >> * move clearing of TS skbs to bnxt_free_tx_skbs
> >> * remove spinlock as no TX is possible after bnxt_tx_disable()
> >> * remove extra FIFO clearing in bnxt_ptp_clear()
> >> ---
> >> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 ++--
> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 29 ++++++++++++++-----
> >> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 1 +
> >> 3 files changed, 25 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> >> index c8e3468eee61..2c8e2c19d854 100644
> >> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> >> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> >> @@ -3414,6 +3414,9 @@ static void bnxt_free_tx_skbs(struct bnxt *bp)
> >>
> >> bnxt_free_one_tx_ring_skbs(bp, txr, i);
> >> }
> >> +
> >> + if (bp->ptp_cfg && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
> >> + bnxt_ptp_free_txts_skbs(bp->ptp_cfg);
> >> }
> >>
> >> static void bnxt_free_one_rx_ring(struct bnxt *bp, struct bnxt_rx_ring_info *rxr)
> >> @@ -12797,8 +12800,6 @@ static int __bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
> >> /* VF-reps may need to be re-opened after the PF is re-opened */
> >> if (BNXT_PF(bp))
> >> bnxt_vf_reps_open(bp);
> >> - if (bp->ptp_cfg && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
> >> - WRITE_ONCE(bp->ptp_cfg->tx_avail, BNXT_MAX_TX_TS);
> >> bnxt_ptp_init_rtc(bp, true);
> >> bnxt_ptp_cfg_tstamp_filters(bp);
> >> if (BNXT_SUPPORTS_MULTI_RSS_CTX(bp))
> >> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> >> index 2d4e19b96ee7..0669d43472f5 100644
> >> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> >> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> >> @@ -794,6 +794,27 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
> >> return HZ;
> >> }
> >>
> >> +void bnxt_ptp_free_txts_skbs(struct bnxt_ptp_cfg *ptp)
> >> +{
> >> + struct bnxt_ptp_tx_req *txts_req;
> >> + u16 cons = ptp->txts_cons;
> >> +
> >> + /* make sure ptp aux worker finished with
> >> + * possible BNXT_STATE_OPEN set
> >> + */
> >> + ptp_cancel_worker_sync(ptp->ptp_clock);
> >> +
> >> + ptp->tx_avail = BNXT_MAX_TX_TS;
> >> + while (cons != ptp->txts_prod) {
> >> + txts_req = &ptp->txts_req[cons];
> >> + if (!IS_ERR_OR_NULL(txts_req->tx_skb))
> >> + dev_kfree_skb_any(txts_req->tx_skb);
> >> + cons = NEXT_TXTS(cons);
> >> + }
> >> + ptp->txts_cons = cons;
> >> + ptp_schedule_worker(ptp->ptp_clock, 0);
> >> +}
> >> +
> >> int bnxt_ptp_get_txts_prod(struct bnxt_ptp_cfg *ptp, u16 *prod)
> >> {
> >> spin_lock_bh(&ptp->ptp_tx_lock);
> >> @@ -1105,7 +1126,6 @@ int bnxt_ptp_init(struct bnxt *bp)
> >> void bnxt_ptp_clear(struct bnxt *bp)
> >> {
> >> struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
> >> - int i;
> >>
> >> if (!ptp)
> >> return;
> >> @@ -1117,12 +1137,5 @@ void bnxt_ptp_clear(struct bnxt *bp)
> >> kfree(ptp->ptp_info.pin_config);
> >> ptp->ptp_info.pin_config = NULL;
> >>
> >> - for (i = 0; i < BNXT_MAX_TX_TS; i++) {
> >> - if (ptp->txts_req[i].tx_skb) {
> >> - dev_kfree_skb_any(ptp->txts_req[i].tx_skb);
> >> - ptp->txts_req[i].tx_skb = NULL;
> >> - }
> >> - }
> >> -
> >> bnxt_unmap_ptp_regs(bp);
> >> }
> >> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
> >> index a95f05e9c579..0481161d26ef 100644
> >> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
> >> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
> >> @@ -162,6 +162,7 @@ int bnxt_ptp_cfg_tstamp_filters(struct bnxt *bp);
> >> void bnxt_ptp_reapply_pps(struct bnxt *bp);
> >> int bnxt_hwtstamp_set(struct net_device *dev, struct ifreq *ifr);
> >> int bnxt_hwtstamp_get(struct net_device *dev, struct ifreq *ifr);
> >> +void bnxt_ptp_free_txts_skbs(struct bnxt_ptp_cfg *ptp);
> >> int bnxt_ptp_get_txts_prod(struct bnxt_ptp_cfg *ptp, u16 *prod);
> >> void bnxt_get_tx_ts_p5(struct bnxt *bp, struct sk_buff *skb, u16 prod);
> >> int bnxt_get_rx_ts_p5(struct bnxt *bp, u64 *ts, u32 pkt_ts);
> >> --
> >> 2.47.1
> >>
> >>
> >
> > I’ve encountered a kernel panic that I think is related to this patch.
> > Could you please investigate it?
> >
> > Reproducer:
> > ip link set $interface up
> > modprobe -rv bnxt_en
> >
>
> Hi Taehee!
>
> Yeah, looks like there are some issues on the remove path.
> Could you please test the diff which may fix the problem:
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 78e496b0ec26..86a5de44b6f3 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -16006,8 +16006,8 @@ static void bnxt_remove_one(struct pci_dev *pdev)
>
> bnxt_rdma_aux_device_del(bp);
>
> - bnxt_ptp_clear(bp);
> unregister_netdev(dev);
> + bnxt_ptp_clear(bp);
>
> bnxt_rdma_aux_device_uninit(bp);
>
Thanks! It seems to fix the issue.
The above reproducer can't reproduce a kernel panic.
Powered by blists - more mailing lists