netdev - Re: [PATCH net-next V1 1/2] net: Header length compution function

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:	Mon, 28 Jul 2014 16:08:46 -0700
From:	Alexander Duyck <alexander.h.duyck@...el.com>
To:	David Miller <davem@...emloft.net>, cwang@...pensource.com
CC:	amirv@...lanox.com, edumazet@...gle.com, netdev@...r.kernel.org,
	ogerlitz@...lanox.com, yevgenyp@...lanox.com, idos@...lanox.com,
	eric.dumazet@...il.com
Subject: Re: [PATCH net-next V1 1/2] net: Header length compution function

On 07/28/2014 03:42 PM, David Miller wrote:
> From: Cong Wang <cwang@...pensource.com>
> Date: Mon, 28 Jul 2014 14:26:08 -0700
> 
>> On Mon, Jul 28, 2014 at 7:50 AM, Alexander Duyck
>> <alexander.h.duyck@...el.com> wrote:
>>> On 07/28/2014 04:28 AM, Amir Vadai wrote:
>>>> +u32 eth_frame_headlen(void *data, unsigned int len)
>>>> +{
>>>> +     const struct ethhdr *eth = data;
>>>> +     struct sk_buff skb;
>>>> +
>>>> +     if (unlikely(len < ETH_HLEN))
>>>> +             return len;
>>>> +
>>>> +     skb.protocol = eth->h_proto;
>>>> +     skb.head = data + ETH_HLEN;
>>>> +     skb.data = skb.head;
>>>> +     skb_reset_network_header(&skb);
>>>> +     skb.len = len - ETH_HLEN;
>>>> +     skb.data_len = 0;
>>>> +     return __skb_get_poff(&skb) + ETH_HLEN;
>>>> +}
>>>
>>> I'm still not a big fan of allocating an sk_buff on the stack.  Seems
>>> like it isn't maintainable and really opens things up to possible issues
>>> if someone ever extends the __skb_get_poff call.  But I'm not going to
>>> force the issue since for now this isn't impacting igb or ixgbe.
>>>
>>
>> +1
>>
>> I think you can refactor the code to pass all these input as
>> arguments instead of a whole skbuff.
> 
> I was going to say the same thing, but if you take a look it's not so
> simple.
> 
> The code currently handles fragmented SKBs just fine, and you'd
> therefore have to make a seperate code path for purely linear buffers,
> and thus code duplication.
> 
> I'm still not sure what's better, to be honest.  Currently I'm leaning
> towards allowing the version in this patch set, even though it's a bit
> risky this is in the fast path so perhaps warrants such tricks for
> performance's sake.
> 

I think if anything it might be nice to add some warnings to
__skb_get_poff, and skb_flow_dissect pointing back to this code in order
to warn someone thinking of optimizing it so that they are aware that
there is a caller that provides partially initialized and incomplete SKBs.

Thanks,

Alex




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html