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
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
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.