Re: [Patch net] igb: pass the correct maxlen for eth_get_headlen()

[Alexander Duyck <alexander.duyck@gmail.com>]

On 04/22/2015 02:56 PM, Cong Wang wrote:
> On Wed, Apr 22, 2015 at 2:42 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On 04/22/2015 01:33 PM, Cong Wang wrote:
>>> First, make sure you don't miss the TSIP case right above:
>>>
>>> The frag starting pointer and its size are advanced by:
>>>
>>> skb_frag_size_sub(frag, IGB_TS_HDR_LEN);
>>> ...
>>> va += IGB_TS_HDR_LEN;
>>>
>>> even though we unlikely pull header longer than
>>> IGB_RX_HDR_LEN - IGB_TS_HDR_LEN either.
>> So I believe this is a possible bug, one heck of a corner case to get
>> into though.  It requires timestamp in packet, size 240 - 256, and a
>> malformed header.
>>
>> The proper fix would probably be to pull the timestamp out of the packet
>> before we add it to the frame.  I'll submit a patch to address this.
>>
>
> Huh? Doesn't my patch already fix this? skb_frag_size() is always
> up to date. Or you mean another different problem?

Your patch has other issues.  I am still NAKing your patch, but there is
an issue with igb that you have pointed out.  The proper fix would be to
deal with the timestamp before we add the page fragment to the skb.

>
>>> Second, the check you mentioned above is:
>>>
>>> if ((size <= IGB_RX_HDR_LEN) && !skb_is_nonlinear(skb))
>>>
>>> skb is nonlinear _after_ the first igb_add_rx_frag(), a second
>>> igb_add_rx_frag() is possible since igb_is_non_eop() could
>>> return true.
>> I'm not sure this part makes any sense.  We pull the data out of the
>> first fragment always.  If skb_is_nonlinear is set then we should have
>> at least 2K - 16B in the case of igb.  We will never have a second
>> fragment without at least 2K of data being given in the first.
> Apparently my igb knowledge isn't enough to verify this, I just did
> logical analysis.

The logic with igb is that if skb_is_nonlinear it means that a page has
already been added.  The way the hardware works is that it will break a
frame up into 2K segments until the entire frame is written.  So the
only way to get a second page fragment is if the first has used the
entire 2K buffer it was given.

- Alex