netdev - Re: [PATCH v4 bpf-next 03/10] ice: check DD bit on Rx descriptor rather than (EOP

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: <YrBRxeIXhmxBDOgr@boxer>
Date:   Mon, 20 Jun 2022 12:53:57 +0200
From:   Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        netdev@...r.kernel.org, magnus.karlsson@...el.com,
        bjorn@...nel.org, kuba@...nel.org
Subject: Re: [PATCH v4 bpf-next 03/10] ice: check DD bit on Rx descriptor
 rather than (EOP | RS)

On Fri, Jun 17, 2022 at 06:58:57PM -0700, John Fastabend wrote:
> Maciej Fijalkowski wrote:
> > Tx side sets EOP and RS bits on descriptors to indicate that a
> > particular descriptor is the last one and needs to generate an irq when
> > it was sent. These bits should not be checked on completion path
> > regardless whether it's the Tx or the Rx. DD bit serves this purpose and
> > it indicates that a particular descriptor is either for Rx or was
> > successfully Txed.
> > 
> > Look at DD bit being set in ice_lbtest_receive_frames() instead of EOP
> > and RS pair.
> > 
> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
> 
> Is this a bugfix? If so it should go to bpf tree.

Previous logic worked without this patch by an accident, so I don't change
the behaviour of the loopback self test itself, therefore I was not sure
if this could be classified as a bugfix.

Rx descriptor's status_error0 field has ICE_RX_FLEX_DESC_STATUS0_DD_S and
ICE_RX_FLEX_DESC_STATUS0_EOF_S set, which have the following values:

enum ice_rx_flex_desc_status_error_0_bits {
	/* Note: These are predefined bit offsets */
	ICE_RX_FLEX_DESC_STATUS0_DD_S = 0,
	ICE_RX_FLEX_DESC_STATUS0_EOF_S,
	(...)
};

Old code was only ORing two following enums:

enum ice_tx_desc_cmd_bits {
	ICE_TX_DESC_CMD_EOP			= 0x0001,
	ICE_TX_DESC_CMD_RS			= 0x0002,
	(...)
};

If BIT() was used around ICE_TX_DESC_CMD_EOP and ICE_TX_DESC_CMD_RS and if
they were checked separately then on RS bit we would fail.

(let me also check for EOF bit in next revision)

> 
> > ---
> >  drivers/net/ethernet/intel/ice/ice_ethtool.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> > index 1e71b70f0e52..b6275a29fa0d 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> > @@ -658,7 +658,7 @@ static int ice_lbtest_receive_frames(struct ice_rx_ring *rx_ring)
> >  		rx_desc = ICE_RX_DESC(rx_ring, i);
> >  
> >  		if (!(rx_desc->wb.status_error0 &
> > -		    cpu_to_le16(ICE_TX_DESC_CMD_EOP | ICE_TX_DESC_CMD_RS)))
> > +		    cpu_to_le16(BIT(ICE_RX_FLEX_DESC_STATUS0_DD_S))))
> >  			continue;
> >  
> >  		rx_buf = &rx_ring->rx_buf[i];
> > -- 
> > 2.27.0
> > 
> 
>