| Message ID | 20200511183742.GB225608@mwanda (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/rxe: Return -EFAULT if copy_from_user() fails | expand |
Does this "err = -EFAULT;" make any sense in your commit? Zhu Yanjun -----Original Message----- From: Dan Carpenter <dan.carpenter@oracle.com> Sent: Tuesday, May 12, 2020 2:38 AM To: Yanjun Zhu <yanjunz@mellanox.com>; Sudip Mukherjee <sudipm.mukherjee@gmail.com> Cc: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@ziepe.ca>; linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org Subject: [PATCH] RDMA/rxe: Return -EFAULT if copy_from_user() fails This function used to always return -EINVAL but we updated it to try preserve the error codes. Unfortunately the copy_to_user() is returning the number of bytes remaining to be copied instead of a negative error code. Fixes: a3a974b4654d ("RDMA/rxe: Always return ERR_PTR from rxe_create_mmap_info()") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/infiniband/sw/rxe/rxe_queue.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/infiniband/sw/rxe/rxe_queue.c b/drivers/infiniband/sw/rxe/rxe_queue.c index fef2ab5112de5..245040c3a35d0 100644 --- a/drivers/infiniband/sw/rxe/rxe_queue.c +++ b/drivers/infiniband/sw/rxe/rxe_queue.c @@ -50,9 +50,10 @@ int do_mmap_info(struct rxe_dev *rxe, struct mminfo __user *outbuf, goto err1; } - err = copy_to_user(outbuf, &ip->info, sizeof(ip->info)); - if (err) + if (copy_to_user(outbuf, &ip->info, sizeof(ip->info))) { + err = -EFAULT; goto err2; + } spin_lock_bh(&rxe->pending_lock); list_add(&ip->pending_mmaps, &rxe->pending_mmaps); -- 2.26.2
On Tue, May 12, 2020 at 01:12:38AM +0000, Yanjun Zhu wrote:
> Does this "err = -EFAULT;" make any sense in your commit?
Yanjun, please stop top-posting, it is annoying.
Thanks
On Mon, May 11, 2020 at 09:37:42PM +0300, Dan Carpenter wrote: > This function used to always return -EINVAL but we updated it to try > preserve the error codes. Unfortunately the copy_to_user() is returning > the number of bytes remaining to be copied instead of a negative error > code. > > Fixes: a3a974b4654d ("RDMA/rxe: Always return ERR_PTR from rxe_create_mmap_info()") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/infiniband/sw/rxe/rxe_queue.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Thanks, Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
On Tue, May 12, 2020 at 09:29:36AM +0300, Leon Romanovsky wrote: > On Mon, May 11, 2020 at 09:37:42PM +0300, Dan Carpenter wrote: > > This function used to always return -EINVAL but we updated it to try > > preserve the error codes. Unfortunately the copy_to_user() is returning > > the number of bytes remaining to be copied instead of a negative error > > code. > > > > Fixes: a3a974b4654d ("RDMA/rxe: Always return ERR_PTR from rxe_create_mmap_info()") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > drivers/infiniband/sw/rxe/rxe_queue.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > Thanks, > Reviewed-by: Leon Romanovsky <leonro@mellanox.com> Actually Yanjun is right and "err" can be removed. Thanks
On Tue, May 12, 2020 at 10:02:03AM +0300, Leon Romanovsky wrote: > On Tue, May 12, 2020 at 09:29:36AM +0300, Leon Romanovsky wrote: > > On Mon, May 11, 2020 at 09:37:42PM +0300, Dan Carpenter wrote: > > > This function used to always return -EINVAL but we updated it to try > > > preserve the error codes. Unfortunately the copy_to_user() is returning > > > the number of bytes remaining to be copied instead of a negative error > > > code. > > > > > > Fixes: a3a974b4654d ("RDMA/rxe: Always return ERR_PTR from rxe_create_mmap_info()") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > --- > > > drivers/infiniband/sw/rxe/rxe_queue.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > Thanks, > > Reviewed-by: Leon Romanovsky <leonro@mellanox.com> > > Actually Yanjun is right and "err" can be removed. > > Thanks I don't know if the code you guys are looking at is older or newer than linux-next... :P drivers/infiniband/sw/rxe/rxe_queue.c 39 int do_mmap_info(struct rxe_dev *rxe, struct mminfo __user *outbuf, 40 struct ib_udata *udata, struct rxe_queue_buf *buf, 41 size_t buf_size, struct rxe_mmap_info **ip_p) 42 { 43 int err; ^^^ 44 struct rxe_mmap_info *ip = NULL; 45 46 if (outbuf) { 47 ip = rxe_create_mmap_info(rxe, buf_size, udata, buf); 48 if (IS_ERR(ip)) { 49 err = PTR_ERR(ip); 50 goto err1; ^^^^^^^^^ 51 } 52 53 err = copy_to_user(outbuf, &ip->info, sizeof(ip->info)); 54 if (err) 55 goto err2; ^^^^^^^^^ 56 57 spin_lock_bh(&rxe->pending_lock); 58 list_add(&ip->pending_mmaps, &rxe->pending_mmaps); 59 spin_unlock_bh(&rxe->pending_lock); 60 } 61 62 *ip_p = ip; 63 64 return 0; 65 66 err2: 67 kfree(ip); 68 err1: 69 return err; ^^^ 70 } regards, dan carpenter
On Tue, May 12, 2020 at 11:17:06AM +0300, Dan Carpenter wrote: > On Tue, May 12, 2020 at 10:02:03AM +0300, Leon Romanovsky wrote: > > On Tue, May 12, 2020 at 09:29:36AM +0300, Leon Romanovsky wrote: > > > On Mon, May 11, 2020 at 09:37:42PM +0300, Dan Carpenter wrote: > > > > This function used to always return -EINVAL but we updated it to try > > > > preserve the error codes. Unfortunately the copy_to_user() is returning > > > > the number of bytes remaining to be copied instead of a negative error > > > > code. > > > > > > > > Fixes: a3a974b4654d ("RDMA/rxe: Always return ERR_PTR from rxe_create_mmap_info()") > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > --- > > > > drivers/infiniband/sw/rxe/rxe_queue.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > Thanks, > > > Reviewed-by: Leon Romanovsky <leonro@mellanox.com> > > > > Actually Yanjun is right and "err" can be removed. > > > > Thanks > > I don't know if the code you guys are looking at is older or newer than > linux-next... :P We both looked on rdma-next, but the wrong code was added to -rc. Jason, that patch was marked as stable@. Thanks
-----Original Message----- From: Leon Romanovsky <leon@kernel.org> Sent: Tuesday, May 12, 2020 4:31 PM To: Dan Carpenter <dan.carpenter@oracle.com>; Jason Gunthorpe <jgg@ziepe.ca> Cc: Yanjun Zhu <yanjunz@mellanox.com>; Sudip Mukherjee <sudipm.mukherjee@gmail.com>; Doug Ledford <dledford@redhat.com>; linux-rdma@vger.kernel.org; kernel-janitors@vger.kernel.org Subject: Re: [PATCH] RDMA/rxe: Return -EFAULT if copy_from_user() fails On Tue, May 12, 2020 at 11:17:06AM +0300, Dan Carpenter wrote: > On Tue, May 12, 2020 at 10:02:03AM +0300, Leon Romanovsky wrote: > > On Tue, May 12, 2020 at 09:29:36AM +0300, Leon Romanovsky wrote: > > > On Mon, May 11, 2020 at 09:37:42PM +0300, Dan Carpenter wrote: > > > > This function used to always return -EINVAL but we updated it to > > > > try preserve the error codes. Unfortunately the copy_to_user() > > > > is returning the number of bytes remaining to be copied instead > > > > of a negative error code. > > > > > > > > Fixes: a3a974b4654d ("RDMA/rxe: Always return ERR_PTR from > > > > rxe_create_mmap_info()") > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > --- > > > > drivers/infiniband/sw/rxe/rxe_queue.c | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > Thanks, > > > Reviewed-by: Leon Romanovsky <leonro@mellanox.com> > > > > Actually Yanjun is right and "err" can be removed. > > > > Thanks > > I don't know if the code you guys are looking at is older or newer > than linux-next... :P > We both looked on rdma-next, but the wrong code was added to -rc. Yes. I agree with you. Zhu Yanjun > Jason, that patch was marked as stable@. Thanks
On Mon, May 11, 2020 at 09:37:42PM +0300, Dan Carpenter wrote: > This function used to always return -EINVAL but we updated it to try > preserve the error codes. Unfortunately the copy_to_user() is returning > the number of bytes remaining to be copied instead of a negative error > code. > > Fixes: a3a974b4654d ("RDMA/rxe: Always return ERR_PTR from rxe_create_mmap_info()") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/infiniband/sw/rxe/rxe_queue.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) I squashed this into to the above patch since it is marked for -stable This means for-rc gets rebased Thanks, Jason
diff --git a/drivers/infiniband/sw/rxe/rxe_queue.c b/drivers/infiniband/sw/rxe/rxe_queue.c index fef2ab5112de5..245040c3a35d0 100644 --- a/drivers/infiniband/sw/rxe/rxe_queue.c +++ b/drivers/infiniband/sw/rxe/rxe_queue.c @@ -50,9 +50,10 @@ int do_mmap_info(struct rxe_dev *rxe, struct mminfo __user *outbuf, goto err1; } - err = copy_to_user(outbuf, &ip->info, sizeof(ip->info)); - if (err) + if (copy_to_user(outbuf, &ip->info, sizeof(ip->info))) { + err = -EFAULT; goto err2; + } spin_lock_bh(&rxe->pending_lock); list_add(&ip->pending_mmaps, &rxe->pending_mmaps);
This function used to always return -EINVAL but we updated it to try preserve the error codes. Unfortunately the copy_to_user() is returning the number of bytes remaining to be copied instead of a negative error code. Fixes: a3a974b4654d ("RDMA/rxe: Always return ERR_PTR from rxe_create_mmap_info()") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/infiniband/sw/rxe/rxe_queue.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)