[PATCH] Btrfs: do not overwrite error return value in scrub progress ioctl

[PATCH] Btrfs: do not overwrite error return value in scrub progress ioctl

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

If the call to btrfs_scrub_progress() failed we would overwrite the error
returned to user space with -EFAULT if the call to copy_to_user() failed
as well. Fix that by calling copy_to_user() only if btrfs_scrub_progress()
returned success.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 01d18e1a393e..76848214a39f 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4331,7 +4331,7 @@ static long btrfs_ioctl_scrub_progress(struct btrfs_fs_info *fs_info,
 
 	ret = btrfs_scrub_progress(fs_info, sa->devid, &sa->progress);
 
-	if (copy_to_user(arg, sa, sizeof(*sa)))
+	if (ret == 0 && copy_to_user(arg, sa, sizeof(*sa)))
 		ret = -EFAULT;
 
 	kfree(sa);
-- 
2.11.0

Re: [PATCH] Btrfs: do not overwrite error return value in scrub progress ioctl

[Nikolay Borisov]

On 14.12.18 г. 21:45 ч., fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If the call to btrfs_scrub_progress() failed we would overwrite the error
> returned to user space with -EFAULT if the call to copy_to_user() failed
> as well. Fix that by calling copy_to_user() only if btrfs_scrub_progress()
> returned success.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 01d18e1a393e..76848214a39f 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4331,7 +4331,7 @@ static long btrfs_ioctl_scrub_progress(struct btrfs_fs_info *fs_info,
>  
>  	ret = btrfs_scrub_progress(fs_info, sa->devid, &sa->progress);
>  
> -	if (copy_to_user(arg, sa, sizeof(*sa)))
> +	if (ret == 0 && copy_to_user(arg, sa, sizeof(*sa)))

While this is ok it's a bit counter intuitive considering the code
convention. Because you predicate the execution of copy_to_user on the
ret value of btrfs_scrub_progress in the same if. Perhaps,

if (ret)
  return ret;

if (copy_to_user)
  return -EFAULT


Same feedback applies to your other patches, but I'm fine if you leave
it as is so:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

>  		ret = -EFAULT;
>  
>  	kfree(sa);
> 

Re: [PATCH] Btrfs: do not overwrite error return value in scrub progress ioctl

[Anand Jain]

On 12/15/2018 03:45 AM, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If the call to btrfs_scrub_progress() failed we would overwrite the error
> returned to user space with -EFAULT if the call to copy_to_user() failed
> as well. Fix that by calling copy_to_user() only if btrfs_scrub_progress()
> returned success.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>

> ---
>   fs/btrfs/ioctl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 01d18e1a393e..76848214a39f 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4331,7 +4331,7 @@ static long btrfs_ioctl_scrub_progress(struct btrfs_fs_info *fs_info,
>   
>   	ret = btrfs_scrub_progress(fs_info, sa->devid, &sa->progress);
>   
> -	if (copy_to_user(arg, sa, sizeof(*sa)))
> +	if (ret == 0 && copy_to_user(arg, sa, sizeof(*sa)))
>   		ret = -EFAULT;
>   
>   	kfree(sa);
> 

Re: [PATCH] Btrfs: do not overwrite error return value in scrub progress ioctl

[David Sterba]

On Mon, Dec 17, 2018 at 09:33:43AM +0200, Nikolay Borisov wrote:
> 
> 
> On 14.12.18 г. 21:45 ч., fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> > 
> > If the call to btrfs_scrub_progress() failed we would overwrite the error
> > returned to user space with -EFAULT if the call to copy_to_user() failed
> > as well. Fix that by calling copy_to_user() only if btrfs_scrub_progress()
> > returned success.
> > 
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > ---
> >  fs/btrfs/ioctl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index 01d18e1a393e..76848214a39f 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -4331,7 +4331,7 @@ static long btrfs_ioctl_scrub_progress(struct btrfs_fs_info *fs_info,
> >  
> >  	ret = btrfs_scrub_progress(fs_info, sa->devid, &sa->progress);
> >  
> > -	if (copy_to_user(arg, sa, sizeof(*sa)))
> > +	if (ret == 0 && copy_to_user(arg, sa, sizeof(*sa)))
> 
> While this is ok it's a bit counter intuitive considering the code
> convention. Because you predicate the execution of copy_to_user on the
> ret value of btrfs_scrub_progress in the same if. Perhaps,
> 
> if (ret)
>   return ret;
> 
> if (copy_to_user)
>   return -EFAULT
> 
> 
> Same feedback applies to your other patches, but I'm fine if you leave
> it as is so:

I've checked how common is "if (ret == 0 && copy_to_user...)" and there
are several instances. The additional condition is quite short so the
copy_to_user call is not lost in the noise, so I'm ok with the proposed
style. I would not even mind to unify other calls that do not follow
some common pattern eg. in btrfs_ioctl_set_received_subvol or
btrfs_ioctl_get_fslabel.