Btrfs: do not overwrite error return value in scrub progress ioctl
PATCH 1/1 Btrfs: do not overwrite error return value in scrub progress ioctl
From: Filipe Manana <[email protected]>
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.
--- fs/btrfs/ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/btrfs/ioctl.c +1 -1
@@ -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 &©_to_user (arg, sa,sizeof (*sa))) ret = -EFAULT ;kfree (sa);
Signature · 1 line
2.11.0
Quoted 17 lines · On 14.12.18 г. 21:45 ч., [email protected] wrote:
From: Filipe Manana <[email protected]>
If the call to
btrfs_scrub_progress()failed we would overwrite the error returned to user space with-EFAULTif the call tocopy_to_user()failed as well. Fix that by callingcopy_to_user()only ifbtrfs_scrub_progress()returned success.Signed-off-by Filipe Manana <[email protected]>--- fs/btrfs/ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/btrfs/ioctl.c +0
-0
fs/btrfs/ioctl.c static long btrfs_ioctl_scrub_progress(struct btrfs_fs_info *fs_info, · line 4334
Quoted 5 lines
fs/btrfs/ioctl.c +1
-1@@ -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 &©_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:
Quoted 4 lines
fs/btrfs/ioctl.c +0
-0ret = -EFAULT ;kfree (sa);
Quoted 33 lines · Nikolay Borisov
Quoted 17 lines · On 14.12.18 г. 21:45 ч., [email protected] wrote:
From: Filipe Manana <[email protected]>
If the call to
btrfs_scrub_progress()failed we would overwrite the error returned to user space with-EFAULTif the call tocopy_to_user()failed as well. Fix that by callingcopy_to_user()only ifbtrfs_scrub_progress()returned success.Signed-off-by Filipe Manana <[email protected]>--- fs/btrfs/ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/btrfs/ioctl.c +0
-0fs/btrfs/ioctl.c static long btrfs_ioctl_scrub_progress(struct btrfs_fs_info *fs_info, · line 4334
Quoted 5 lines
fs/btrfs/ioctl.c +1
-1@@ -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 &©_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_useron the ret value ofbtrfs_scrub_progressin the same if. Perhaps,if (ret)
return ret;
Quoted 6 lines
if (
copy_to_user)
return-EFAULTSame 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.
Quoted 8 lines · On 12/15/2018 03:45 AM, [email protected] wrote:
From: Filipe Manana <[email protected]>
If the call to
btrfs_scrub_progress()failed we would overwrite the error returned to user space with-EFAULTif the call tocopy_to_user()failed as well. Fix that by callingcopy_to_user()only ifbtrfs_scrub_progress()returned success.Signed-off-by Filipe Manana <[email protected]>
Quoted 18 lines
--- fs/btrfs/ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/btrfs/ioctl.c +1
-1@@ -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 &©_to_user (arg, sa,sizeof (*sa))) ret = -EFAULT ;kfree (sa);