| Message ID | 20181214194513.21741-1-fdmanana@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Btrfs: do not overwrite error return value in scrub progress ioctl | expand |
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); >
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); >
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.
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);