Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1640821 > unrolled thread

Re: [git pull] uaccess-related bits of vfs.git

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [git pull] uaccess-related bits of vfs.git Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-13 03:10 +0200
    Re: [git pull] uaccess-related bits of vfs.git Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-13 09:00 +0200
      Re: [git pull] uaccess-related bits of vfs.git Adam Borowski <kilobyte@angband.pl> - 2017-05-13 14:10 +0200
        Re: [git pull] uaccess-related bits of vfs.git Brian Gerst <brgerst@gmail.com> - 2017-05-13 15:50 +0200
        Re: [git pull] uaccess-related bits of vfs.git Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-13 18:50 +0200
      Re: [git pull] uaccess-related bits of vfs.git Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-13 18:20 +0200
      Re: [git pull] uaccess-related bits of vfs.git Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-13 18:20 +0200
        Re: [git pull] uaccess-related bits of vfs.git Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-13 19:10 +0200
          Re: [git pull] uaccess-related bits of vfs.git Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-13 19:20 +0200
            Re: [git pull] uaccess-related bits of vfs.git Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-13 20:10 +0200
              Re: [git pull] uaccess-related bits of vfs.git Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-13 20:30 +0200
                Re: [git pull] uaccess-related bits of vfs.git Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-13 21:20 +0200
                  Re: [git pull] uaccess-related bits of vfs.git Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-13 21:40 +0200
              Re: [git pull] uaccess-related bits of vfs.git Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-13 21:10 +0200
                Re: [git pull] uaccess-related bits of vfs.git Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-13 21:20 +0200
                Re: [git pull] uaccess-related bits of vfs.git Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-13 22:00 +0200
                  Re: [git pull] uaccess-related bits of vfs.git Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-13 22:10 +0200
                    Re: [git pull] uaccess-related bits of vfs.git Geert Uytterhoeven <geert@linux-m68k.org> - 2017-05-13 22:40 +0200
                      Re: [git pull] uaccess-related bits of vfs.git Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-13 22:50 +0200
                Re: [git pull] uaccess-related bits of vfs.git Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-13 22:40 +0200
                  Re: [git pull] uaccess-related bits of vfs.git Linus Torvalds <torvalds@linux-foundation.org> - 2017-05-13 23:00 +0200
                    Re: [git pull] uaccess-related bits of vfs.git Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-13 23:30 +0200
          Re: [git pull] uaccess-related bits of vfs.git Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-13 19:20 +0200
        Re: [git pull] uaccess-related bits of vfs.git Ingo Molnar <mingo@kernel.org> - 2017-05-14 20:20 +0200
          Re: [git pull] uaccess-related bits of vfs.git Al Viro <viro@ZenIV.linux.org.uk> - 2017-05-14 21:00 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1641009

On Sat, May 13, 2017 at 1:37 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> That's a valid point and it might apply to memdup_user() callers out there.
> Potential variants:
>         * add an explicit upper bound on the size and turn that into
> memdup_user() (and check that all memdup_user() callers are bounded).
>         * have memdup_user() itself pass __GFP_NOWARN.
>         * add kvmemdup_user() that would use kvmalloc() (with its callers
> expected to use kvfree()); see who else might benefit from conversion.

All of the above sound reasonable.

I wouldn't change the existing "memdup_user()" interface itself, but
if there really are users that can validly pass in a maxbyte value,
why not add a new helper:

  void *memdup_user_limit(userptr, nmember, nsize, maxsize);

and then have

  #define memdup_user(ptr,size) memdup_user_limit(ptr, size, 1, -1)

or something. I definitely see a couple of memdup_user() people who do
that "num*size" multiplication by hand, and it's very easy to get
wrong and have an overflow.

And for a kvmalloc/kvfree() interface, you *definitely* want that
maxsize thing, since it absolutely needs an upper limit.

             Linus

[toc] | [prev] | [next] | [standalone]

#1641013

On Sat, May 13, 2017 at 01:52:29PM -0700, Linus Torvalds wrote:
> I wouldn't change the existing "memdup_user()" interface itself, but
> if there really are users that can validly pass in a maxbyte value,
> why not add a new helper:
> 
>   void *memdup_user_limit(userptr, nmember, nsize, maxsize);
> 
> and then have
> 
>   #define memdup_user(ptr,size) memdup_user_limit(ptr, size, 1, -1)
> 
> or something. I definitely see a couple of memdup_user() people who do
> that "num*size" multiplication by hand, and it's very easy to get
> wrong and have an overflow.
> 
> And for a kvmalloc/kvfree() interface, you *definitely* want that
> maxsize thing, since it absolutely needs an upper limit.

