netdev - Re: [PATCH net] 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: <CACKFLin8=vm8MBBFpFgJv2Jw4Vn_m43cvXZUEMTfhgFnjZ+m1w@mail.gmail.com>
Date: Wed, 16 Apr 2025 10:03:22 -0700
From: Michael Chan <michael.chan@...adcom.com>
To: Vadim Fedorenko <vadfed@...a.com>
Cc: Vadim Fedorenko <vadim.fedorenko@...ux.dev>, Pavan Chebbi <pavan.chebbi@...adcom.com>, 
	Jakub Kicinski <kuba@...nel.org>, Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net] bnxt_en: improve TX timestamping FIFO configuration

On Wed, Apr 16, 2025 at 8:01 AM 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>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  4 ++--
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 23 +++++++++++++++++++
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  1 +
>  3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index c8e3468eee61..45d178586316 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -3517,6 +3517,8 @@ static void bnxt_free_skbs(struct bnxt *bp)
>  {
>         bnxt_free_tx_skbs(bp);
>         bnxt_free_rx_skbs(bp);
> +       if (bp->ptp_cfg && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
> +               bnxt_ptp_free_txts_skbs(bp->ptp_cfg);

Since these are TX SKBs, it's slightly more logical if we put this in
bnxt_free_tx_skbs().

>  }
>
>  static void bnxt_init_ctx_mem(struct bnxt_ctx_mem_type *ctxm, void *p, int len)

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> index 2d4e19b96ee7..39dc4f1f651a 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
> @@ -794,6 +794,29 @@ 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);
> +
> +       spin_lock_bh(&ptp->ptp_tx_lock);

I think the spinlock is not needed because bnxt_tx_disable() should
have been called already in the close path.

> +       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);

I think we can remove the similar code we have in bnxt_ptp_clear().
We should always go through this path before bnxt_ptp_clear().

Thanks for the patch.

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