[Intel-wired-lan] [PATCH 1/3] igb: Pull timestamp from fragment before adding it to skb

[Intel-wired-lan] [PATCH 1/3] igb: Pull timestamp from fragment before adding it to skb

[Alexander Duyck]

This change makes it so that we pull the timestamp from the fragment before
we add it to the skb.  By doing this we can avoid a possible issue in which
the fragment can possibly be less than IGB_RX_HDR_LEN due to the timestamp
being pulled after the copybreak check.

While making this change I realized we could also pull the rest of the
igb_pull_tail function into igb_add_rx_frag since in the case of igb,
unlike ixgbe, we are able to unmap the entire buffer before calling
add_rx_frag so merging the two allows for sharing of code between the two
merged functions.

Reported-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c |   94 ++++++++---------------------
 1 file changed, 25 insertions(+), 69 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 8457d0306e3a..cab50593c315 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6637,22 +6637,25 @@ static bool igb_add_rx_frag(struct igb_ring *rx_ring,
 			    struct sk_buff *skb)
 {
 	struct page *page = rx_buffer->page;
+	unsigned char *va = page_address(page) + rx_buffer->page_offset;
 	unsigned int size = le16_to_cpu(rx_desc->wb.upper.length);
 #if (PAGE_SIZE < 8192)
 	unsigned int truesize = IGB_RX_BUFSZ;
 #else
-	unsigned int truesize = ALIGN(size, L1_CACHE_BYTES);
+	unsigned int truesize = SKB_DATA_ALIGN(size);
 #endif
+	unsigned int pull_len;
 
-	if ((size <= IGB_RX_HDR_LEN) && !skb_is_nonlinear(skb)) {
-		unsigned char *va = page_address(page) + rx_buffer->page_offset;
+	if (unlikely(skb_is_nonlinear(skb)))
+		goto add_tail_frag;
 
-		if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) {
-			igb_ptp_rx_pktstamp(rx_ring->q_vector, va, skb);
-			va += IGB_TS_HDR_LEN;
-			size -= IGB_TS_HDR_LEN;
-		}
+	if (unlikely(igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP))) {
+		igb_ptp_rx_pktstamp(rx_ring->q_vector, va, skb);
+		va += IGB_TS_HDR_LEN;
+		size -= IGB_TS_HDR_LEN;
+	}
 
+	if (likely(size <= IGB_RX_HDR_LEN)) {
 		memcpy(__skb_put(skb, size), va, ALIGN(size, sizeof(long)));
 
 		/* page is not reserved, we can reuse buffer as-is */
@@ -6664,8 +6667,21 @@ static bool igb_add_rx_frag(struct igb_ring *rx_ring,
 		return false;
 	}
 
+	/* we need the header to contain the greater of either ETH_HLEN or
+	 * 60 bytes if the skb->len is less than 60 for skb_pad.
+	 */
+	pull_len = eth_get_headlen(va, IGB_RX_HDR_LEN);
+
+	/* align pull length to size of long to optimize memcpy performance */
+	memcpy(__skb_put(skb, pull_len), va, ALIGN(pull_len, sizeof(long)));
+
+	/* update all of the pointers */
+	va += pull_len;
+	size -= pull_len;
+
+add_tail_frag:
 	skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page,
-			rx_buffer->page_offset, size, truesize);
+			(unsigned long)va & ~PAGE_MASK, size, truesize);
 
 	return igb_can_reuse_rx_page(rx_buffer, page, truesize);
 }