*nod*

Speaking of insanities around open-coded memdup_user()...  Enjoy:

                ias_opt = kmalloc(sizeof(struct irda_ias_set), GFP_ATOMIC);
                if (ias_opt == NULL) {
                        err = -ENOMEM;
                        goto out;
                }

                /* Copy query to the driver. */
                if (copy_from_user(ias_opt, optval, optlen)) {

Can't have it block, sir, has to be GFP_ATOMIC...  Whaddya mean, "what
if copy_from_user() blocks?"  As far as I can see, that came in circa
2.4.6, when local sturct irda_ias_set got switched to dynamic allocation...

[toc] | [prev] | [next] | [standalone]

#1640965

On Sat, May 13, 2017 at 06:00:56PM +0100, Al Viro wrote:

> > But I don't see the excuse for not just doing it. If nobody notices,
> > it's an obvious improvement. And if somebody *does* notice, we know
> > how to do it properly with unsafe_xyz_user(), because "__xyz_user()"
> > most definitely isn't it.
> 
> I think we ought to actually look through those places - there are few
> enough of them (outside of arch/, that is) and stac/clac overhead is
> not the only problem they tend to have.

PS: just to make it clear - I do _not_ propose to keep that shit around
indefinitely; I want __get_user()/__put_user() gone by the end of that
work.

[toc] | [prev] | [next] | [standalone]

#1641174

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, May 12, 2017 at 11:57 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > First, some stats: there's a thousand-odd callers of __get_user().  Out of
> > those, about 70% are in arch/, mostly in sigframe-related code.
> 
> Sure. And they can be trivially converted, and none of them should care at all.
> 
> > IOW, we have
> >         * most of users in arch/* (heavily dominated by signal-related code,
> > both loads and stores).  Those need careful massage; maybe unsafe-based
> > solution, maybe something else, but it's obviously per-architecture work
> > and these paths are sensitive.
> 
> Why are they sensitive?
> 
> Why not just do this:
> 
>   git grep -l '\<__\(\(get\)\|\(put\)\)_user(' -- arch/x86
> :^arch/x86/include/asm/uaccess.h
>         | xargs sed -i 's/__\(\(\(get\)\|\(put\)\)_user(\)/\1/g'
> 
> which converts all the x86 uses in one go.
> 
> Anybody who *relies* on not checking the address_limit is so broken as
> to be not even funny. And anything that is so performance-sensitive
> that anybody can even measure the effect of the above we can convert
> later.

I'd say that the CLAC/STAC addition pretty much killed any argument in favor of 
"optimized" __get_user() code, so I'd be very happy to see these interfaces gone 
altogether.

So as far as x86 usage goes:

  Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1641197

On Sun, May 14, 2017 at 08:13:56PM +0200, Ingo Molnar wrote:

> I'd say that the CLAC/STAC addition pretty much killed any argument in favor of 
> "optimized" __get_user() code, so I'd be very happy to see these interfaces gone 
> altogether.

You and everybody else - these interfaces suck.  If anything, we want paired
brackets around a series of accesses instead of a single check in front of it.

> So as far as x86 usage goes:
> 
>   Acked-by: Ingo Molnar <mingo@kernel.org>

Umm...  Could you elaborate the situation with xen/page.h stuff?  I don't
see any obvious reasons that would guaratee that addresses passed to
__get_user() and __put_user() there would match the set_fs() state.

It might very well be true, but it's not obvious from that code...

BTW, does anybody have a suggestion regarding a test load that would hit
wait4/waitid as hard as possible?  I've turned sys_wait4/sys_waitid into
long kernel_wait4(pid_t upid, int *stat_addr, int options, struct rusage *ru)
and
static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
                int options, struct rusage *ru)
(with struct waitid_info {
        pid_t pid;
        uid_t uid;
        int status;   
        int why;
};), so that all copying to userland is done in sys_wait4() and friends.
It seems to survive testing without any noticable slowdowns, but that's
just LTP and xfstests - and a bug in my earlier version of that was _not_
caught by the LTP side; xfstests caught it...  So any extra tests (both
for correctness and timing) would be very much appreciated...

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web