| Message ID | 1507570048.2674.17.camel@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 09/10/2017 20:27, Bart Van Assche wrote: > On Mon, 2017-10-09 at 10:40 +0300, Matan Barak (External) wrote: >> On 07/10/2017 00:32, Bart Van Assche wrote: >>> This patch reduces the number of #ifdefs and also avoids that >>> smatch reports the following: >>> >>> drivers/infiniband/core/uverbs_ioctl.c:276: ib_uverbs_cmd_verbs() warn: if statement not indented >>> drivers/infiniband/core/uverbs_ioctl.c:280: ib_uverbs_cmd_verbs() warn: possible memory leak of 'ctx' >>> drivers/infiniband/core/uverbs_ioctl.c:315: ib_uverbs_cmd_verbs() warn: if statement not indented >>> >>> References: commit fac9658cabb9 ("IB/core: Add new ioctl interface") >>> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> >>> Cc: Matan Barak <matanb@mellanox.com> >>> Cc: Yishai Hadas <yishaih@mellanox.com> >>> --- >>> drivers/infiniband/core/uverbs_ioctl.c | 20 +++++++++----------- >>> 1 file changed, 9 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c >>> index 5286ad57d903..53f780ea49fe 100644 >>> --- a/drivers/infiniband/core/uverbs_ioctl.c >>> +++ b/drivers/infiniband/core/uverbs_ioctl.c >>> @@ -234,7 +234,7 @@ static long ib_uverbs_cmd_verbs(struct ib_device *ib_dev, >>> const struct uverbs_method_spec *method_spec; >>> long err = 0; >>> unsigned int i; >>> - struct { >>> + struct attr_and_bundle { >>> struct ib_uverbs_attr *uattrs; >>> struct uverbs_attr_bundle *uverbs_attr_bundle; >>> } *ctx = NULL; >>> @@ -242,7 +242,10 @@ static long ib_uverbs_cmd_verbs(struct ib_device *ib_dev, >>> unsigned long *curr_bitmap; >>> size_t ctx_size; >>> #ifdef UVERBS_OPTIMIZE_USING_STACK_SZ >>> - uintptr_t data[UVERBS_OPTIMIZE_USING_STACK_SZ / sizeof(uintptr_t)]; >>> + uintptr_t __data[UVERBS_OPTIMIZE_USING_STACK_SZ / sizeof(uintptr_t)]; >>> + struct attr_and_bundle *const data = (void *)__data; >>> +#else >>> + struct attr_and_bundle *const data = NULL; >>> #endif >>> >>> if (hdr->reserved) >>> @@ -269,13 +272,10 @@ static long ib_uverbs_cmd_verbs(struct ib_device *ib_dev, >>> (method_spec->num_child_attrs / BITS_PER_LONG + >>> method_spec->num_buckets); >>> >>> -#ifdef UVERBS_OPTIMIZE_USING_STACK_SZ >>> if (ctx_size <= UVERBS_OPTIMIZE_USING_STACK_SZ) >> >> If UVERBS_OPTIMIZE_USING_STACK_SZ isn't defined, you'll get an error here. >> Maybe we could define it as 0 in the else part of the local variables deceleration. > > Hello Matan, > > That's an interesting suggestion but that wouldn't work well with the "#ifdef > UVERBS_OPTIMIZE_USING_STACK_SZ" code in the declaration section. How about > changing "if (ctx_size <= UVERBS_OPTIMIZE_USING_STACK_SZ) ctx = data;" into > "ctx = data;" such that it becomes again safe not to define UVERBS_OPTIMIZE_USING_STACK_SZ? > With that change this patch looks as follows: > > > diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c > index 5286ad57d903..81c05ba2a58b 100644 > --- a/drivers/infiniband/core/uverbs_ioctl.c > +++ b/drivers/infiniband/core/uverbs_ioctl.c > @@ -234,7 +234,7 @@ static long ib_uverbs_cmd_verbs(struct ib_device *ib_dev, > const struct uverbs_method_spec *method_spec; > long err = 0; > unsigned int i; > - struct { > + struct attr_and_bundle { > struct ib_uverbs_attr *uattrs; > struct uverbs_attr_bundle *uverbs_attr_bundle; > } *ctx = NULL; > @@ -242,7 +242,10 @@ static long ib_uverbs_cmd_verbs(struct ib_device *ib_dev, > unsigned long *curr_bitmap; > size_t ctx_size; > #ifdef UVERBS_OPTIMIZE_USING_STACK_SZ > - uintptr_t data[UVERBS_OPTIMIZE_USING_STACK_SZ / sizeof(uintptr_t)]; > + uintptr_t __data[UVERBS_OPTIMIZE_USING_STACK_SZ / sizeof(uintptr_t)]; > + struct attr_and_bundle *const data = (void *)__data; > +#else > + struct attr_and_bundle *const data = NULL; I meant to define that macro here: #define UVERBS_OPTIMIZE_USING_STACK_SZ 0 > #endif > > if (hdr->reserved) > @@ -269,13 +272,9 @@ static long ib_uverbs_cmd_verbs(struct ib_device *ib_dev, > (method_spec->num_child_attrs / BITS_PER_LONG + > method_spec->num_buckets); > > -#ifdef UVERBS_OPTIMIZE_USING_STACK_SZ > - if (ctx_size <= UVERBS_OPTIMIZE_USING_STACK_SZ) > - ctx = (void *)data; > - > + ctx = data; > if (!ctx) > -#endif > - ctx = kmalloc(ctx_size, GFP_KERNEL); > + ctx = kmalloc(ctx_size, GFP_KERNEL); But what about cases where UVERBS_OPTIMIZE_USING_STACK_SZ is defined but is insufficient in size? In the suggestion I posted above, I meant: /* Here UVERBS_OPTIMIZE_USING_STACK_SZ is always defined */ if (ctx_size <= UVERBS_OPTIMIZE_USING_STACK_SZ) ctx = data; if (!ctx) ctx = kmalloc(ctx_size, GFP_KERNEL); The last hunk (free part) stays the same. > if (!ctx) > return -ENOMEM; > > @@ -311,10 +310,8 @@ static long ib_uverbs_cmd_verbs(struct ib_device *ib_dev, > err = uverbs_handle_method(buf, ctx->uattrs, hdr->num_attrs, ib_dev, > file, method_spec, ctx->uverbs_attr_bundle); > out: > -#ifdef UVERBS_OPTIMIZE_USING_STACK_SZ > - if (ctx_size > UVERBS_OPTIMIZE_USING_STACK_SZ) > -#endif > - kfree(ctx); > + if (ctx != data) > + kfree(ctx); > return err; > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c index 5286ad57d903..81c05ba2a58b 100644 --- a/drivers/infiniband/core/uverbs_ioctl.c +++ b/drivers/infiniband/core/uverbs_ioctl.c @@ -234,7 +234,7 @@ static long ib_uverbs_cmd_verbs(struct ib_device *ib_dev, const struct uverbs_method_spec *method_spec; long err = 0; unsigned int i; - struct { + struct attr_and_bundle { struct ib_uverbs_attr *uattrs; struct uverbs_attr_bundle *uverbs_attr_bundle; } *ctx = NULL; @@ -242,7 +242,10 @@ static long ib_uverbs_cmd_verbs(struct ib_device *ib_dev, unsigned long *curr_bitmap; size_t ctx_size; #ifdef UVERBS_OPTIMIZE_USING_STACK_SZ - uintptr_t data[UVERBS_OPTIMIZE_USING_STACK_SZ / sizeof(uintptr_t)]; + uintptr_t __data[UVERBS_OPTIMIZE_USING_STACK_SZ / sizeof(uintptr_t)]; + struct attr_and_bundle *const data = (void *)__data; +#else + struct attr_and_bundle *const data = NULL; #endif if (hdr->reserved) @@ -269,13 +272,9 @@ static long ib_uverbs_cmd_verbs(struct ib_device *ib_dev, (method_spec->num_child_attrs / BITS_PER_LONG + method_spec->num_buckets); -#ifdef UVERBS_OPTIMIZE_USING_STACK_SZ - if (ctx_size <= UVERBS_OPTIMIZE_USING_STACK_SZ) - ctx = (void *)data; - + ctx = data; if (!ctx) -#endif - ctx = kmalloc(ctx_size, GFP_KERNEL); + ctx = kmalloc(ctx_size, GFP_KERNEL); if (!ctx) return -ENOMEM; @@ -311,10 +310,8 @@ static long ib_uverbs_cmd_verbs(struct ib_device *ib_dev, err = uverbs_handle_method(buf, ctx->uattrs, hdr->num_attrs, ib_dev, file, method_spec, ctx->uverbs_attr_bundle); out: -#ifdef UVERBS_OPTIMIZE_USING_STACK_SZ - if (ctx_size > UVERBS_OPTIMIZE_USING_STACK_SZ) -#endif - kfree(ctx); + if (ctx != data) + kfree(ctx); return err; }