netdev - Re: [PATCH][RFC] network splice receive

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]

Message-ID: <20070612131153.GC16477@2ka.mipt.ru>
Date:	Tue, 12 Jun 2007 17:11:53 +0400
From:	Evgeniy Polyakov <johnpol@....mipt.ru>
To:	Jens Axboe <jens.axboe@...cle.com>
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH][RFC] network splice receive

On Tue, Jun 12, 2007 at 02:40:05PM +0200, Jens Axboe (jens.axboe@...cle.com) wrote:
> On Tue, Jun 12 2007, Evgeniy Polyakov wrote:
> > > > > > and putting cloned skb into private field instead of 
> > > > > > original on in spd_fill_page() ends up without kernel hung.
> > > > > 
> > > > > Why? Seems pointless to allocate a clone just to hold on to the skb, a
> > > > > reference should be equally good. I would not be opposed to doing it
> > > > > this way, I just don't see what a clone buys us as compared to just
> > > > > holding that reference to the skb.
> > > > 
> > > > Receiving code does not expect shared skbs - too many fields are changed
> > > > with assumptions that it is a private copy.
> > > 
> > > Actually the main problem is that tcp_read_sock() unconditionally frees
> > > the skb, so it wouldn't help if we grabbed a reference to it. I've yet
> > > to receive an explanation of why it does so, seem awkward and violates
> > > the whole principle of reference counted objects. Davem??
> > > 
> > > So for now, skb_splice_bits() clones the incoming skb to avoid that. I'd
> > > hope we can get rid of that by fixing tcp_read_sock(), though.
> > 
> > It does that because it knows, that skb is not allowed to be shared
> > there. Similar things are being done in udp for example - code changes
> > internal mebers of skb, since it knows skb is not shared.
> > 
> > For example generic_make_request() is not allowed to change, say, 
> > bio->bi_sector or bi_destructor, since it does not own a block request, 
> > not matter what bi_cnt is. From another side, ->bi_destructor() can do
> > whatever it wants with bio without any check for its reference counter.
> 
> But generic_make_request() DOES change ->bi_sector, that's how partition
> remapping works :-). The destructor can of course do whatever it wants,
> by definition the bio is not referenced at that point (or it would not
> have been called). So while I think your analogy is quite poor, I do now
> follow the code (even if I think it's ugly). There's quite a big

Yeah, that was quite long time ago I hacked block layer :)
Good we found a way to explain the issue.

> difference between changing parts of the elements of a structure to just
> grabbing a reference to it. If the skb cannot be referenced, skb_get()
> should return NULL.
> 
> But that aside, I see the issue. I'll just stick to the clone, it works
> fine as-is (well almost, there's a leak there, but functionally it's
> ok!).

Btw, is it allowed to use splice from network with, say, nfs?
Since RPC code uses sk_user_data as long as network splice.

> -- 
> Jens Axboe

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