On Wed, Apr 22, 2015 at 3:34 PM, Alexander Duyck <alexander.du...@gmail.com> wrote: > On 04/22/2015 02:56 PM, Cong Wang wrote: >> On Wed, Apr 22, 2015 at 2:42 PM, Alexander Duyck >> <alexander.du...@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. >
If the first frag is always 2K, then this is not a problem either. IGB_TS_HDR_LEN + IGB_RX_HDR_LEN < 2K. >> >>>> 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. So the first frag is always 2K, at least more than IGB_RX_HDR_LEN then we are fine even for TSIP case. The code looks correct to me now, except it is suspicious skb->len is not updated after skb_copy_to_linear_data() while skb->tail is advanced already. I need to think more before submitting a patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
