linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance()
@ 2022-04-21  9:51 Haowen Bai
  2022-04-21 10:01 ` Qu Wenruo
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Haowen Bai @ 2022-04-21  9:51 UTC (permalink / raw)
  To: Chris Mason, Josef Bacik, David Sterba
  Cc: Haowen Bai, linux-btrfs, linux-kernel

Free "bargs" before return.

Signed-off-by: Haowen Bai <baihaowen@meizu.com>
---
 fs/btrfs/ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index f08233c2b0b2..d4c8bea914b7 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4389,13 +4389,13 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
 			/* this is (2) */
 			mutex_unlock(&fs_info->balance_mutex);
 			ret = -EINPROGRESS;
-			goto out;
+			goto out_bargs;
 		}
 	} else {
 		/* this is (1) */
 		mutex_unlock(&fs_info->balance_mutex);
 		ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
-		goto out;
+		goto out_bargs;
 	}
 
 locked:
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance()
  2022-04-21  9:51 [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance() Haowen Bai
@ 2022-04-21 10:01 ` Qu Wenruo
  2022-04-21 10:47 ` Filipe Manana
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Qu Wenruo @ 2022-04-21 10:01 UTC (permalink / raw)
  To: Haowen Bai, Chris Mason, Josef Bacik, David Sterba
  Cc: linux-btrfs, linux-kernel



On 2022/4/21 17:51, Haowen Bai wrote:
> Free "bargs" before return.
>
> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
> ---
>   fs/btrfs/ioctl.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index f08233c2b0b2..d4c8bea914b7 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4389,13 +4389,13 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
>   			/* this is (2) */
>   			mutex_unlock(&fs_info->balance_mutex);
>   			ret = -EINPROGRESS;
> -			goto out;
> +			goto out_bargs;
>   		}
>   	} else {
>   		/* this is (1) */
>   		mutex_unlock(&fs_info->balance_mutex);
>   		ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
> -		goto out;
> +		goto out_bargs;

out_bargs will also unlock balance mutex, causing a double unlock.


>   	}
>
>   locked:

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance()
  2022-04-21  9:51 [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance() Haowen Bai
  2022-04-21 10:01 ` Qu Wenruo
@ 2022-04-21 10:47 ` Filipe Manana
  2022-04-21 11:21 ` Nikolay Borisov
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Filipe Manana @ 2022-04-21 10:47 UTC (permalink / raw)
  To: Haowen Bai
  Cc: Chris Mason, Josef Bacik, David Sterba, linux-btrfs, linux-kernel

On Thu, Apr 21, 2022 at 05:51:17PM +0800, Haowen Bai wrote:
> Free "bargs" before return.
> 
> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
> ---
>  fs/btrfs/ioctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index f08233c2b0b2..d4c8bea914b7 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4389,13 +4389,13 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
>  			/* this is (2) */
>  			mutex_unlock(&fs_info->balance_mutex);
>  			ret = -EINPROGRESS;
> -			goto out;
> +			goto out_bargs;
>  		}
>  	} else {
>  		/* this is (1) */
>  		mutex_unlock(&fs_info->balance_mutex);
>  		ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
> -		goto out;
> +		goto out_bargs;

In addition to Qu's comment about the double unlock, this is also a fix
for a recent patch that is not yet on Linus' tree:

    btrfs: simplify codeflow in btrfs_ioctl_balance

    (https://lore.kernel.org/linux-btrfs/20220330091407.1319454-4-nborisov@suse.com/)

Something usually worth mentioning, as we can't add a Fixes tag in this
case.

Thanks.

>  	}
>  
>  locked:
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance()
  2022-04-21  9:51 [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance() Haowen Bai
  2022-04-21 10:01 ` Qu Wenruo
  2022-04-21 10:47 ` Filipe Manana
@ 2022-04-21 11:21 ` Nikolay Borisov
  2022-04-21 11:25   ` Nikolay Borisov
  2022-04-21 17:35 ` kernel test robot
  2022-04-22 20:28 ` David Sterba
  4 siblings, 1 reply; 10+ messages in thread
From: Nikolay Borisov @ 2022-04-21 11:21 UTC (permalink / raw)
  To: Haowen Bai, Chris Mason, Josef Bacik, David Sterba
  Cc: linux-btrfs, linux-kernel



On 21.04.22 г. 12:51 ч., Haowen Bai wrote:
> Free "bargs" before return.
> 
> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
> ---
>   fs/btrfs/ioctl.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index f08233c2b0b2..d4c8bea914b7 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4389,13 +4389,13 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
>   			/* this is (2) */
>   			mutex_unlock(&fs_info->balance_mutex);
>   			ret = -EINPROGRESS;
> -			goto out;
> +			goto out_bargs;
>   		}
>   	} else {
>   		/* this is (1) */
>   		mutex_unlock(&fs_info->balance_mutex);
>   		ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
> -		goto out;
> +		goto out_bargs;
>   	}
>   
>   locked:

