Clarification needed on use of put_user inside a loop

Clarification needed on use of put_user inside a loop

[Kumar Gaurav]

Hi All,

function put_user() is used to transfer small bytes of data (1-8 byte) 
from kernel space to user space and before transferring, it checks for 
the user's access over that memory area (in user space of-course) using 
function access_ok(). function __put_user() is used for same purpose but 
it skips checking permission part.

Hence when transferring data involves loops then checking permission 
(using access_ok()) once should be good to go then after we can simply 
transfer data using __put_user(), instead of using put_user() itself in 
loop.

I have  found some codes in the driver which use put_user() in loop. Can 
we avoid the overhead of checking the same memory area( where put_user() 
writes) again n again using __put_user() in side loop and checking 
permission using access_ok before entering the loop?

Below is one of the codes I found.
File Name:sound/pci/hda/patch_hdmi.c

Code
-----------
for (i = 0; i < ARRAY_SIZE(channel_allocations); i++, cap++) { //line 
number 1928
                         int chs_bytes = chs * 4;
                         int type = 
spec->ops.chmap_cea_alloc_validate_get_type(cap, chs);
                         unsigned int tlv_chmap[8];

                         if (type < 0)
                                 continue;
                         if (size < 8)
                                 return -ENOMEM;
                         if (put_user(type, dst) ||
                             put_user(chs_bytes, dst + 1))
                                 return -EFAULT;
                         dst += 2;
                         size -= 8;
                         count += 8;
                         if (size < chs_bytes)
                                 return -ENOMEM;
                         size -= chs_bytes;
                         count += chs_bytes;
                         spec->ops.cea_alloc_to_tlv_chmap(cap, 
tlv_chmap, chs);
                         if (copy_to_user(dst, tlv_chmap, chs_bytes))
                                 return -EFAULT;
                         dst += chs;
                 }
---------------------------
Please revert with comment on whether I am correct or not. If yes, I'll 
submit the patches for upgrading codes to skip the overhead of checking 
memory area for permission.

Regards,
Kumar Gaurav

Re: Clarification needed on use of put_user inside a loop

[Mateusz Guzik]

On Fri, Apr 25, 2014 at 09:39:57PM +0530, Kumar Gaurav wrote:
> Hence when transferring data involves loops then checking permission
> (using access_ok()) once should be good to go then after we can
> simply transfer data using __put_user(), instead of using put_user()
> itself in loop.
> 

Well, I can't tell you whether this is a good idea, but:

This looks correct and other code is doing this already.

However, put_user calls might_fault, but __put_user consumers I found
(e.g. copy_siginfo_to_user) don't do that.

While it has only debugging purposes and would not change anything for
those consumers, it seems to be a bug to not include it.

Thus I suggest adding access_ok variant which calls might_fault.

-- 
Mateusz Guzik

Re: Clarification needed on use of put_user inside a loop

[Al Viro]

On Fri, Apr 25, 2014 at 09:39:57PM +0530, Kumar Gaurav wrote:

> I have  found some codes in the driver which use put_user() in loop.
> Can we avoid the overhead of checking the same memory area( where
> put_user() writes) again n again using __put_user() in side loop and
> checking permission using access_ok before entering the loop?

>                         if (put_user(type, dst) ||
>                             put_user(chs_bytes, dst + 1))
>                                 return -EFAULT;
>                         dst += 2;
			  ^^^^^^^^^
Note that increment.  It's *not* "the same memory area" next time
around.  Sure, you can check the whole range once before the loop
and switch the stuff inside to __put_user()/__copy_to_user(), but
it's not guaranteed to buy you any speedup.

BTW, you might be a bit confused about the work done by access_ok() - e.g.
on an architectures with separate kernel and userland MMU contexts it might
very well be a no-op (always return true).  It's *not* checking if user has
permissions of some sort.