netdev - Re: [PATCH net-next 09/10] bnxt_en: Increase the max total outstanding PTP TX packets to 4

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 for Android: free password hash cracker in your pocket

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

Date: Fri, 28 Jun 2024 10:37:19 -0700
From: Michael Chan <michael.chan@...adcom.com>
To: Simon Horman <horms@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, pavan.chebbi@...adcom.com, 
	andrew.gospodarek@...adcom.com, richardcochran@...il.com
Subject: Re: [PATCH net-next 09/10] bnxt_en: Increase the max total
 outstanding PTP TX packets to 4

On Fri, Jun 28, 2024 at 10:03 AM Simon Horman <horms@...nel.org> wrote:
>
> On Wed, Jun 26, 2024 at 09:43:06AM -0700, Michael Chan wrote:
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index ed2bbdf6b25f..0867861c14bd 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -457,8 +457,8 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >       unsigned int length, pad = 0;
> >       u32 len, free_size, vlan_tag_flags, cfa_action, flags;
> >       struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
> > -     u16 prod, last_frag;
> >       struct pci_dev *pdev = bp->pdev;
> > +     u16 prod, last_frag, txts_prod;
> >       struct bnxt_tx_ring_info *txr;
> >       struct bnxt_sw_tx_bd *tx_buf;
> >       __le32 lflags = 0;
> > @@ -526,11 +526,19 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >                       if (!bnxt_ptp_parse(skb, &seq_id, &hdr_off)) {
> >                               if (vlan_tag_flags)
> >                                       hdr_off += VLAN_HLEN;
> > -                             ptp->txts_req.tx_seqid = seq_id;
> > -                             ptp->txts_req.tx_hdr_off = hdr_off;
> >                               lflags |= cpu_to_le32(TX_BD_FLAGS_STAMP);
> >                               tx_buf->is_ts_pkt = 1;
> >                               skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> > +
> > +                             spin_lock_bh(&ptp->ptp_tx_lock);
> > +                             txts_prod = ptp->txts_prod;
> > +                             ptp->txts_prod = NEXT_TXTS(txts_prod);
> > +                             spin_unlock_bh(&ptp->ptp_tx_lock);
> > +
> > +                             ptp->txts_req[txts_prod].tx_seqid = seq_id;
> > +                             ptp->txts_req[txts_prod].tx_hdr_off = hdr_off;
> > +                             tx_buf->txts_prod = txts_prod;
> > +
> >                       } else {
> >                               atomic_inc(&bp->ptp_cfg->tx_avail);
> >                       }
> > @@ -770,7 +778,9 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >  tx_kick_pending:
> >       if (BNXT_TX_PTP_IS_SET(lflags)) {
> >               atomic64_inc(&bp->ptp_cfg->stats.ts_err);
> > -             atomic_inc(&bp->ptp_cfg->tx_avail);
> > +             if (!(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
> > +                     /* set SKB to err so PTP worker will clean up */
> > +                     ptp->txts_req[txts_prod].tx_skb = ERR_PTR(-EIO);
>
> Hi Michael
>
> Sparse complains that previously it was assumed that ptp could be NULL,
> but here it is accessed without checking for that.
>
> Perhaps it can't occur, but my brief check leads me to think it might.

Simon, thanks for the review.  The key is this if statement:

if (BNXT_TX_PTP_IS_SET(lflags))

This if statement is true if the lflags have the TX_BD_FLAGS_STAMP
set.  This flag is set only if ptp is valid because this flag tells
the hardware to take the timestamp.

>
> On line 488 there is the following:
>
>         if (unlikely(ipv6_hopopt_jumbo_remove(skb)))
>                 goto tx_free;
>
> Which will lead to the code in the hunk above.

The lflags will not have the TX_BD_FLAGS_STAMP flag set if we jump from here.

>
> Then on line 513 there is a check for ptp being NULL:
>
>
>         if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && ptp &&
>             ptp->tx_tstamp_en) {
>
> And ptp is not set between lines 488 and 513.
>
>
> Sparse also complains that txts_prod may be used uninitaialised.
> This also seems to be a valid concern as it does seem to be the case
> on line 488.

Same explanation for txts_prod.  txts_prod will always be set if
lflags has TX_BD_FLAGS_STAMP set and this condition is false:

(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP)

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