[PATCH] Btrfs: always copy scrub arguments back to user space

[PATCH] Btrfs: always copy scrub arguments back to user space

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

If scrub returns an error we are not copying back the scrub arguments
structure to user space. This prevents user space to know how much progress
scrub has done if an error happened - this includes -ECANCELED which is
returned when users ask for scrub to stop. A particular use case, which is
used in btrfs-progs, is to resume scrub after it is canceled, in that case
it relies on checking the progress from the scrub arguments structure and
then use that progress in a call to resume scrub.

So fix this by always copying the scrub arguments structure to user space,
overwriting the value returned to user space with -EFAULT only if copying
the structure failed to let user space know that either that copying did
not happen, and therefore the structure is stale, or it happened partially
and the structure is probably not valid and corrupt due to the partial
copy.

Reported-by: Graham Cobb <g.btrfs@cobb.uk.net>
Link: https://lore.kernel.org/linux-btrfs/d0a97688-78be-08de-ca7d-bcb4c7fb397e@cobb.uk.net/
Fixes: 06fe39ab15a6a4 ("Btrfs: do not overwrite scrub error with fault error in scrub ioctl")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ioctl.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 3a4bd5cd67fa..173758d86feb 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4253,7 +4253,19 @@ static long btrfs_ioctl_scrub(struct file *file, void __user *arg)
 			      &sa->progress, sa->flags & BTRFS_SCRUB_READONLY,
 			      0);
 
-	if (ret == 0 && copy_to_user(arg, sa, sizeof(*sa)))
+	/*
+	 * Copy scrub args to user space even if btrfs_scrub_dev() returned an
+	 * error. This is important as it allows user space to know how much
+	 * progress scrub has done. For example, if scrub is canceled we get
+	 * -ECANCELED from btrfs_scrub_dev() and return that error back to user
+	 * space. Later user space can inspect the progress from the structure
+	 * btrfs_ioctl_scrub_args and resume scrub from where it left off
+	 * previously (btrfs-progs does this).
+	 * If we fail to copy the btrfs_ioctl_scrub_args structure to user space
+	 * then return -EFAULT to signal the structure was not copied or it may
+	 * be corrupt and unreliable due to a partial copy.
+	 */
+	if (copy_to_user(arg, sa, sizeof(*sa)))
 		ret = -EFAULT;
 
 	if (!(sa->flags & BTRFS_SCRUB_READONLY))
-- 
2.11.0

Re: [PATCH] Btrfs: always copy scrub arguments back to user space

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
    
    

Re: [PATCH] Btrfs: always copy scrub arguments back to user space

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2698 bytes --]



On 2020/1/16 下午7:29, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If scrub returns an error we are not copying back the scrub arguments
> structure to user space. This prevents user space to know how much progress
> scrub has done if an error happened - this includes -ECANCELED which is
> returned when users ask for scrub to stop. A particular use case, which is
> used in btrfs-progs, is to resume scrub after it is canceled, in that case
> it relies on checking the progress from the scrub arguments structure and
> then use that progress in a call to resume scrub.
> 
> So fix this by always copying the scrub arguments structure to user space,
> overwriting the value returned to user space with -EFAULT only if copying
> the structure failed to let user space know that either that copying did
> not happen, and therefore the structure is stale, or it happened partially
> and the structure is probably not valid and corrupt due to the partial
> copy.
> 
> Reported-by: Graham Cobb <g.btrfs@cobb.uk.net>
> Link: https://lore.kernel.org/linux-btrfs/d0a97688-78be-08de-ca7d-bcb4c7fb397e@cobb.uk.net/
> Fixes: 06fe39ab15a6a4 ("Btrfs: do not overwrite scrub error with fault error in scrub ioctl")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>  fs/btrfs/ioctl.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 3a4bd5cd67fa..173758d86feb 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4253,7 +4253,19 @@ static long btrfs_ioctl_scrub(struct file *file, void __user *arg)
>  			      &sa->progress, sa->flags & BTRFS_SCRUB_READONLY,
>  			      0);
>  
> -	if (ret == 0 && copy_to_user(arg, sa, sizeof(*sa)))
> +	/*
> +	 * Copy scrub args to user space even if btrfs_scrub_dev() returned an
> +	 * error. This is important as it allows user space to know how much
> +	 * progress scrub has done. For example, if scrub is canceled we get
> +	 * -ECANCELED from btrfs_scrub_dev() and return that error back to user
> +	 * space. Later user space can inspect the progress from the structure
> +	 * btrfs_ioctl_scrub_args and resume scrub from where it left off
> +	 * previously (btrfs-progs does this).
> +	 * If we fail to copy the btrfs_ioctl_scrub_args structure to user space
> +	 * then return -EFAULT to signal the structure was not copied or it may
> +	 * be corrupt and unreliable due to a partial copy.
> +	 */
> +	if (copy_to_user(arg, sa, sizeof(*sa)))
>  		ret = -EFAULT;
>  
>  	if (!(sa->flags & BTRFS_SCRUB_READONLY))
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Btrfs: always copy scrub arguments back to user space

