netdev - Re: [Xen-devel] [PATCH] xen-netfront: pull on receive skb may need to happen earlier

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Wed, 10 Jul 2013 07:58:06 +0100
From:	"Jan Beulich" <JBeulich@...e.com>
To:	"Wei Liu" <wei.liu2@...rix.com>
Cc:	"Ian Campbell" <ian.campbell@...rix.com>, <davem@...emloft.net>,
	"Dion Kant" <g.w.kant@...enet.nl>, <xen-devel@...ts.xen.org>,
	<netdev@...r.kernel.org>, <stable@...r.kernel.org>
Subject: Re: [Xen-devel] [PATCH] xen-netfront: pull on receive skb may
 need to happen earlier

>>> On 09.07.13 at 18:51, Wei Liu <wei.liu2@...rix.com> wrote:
> On Tue, Jul 09, 2013 at 07:52:31AM +0100, Jan Beulich wrote:
>> >>> On 08.07.13 at 17:48, Wei Liu <wei.liu2@...rix.com> wrote:
>> > On Mon, Jul 08, 2013 at 03:20:26PM +0100, Jan Beulich wrote:
>> >> @@ -1014,7 +1025,7 @@ err:
>> >>  
>> >>  		skb_shinfo(skb)->frags[0].page_offset = rx->offset;
>> >>  		skb_frag_size_set(&skb_shinfo(skb)->frags[0], rx->status);
>> >> -		skb->data_len = rx->status;
>> >> +		skb->len = skb->data_len = rx->status;
>> > 
>> > This is not correct. You should not be needing this. Now you lose count
>> > of SKB head len. Try to go without the above line and see if it makes a
>> > difference?
>> 
>> I don't follow - at this point, there's 0 bytes of head (this only
>> changes with the first call to __pskb_pull_tail()). Hence ->len ==
>> ->data_len seems correct to me (and afaict pulling would do the
>> wrong thing if I dropped that change).
>> 
> 
> My bad, I suggested the wrong thing. :-(
> 
> But I would prefer skb->len += skb->data_len. In the case that skb->len
> == 0 it's the same as your line while skb->len is not zero it would also
> do the right thing.

I can do that, albeit I don't see how ->len could end up non-zero
here.

> As for the warning in skb_try_coalesce, I don't see any direct call to
> it in netfront, I will need to think about it. It looks like it's really
> something very deep in the stack.

Yes, as the call stack provided by Dion proves. The question
really is whether the patch somehow results in ->truesize to be
incorrect, or whether - as Eric points out - this is "normal" for
the sort of special SKBs here (having a rather small headlen). If
what he says is applicable here, it may hint at the pulling we do
still not being sufficient for the full TCP header to be in the linear
part (which iirc is the main [if not the only purpose] of us doing
the pull in the first place).

Jan

--
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