re: btrfs: check unsupported filters in balance arguments

[Dan Carpenter]

Hello David Sterba,

The patch 8eb934591f8b: "btrfs: check unsupported filters in balance
arguments" from Oct 12, 2015, leads to the following static checker
warning:

fs/btrfs/ioctl.c:4673 btrfs_ioctl_balance()
warn: possible memory leak of 'bctl'

fs/btrfs/ioctl.c
  4624  bctl = kzalloc(sizeof(*bctl), GFP_NOFS);
  4625  if (!bctl) {
  4626  ret = -ENOMEM;
  4627  goto out_bargs;
  4628  }
  4629  
  4630  bctl->fs_info = fs_info;
  4631  if (arg) {
  4632  memcpy(>data, >data, sizeof(bctl->data));
  4633  memcpy(>meta, >meta, sizeof(bctl->meta));
  4634  memcpy(>sys, >sys, sizeof(bctl->sys));
  4635  
  4636  bctl->flags = bargs->flags;
  4637  } else {
  4638  /* balance everything - no filters */
  4639  bctl->flags |= BTRFS_BALANCE_TYPE_MASK;
  4640  }
  4641  
  4642  if (bctl->flags & ~(BTRFS_BALANCE_ARGS_MASK | 
BTRFS_BALANCE_TYPE_MASK)) {
  4643  ret = -EINVAL;
  4644  goto out_bargs;

Memory leak on this path.

  4645  }
  4646  
  4647  do_balance:
  4648  /*
  4649   * Ownership of bctl and mutually_exclusive_operation_running
  4650   * goes to to btrfs_balance.  bctl is freed in __cancel_balance,
  4651   * or, if restriper was paused all the way until unmount, in
  4652   * free_fs_info.  mutually_exclusive_operation_running is
  4653   * cleared in __cancel_balance.
  4654   */
  4655  need_unlock = false;
  4656  
  4657  ret = btrfs_balance(bctl, bargs);

We free bctl in btrfs_balance() most times.

  4658  
  4659  if (arg) {
  4660  if (copy_to_user(arg, bargs, sizeof(*bargs)))
  4661  ret = -EFAULT;
  4662  }
  4663  
  4664  out_bargs:
  4665  kfree(bargs);
  4666  out_unlock:
  4667  mutex_unlock(_info->balance_mutex);
  4668  mutex_unlock(_info->volume_mutex);
  4669  if (need_unlock)
  4670  
atomic_set(_info->mutually_exclusive_operation_running, 0);
  4671  out:
  4672  mnt_drop_write_file(file);
  4673  return ret;
  4674  }

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: btrfs: check unsupported filters in balance arguments

[David Sterba]

On Wed, Oct 21, 2015 at 11:55:00PM +0300, Dan Carpenter wrote:
> Hello David Sterba,
> 
> The patch 8eb934591f8b: "btrfs: check unsupported filters in balance
> arguments" from Oct 12, 2015, leads to the following static checker
> warning:
> 
>   fs/btrfs/ioctl.c:4673 btrfs_ioctl_balance()
>   warn: possible memory leak of 'bctl'

Thanks for the report, the fix is on the way:
https://patchwork.kernel.org/patch/7453231/
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: btrfs: check unsupported filters in balance arguments

[Chris Mason]

On Thu, Oct 22, 2015 at 12:52:37AM +0200, David Sterba wrote:
> On Wed, Oct 21, 2015 at 11:55:00PM +0300, Dan Carpenter wrote:
> > Hello David Sterba,
> > 
> > The patch 8eb934591f8b: "btrfs: check unsupported filters in balance
> > arguments" from Oct 12, 2015, leads to the following static checker
> > warning:
> > 
> > fs/btrfs/ioctl.c:4673 btrfs_ioctl_balance()
> > warn: possible memory leak of 'bctl'
> 
> Thanks for the report, the fix is on the way:
> https://patchwork.kernel.org/patch/7453231/

Thanks, I've picked this up.

-chris
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH for 4.3] btrfs: check unsupported filters in balance arguments

[David Sterba]

We don't verify that all the balance filter arguments supplemented by
the flags are actually known to the kernel. Thus we let it silently pass
and do nothing.

At the moment this means only the 'limit' filter, but we're going to add
a few more soon so it's better to have that fixed. Also in older stable
kernels so that it works with newer userspace tools.

Cc: sta...@vger.kernel.org # 3.16+
Signed-off-by: David Sterba 
---

Please try to get it into 4.3, before the new balance filters (stripes,
enhanced 'limit') get merged. This would cause us headaches when we would not
have the kernel checks and try to run updated btrfs-progs against that.

 fs/btrfs/ioctl.c   | 5 +
 fs/btrfs/volumes.h | 8 
 2 files changed, 13 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0adf5422fce9..3e3e6130637f 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4639,6 +4639,11 @@ static long btrfs_ioctl_balance(struct file *file, void 
__user *arg)
bctl->flags |= BTRFS_BALANCE_TYPE_MASK;
}
 
+   if (bctl->flags & ~(BTRFS_BALANCE_ARGS_MASK | BTRFS_BALANCE_TYPE_MASK)) 
{
+   ret = -EINVAL;
+   goto out_bargs;
+   }
+
 do_balance:
/*
 * Ownership of bctl and mutually_exclusive_operation_running
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 2ca784a14e84..595279a8b99f 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -376,6 +376,14 @@ struct map_lookup {
 #define BTRFS_BALANCE_ARGS_VRANGE  (1ULL << 4)
 #define BTRFS_BALANCE_ARGS_LIMIT   (1ULL << 5)
 
+#define BTRFS_BALANCE_ARGS_MASK\
+   (BTRFS_BALANCE_ARGS_PROFILES |  \
+BTRFS_BALANCE_ARGS_USAGE | \
+BTRFS_BALANCE_ARGS_DEVID | \
+BTRFS_BALANCE_ARGS_DRANGE |\
+BTRFS_BALANCE_ARGS_VRANGE |\
+BTRFS_BALANCE_ARGS_LIMIT)
+
 /*
  * Profile changing flags.  When SOFT is set we won't relocate chunk if
  * it already has the target profile (even though it may be
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html