netdev - Re: [PATCH][CFT] Saner error handling in skb_copy_datagram

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: <20170220.101404.1931404813887976325.davem@davemloft.net>
Date:   Mon, 20 Feb 2017 10:14:04 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     viro@...IV.linux.org.uk
Cc:     netdev@...r.kernel.org, eric.dumazet@...il.com, rlwinm@....org,
        alexmcwhirter@...adic.us, chunkeey@...glemail.com
Subject: Re: [PATCH][CFT] Saner error handling in skb_copy_datagram_iter()
 et.al.

From: Al Viro <viro@...IV.linux.org.uk>
Date: Sat, 18 Feb 2017 00:02:14 +0000

> On Fri, Feb 17, 2017 at 05:03:15PM +0000, Al Viro wrote:
>> On Fri, Feb 17, 2017 at 10:54:20AM -0500, David Miller wrote:
>> > From: Al Viro <viro@...IV.linux.org.uk>
>> > Date: Tue, 14 Feb 2017 01:33:06 +0000
>> > 
>> > > OK...  Remaining interesting question is whether it adds a noticable
>> > > overhead.  Could somebody try it on assorted benchmarks and see if
>> > > it slows the things down?  The patch in question follows:
>> > 
>> > That's about a 40 byte copy onto the stack for each invocation of this
>> > thing.  You can benchmark all you want, but it's clear that this is
>> > non-trivial amount of work and will take some operations from fitting
>> > in the cache to not doing so for sure.
>> 
>> In principle, that could be reduced a bit (32 bytes - ->type is never
>> changed, so we don't need to restore it), but that's not much of improvement...
> 
> Actually, I've a better solution.  Namely, analogue of iov_iter_advance()
> for going backwards.  The restriction is that you should never unroll
> further than where you've initially started *or* have the iovec, etc.
> array modified under you.  For iovec/kvec/bio_vec it's trivial, for pipe -
> a bit more convoluted, but still doable.  Then net/core/datagram.c stuff
> could simply use iov_iter_unroll() in case of error - all we need to keep
> track of is how much had we copied and that's easy to do.
> 
> The patch below is completely untested, but if it works it should avoid
> buggering the fast paths at all, still giving the same semantics re
> reverting ->msg_iter both on EINVAL and EFAULT.  Comments?

This looks a lot better to me.