* [PATCH] mm/hmm/test: Fix some copy_to_user() error handling
@ 2020-05-11 18:37 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-05-11 18:37 UTC (permalink / raw)
To: Jérôme Glisse, Ralph Campbell
Cc: Jason Gunthorpe, linux-mm, kernel-janitors
The copy_to_user() function returns the number of bytes which weren't
copied but we want to return negative error codes. Also in dmirror_write()
if the copy_from_user() fails then there is some cleanup needed before
we can return so I fixed that as well.
Fixes: 5d5e54be8a1e3 ("mm/hmm/test: add selftest driver for HMM")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
lib/test_hmm.c | 41 +++++++++++++++++++++++++----------------
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/lib/test_hmm.c b/lib/test_hmm.c
index 00bca6116f930..fd4889f7b3d90 100644
--- a/lib/test_hmm.c
+++ b/lib/test_hmm.c
@@ -360,9 +360,11 @@ static int dmirror_read(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
cmd->faults++;
}
- if (ret = 0)
- ret = copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
- bounce.size);
+ if (ret = 0) {
+ if (copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
+ bounce.size))
+ ret = -EFAULT;
+ }
cmd->cpages = bounce.cpages;
dmirror_bounce_fini(&bounce);
return ret;
@@ -412,10 +414,11 @@ static int dmirror_write(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
ret = dmirror_bounce_init(&bounce, start, size);
if (ret)
return ret;
- ret = copy_from_user(bounce.ptr, u64_to_user_ptr(cmd->ptr),
- bounce.size);
- if (ret)
- return ret;
+ if (copy_from_user(bounce.ptr, u64_to_user_ptr(cmd->ptr),
+ bounce.size)) {
+ ret = -EFAULT;
+ goto fini;
+ }
while (1) {
mutex_lock(&dmirror->mutex);
@@ -431,6 +434,7 @@ static int dmirror_write(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
cmd->faults++;
}
+fini:
cmd->cpages = bounce.cpages;
dmirror_bounce_fini(&bounce);
return ret;
@@ -715,9 +719,11 @@ static int dmirror_migrate(struct dmirror *dmirror,
mutex_lock(&dmirror->mutex);
ret = dmirror_do_read(dmirror, start, end, &bounce);
mutex_unlock(&dmirror->mutex);
- if (ret = 0)
- ret = copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
- bounce.size);
+ if (ret = 0) {
+ if (copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
+ bounce.size))
+ ret = -EFAULT;
+ }
cmd->cpages = bounce.cpages;
dmirror_bounce_fini(&bounce);
return ret;
@@ -886,9 +892,10 @@ static int dmirror_snapshot(struct dmirror *dmirror,
break;
n = (range.end - range.start) >> PAGE_SHIFT;
- ret = copy_to_user(uptr, perm, n);
- if (ret)
+ if (copy_to_user(uptr, perm, n)) {
+ ret = -EFAULT;
break;
+ }
cmd->cpages += n;
uptr += n;
@@ -911,9 +918,8 @@ static long dmirror_fops_unlocked_ioctl(struct file *filp,
if (!dmirror)
return -EINVAL;
- ret = copy_from_user(&cmd, uarg, sizeof(cmd));
- if (ret)
- return ret;
+ if (copy_from_user(&cmd, uarg, sizeof(cmd)))
+ return -EFAULT;
if (cmd.addr & ~PAGE_MASK)
return -EINVAL;
@@ -946,7 +952,10 @@ static long dmirror_fops_unlocked_ioctl(struct file *filp,
if (ret)
return ret;
- return copy_to_user(uarg, &cmd, sizeof(cmd));
+ if (copy_to_user(uarg, &cmd, sizeof(cmd)))
+ return -EFAULT;
+
+ return 0;
}
static const struct file_operations dmirror_fops = {
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] mm/hmm/test: Fix some copy_to_user() error handling
@ 2020-05-11 18:37 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-05-11 18:37 UTC (permalink / raw)
To: Jérôme Glisse, Ralph Campbell
Cc: Jason Gunthorpe, linux-mm, kernel-janitors
The copy_to_user() function returns the number of bytes which weren't
copied but we want to return negative error codes. Also in dmirror_write()
if the copy_from_user() fails then there is some cleanup needed before
we can return so I fixed that as well.
Fixes: 5d5e54be8a1e3 ("mm/hmm/test: add selftest driver for HMM")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
lib/test_hmm.c | 41 +++++++++++++++++++++++++----------------
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/lib/test_hmm.c b/lib/test_hmm.c
index 00bca6116f930..fd4889f7b3d90 100644
--- a/lib/test_hmm.c
+++ b/lib/test_hmm.c
@@ -360,9 +360,11 @@ static int dmirror_read(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
cmd->faults++;
}
- if (ret == 0)
- ret = copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
- bounce.size);
+ if (ret == 0) {
+ if (copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
+ bounce.size))
+ ret = -EFAULT;
+ }
cmd->cpages = bounce.cpages;
dmirror_bounce_fini(&bounce);
return ret;
@@ -412,10 +414,11 @@ static int dmirror_write(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
ret = dmirror_bounce_init(&bounce, start, size);
if (ret)
return ret;
- ret = copy_from_user(bounce.ptr, u64_to_user_ptr(cmd->ptr),
- bounce.size);
- if (ret)
- return ret;
+ if (copy_from_user(bounce.ptr, u64_to_user_ptr(cmd->ptr),
+ bounce.size)) {
+ ret = -EFAULT;
+ goto fini;
+ }
while (1) {
mutex_lock(&dmirror->mutex);
@@ -431,6 +434,7 @@ static int dmirror_write(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
cmd->faults++;
}
+fini:
cmd->cpages = bounce.cpages;
dmirror_bounce_fini(&bounce);
return ret;
@@ -715,9 +719,11 @@ static int dmirror_migrate(struct dmirror *dmirror,
mutex_lock(&dmirror->mutex);
ret = dmirror_do_read(dmirror, start, end, &bounce);
mutex_unlock(&dmirror->mutex);
- if (ret == 0)
- ret = copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
- bounce.size);
+ if (ret == 0) {
+ if (copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
+ bounce.size))
+ ret = -EFAULT;
+ }
cmd->cpages = bounce.cpages;
dmirror_bounce_fini(&bounce);
return ret;
@@ -886,9 +892,10 @@ static int dmirror_snapshot(struct dmirror *dmirror,
break;
n = (range.end - range.start) >> PAGE_SHIFT;
- ret = copy_to_user(uptr, perm, n);
- if (ret)
+ if (copy_to_user(uptr, perm, n)) {
+ ret = -EFAULT;
break;
+ }
cmd->cpages += n;
uptr += n;
@@ -911,9 +918,8 @@ static long dmirror_fops_unlocked_ioctl(struct file *filp,
if (!dmirror)
return -EINVAL;
- ret = copy_from_user(&cmd, uarg, sizeof(cmd));
- if (ret)
- return ret;
+ if (copy_from_user(&cmd, uarg, sizeof(cmd)))
+ return -EFAULT;
if (cmd.addr & ~PAGE_MASK)
return -EINVAL;
@@ -946,7 +952,10 @@ static long dmirror_fops_unlocked_ioctl(struct file *filp,
if (ret)
return ret;
- return copy_to_user(uarg, &cmd, sizeof(cmd));
+ if (copy_to_user(uarg, &cmd, sizeof(cmd)))
+ return -EFAULT;
+
+ return 0;
}
static const struct file_operations dmirror_fops = {
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/hmm/test: Fix some copy_to_user() error handling
2020-05-11 18:37 ` Dan Carpenter
@ 2020-05-11 19:49 ` Ralph Campbell
-1 siblings, 0 replies; 6+ messages in thread
From: Ralph Campbell @ 2020-05-11 19:49 UTC (permalink / raw)
To: Dan Carpenter, Jérôme Glisse
Cc: Jason Gunthorpe, linux-mm, kernel-janitors
On 5/11/20 11:37 AM, Dan Carpenter wrote:
> The copy_to_user() function returns the number of bytes which weren't
> copied but we want to return negative error codes. Also in dmirror_write()
> if the copy_from_user() fails then there is some cleanup needed before
> we can return so I fixed that as well.
>
> Fixes: 5d5e54be8a1e3 ("mm/hmm/test: add selftest driver for HMM")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Thanks for fixing this.
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
> ---
> lib/test_hmm.c | 41 +++++++++++++++++++++++++----------------
> 1 file changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/lib/test_hmm.c b/lib/test_hmm.c
> index 00bca6116f930..fd4889f7b3d90 100644
> --- a/lib/test_hmm.c
> +++ b/lib/test_hmm.c
> @@ -360,9 +360,11 @@ static int dmirror_read(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
> cmd->faults++;
> }
>
> - if (ret = 0)
> - ret = copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
> - bounce.size);
> + if (ret = 0) {
> + if (copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
> + bounce.size))
> + ret = -EFAULT;
> + }
> cmd->cpages = bounce.cpages;
> dmirror_bounce_fini(&bounce);
> return ret;
> @@ -412,10 +414,11 @@ static int dmirror_write(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
> ret = dmirror_bounce_init(&bounce, start, size);
> if (ret)
> return ret;
> - ret = copy_from_user(bounce.ptr, u64_to_user_ptr(cmd->ptr),
> - bounce.size);
> - if (ret)
> - return ret;
> + if (copy_from_user(bounce.ptr, u64_to_user_ptr(cmd->ptr),
> + bounce.size)) {
> + ret = -EFAULT;
> + goto fini;
> + }
>
> while (1) {
> mutex_lock(&dmirror->mutex);
> @@ -431,6 +434,7 @@ static int dmirror_write(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
> cmd->faults++;
> }
>
> +fini:
> cmd->cpages = bounce.cpages;
> dmirror_bounce_fini(&bounce);
> return ret;
> @@ -715,9 +719,11 @@ static int dmirror_migrate(struct dmirror *dmirror,
> mutex_lock(&dmirror->mutex);
> ret = dmirror_do_read(dmirror, start, end, &bounce);
> mutex_unlock(&dmirror->mutex);
> - if (ret = 0)
> - ret = copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
> - bounce.size);
> + if (ret = 0) {
> + if (copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
> + bounce.size))
> + ret = -EFAULT;
> + }
> cmd->cpages = bounce.cpages;
> dmirror_bounce_fini(&bounce);
> return ret;
> @@ -886,9 +892,10 @@ static int dmirror_snapshot(struct dmirror *dmirror,
> break;
>
> n = (range.end - range.start) >> PAGE_SHIFT;
> - ret = copy_to_user(uptr, perm, n);
> - if (ret)
> + if (copy_to_user(uptr, perm, n)) {
> + ret = -EFAULT;
> break;
> + }
>
> cmd->cpages += n;
> uptr += n;
> @@ -911,9 +918,8 @@ static long dmirror_fops_unlocked_ioctl(struct file *filp,
> if (!dmirror)
> return -EINVAL;
>
> - ret = copy_from_user(&cmd, uarg, sizeof(cmd));
> - if (ret)
> - return ret;
> + if (copy_from_user(&cmd, uarg, sizeof(cmd)))
> + return -EFAULT;
>
> if (cmd.addr & ~PAGE_MASK)
> return -EINVAL;
> @@ -946,7 +952,10 @@ static long dmirror_fops_unlocked_ioctl(struct file *filp,
> if (ret)
> return ret;
>
> - return copy_to_user(uarg, &cmd, sizeof(cmd));
> + if (copy_to_user(uarg, &cmd, sizeof(cmd)))
> + return -EFAULT;
> +
> + return 0;
> }
>
> static const struct file_operations dmirror_fops = {
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/hmm/test: Fix some copy_to_user() error handling
@ 2020-05-11 19:49 ` Ralph Campbell
0 siblings, 0 replies; 6+ messages in thread
From: Ralph Campbell @ 2020-05-11 19:49 UTC (permalink / raw)
To: Dan Carpenter, Jérôme Glisse
Cc: Jason Gunthorpe, linux-mm, kernel-janitors
On 5/11/20 11:37 AM, Dan Carpenter wrote:
> The copy_to_user() function returns the number of bytes which weren't
> copied but we want to return negative error codes. Also in dmirror_write()
> if the copy_from_user() fails then there is some cleanup needed before
> we can return so I fixed that as well.
>
> Fixes: 5d5e54be8a1e3 ("mm/hmm/test: add selftest driver for HMM")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Thanks for fixing this.
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
> ---
> lib/test_hmm.c | 41 +++++++++++++++++++++++++----------------
> 1 file changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/lib/test_hmm.c b/lib/test_hmm.c
> index 00bca6116f930..fd4889f7b3d90 100644
> --- a/lib/test_hmm.c
> +++ b/lib/test_hmm.c
> @@ -360,9 +360,11 @@ static int dmirror_read(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
> cmd->faults++;
> }
>
> - if (ret == 0)
> - ret = copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
> - bounce.size);
> + if (ret == 0) {
> + if (copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
> + bounce.size))
> + ret = -EFAULT;
> + }
> cmd->cpages = bounce.cpages;
> dmirror_bounce_fini(&bounce);
> return ret;
> @@ -412,10 +414,11 @@ static int dmirror_write(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
> ret = dmirror_bounce_init(&bounce, start, size);
> if (ret)
> return ret;
> - ret = copy_from_user(bounce.ptr, u64_to_user_ptr(cmd->ptr),
> - bounce.size);
> - if (ret)
> - return ret;
> + if (copy_from_user(bounce.ptr, u64_to_user_ptr(cmd->ptr),
> + bounce.size)) {
> + ret = -EFAULT;
> + goto fini;
> + }
>
> while (1) {
> mutex_lock(&dmirror->mutex);
> @@ -431,6 +434,7 @@ static int dmirror_write(struct dmirror *dmirror, struct hmm_dmirror_cmd *cmd)
> cmd->faults++;
> }
>
> +fini:
> cmd->cpages = bounce.cpages;
> dmirror_bounce_fini(&bounce);
> return ret;
> @@ -715,9 +719,11 @@ static int dmirror_migrate(struct dmirror *dmirror,
> mutex_lock(&dmirror->mutex);
> ret = dmirror_do_read(dmirror, start, end, &bounce);
> mutex_unlock(&dmirror->mutex);
> - if (ret == 0)
> - ret = copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
> - bounce.size);
> + if (ret == 0) {
> + if (copy_to_user(u64_to_user_ptr(cmd->ptr), bounce.ptr,
> + bounce.size))
> + ret = -EFAULT;
> + }
> cmd->cpages = bounce.cpages;
> dmirror_bounce_fini(&bounce);
> return ret;
> @@ -886,9 +892,10 @@ static int dmirror_snapshot(struct dmirror *dmirror,
> break;
>
> n = (range.end - range.start) >> PAGE_SHIFT;
> - ret = copy_to_user(uptr, perm, n);
> - if (ret)
> + if (copy_to_user(uptr, perm, n)) {
> + ret = -EFAULT;
> break;
> + }
>
> cmd->cpages += n;
> uptr += n;
> @@ -911,9 +918,8 @@ static long dmirror_fops_unlocked_ioctl(struct file *filp,
> if (!dmirror)
> return -EINVAL;
>
> - ret = copy_from_user(&cmd, uarg, sizeof(cmd));
> - if (ret)
> - return ret;
> + if (copy_from_user(&cmd, uarg, sizeof(cmd)))
> + return -EFAULT;
>
> if (cmd.addr & ~PAGE_MASK)
> return -EINVAL;
> @@ -946,7 +952,10 @@ static long dmirror_fops_unlocked_ioctl(struct file *filp,
> if (ret)
> return ret;
>
> - return copy_to_user(uarg, &cmd, sizeof(cmd));
> + if (copy_to_user(uarg, &cmd, sizeof(cmd)))
> + return -EFAULT;
> +
> + return 0;
> }
>
> static const struct file_operations dmirror_fops = {
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/hmm/test: Fix some copy_to_user() error handling
2020-05-11 18:37 ` Dan Carpenter
@ 2020-05-12 20:00 ` Jason Gunthorpe
-1 siblings, 0 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2020-05-12 20:00 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jérôme Glisse, Ralph Campbell, linux-mm, kernel-janitors
On Mon, May 11, 2020 at 09:37:04PM +0300, Dan Carpenter wrote:
> The copy_to_user() function returns the number of bytes which weren't
> copied but we want to return negative error codes. Also in dmirror_write()
> if the copy_from_user() fails then there is some cleanup needed before
> we can return so I fixed that as well.
>
> Fixes: 5d5e54be8a1e3 ("mm/hmm/test: add selftest driver for HMM")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> lib/test_hmm.c | 41 +++++++++++++++++++++++++----------------
> 1 file changed, 25 insertions(+), 16 deletions(-)
Thank you, I squashed this into the original commit.
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/hmm/test: Fix some copy_to_user() error handling
@ 2020-05-12 20:00 ` Jason Gunthorpe
0 siblings, 0 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2020-05-12 20:00 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jérôme Glisse, Ralph Campbell, linux-mm, kernel-janitors
On Mon, May 11, 2020 at 09:37:04PM +0300, Dan Carpenter wrote:
> The copy_to_user() function returns the number of bytes which weren't
> copied but we want to return negative error codes. Also in dmirror_write()
> if the copy_from_user() fails then there is some cleanup needed before
> we can return so I fixed that as well.
>
> Fixes: 5d5e54be8a1e3 ("mm/hmm/test: add selftest driver for HMM")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> lib/test_hmm.c | 41 +++++++++++++++++++++++++----------------
> 1 file changed, 25 insertions(+), 16 deletions(-)
Thank you, I squashed this into the original commit.
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-12 20:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-11 18:37 [PATCH] mm/hmm/test: Fix some copy_to_user() error handling Dan Carpenter
2020-05-11 18:37 ` Dan Carpenter
2020-05-11 19:49 ` Ralph Campbell
2020-05-11 19:49 ` Ralph Campbell
2020-05-12 20:00 ` Jason Gunthorpe
2020-05-12 20:00 ` Jason Gunthorpe
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.