linux-kernel - Re: [PATCH] Convert filldir[64]() from __put_user() to unsafe_put

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: <20191007182230.GN26530@ZenIV.linux.org.uk>
Date:   Mon, 7 Oct 2019 19:22:30 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Guenter Roeck <linux@...ck-us.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] Convert filldir[64]() from __put_user() to
 unsafe_put_user()

On Mon, Oct 07, 2019 at 11:13:27AM -0700, Linus Torvalds wrote:
> On Mon, Oct 7, 2019 at 10:34 AM Al Viro <viro@...iv.linux.org.uk> wrote:
> >
> > Tangentially related: copy_regster_to_user() and copy_regset_from_user().
> 
> Not a worry. It's not performance-critical code, and if it ever is, it
> needs to be rewritten anyway.
> 
> > The former variant tends to lead to few calls
> > of __copy_{to,from}_user(); the latter...  On x86 it ends up doing
> > this:
> 
> Just replace the __put_user() with a put_user() and be done with it.
> That code isn't acceptable, and if somebody ever complains about
> performance it's not the lack of __put_user that is the problem.

I wonder if it would be better off switching to several "copy in bulk"
like e.g. ppc does.  That boilerplate with parallel "to/from kernel"
and "to/from userland" loops is asking for bugs - the calling
conventions like "pass kbuf and ubuf; exactly one must be NULL"
tend to be trouble, IME; I'm not saying we should just pass
struct iov_iter * instead of count+pos+kbuf+ubuf to ->get() and
->set(), but it might clean the things up nicely.

Let me look into that zoo a bit more...