Re: [PATCH 01/11] x86: Use asm goto to implement better modify_and_test() functions

[Ingo Molnar <mingo@kernel.org>]

* Andi Kleen <ak@linux.intel.com> wrote:

> On Wed, Sep 18, 2013 at 02:02:37PM -0500, H. Peter Anvin wrote:
>
> > Yes, a bit sad.  We allow bracketing with the get/put_user_try/catch 
> > blocks, but that is x86-specific. I don't think a generic option is 
> > possible without compiler support, but it might be possible to do 
> > better than we do know.
> 
> Letting the compiler do it is a bit risky, because it may open it up for 
> really large blocks, thus defeating the security advantages.

Yeah, the compiler could cover other pointer dereferences in the put_user 
block and that won't result in any visible breakage, so it's difficult to 
prevent the compiler doing it accidentally or even intentionally.

Then again the many repeated STAC/CLAC sequences are really not nice.

So maybe we could add some macro magic to generate better assembly here - 
if we coded up a __put_user_2field() primitive then we could already 
optimize the filldir() case:

before:

        if (__put_user(d_ino, &dirent->d_ino))
                goto efault;
        if (__put_user(reclen, &dirent->d_reclen))
                goto efault;
        if (copy_to_user(dirent->d_name, name, namlen))
                goto efault;
        if (__put_user(0, dirent->d_name + namlen))
                goto efault;
        if (__put_user(d_type, (char __user *) dirent + reclen - 1))
                goto efault;

after:

        if (__put_user_2field(d_ino, &dirent->d_ino, reclen, &dirent->d_reclen))
                goto efault;
        if (copy_to_user(dirent->d_name, name, namlen))
                goto efault;
        if (__put_user_2field(0, dirent->d_name + namlen, d_type, (char __user *) dirent + reclen - 1)))
                goto efault;

That cuts down the inlined STAC/CLAC pairs from 4 to 2.

__put_user_2field() would be some truly disgusting (but hidden from most 
people) macro and assembly magic.

We could also add __put_user_4field() and slightly reorder filldir():

        if (__put_user_4field(	d_ino,		&dirent->d_ino,
				reclen,		&dirent->d_reclen,
        			0,		dirent->d_name + namlen,
				d_type,		(char __user *) dirent + reclen - 1)))
                goto efault;

        if (copy_to_user(dirent->d_name, name, namlen))
                goto efault;

That would reduce the inlined STAC/CLAC pairs to a minimal 1 (only one of 
which would be visible in the filldir() disassembly).

In theory we could do something generic:

        if (__put_user_fields(	4,
				d_ino,		&dirent->d_ino,
				reclen,		&dirent->d_reclen,
        			0,		 dirent->d_name + namlen,
				d_type,		(char __user *)dirent + reclen-1 ))
                goto efault;

        if (copy_to_user(dirent->d_name, name, namlen))
                goto efault;

and implement it up to 4 or so. It will be some truly disgusting lowlevel 
code (especially due to the size variations which could make it explode 
combinatorically), with some generic header fallback that utilizes 
existing put_user primitives.

But it's solvable IMO, if we want to solve it. On the high level it's also 
more readable in a fashion and hence perhaps a bit less fragile than our 
usual __put_user() patterns.

Btw., while at it we could also maybe fix the assignment ordering and use 
copy_to_user() naming:

        if (__copy_to_user_fields(4,

			&dirent->d_ino,				d_ino,
			&dirent->d_reclen,			reclen,		
        		dirent->d_name + namlen,		0,
			(char __user *)dirent + reclen-1,	d_type	))

                goto efault;

        if (copy_to_user(dirent->d_name, name, namlen))
                goto efault;

That would make it even more readable.

(Thinking about the macro tricks needed for something like this gave me a 
bad headache though.)

Thanks,

	Ingo