@@ -6807,62 +6823,6 @@ static bool igb_is_non_eop(struct igb_ring *rx_ring,
 }
 
 /**
- *  igb_pull_tail - igb specific version of skb_pull_tail
- *  @rx_ring: rx descriptor ring packet is being transacted on
- *  @rx_desc: pointer to the EOP Rx descriptor
- *  @skb: pointer to current skb being adjusted
- *
- *  This function is an igb specific version of __pskb_pull_tail.  The
- *  main difference between this version and the original function is that
- *  this function can make several assumptions about the state of things
- *  that allow for significant optimizations versus the standard function.
- *  As a result we can do things like drop a frag and maintain an accurate
- *  truesize for the skb.
- */
-static void igb_pull_tail(struct igb_ring *rx_ring,
-			  union e1000_adv_rx_desc *rx_desc,
-			  struct sk_buff *skb)
-{
-	struct skb_frag_struct *frag = &skb_shinfo(skb)->frags[0];
-	unsigned char *va;
-	unsigned int pull_len;
-
-	/* it is valid to use page_address instead of kmap since we are
-	 * working with pages allocated out of the lomem pool per
-	 * alloc_page(GFP_ATOMIC)
-	 */
-	va = skb_frag_address(frag);
-
-	if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) {
-		/* retrieve timestamp from buffer */
-		igb_ptp_rx_pktstamp(rx_ring->q_vector, va, skb);
-
-		/* update pointers to remove timestamp header */
-		skb_frag_size_sub(frag, IGB_TS_HDR_LEN);
-		frag->page_offset += IGB_TS_HDR_LEN;
-		skb->data_len -= IGB_TS_HDR_LEN;
-		skb->len -= IGB_TS_HDR_LEN;
-
-		/* move va to start of packet data */
-		va += IGB_TS_HDR_LEN;
-	}
-
-	/* we need the header to contain the greater of either ETH_HLEN or
-	 * 60 bytes if the skb->len is less than 60 for skb_pad.
-	 */
-	pull_len = eth_get_headlen(va, IGB_RX_HDR_LEN);
-
-	/* align pull length to size of long to optimize memcpy performance */
-	skb_copy_to_linear_data(skb, va, ALIGN(pull_len, sizeof(long)));
-
-	/* update all of the pointers */
-	skb_frag_size_sub(frag, pull_len);
-	frag->page_offset += pull_len;
-	skb->data_len -= pull_len;
-	skb->tail += pull_len;
-}
-
-/**
  *  igb_cleanup_headers - Correct corrupted or empty headers
  *  @rx_ring: rx descriptor ring packet is being transacted on
  *  @rx_desc: pointer to the EOP Rx descriptor
@@ -6889,10 +6849,6 @@ static bool igb_cleanup_headers(struct igb_ring *rx_ring,
 		}
 	}
 
-	/* place header in linear portion of buffer */
-	if (skb_is_nonlinear(skb))
-		igb_pull_tail(rx_ring, rx_desc, skb);
-
 	/* if eth_skb_pad returns an error the skb was freed */
 	if (eth_skb_pad(skb))
 		return true;

[Intel-wired-lan] [PATCH 1/3] igb: Pull timestamp from fragment before adding it to skb

[Cong Wang]

On Wed, Apr 22, 2015 at 9:49 PM, Alexander Duyck
<alexander.h.duyck@redhat.com> wrote:
> This change makes it so that we pull the timestamp from the fragment before
> we add it to the skb.  By doing this we can avoid a possible issue in which
> the fragment can possibly be less than IGB_RX_HDR_LEN due to the timestamp
> being pulled after the copybreak check.


You said the first frag is always 2K if it exists. If this is not true, then
my patch is needed too.

>
> While making this change I realized we could also pull the rest of the
> igb_pull_tail function into igb_add_rx_frag since in the case of igb,
> unlike ixgbe, we are able to unmap the entire buffer before calling
> add_rx_frag so merging the two allows for sharing of code between the two
> merged functions.

Can you make a simple change for stable? Of course unless this
patch is targeted for net-next, but $subject doesn't say so.

Also, please Cc netdev, I am not on intel-wired-lan list.

BTW, it would be nice to Cc me for all 3 patches, as they might
be related for review.

[Intel-wired-lan] [PATCH 1/3] igb: Pull timestamp from fragment before adding it to skb

[Alexander Duyck]

On 04/23/2015 10:10 AM, Cong Wang wrote:
> On Wed, Apr 22, 2015 at 9:49 PM, Alexander Duyck
> <alexander.h.duyck@redhat.com> wrote:
>> This change makes it so that we pull the timestamp from the fragment before
>> we add it to the skb.  By doing this we can avoid a possible issue in which
>> the fragment can possibly be less than IGB_RX_HDR_LEN due to the timestamp
>> being pulled after the copybreak check.
>
> You said the first frag is always 2K if it exists. If this is not true, then
> my patch is needed too.

I said the first fragment is always 2K if a secondary frag exists.  We
always reserve 2K for each buffer so the memory is there and could be
read, it just wouldn't contain valid header data once you get past the
size reported by the Rx descriptor.

