netdev - Re: [PATCH 3/4 v2 net-next] net: make GRO aware of skb->head

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 01 May 2012 09:17:35 -0700
From:	Alexander Duyck <alexander.h.duyck@...el.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	Alexander Duyck <alexander.duyck@...il.com>,
	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	Neal Cardwell <ncardwell@...gle.com>,
	Tom Herbert <therbert@...gle.com>,
	Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
	Michael Chan <mchan@...adcom.com>,
	Matt Carlson <mcarlson@...adcom.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Ben Hutchings <bhutchings@...arflare.com>,
	Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>,
	Maciej Żenczykowski <maze@...gle.com>
Subject: Re: [PATCH 3/4 v2 net-next] net: make GRO aware of skb->head_frag

On 04/30/2012 11:39 PM, Eric Dumazet wrote:
> On Mon, 2012-04-30 at 22:33 -0700, Alexander Duyck wrote:
>
>> The question I had was more specific to GRO.  As long as we have
>> skb->users == 1 and the skb isn't cloned we should be fine.   It just
>> hadn't occurred to me before that napi_gro_receive had the extra
>> requirement that the skb couldn't be cloned.
>>
> OK
>
> By the way, even if skb was cloned, we would be allowed to steal
> skb->head.
>
> When we clone an oskb we :
>
> 1) allocate a struct nskb sk_buff (or use the shadow in case of TCP)
> 2) increment dataref
The problem I have is with this piece right here.  So you increment
dataref.  Now you have an skb that is still pointing to the shared info
on this page and dataref is 2.  What about the side that is stealing the
head?  Is it going to be tracking the dataref as well and decrementing
it before put_page or does it just assume that dataref is 1 and call
put_page directly?  I am guessing the latter since I didn't see anything
that allowed for tracking the dataref of stolen heads.

> 3) link skb->head
>
> After cloning()  nskb->users == 1, and oskb->users is unchanged
>
> I believe you are referring to skb_get(oskb), that ones increment
> oskb->users and skb_shared(oskb) then returns true.
skb_clone and skb_get are two completely separate things.  My concern
with the skb_get portion is if skb->users is greater than 1 we should
not be freeing the skbuff back to the head cache.  This was addressed
with the fact that GRO is requiring that skb->users be 1 before it is
handed the skb, although I didn't look too closely at the other patches
that are also stealing skb->head.  My concern with skb_clone, as I
mentioned above, is that I am not sure how the dataref tracking will
still work.

It looks like Dave applied the patches last night so I will pull the
latest net-next and do some poking around.  It is possible I am just
being dense and not getting this, but I just feel like there is
something that got missed or overlooked.

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