[David Sterba]

On Thu, Jan 16, 2020 at 11:29:20AM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If scrub returns an error we are not copying back the scrub arguments
> structure to user space. This prevents user space to know how much progress
> scrub has done if an error happened - this includes -ECANCELED which is
> returned when users ask for scrub to stop. A particular use case, which is
> used in btrfs-progs, is to resume scrub after it is canceled, in that case
> it relies on checking the progress from the scrub arguments structure and
> then use that progress in a call to resume scrub.
> 
> So fix this by always copying the scrub arguments structure to user space,
> overwriting the value returned to user space with -EFAULT only if copying
> the structure failed to let user space know that either that copying did
> not happen, and therefore the structure is stale, or it happened partially
> and the structure is probably not valid and corrupt due to the partial
> copy.
> 
> Reported-by: Graham Cobb <g.btrfs@cobb.uk.net>
> Link: https://lore.kernel.org/linux-btrfs/d0a97688-78be-08de-ca7d-bcb4c7fb397e@cobb.uk.net/
> Fixes: 06fe39ab15a6a4 ("Btrfs: do not overwrite scrub error with fault error in scrub ioctl")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Added to 5.5-rc queue, thanks.

Re: [PATCH] Btrfs: always copy scrub arguments back to user space

[Graham Cobb]

On 16/01/2020 14:12, David Sterba wrote:
> On Thu, Jan 16, 2020 at 11:29:20AM +0000, fdmanana@kernel.org wrote:
>> From: Filipe Manana <fdmanana@suse.com>
>>
>> If scrub returns an error we are not copying back the scrub arguments
>> structure to user space. This prevents user space to know how much progress
>> scrub has done if an error happened - this includes -ECANCELED which is
>> returned when users ask for scrub to stop. A particular use case, which is
>> used in btrfs-progs, is to resume scrub after it is canceled, in that case
>> it relies on checking the progress from the scrub arguments structure and
>> then use that progress in a call to resume scrub.
>>
>> So fix this by always copying the scrub arguments structure to user space,
>> overwriting the value returned to user space with -EFAULT only if copying
>> the structure failed to let user space know that either that copying did
>> not happen, and therefore the structure is stale, or it happened partially
>> and the structure is probably not valid and corrupt due to the partial
>> copy.
>>
>> Reported-by: Graham Cobb <g.btrfs@cobb.uk.net>
>> Link: https://lore.kernel.org/linux-btrfs/d0a97688-78be-08de-ca7d-bcb4c7fb397e@cobb.uk.net/
>> Fixes: 06fe39ab15a6a4 ("Btrfs: do not overwrite scrub error with fault error in scrub ioctl")
>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> 
> Added to 5.5-rc queue, thanks.

Just to let you know... As promised I have built a Debian Testing kernel
(5.3) with this patch applied. My btrfs-scrub-slowly script has now run
successfully to completion with 32 cancel/resume cycles (runs for 30
mins then takes 10 min break).

If it is useful, feel free to add:

Tested-by: Graham Cobb <g.btrfs@cobb.uk.net>

Thanks Felipe!