The issue is the data_len can get mangled due to the 16 bytes for the
timestamp being unaccounted for.  The goal was to fix this and not
introduce any other vulnerabilities or performance regressions which was
my concern with your patch.  I would prefer to err on the side of
parsing fewer bytes than possibly parsing a full frame if someone
decided to submit one that was nothing but tunnel headers for instance.

>> While making this change I realized we could also pull the rest of the
>> igb_pull_tail function into igb_add_rx_frag since in the case of igb,
>> unlike ixgbe, we are able to unmap the entire buffer before calling
>> add_rx_frag so merging the two allows for sharing of code between the two
>> merged functions.
> Can you make a simple change for stable? Of course unless this
> patch is targeted for net-next, but $subject doesn't say so.

I assume net-next since that is where most of the changes on this list
are submitted to.

> Also, please Cc netdev, I am not on intel-wired-lan list.

I'll try to remember that for next time.

> BTW, it would be nice to Cc me for all 3 patches, as they might
> be related for review.

I honestly don't think this is worth the effort to backport to stable. 
If you really feel the need your best bet would probably be to just
modify the eth_get_headlen line and pass it IGB_RX_HDR_LEN -
IGB_TS_HDR_LEN, then you would be guaranteed that the issue cannot occur
and still should be able to get a reasonable header size pulled in.  The
fact is this is an extreme corner case and the odds are extremely low of
anyone ever encountering it.

- Alex

[Intel-wired-lan] [PATCH 1/3] igb: Pull timestamp from fragment before adding it to skb

[Cong Wang]

On Thu, Apr 23, 2015 at 11:19 AM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On 04/23/2015 10:10 AM, Cong Wang wrote:
>> On Wed, Apr 22, 2015 at 9:49 PM, Alexander Duyck
>> <alexander.h.duyck@redhat.com> wrote:
>>> This change makes it so that we pull the timestamp from the fragment before
>>> we add it to the skb.  By doing this we can avoid a possible issue in which
>>> the fragment can possibly be less than IGB_RX_HDR_LEN due to the timestamp
>>> being pulled after the copybreak check.
>>
>> You said the first frag is always 2K if it exists. If this is not true, then
>> my patch is needed too.
>
> I said the first fragment is always 2K if a secondary frag exists.  We
> always reserve 2K for each buffer so the memory is there and could be
> read, it just wouldn't contain valid header data once you get past the
> size reported by the Rx descriptor.
>
> The issue is the data_len can get mangled due to the 16 bytes for the
> timestamp being unaccounted for.  The goal was to fix this and not
> introduce any other vulnerabilities or performance regressions which was
> my concern with your patch.  I would prefer to err on the side of
> parsing fewer bytes than possibly parsing a full frame if someone
> decided to submit one that was nothing but tunnel headers for instance.

Looking deeper into this, I think we need to do nothing here.

a) We pass skb as NULL in eth_get_headlen(), in which case tunnel
case is not handled;

b) Even with tunnel headers, I can't think out a case we can construct
a valid header longer than 256 to fool eth_get_headlen().

I am fine with leaving as it is.

[Intel-wired-lan] [PATCH 1/3] igb: Pull timestamp from fragment before adding it to skb

[Brown, Aaron F]

> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Alexander Duyck
> Sent: Wednesday, April 22, 2015 9:49 PM
> To: intel-wired-lan at lists.osuosl.org
> Cc: Cong Wang
> Subject: [Intel-wired-lan] [PATCH 1/3] igb: Pull timestamp from fragment
> before adding it to skb
> 
> This change makes it so that we pull the timestamp from the fragment
> before
> we add it to the skb.  By doing this we can avoid a possible issue in
> which
> the fragment can possibly be less than IGB_RX_HDR_LEN due to the timestamp
> being pulled after the copybreak check.
> 
> While making this change I realized we could also pull the rest of the
> igb_pull_tail function into igb_add_rx_frag since in the case of igb,
> unlike ixgbe, we are able to unmap the entire buffer before calling
> add_rx_frag so merging the two allows for sharing of code between the two
> merged functions.
> 
> Reported-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c |   94 ++++++++----------------
> -----
>  1 file changed, 25 insertions(+), 69 deletions(-)

Tested-by: Aaron Brown <aaron.f.brown@intel.com>