netdev - Re: [PATCH net v2] bnxt_en: improve TX timestamping FIFO configuration

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALs4sv1sKLABmzHNj3DuMRYjJBm2_t2WZttr56DfHozpA7kqrQ@mail.gmail.com>
Date: Mon, 21 Apr 2025 15:08:59 +0530
From: Pavan Chebbi <pavan.chebbi@...adcom.com>
To: Vadim Fedorenko <vadfed@...a.com>
Cc: Michael Chan <michael.chan@...adcom.com>, Jakub Kicinski <kuba@...nel.org>, 
	Vadim Fedorenko <vadim.fedorenko@...ux.dev>, Richard Cochran <richardcochran@...il.com>, 
	netdev@...r.kernel.org
Subject: Re: [PATCH net v2] bnxt_en: improve TX timestamping FIFO configuration

On Thu, Apr 17, 2025 at 9:31 PM Vadim Fedorenko <vadfed@...a.com> wrote:
>
> 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")
> Signed-off-by: Vadim Fedorenko <vadfed@...a.com>
> ---
> 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 | 28 ++++++++++++++-----
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  1 +
>  3 files changed, 25 insertions(+), 9 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..197893220070 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);

For completeness, should we not set txts_req->tx_skb = NULL here, just
like we did in bnxt_ptp_clear which is now gone.

> +               cons = NEXT_TXTS(cons);
> +       }
> +       ptp->txts_cons = cons;
> +       ptp_schedule_worker(ptp->ptp_clock, 0);
> +}
> +

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