I think this is a better fix:

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 7a6974e877f4..906ed1b93654 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4391,15 +4391,13 @@ static long btrfs_ioctl_balance(struct file 
*file, void __user *arg)
                         goto again;
                 } else {
                         /* this is (2) */
-                       mutex_unlock(&fs_info->balance_mutex);
                         ret = -EINPROGRESS;
-                       goto out;
+                       goto out_bargs;
                 }
         } else {
                 /* this is (1) */
-               mutex_unlock(&fs_info->balance_mutex);
                 ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
-               goto out;
+               goto out_bargs;
         }

  locked:

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance()
  2022-04-21 11:21 ` Nikolay Borisov
@ 2022-04-21 11:25   ` Nikolay Borisov
  2022-04-21 11:34     ` Nikolay Borisov
  2022-04-21 11:41     ` Qu Wenruo
  0 siblings, 2 replies; 10+ messages in thread
From: Nikolay Borisov @ 2022-04-21 11:25 UTC (permalink / raw)
  To: Haowen Bai, Chris Mason, Josef Bacik, David Sterba
  Cc: linux-btrfs, linux-kernel



On 21.04.22 г. 14:21 ч., Nikolay Borisov wrote:
> 
> 
> On 21.04.22 г. 12:51 ч., Haowen Bai wrote:
>> Free "bargs" before return.
>>
>> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
>> ---
>>   fs/btrfs/ioctl.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index f08233c2b0b2..d4c8bea914b7 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -4389,13 +4389,13 @@ static long btrfs_ioctl_balance(struct file 
>> *file, void __user *arg)
>>               /* this is (2) */
>>               mutex_unlock(&fs_info->balance_mutex);
>>               ret = -EINPROGRESS;
>> -            goto out;
>> +            goto out_bargs;
>>           }
>>       } else {
>>           /* this is (1) */
>>           mutex_unlock(&fs_info->balance_mutex);
>>           ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
>> -        goto out;
>> +        goto out_bargs;
>>       }
>>   locked:
> 
> I think this is a better fix:
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 7a6974e877f4..906ed1b93654 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4391,15 +4391,13 @@ static long btrfs_ioctl_balance(struct file 
> *file, void __user *arg)
>                          goto again;
>                  } else {
>                          /* this is (2) */
> -                       mutex_unlock(&fs_info->balance_mutex);
>                          ret = -EINPROGRESS;
> -                       goto out;
> +                       goto out_bargs;
>                  }
>          } else {
>                  /* this is (1) */
> -               mutex_unlock(&fs_info->balance_mutex);
>                  ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
> -               goto out;
> +               goto out_bargs;
>          }
> 
>   locked:
> 


Actually to simplify further:

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 7a6974e877f4..bbda55d41a06 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4353,6 +4353,7 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
         bargs = memdup_user(arg, sizeof(*bargs));
         if (IS_ERR(bargs)) {
                 ret = PTR_ERR(bargs);
+               bargs = NULL;
                 goto out;
         }
  
@@ -4391,13 +4392,11 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
                         goto again;
                 } else {
                         /* this is (2) */
-                       mutex_unlock(&fs_info->balance_mutex);
                         ret = -EINPROGRESS;
                         goto out;
                 }
         } else {
                 /* this is (1) */
-               mutex_unlock(&fs_info->balance_mutex);
                 ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
                 goto out;
         }
