On Mon, Dec 17, 2018 at 09:33:43AM +0200, Nikolay Borisov wrote: > > > On 14.12.18 г. 21:45 ч., fdmanana@xxxxxxxxxx wrote: > > From: Filipe Manana <fdmanana@xxxxxxxx> > > > > 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@xxxxxxxx> > > --- > > 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.
