[PATCH 2/2] Return the result from user admin command IOCTL even in case of failure

[PATCH 2/2] Return the result from user admin command IOCTL even in case of failure

[Chayan Biswas]

We copy the result to user irrespective of the completion status of
the user Admin command. The user application may expect the error
code in the result field in case of failure.

Signed-off-by: Chayan Biswas <Chayan.Biswas at sandisk.com>
---
 drivers/block/nvme.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/block/nvme.c b/drivers/block/nvme.c
index 4b706b5..311d753 100644
--- a/drivers/block/nvme.c
+++ b/drivers/block/nvme.c
@@ -1248,8 +1248,7 @@ static int nvme_user_admin_cmd(struct nvme_dev *dev,
                nvme_free_iod(dev, iod);
        }

-       if (!status && copy_to_user(&ucmd->result, &cmd.result,
-                                                       sizeof(cmd.result)))
+       if (copy_to_user(&ucmd->result, &cmd.result, sizeof(cmd.result)))
                status = -EFAULT;

        return status;
--
1.7.1


________________________________

PLEASE NOTE: The information contained in this electronic mail message is intended only for the use of the designated recipient(s) named above. If the reader of this message is not the intended recipient, you are hereby notified that you have received this message in error and that any review, dissemination, distribution, or copying of this message is strictly prohibited. If you have received this communication in error, please notify the sender by telephone or e-mail (as shown above) immediately and destroy any and all copies of this message in your possession (whether hard copies or electronically stored copies).

[PATCH 2/2] Return the result from user admin command IOCTL even in case of failureX

[Keith Busch]

On Thu, 28 Feb 2013, Chayan Biswas wrote:
> -       if (!status && copy_to_user(&ucmd->result, &cmd.result,
> -                                                       sizeof(cmd.result)))
> +       if (copy_to_user(&ucmd->result, &cmd.result, sizeof(cmd.result)))
>                status = -EFAULT;

I think you want to change this to a check for 'status >= 0' before
copying the result to the user since a status < 0 means the command was
not completed by the controller (successfully or otherwise) and you'd
be copying uninitialized data to the user in that case.

[PATCH 2/2] Return the result from user admin command IOCTL even in case of failureX

[Matthew Wilcox]

On Fri, Mar 01, 2013@03:05:06PM -0700, Keith Busch wrote:
> On Thu, 28 Feb 2013, Chayan Biswas wrote:
> >-       if (!status && copy_to_user(&ucmd->result, &cmd.result,
> >-                                                       sizeof(cmd.result)))
> >+       if (copy_to_user(&ucmd->result, &cmd.result, sizeof(cmd.result)))
> >               status = -EFAULT;
> 
> I think you want to change this to a check for 'status >= 0' before
> copying the result to the user since a status < 0 means the command was
> not completed by the controller (successfully or otherwise) and you'd
> be copying uninitialized data to the user in that case.

Going back through my old patches ... Chayan, do you want to make the
change Keith suggested and resubmit?  Or do you think he's wrong?

[PATCH 2/2] Return the result from user admin command IOCTL even in case of failureX

[Chayan Biswas]

I am going to resubmit the patch today.

Thanks,
Chayan

> -----Original Message-----
> From: Matthew Wilcox [mailto:willy at linux.intel.com]
> Sent: Wednesday, May 22, 2013 1:24 PM
> To: Keith Busch
> Cc: Chayan Biswas; linux-nvme at lists.infradead.org
> Subject: Re: [PATCH 2/2] Return the result from user admin command IOCTL
> even in case of failureX
>
> On Fri, Mar 01, 2013@03:05:06PM -0700, Keith Busch wrote:
> > On Thu, 28 Feb 2013, Chayan Biswas wrote:
> > >-       if (!status && copy_to_user(&ucmd->result, &cmd.result,
> > >-
> sizeof(cmd.result)))
> > >+       if (copy_to_user(&ucmd->result, &cmd.result,
> sizeof(cmd.result)))
> > >               status = -EFAULT;
> >
> > I think you want to change this to a check for 'status >= 0' before
> > copying the result to the user since a status < 0 means the command was
> > not completed by the controller (successfully or otherwise) and you'd
> > be copying uninitialized data to the user in that case.
>
> Going back through my old patches ... Chayan, do you want to make the
> change Keith suggested and resubmit?  Or do you think he's wrong?


________________________________

PLEASE NOTE: The information contained in this electronic mail message is intended only for the use of the designated recipient(s) named above. If the reader of this message is not the intended recipient, you are hereby notified that you have received this message in error and that any review, dissemination, distribution, or copying of this message is strictly prohibited. If you have received this communication in error, please notify the sender by telephone or e-mail (as shown above) immediately and destroy any and all copies of this message in your possession (whether hard copies or electronically stored copies).