@@ -4406,7 +4405,7 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
         if (bargs->flags & BTRFS_BALANCE_RESUME) {
                 if (!fs_info->balance_ctl) {
                         ret = -ENOTCONN;
-                       goto out_bargs;
+                       goto out;
                 }
  
                 bctl = fs_info->balance_ctl;
@@ -4420,18 +4419,18 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
  
         if (bargs->flags & ~(BTRFS_BALANCE_ARGS_MASK | BTRFS_BALANCE_TYPE_MASK)) {
                 ret = -EINVAL;
-               goto out_bargs;
+               goto out;
         }
  
         if (fs_info->balance_ctl) {
                 ret = -EINPROGRESS;
-               goto out_bargs;
+               goto out;
         }
  
         bctl = kzalloc(sizeof(*bctl), GFP_KERNEL);
         if (!bctl) {
                 ret = -ENOMEM;
-               goto out_bargs;
+               goto out;
         }
  
         memcpy(&bctl->data, &bargs->data, sizeof(bctl->data));
@@ -4457,12 +4456,11 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
         }
  
         kfree(bctl);
-out_bargs:
+out:
         kfree(bargs);
         mutex_unlock(&fs_info->balance_mutex);
         if (need_unlock)
                 btrfs_exclop_finish(fs_info);
-out:
         mnt_drop_write_file(file);
         return ret;
  }





^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance()
  2022-04-21 11:25   ` Nikolay Borisov
@ 2022-04-21 11:34     ` Nikolay Borisov
  2022-04-21 13:30       ` David Sterba
  2022-04-21 11:41     ` Qu Wenruo
  1 sibling, 1 reply; 10+ messages in thread
From: Nikolay Borisov @ 2022-04-21 11:34 UTC (permalink / raw)
  To: Haowen Bai, Chris Mason, Josef Bacik, David Sterba
  Cc: linux-btrfs, linux-kernel

<snip>

> 
> Actually to simplify further:
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 7a6974e877f4..bbda55d41a06 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4353,6 +4353,7 @@ static long btrfs_ioctl_balance(struct file *file, 
> void __user *arg)
>          bargs = memdup_user(arg, sizeof(*bargs));
>          if (IS_ERR(bargs)) {
>                  ret = PTR_ERR(bargs);
> +               bargs = NULL;
>                  goto out;
>          }

Unf, this also leads to the double free ...

<snip>

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance()
  2022-04-21 11:25   ` Nikolay Borisov
  2022-04-21 11:34     ` Nikolay Borisov
