| Message ID | 1474354107-18774-1-git-send-email-shailendra.v@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 9/20/16 2:48 AM, Shailendra Verma wrote: > This is of course wrong to call kfree() if memdup_user() fails, > no memory was allocated and the error in the error-valued pointer > should be returned. > > Reviewed-by: Ravikant Sharma <ravikant.s2@samsung.com> > Signed-off-by: Shailendra Verma <shailendra.v@samsung.com> Hi Shailendra - In all three cases, the return value is set using the error-valued pointer and the pointer is set to NULL. kfree() checks to see if the pointer is NULL and, if so, does nothing. This allows us to use a common exit path which is an extremely common pattern in the kernel. So there's never any possible ERR_PTR dereferencing happening. However, in all three cases, the allocation you're checking is the first in each routine and there's no additional cleanup to do. So your patch is an improvement, but it's an improvement in code readability instead of a bug fix. I'd ask that you re-submit with a commit message that reflects that. Thanks, -Jeff > --- > fs/btrfs/ioctl.c | 21 ++++++--------------- > 1 file changed, 6 insertions(+), 15 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index da94138..58a40f8 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -4512,11 +4512,8 @@ static long btrfs_ioctl_logical_to_ino(struct btrfs_root *root, > return -EPERM; > > loi = memdup_user(arg, sizeof(*loi)); > - if (IS_ERR(loi)) { > - ret = PTR_ERR(loi); > - loi = NULL; > - goto out; > - } > + if (IS_ERR(loi)) > + return PTR_ERR(loi); > > path = btrfs_alloc_path(); > if (!path) { > @@ -5143,11 +5140,8 @@ static long btrfs_ioctl_set_received_subvol_32(struct file *file, > int ret = 0; > > args32 = memdup_user(arg, sizeof(*args32)); > - if (IS_ERR(args32)) { > - ret = PTR_ERR(args32); > - args32 = NULL; > - goto out; > - } > + if (IS_ERR(args32)) > + return PTR_ERR(args32); > > args64 = kmalloc(sizeof(*args64), GFP_NOFS); > if (!args64) { > @@ -5195,11 +5189,8 @@ static long btrfs_ioctl_set_received_subvol(struct file *file, > int ret = 0; > > sa = memdup_user(arg, sizeof(*sa)); > - if (IS_ERR(sa)) { > - ret = PTR_ERR(sa); > - sa = NULL; > - goto out; > - } > + if (IS_ERR(sa)) > + return PTR_ERR(sa); > > ret = _btrfs_ioctl_set_received_subvol(file, sa); > >
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index da94138..58a40f8 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -4512,11 +4512,8 @@ static long btrfs_ioctl_logical_to_ino(struct btrfs_root *root, return -EPERM; loi = memdup_user(arg, sizeof(*loi)); - if (IS_ERR(loi)) { - ret = PTR_ERR(loi); - loi = NULL; - goto out; - } + if (IS_ERR(loi)) + return PTR_ERR(loi); path = btrfs_alloc_path(); if (!path) { @@ -5143,11 +5140,8 @@ static long btrfs_ioctl_set_received_subvol_32(struct file *file, int ret = 0; args32 = memdup_user(arg, sizeof(*args32)); - if (IS_ERR(args32)) { - ret = PTR_ERR(args32); - args32 = NULL; - goto out; - } + if (IS_ERR(args32)) + return PTR_ERR(args32); args64 = kmalloc(sizeof(*args64), GFP_NOFS); if (!args64) { @@ -5195,11 +5189,8 @@ static long btrfs_ioctl_set_received_subvol(struct file *file, int ret = 0; sa = memdup_user(arg, sizeof(*sa)); - if (IS_ERR(sa)) { - ret = PTR_ERR(sa); - sa = NULL; - goto out; - } + if (IS_ERR(sa)) + return PTR_ERR(sa); ret = _btrfs_ioctl_set_received_subvol(file, sa);