diff mbox

[05/47] RDMA/uverbs: Make the code in ib_uverbs_cmd_verbs() less confusing

Message ID 1507735840.2477.8.camel@wdc.com (mailing list archive)
State Superseded
Headers show

Commit Message

Bart Van Assche Oct. 11, 2017, 3:30 p.m. UTC
On Wed, 2017-10-11 at 08:19 +0300, Matan Barak wrote:
> But what about cases where UVERBS_OPTIMIZE_USING_STACK_SZ is defined but 

> is insufficient in size?


Sorry. How about the patch below? The most significant change compared to the
current code is that UVERBS_OPTIMIZE_USING_STACK_SZ has to be defined as 0
instead of not defining that macro to disable the optimization. A new change
is that this version uses gcc's support for zero-length arrays (which is an
extension of the C standard and is already used elsewhere in the kernel).


Subject: [PATCH] RDMA/uverbs: Make the code in ib_uverbs_cmd_verbs() less
 confusing

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 | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

-- 
2.14.2

Comments

Matan Barak Oct. 15, 2017, 7:37 a.m. UTC | #1
On 11/10/2017 18:30, Bart Van Assche wrote:
> On Wed, 2017-10-11 at 08:19 +0300, Matan Barak wrote:
>> But what about cases where UVERBS_OPTIMIZE_USING_STACK_SZ is defined but
>> is insufficient in size?
> 
> Sorry. How about the patch below? The most significant change compared to the
> current code is that UVERBS_OPTIMIZE_USING_STACK_SZ has to be defined as 0
> instead of not defining that macro to disable the optimization. A new change
> is that this version uses gcc's support for zero-length arrays (which is an
> extension of the C standard and is already used elsewhere in the kernel).
> 
> 
> Subject: [PATCH] RDMA/uverbs: Make the code in ib_uverbs_cmd_verbs() less
>   confusing
> 
> 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 | 13 +++----------
>   1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
> index 5286ad57d903..71ff2644e053 100644
> --- a/drivers/infiniband/core/uverbs_ioctl.c
> +++ b/drivers/infiniband/core/uverbs_ioctl.c
> @@ -241,9 +241,7 @@ static long ib_uverbs_cmd_verbs(struct ib_device *ib_dev,
>   	struct uverbs_attr *curr_attr;
>   	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)];
> -#endif
>   
>   	if (hdr->reserved)
>   		return -EINVAL;
> @@ -269,13 +267,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)
>   		ctx = (void *)data;
> -
>   	if (!ctx)
> -#endif
> -	ctx = kmalloc(ctx_size, GFP_KERNEL);
> +		ctx = kmalloc(ctx_size, GFP_KERNEL);
>   	if (!ctx)
>   		return -ENOMEM;
>   
> @@ -311,10 +306,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 != (void *)data)
> +		kfree(ctx);
>   	return err;
>   }
>   
> 

Simple and looks good to me. Thanks.
Acked-by: Matan Barak <matanb@mellanox.com>
--
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 mbox

Patch

diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
index 5286ad57d903..71ff2644e053 100644
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -241,9 +241,7 @@  static long ib_uverbs_cmd_verbs(struct ib_device *ib_dev,
 	struct uverbs_attr *curr_attr;
 	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)];
-#endif
 
 	if (hdr->reserved)
 		return -EINVAL;
@@ -269,13 +267,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)
 		ctx = (void *)data;
-
 	if (!ctx)
-#endif
-	ctx = kmalloc(ctx_size, GFP_KERNEL);
+		ctx = kmalloc(ctx_size, GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
 
@@ -311,10 +306,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 != (void *)data)
+		kfree(ctx);
 	return err;
 }