@ 2022-04-21 11:41     ` Qu Wenruo
  1 sibling, 0 replies; 10+ messages in thread
From: Qu Wenruo @ 2022-04-21 11:41 UTC (permalink / raw)
  To: Nikolay Borisov, Haowen Bai, Chris Mason, Josef Bacik, David Sterba
  Cc: linux-btrfs, linux-kernel



On 2022/4/21 19:25, Nikolay Borisov wrote:
> 
> 
> On 21.04.22 г. 14:21 ч., Nikolay Borisov wrote:
>>
>>
>> On 21.04.22 г. 12:51 ч., Haowen Bai wrote:
>>> Free "bargs" before return.
>>>
>>> Signed-off-by: Haowen Bai <baihaowen@meizu.com>
>>> ---
>>>   fs/btrfs/ioctl.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>> index f08233c2b0b2..d4c8bea914b7 100644
>>> --- a/fs/btrfs/ioctl.c
>>> +++ b/fs/btrfs/ioctl.c
>>> @@ -4389,13 +4389,13 @@ static long btrfs_ioctl_balance(struct file 
>>> *file, void __user *arg)
>>>               /* this is (2) */
>>>               mutex_unlock(&fs_info->balance_mutex);
>>>               ret = -EINPROGRESS;
>>> -            goto out;
>>> +            goto out_bargs;
>>>           }
>>>       } else {
>>>           /* this is (1) */
>>>           mutex_unlock(&fs_info->balance_mutex);
>>>           ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
>>> -        goto out;
>>> +        goto out_bargs;
>>>       }
>>>   locked:
>>
>> I think this is a better fix:
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 7a6974e877f4..906ed1b93654 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -4391,15 +4391,13 @@ static long btrfs_ioctl_balance(struct file 
>> *file, void __user *arg)
>>                          goto again;
>>                  } else {
>>                          /* this is (2) */
>> -                       mutex_unlock(&fs_info->balance_mutex);
>>                          ret = -EINPROGRESS;
>> -                       goto out;
>> +                       goto out_bargs;
>>                  }
>>          } else {
>>                  /* this is (1) */
>> -               mutex_unlock(&fs_info->balance_mutex);
>>                  ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
>> -               goto out;
>> +               goto out_bargs;
>>          }
>>
>>   locked:
>>
> 
> 
> Actually to simplify further:
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 7a6974e877f4..bbda55d41a06 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4353,6 +4353,7 @@ static long btrfs_ioctl_balance(struct file *file, 
> void __user *arg)
>          bargs = memdup_user(arg, sizeof(*bargs));
>          if (IS_ERR(bargs)) {
>                  ret = PTR_ERR(bargs);
> +               bargs = NULL;
>                  goto out;
>          }
> 
> @@ -4391,13 +4392,11 @@ static long btrfs_ioctl_balance(struct file 
> *file, void __user *arg)
>                          goto again;
>                  } else {
>                          /* this is (2) */
> -                       mutex_unlock(&fs_info->balance_mutex);
>                          ret = -EINPROGRESS;
>                          goto out;
>                  }
>          } else {
>                  /* this is (1) */
> -               mutex_unlock(&fs_info->balance_mutex);
>                  ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
>                  goto out;
>          }
> @@ -4406,7 +4405,7 @@ static long btrfs_ioctl_balance(struct file *file, 
> void __user *arg)
>          if (bargs->flags & BTRFS_BALANCE_RESUME) {
>                  if (!fs_info->balance_ctl) {
>                          ret = -ENOTCONN;
> -                       goto out_bargs;
> +                       goto out;
>                  }
> 
>                  bctl = fs_info->balance_ctl;
> @@ -4420,18 +4419,18 @@ static long btrfs_ioctl_balance(struct file 
> *file, void __user *arg)
> 
>          if (bargs->flags & ~(BTRFS_BALANCE_ARGS_MASK | 
> BTRFS_BALANCE_TYPE_MASK)) {
>                  ret = -EINVAL;
> -               goto out_bargs;
> +               goto out;
>          }
> 
>          if (fs_info->balance_ctl) {
>                  ret = -EINPROGRESS;
> -               goto out_bargs;
> +               goto out;
>          }
> 
>          bctl = kzalloc(sizeof(*bctl), GFP_KERNEL);
>          if (!bctl) {
>                  ret = -ENOMEM;
> -               goto out_bargs;
> +               goto out;
>          }
> 
>          memcpy(&bctl->data, &bargs->data, sizeof(bctl->data));
> @@ -4457,12 +4456,11 @@ static long btrfs_ioctl_balance(struct file 
> *file, void __user *arg)
>          }
> 
>          kfree(bctl);
> -out_bargs:

This looks much better, without the extra label it's easier to read.

But, the @need_unlock variable seems to be uninitialized now, as it's 
only set in case (3), but case (2) and (1) now goes through out which 
needs to check @need_unlock.

Thanks,
Qu

> +out:
>          kfree(bargs);
>          mutex_unlock(&fs_info->balance_mutex);
>          if (need_unlock)
>                  btrfs_exclop_finish(fs_info);
> -out:
>          mnt_drop_write_file(file);
>          return ret;
>   }
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance()
  2022-04-21 11:34     ` Nikolay Borisov
