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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20240628181332.GD837606@kernel.org>
Date: Fri, 28 Jun 2024 19:13:32 +0100
From: Simon Horman <horms@...nel.org>
To: Michael Chan <michael.chan@...adcom.com>
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:37:19AM -0700, Michael Chan wrote:
> 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)

Hi Michael,

Thanks for your quick response, it is the kind of explanation that
I was looking for, but wasn't sufficiently familiar with the code
to find myself.