Re: [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors

[Dmitry Kalinkin]

On Mon, Jul 6, 2015 at 3:51 PM, Martyn Welch <martyn.welch@xxxxxx> wrote:
> On 26/06/15 21:39, Dmitry Kalinkin wrote:
>>
>> Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@xxxxxxxxx>
>> ---
>>   drivers/staging/vme/devices/vme_user.c | 47
>> ++++++++--------------------------
>>   1 file changed, 11 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/staging/vme/devices/vme_user.c
>> b/drivers/staging/vme/devices/vme_user.c
>> index a2345db..ef876a4 100644
>> --- a/drivers/staging/vme/devices/vme_user.c
>> +++ b/drivers/staging/vme/devices/vme_user.c
>> @@ -123,7 +123,6 @@ struct vme_user_vma_priv {
>>   static ssize_t resource_to_user(int minor, char __user *buf, size_t
>> count,
>>                                 loff_t *ppos)
>>   {
>> -       ssize_t retval;
>>         ssize_t copied = 0;
>>
>>         if (count > image[minor].size_buf)
>> @@ -135,13 +134,8 @@ static ssize_t resource_to_user(int minor, char
>> __user *buf, size_t count,
>>         if (copied < 0)
>>                 return (int)copied;
>>
>> -       retval = __copy_to_user(buf, image[minor].kern_buf,
>> -                               (unsigned long)copied);
>> -       if (retval != 0) {
>> -               copied = (copied - retval);
>> -               pr_info("User copy failed\n");
>> -               return -EINVAL;
>> -       }
>> +       if (__copy_to_user(buf, image[minor].kern_buf, (unsigned
>> long)copied))
>> +               return -EFAULT;
>>
>>         return copied;
>
>
> Does __copy_to_user() not return the number of bytes still to be copied? The
> above seems to add the assumption that __copy_to_user()
> can't return part way through a copy.
Yes it does. And this information is useless in userspace.
Returning -EFAULT would be more informative in this case.
I couldn't find an example of kernel code doing this any other way:
http://lxr.free-electrons.com/ident?i=__copy_to_user
>
>>   }
>> @@ -149,21 +143,16 @@ static ssize_t resource_to_user(int minor, char
>> __user *buf, size_t count,
>>   static ssize_t resource_from_user(unsigned int minor, const char __user
>> *buf,
>>                                   size_t count, loff_t *ppos)
>>   {
>> -       ssize_t retval;
>>         ssize_t copied = 0;
>>
>>         if (count > image[minor].size_buf)
>>                 count = image[minor].size_buf;
>>
>> -       retval = __copy_from_user(image[minor].kern_buf, buf,
>> -                                 (unsigned long)count);
>> -       if (retval != 0)
>> -               copied = (copied - retval);
>> -       else
>> -               copied = count;
>> +       if (__copy_from_user(image[minor].kern_buf, buf, (unsigned
>> long)count))
>> +               return -EFAULT;
>>
>
> Same here.
>
>>         copied = vme_master_write(image[minor].resource,
>> image[minor].kern_buf,
>> -                                 copied, *ppos);
>> +                                 count, *ppos);
>>
>>         return copied;
>>   }
>> @@ -172,38 +161,24 @@ static ssize_t buffer_to_user(unsigned int minor,
>> char __user *buf,
>>                               size_t count, loff_t *ppos)
>>   {
>>         void *image_ptr;
>> -       ssize_t retval;
>>
>>         image_ptr = image[minor].kern_buf + *ppos;
>> +       if (__copy_to_user(buf, image_ptr, (unsigned long)count))
>> +               return -EFAULT;
>>
>
> Ditto.
>
>> -       retval = __copy_to_user(buf, image_ptr, (unsigned long)count);
>> -       if (retval != 0) {
>> -               retval = (count - retval);
>> -               pr_warn("Partial copy to userspace\n");
>> -       } else
>> -               retval = count;
>> -
>> -       /* Return number of bytes successfully read */
>> -       return retval;
>> +       return count;
>>   }
>>
>>   static ssize_t buffer_from_user(unsigned int minor, const char __user
>> *buf,
>>                                 size_t count, loff_t *ppos)
>>   {
>>         void *image_ptr;
>> -       size_t retval;
>>
>>         image_ptr = image[minor].kern_buf + *ppos;
>> +       if (__copy_from_user(image_ptr, buf, (unsigned long)count))
>> +               return -EFAULT;
>>
>
> And here.
>
>> -       retval = __copy_from_user(image_ptr, buf, (unsigned long)count);
>> -       if (retval != 0) {
>> -               retval = (count - retval);
>> -               pr_warn("Partial copy to userspace\n");
>> -       } else
>> -               retval = count;
>> -
>> -       /* Return number of bytes successfully read */
>> -       return retval;
>> +       return count;
>>   }
>>
>>   static ssize_t vme_user_read(struct file *file, char __user *buf, size_t
>> count,
>>
>
> --
> Martyn Welch (Lead Software Engineer)  | Registered in England and Wales
> GE Intelligent Platforms               | (3828642) at 100 Barbirolli Square
> T +44(0)1327322748                     | Manchester, M2 3AB
> E martyn.welch@xxxxxx                  | VAT:GB 927559189
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/