@ 2022-04-21 13:30       ` David Sterba
  0 siblings, 0 replies; 10+ messages in thread
From: David Sterba @ 2022-04-21 13:30 UTC (permalink / raw)
  To: Nikolay Borisov
  Cc: Haowen Bai, Chris Mason, Josef Bacik, David Sterba, linux-btrfs,
	linux-kernel

On Thu, Apr 21, 2022 at 02:34:47PM +0300, Nikolay Borisov wrote:
> <snip>
> 
> > 
> > Actually to simplify further:
> > 
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index 7a6974e877f4..bbda55d41a06 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -4353,6 +4353,7 @@ static long btrfs_ioctl_balance(struct file *file, 
> > void __user *arg)
> >          bargs = memdup_user(arg, sizeof(*bargs));
> >          if (IS_ERR(bargs)) {
> >                  ret = PTR_ERR(bargs);
> > +               bargs = NULL;
> >                  goto out;
> >          }
> 
> Unf, this also leads to the double free ...

Please send me an incremental diff that I can fold to the patch, thanks.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance()
  2022-04-21  9:51 [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance() Haowen Bai
                   ` (2 preceding siblings ...)
  2022-04-21 11:21 ` Nikolay Borisov
@ 2022-04-21 17:35 ` kernel test robot
  2022-04-22 20:28 ` David Sterba
  4 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2022-04-21 17:35 UTC (permalink / raw)
  To: Haowen Bai, Chris Mason, Josef Bacik, David Sterba
  Cc: llvm, kbuild-all, Haowen Bai, linux-btrfs, linux-kernel

Hi Haowen,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on v5.18-rc3 next-20220421]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Haowen-Bai/btrfs-Fix-a-memory-leak-in-btrfs_ioctl_balance/20220421-175644
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: x86_64-randconfig-a005 (https://download.01.org/0day-ci/archive/20220422/202204220132.EBMTHukr-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project bac6cd5bf85669e3376610cfc4c4f9ca015e7b9b)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/9e36507b94d20118a936198b321a3544931217f0
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Haowen-Bai/btrfs-Fix-a-memory-leak-in-btrfs_ioctl_balance/20220421-175644
        git checkout 9e36507b94d20118a936198b321a3544931217f0
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash fs/btrfs/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> fs/btrfs/ioctl.c:4375:7: warning: variable 'need_unlock' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
                   if (!test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags)) {
                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   fs/btrfs/ioctl.c:4463:6: note: uninitialized use occurs here
           if (need_unlock)
               ^~~~~~~~~~~
   fs/btrfs/ioctl.c:4375:3: note: remove the 'if' if its condition is always true
                   if (!test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags)) {
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   fs/btrfs/ioctl.c:4373:6: warning: variable 'need_unlock' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
           if (fs_info->balance_ctl) {
               ^~~~~~~~~~~~~~~~~~~~
   fs/btrfs/ioctl.c:4463:6: note: uninitialized use occurs here
           if (need_unlock)
               ^~~~~~~~~~~
   fs/btrfs/ioctl.c:4373:2: note: remove the 'if' if its condition is always true
           if (fs_info->balance_ctl) {
           ^~~~~~~~~~~~~~~~~~~~~~~~~~
   fs/btrfs/ioctl.c:4343:18: note: initialize the variable 'need_unlock' to silence this warning
           bool need_unlock; /* for mut. excl. ops lock */
                           ^
                            = 0
   2 warnings generated.


vim +4375 fs/btrfs/ioctl.c

c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4336  
9ba1f6e44ed7a1 Liu Bo               2012-05-11  4337  static long btrfs_ioctl_balance(struct file *file, void __user *arg)
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4338  {
496ad9aa8ef448 Al Viro              2013-01-23  4339  	struct btrfs_root *root = BTRFS_I(file_inode(file))->root;
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4340  	struct btrfs_fs_info *fs_info = root->fs_info;
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4341  	struct btrfs_ioctl_balance_args *bargs;
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4342  	struct btrfs_balance_control *bctl;
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4343  	bool need_unlock; /* for mut. excl. ops lock */
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4344  	int ret;
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4345  
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4346  	if (!capable(CAP_SYS_ADMIN))
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4347  		return -EPERM;
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4348  
e54bfa31044d60 Liu Bo               2012-06-29  4349  	ret = mnt_want_write_file(file);
9ba1f6e44ed7a1 Liu Bo               2012-05-11  4350  	if (ret)
9ba1f6e44ed7a1 Liu Bo               2012-05-11  4351  		return ret;
9ba1f6e44ed7a1 Liu Bo               2012-05-11  4352  
0cb53767e6f475 Nikolay Borisov      2022-03-30  4353  	bargs = memdup_user(arg, sizeof(*bargs));
0cb53767e6f475 Nikolay Borisov      2022-03-30  4354  	if (IS_ERR(bargs)) {
0cb53767e6f475 Nikolay Borisov      2022-03-30  4355  		ret = PTR_ERR(bargs);
0cb53767e6f475 Nikolay Borisov      2022-03-30  4356  		goto out;
0cb53767e6f475 Nikolay Borisov      2022-03-30  4357  	}
0cb53767e6f475 Nikolay Borisov      2022-03-30  4358  
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4359  again:
c3e1f96c37d0f8 Goldwyn Rodrigues    2020-08-25  4360  	if (btrfs_exclop_start(fs_info, BTRFS_EXCLOP_BALANCE)) {
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4361  		mutex_lock(&fs_info->balance_mutex);
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4362  		need_unlock = true;
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4363  		goto locked;
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4364  	}
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4365  
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4366  	/*
0132761017e012 Nicholas D Steeves   2016-05-19  4367  	 * mut. excl. ops lock is locked.  Three possibilities:
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4368  	 *   (1) some other op is running
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4369  	 *   (2) balance is running
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4370  	 *   (3) balance is paused -- special case (think resume)
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4371  	 */
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4372  	mutex_lock(&fs_info->balance_mutex);
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4373  	if (fs_info->balance_ctl) {
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4374  		/* this is either (2) or (3) */
3009a62f3b1823 David Sterba         2018-03-21 @4375  		if (!test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags)) {
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4376  			mutex_unlock(&fs_info->balance_mutex);
dccdb07bc996e9 David Sterba         2018-03-21  4377  			/*
dccdb07bc996e9 David Sterba         2018-03-21  4378  			 * Lock released to allow other waiters to continue,
dccdb07bc996e9 David Sterba         2018-03-21  4379  			 * we'll reexamine the status again.
dccdb07bc996e9 David Sterba         2018-03-21  4380  			 */
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4381  			mutex_lock(&fs_info->balance_mutex);
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4382  
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4383  			if (fs_info->balance_ctl &&
3009a62f3b1823 David Sterba         2018-03-21  4384  			    !test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags)) {
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4385  				/* this is (3) */
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4386  				need_unlock = false;
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4387  				goto locked;
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4388  			}
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4389  
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4390  			mutex_unlock(&fs_info->balance_mutex);
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4391  			goto again;
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4392  		} else {
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4393  			/* this is (2) */
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4394  			mutex_unlock(&fs_info->balance_mutex);
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4395  			ret = -EINPROGRESS;
9e36507b94d201 Haowen Bai           2022-04-21  4396  			goto out_bargs;
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4397  		}
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4398  	} else {
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4399  		/* this is (1) */
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4400  		mutex_unlock(&fs_info->balance_mutex);
e57138b3e96e45 Anand Jain           2013-08-21  4401  		ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
9e36507b94d201 Haowen Bai           2022-04-21  4402  		goto out_bargs;
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4403  	}
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4404  
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4405  locked:
de322263d3a6d4 Ilya Dryomov         2012-01-16  4406  	if (bargs->flags & BTRFS_BALANCE_RESUME) {
de322263d3a6d4 Ilya Dryomov         2012-01-16  4407  		if (!fs_info->balance_ctl) {
de322263d3a6d4 Ilya Dryomov         2012-01-16  4408  			ret = -ENOTCONN;
de322263d3a6d4 Ilya Dryomov         2012-01-16  4409  			goto out_bargs;
de322263d3a6d4 Ilya Dryomov         2012-01-16  4410  		}
de322263d3a6d4 Ilya Dryomov         2012-01-16  4411  
de322263d3a6d4 Ilya Dryomov         2012-01-16  4412  		bctl = fs_info->balance_ctl;
de322263d3a6d4 Ilya Dryomov         2012-01-16  4413  		spin_lock(&fs_info->balance_lock);
de322263d3a6d4 Ilya Dryomov         2012-01-16  4414  		bctl->flags |= BTRFS_BALANCE_RESUME;
de322263d3a6d4 Ilya Dryomov         2012-01-16  4415  		spin_unlock(&fs_info->balance_lock);
efc0e69c2feab8 Nikolay Borisov      2021-11-25  4416  		btrfs_exclop_balance(fs_info, BTRFS_EXCLOP_BALANCE);
de322263d3a6d4 Ilya Dryomov         2012-01-16  4417  
de322263d3a6d4 Ilya Dryomov         2012-01-16  4418  		goto do_balance;
de322263d3a6d4 Ilya Dryomov         2012-01-16  4419  	}
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4420  
0cb53767e6f475 Nikolay Borisov      2022-03-30  4421  	if (bargs->flags & ~(BTRFS_BALANCE_ARGS_MASK | BTRFS_BALANCE_TYPE_MASK)) {
0cb53767e6f475 Nikolay Borisov      2022-03-30  4422  		ret = -EINVAL;
0cb53767e6f475 Nikolay Borisov      2022-03-30  4423  		goto out_bargs;
0cb53767e6f475 Nikolay Borisov      2022-03-30  4424  	}
0cb53767e6f475 Nikolay Borisov      2022-03-30  4425  
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4426  	if (fs_info->balance_ctl) {
837d5b6e46d1a4 Ilya Dryomov         2012-01-16  4427  		ret = -EINPROGRESS;
837d5b6e46d1a4 Ilya Dryomov         2012-01-16  4428  		goto out_bargs;
837d5b6e46d1a4 Ilya Dryomov         2012-01-16  4429  	}
837d5b6e46d1a4 Ilya Dryomov         2012-01-16  4430  
8d2db7855e7b65 David Sterba         2015-11-04  4431  	bctl = kzalloc(sizeof(*bctl), GFP_KERNEL);
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4432  	if (!bctl) {
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4433  		ret = -ENOMEM;
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4434  		goto out_bargs;
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4435  	}
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4436  
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4437  	memcpy(&bctl->data, &bargs->data, sizeof(bctl->data));
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4438  	memcpy(&bctl->meta, &bargs->meta, sizeof(bctl->meta));
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4439  	memcpy(&bctl->sys, &bargs->sys, sizeof(bctl->sys));
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4440  
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4441  	bctl->flags = bargs->flags;
de322263d3a6d4 Ilya Dryomov         2012-01-16  4442  do_balance:
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4443  	/*
c3e1f96c37d0f8 Goldwyn Rodrigues    2020-08-25  4444  	 * Ownership of bctl and exclusive operation goes to btrfs_balance.
c3e1f96c37d0f8 Goldwyn Rodrigues    2020-08-25  4445  	 * bctl is freed in reset_balance_state, or, if restriper was paused
c3e1f96c37d0f8 Goldwyn Rodrigues    2020-08-25  4446  	 * all the way until unmount, in free_fs_info.  The flag should be
c3e1f96c37d0f8 Goldwyn Rodrigues    2020-08-25  4447  	 * cleared after reset_balance_state.
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4448  	 */
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4449  	need_unlock = false;
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4450  
6fcf6e2bffb6cf David Sterba         2018-05-07  4451  	ret = btrfs_balance(fs_info, bctl, bargs);
0f89abf56abbd0 Christian Engelmayer 2015-10-21  4452  	bctl = NULL;
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4453  
6ad365fd1bfcde Nikolay Borisov      2022-03-30  4454  	if (ret == 0 || ret == -ECANCELED) {
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4455  		if (copy_to_user(arg, bargs, sizeof(*bargs)))
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4456  			ret = -EFAULT;
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4457  	}
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4458  
0f89abf56abbd0 Christian Engelmayer 2015-10-21  4459  	kfree(bctl);
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4460  out_bargs:
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4461  	kfree(bargs);
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4462  	mutex_unlock(&fs_info->balance_mutex);
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4463  	if (need_unlock)
c3e1f96c37d0f8 Goldwyn Rodrigues    2020-08-25  4464  		btrfs_exclop_finish(fs_info);
ed0fb78fb6aa29 Ilya Dryomov         2013-01-20  4465  out:
e54bfa31044d60 Liu Bo               2012-06-29  4466  	mnt_drop_write_file(file);
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4467  	return ret;
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4468  }
c9e9f97bdfb64d Ilya Dryomov         2012-01-16  4469  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance()
  2022-04-21  9:51 [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance() Haowen Bai
                   ` (3 preceding siblings ...)
  2022-04-21 17:35 ` kernel test robot
@ 2022-04-22 20:28 ` David Sterba
  4 siblings, 0 replies; 10+ messages in thread
From: David Sterba @ 2022-04-22 20:28 UTC (permalink / raw)
  To: Haowen Bai
  Cc: Chris Mason, Josef Bacik, David Sterba, linux-btrfs, linux-kernel

On Thu, Apr 21, 2022 at 05:51:17PM +0800, Haowen Bai wrote:
> Free "bargs" before return.
> 
> Signed-off-by: Haowen Bai <baihaowen@meizu.com>

Thanks for the report, as the leak was in a staged patch it can be fixed
in place, which I did by applying a fixup from Nikolay.

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-04-22 21:55 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-21  9:51 [PATCH] btrfs: Fix a memory leak in btrfs_ioctl_balance() Haowen Bai
2022-04-21 10:01 ` Qu Wenruo
2022-04-21 10:47 ` Filipe Manana
2022-04-21 11:21 ` Nikolay Borisov
2022-04-21 11:25   ` Nikolay Borisov
2022-04-21 11:34     ` Nikolay Borisov
2022-04-21 13:30       ` David Sterba
2022-04-21 11:41     ` Qu Wenruo
2022-04-21 17:35 ` kernel test robot
2022-04-22 20:28 ` David Sterba

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).