Re: [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors
From: Martyn Welch
Date: Mon Jul 06 2015 - 09:52:24 EST
On 06/07/15 14:10, Dmitry Kalinkin wrote:
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:
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@xxxxxx | 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@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/