Re: [PATCH RFC v2] m68k: remove get_fs()/set_fs()

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Linus,

On 9/07/21 6:20 am, Linus Torvalds wrote:
> On Thu, Jul 8, 2021 at 5:57 AM Christoph Hellwig <hch@lst.de> wrote:
>> I've force pushed a new version to the branch, can you give it a spin?
> Please stop playing broken games with __constant_copy_to_user().
>
> Now you didn't just break the return value, you broke the actual copy
> too. When it is supposed to do a 4-byte copy, the code now does *two*
> 4-byte copies, because that's the way __constant_copy_to_user_asm()
> works - it always does at least two accesses, and then the third one
> is conditional.
>
> So that "6, l, l, )" in
>
>          case 4:
>                  __constant_copy_to_user_asm(res, to, from, tmp, 6, l, l,);
>                  break;
>
> literally means "try to do 2x 'l' sized moves (but not a third one),
> and return 6 if it fails". All of which is very wrong indeed.

In order to get the correct number of bytes copied, the patch would have 
to look like:

        switch (n) {
         case 1:
-               __put_user_asm(res, *(u8 *)from, (u8 __user *)to, b, d, 1);
+               __constant_copy_to_user_asm(res, to, from, tmp, 1, b, );
                 break;
         case 2:
-               __put_user_asm(res, *(u16 *)from, (u16 __user *)to, w, 
r, 2);
+               __constant_copy_to_user_asm(res, to, from, tmp, 2, w, );
                 break;
         case 3:
                 __constant_copy_to_user_asm(res, to, from, tmp, 3, w, b,);
                 break;
         case 4:
-               __put_user_asm(res, *(u32 *)from, (u32 __user *)to, l, 
r, 4);
+               __constant_copy_to_user_asm(res, to, from, tmp, 4, l, );
                 break;

and __constant_copy_to_user_asm() changed to deal with an empty s2 
parameter. Probably too much work.


>
> So commit d36105c942e0 ("m68k: simplify the __constant_copy_to_user
> implementation") is very very broken.

Doesn't appear to matter - though I'll back out those changes to be safe.

Note that in this version, faults in __put_user_asm used from 
__constant_copy_to_user will return -EFAULT, not the remaining number of 
bytes to be copied, which might get confusing for the caller.

Cheers,

     Michael

>
> But the rest looks good to me. Of course, I entirely missed the fact
> that Andreas pointed out - "instr" was inside a string - so who knows
> what I missed this time.
>
>                 Linus