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

[David Sterba <dsterba@suse.cz>]

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.