Messages in this thread | | | Date | Mon, 6 Jul 2015 14:51:59 +0100 | From | Martyn Welch <> | Subject | Re: [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors |
| |
On 06/07/15 14:10, Dmitry Kalinkin wrote: > On Mon, Jul 6, 2015 at 3:51 PM, Martyn Welch <martyn.welch@ge.com> wrote: >> On 26/06/15 21:39, Dmitry Kalinkin wrote: >>> >>> Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com> >>> --- >>> 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: > https://urldefense.proofpoint.com/v2/url?u=http-3A__lxr.free-2Delectrons.com_ident-3Fi-3D-5F-5Fcopy-5Fto-5Fuser&d=AwIBaQ&c=IV_clAzoPDE253xZdHuilRgztyh_RiV3wUrLrDQYWSI&r=o_jX-O9t9rDa5QmNKEzuslLxr8l2x1M2rHid0HdzmY4&m=YqOunx1QvC1jz8WjtHd_feWk5cdNWIVW0utSWesR8ag&s=BdRq0di7Bryi1LqA3LtL_Z9UeSHdpiYwghMElfWbR1A&e=
Fair enough. If that's the approach taken elsewhere, then I'm happy for this to become consistent.
>> >>> } >>> @@ -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@ge.com | VAT:GB 927559189
-- 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@ge.com | VAT:GB 927559189
| |