| 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
| ||
|
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