Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754797AbbGFNK6 (ORCPT ); Mon, 6 Jul 2015 09:10:58 -0400 Received: from mail-wg0-f50.google.com ([74.125.82.50]:33466 "EHLO mail-wg0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753930AbbGFNKy (ORCPT ); Mon, 6 Jul 2015 09:10:54 -0400 MIME-Version: 1.0 In-Reply-To: <559A79C6.4020601@ge.com> References: <1435075419-28211-1-git-send-email-dmitry.kalinkin@gmail.com> <1435351184-19158-1-git-send-email-dmitry.kalinkin@gmail.com> <1435351184-19158-7-git-send-email-dmitry.kalinkin@gmail.com> <559A79C6.4020601@ge.com> Date: Mon, 6 Jul 2015 16:10:53 +0300 Message-ID: Subject: Re: [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors From: Dmitry Kalinkin To: Martyn Welch Cc: linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org, Manohar Vanga , Greg Kroah-Hartman Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5122 Lines: 149 On Mon, Jul 6, 2015 at 3:51 PM, Martyn Welch wrote: > On 26/06/15 21:39, Dmitry Kalinkin wrote: >> >> Signed-off-by: Dmitry Kalinkin >> --- >> 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@ge.com | VAT:GB 927559189 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/