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  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]
Date:   Thu, 8 Oct 2020 07:29:04 +0200
From:   Sven Auhagen <sven.auhagen@...eatech.de>
To:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Cc:     anthony.l.nguyen@...el.com, davem@...emloft.net,
        netdev@...r.kernel.org, nhorman@...hat.com, sassmann@...hat.com,
        sandeep.penigalapati@...el.com, brouer@...hat.com
Subject: Re: [PATCH 5/7] igb: use igb_rx_buffer_flip

On Wed, Oct 07, 2020 at 11:32:57PM +0200, Maciej Fijalkowski wrote:
> On Wed, Oct 07, 2020 at 05:25:04PM +0200, sven.auhagen@...eatech.de wrote:
> > From: Sven Auhagen <sven.auhagen@...eatech.de>
> > 
> > Also use the new helper function igb_rx_buffer_flip in
> > igb_build_skb/igb_add_rx_frag.
> > 
> > Signed-off-by: Sven Auhagen <sven.auhagen@...eatech.de>
> > ---
> >  drivers/net/ethernet/intel/igb/igb_main.c | 87 +++++++++--------------
> >  1 file changed, 35 insertions(+), 52 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> > index 36ff8725fdaf..f34faf24190a 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > @@ -8255,6 +8255,34 @@ static bool igb_can_reuse_rx_page(struct igb_rx_buffer *rx_buffer)
> >  	return true;
> >  }
> >  
> > +static unsigned int igb_rx_frame_truesize(struct igb_ring *rx_ring,
> > +					  unsigned int size)
> > +{
> > +	unsigned int truesize;
> > +
> > +#if (PAGE_SIZE < 8192)
> > +	truesize = igb_rx_pg_size(rx_ring) / 2; /* Must be power-of-2 */
> > +#else
> > +	truesize = ring_uses_build_skb(rx_ring) ?
> > +		SKB_DATA_ALIGN(IGB_SKB_PAD + size) +
> > +		SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) :
> > +		SKB_DATA_ALIGN(size);
> > +#endif
> > +	return truesize;
> > +}
> > +
> > +static void igb_rx_buffer_flip(struct igb_ring *rx_ring,
> > +			       struct igb_rx_buffer *rx_buffer,
> > +			       unsigned int size)
> > +{
> > +	unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
> > +#if (PAGE_SIZE < 8192)
> > +	rx_buffer->page_offset ^= truesize;
> > +#else
> > +	rx_buffer->page_offset += truesize;
> > +#endif
> > +}
> > +
> >  /**
> >   *  igb_add_rx_frag - Add contents of Rx buffer to sk_buff
> >   *  @rx_ring: rx descriptor ring to transact packets on
> > @@ -8269,20 +8297,12 @@ static void igb_add_rx_frag(struct igb_ring *rx_ring,
> >  			    struct sk_buff *skb,
> >  			    unsigned int size)
> >  {
> > -#if (PAGE_SIZE < 8192)
> > -	unsigned int truesize = igb_rx_pg_size(rx_ring) / 2;
> > -#else
> > -	unsigned int truesize = ring_uses_build_skb(rx_ring) ?
> > -				SKB_DATA_ALIGN(IGB_SKB_PAD + size) :
> > -				SKB_DATA_ALIGN(size);
> > -#endif
> > +	unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
> 
> I don't think we need to account the size of skb_shared_info when adding
> another frag as we already have the skb in place with its skb_shared_info.
> 
> Also, please make sure that you list all of the changes that patch
> contains in the commit message, you simply skipped the fact that you're
> making use of igb_rx_frame_truesize on other places.

Do you have a suggestion on how to add the truesize and flip function
at this point? The truesize is not suited here then and returns 
the wrong size for frags.

> 
> > +
> >  	skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, rx_buffer->page,
> >  			rx_buffer->page_offset, size, truesize);
> > -#if (PAGE_SIZE < 8192)
> > -	rx_buffer->page_offset ^= truesize;
> > -#else
> > -	rx_buffer->page_offset += truesize;
> > -#endif
> > +
> > +	igb_rx_buffer_flip(rx_ring, rx_buffer, size);
> >  }
> >  
> >  static struct sk_buff *igb_construct_skb(struct igb_ring *rx_ring,
> > @@ -8345,14 +8365,9 @@ static struct sk_buff *igb_build_skb(struct igb_ring *rx_ring,
> >  				     struct xdp_buff *xdp,
> >  				     union e1000_adv_rx_desc *rx_desc)
> >  {
> > +	unsigned int size = xdp->data_end - xdp->data_hard_start;
> > +	unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
> 
> Here you will be counting the IGB_SKB_PAD twice for pages > 4k.
> xdp->data_end - xdp->data_hard_start already includes the IGB_SKB_PAD and
> then igb_rx_frame_truesize will add this IGB_SKB_PAD once again to the
> size you're providing.
> 
> Please drop the additional usage of igb_rx_frame_truesize in this patch.
> 

The igb_rx_buffer_flip will use the wrong truesize though even when I
remove igb_rx_frame_truesize here.
Should I drop the entire patch as the truesize for the flip will be
incorrect in both places?

> >  	unsigned int metasize = xdp->data - xdp->data_meta;
> > -#if (PAGE_SIZE < 8192)
> > -	unsigned int truesize = igb_rx_pg_size(rx_ring) / 2;
> > -#else
> > -	unsigned int truesize = SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) +
> > -				SKB_DATA_ALIGN(xdp->data_end -
> > -					       xdp->data_hard_start);
> > -#endif
> >  	struct sk_buff *skb;
> >  
> >  	/* prefetch first cache line of first page */
> > @@ -8377,11 +8392,7 @@ static struct sk_buff *igb_build_skb(struct igb_ring *rx_ring,
> >  	}
> >  
> >  	/* update buffer offset */
> > -#if (PAGE_SIZE < 8192)
> > -	rx_buffer->page_offset ^= truesize;
> > -#else
> > -	rx_buffer->page_offset += truesize;
> > -#endif
> > +	igb_rx_buffer_flip(rx_ring, rx_buffer, size);
> >  
> >  	return skb;
> >  }
> > @@ -8431,34 +8442,6 @@ static struct sk_buff *igb_run_xdp(struct igb_adapter *adapter,
> >  	return ERR_PTR(-result);
> >  }
> >  
> > -static unsigned int igb_rx_frame_truesize(struct igb_ring *rx_ring,
> > -					  unsigned int size)
> > -{
> > -	unsigned int truesize;
> > -
> > -#if (PAGE_SIZE < 8192)
> > -	truesize = igb_rx_pg_size(rx_ring) / 2; /* Must be power-of-2 */
> > -#else
> > -	truesize = ring_uses_build_skb(rx_ring) ?
> > -		SKB_DATA_ALIGN(IGB_SKB_PAD + size) +
> > -		SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) :
> > -		SKB_DATA_ALIGN(size);
> > -#endif
> > -	return truesize;
> > -}
> > -
> > -static void igb_rx_buffer_flip(struct igb_ring *rx_ring,
> > -			       struct igb_rx_buffer *rx_buffer,
> > -			       unsigned int size)
> > -{
> > -	unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
> > -#if (PAGE_SIZE < 8192)
> > -	rx_buffer->page_offset ^= truesize;
> > -#else
> > -	rx_buffer->page_offset += truesize;
> > -#endif
> > -}
> > -
> >  static inline void igb_rx_checksum(struct igb_ring *ring,
> >  				   union e1000_adv_rx_desc *rx_desc,
> >  				   struct sk_buff *skb)
> > -- 
> > 2.20.1
> > 

Powered by blists - more mailing lists