[PATCH v4] staging: lustre: llite: Fix variable length array warning

[PATCH v4] staging: lustre: llite: Fix variable length array warning

[Guru Das Srinagesh]

Fix sparse warning "warning: Variable length array is used." by using
kmalloc_array to allocate the required amount of memory instead and
kfree to deallocate memory after use.

Signed-off-by: Guru Das Srinagesh <gurooodas@gmail.com>
---
 v4:
   - Changed kmalloc_array flags from GFP_KERNEL to GFP_ATOMIC

 v3:
   - Fixed checkpatch warning: Comparison to NULL could be written "!fullname"

 v2:
   - Added missing check for NULL return value of kmalloc_array()

 drivers/staging/lustre/lustre/llite/xattr.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
index 6187bff..ae2efd5 100644
--- a/drivers/staging/lustre/lustre/llite/xattr.c
+++ b/drivers/staging/lustre/lustre/llite/xattr.c
@@ -86,13 +86,17 @@ ll_xattr_set_common(const struct xattr_handler *handler,
 		    const char *name, const void *value, size_t size,
 		    int flags)
 {
-	char fullname[strlen(handler->prefix) + strlen(name) + 1];
+	int fullname_len = strlen(handler->prefix) + strlen(name) + 1;
+	char *fullname = kmalloc_array(fullname_len, sizeof(char), GFP_ATOMIC);
 	struct ll_sb_info *sbi = ll_i2sbi(inode);
 	struct ptlrpc_request *req = NULL;
 	const char *pv = value;
 	__u64 valid;
 	int rc;
 
+	if (!fullname)
+		return -ENOMEM;
+
 	if (flags == XATTR_REPLACE) {
 		ll_stats_ops_tally(ll_i2sbi(inode), LPROC_LL_REMOVEXATTR, 1);
 		valid = OBD_MD_FLXATTRRM;
@@ -153,6 +157,9 @@ ll_xattr_set_common(const struct xattr_handler *handler,
 	}
 
 	ptlrpc_req_finished(req);
+
+	kfree(fullname);
+
 	return 0;
 }
 
@@ -363,13 +370,17 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
 			       struct dentry *dentry, struct inode *inode,
 			       const char *name, void *buffer, size_t size)
 {
-	char fullname[strlen(handler->prefix) + strlen(name) + 1];
+	int fullname_len = strlen(handler->prefix) + strlen(name) + 1;
+	char *fullname = kmalloc_array(fullname_len, sizeof(char), GFP_ATOMIC);
 	struct ll_sb_info *sbi = ll_i2sbi(inode);
 #ifdef CONFIG_FS_POSIX_ACL
 	struct ll_inode_info *lli = ll_i2info(inode);
 #endif
 	int rc;
 
+	if (!fullname)
+		return -ENOMEM;
+
 	CDEBUG(D_VFSTRACE, "VFS Op:inode="DFID"(%p)\n",
 	       PFID(ll_inode2fid(inode)), inode);
 
@@ -411,8 +422,12 @@ static int ll_xattr_get_common(const struct xattr_handler *handler,
 		return -ENODATA;
 #endif
 	sprintf(fullname, "%s%s\n", handler->prefix, name);
-	return ll_xattr_list(inode, fullname, handler->flags, buffer, size,
-			     OBD_MD_FLXATTR);
+
+	rc = ll_xattr_list(inode, fullname, handler->flags, buffer, size,
+			   OBD_MD_FLXATTR);
+	kfree(fullname);
+
+	return rc;
 }
 
 static ssize_t ll_getxattr_lov(struct inode *inode, void *buf, size_t buf_size)
-- 
2.7.4

Re: [PATCH v4] staging: lustre: llite: Fix variable length array warning

[Dan Carpenter]

This patch introduces tons of memory leaks so we can't apply it.

Just ignore the warning.  The size is small enough that it won't
overflow the stack.  The other reason to avoid these types of
declarations is that in olden times (5 years ago at least) there was a
GCC for an arch where if you declared the variable inside a loop it
would not free the memory until after the end of the loop.

	while ((foo = frob())) {
		int whatver[x + y];
	}

The memory would keep increasing until we broke from the loop or it
overflowed the stack and crashed.

On Mon, May 08, 2017 at 11:57:16PM -0700, Guru Das Srinagesh wrote:
> Fix sparse warning "warning: Variable length array is used." by using
> kmalloc_array to allocate the required amount of memory instead and
> kfree to deallocate memory after use.
> 
> Signed-off-by: Guru Das Srinagesh <gurooodas@gmail.com>
> ---
>  v4:
>    - Changed kmalloc_array flags from GFP_KERNEL to GFP_ATOMIC
> 
>  v3:
>    - Fixed checkpatch warning: Comparison to NULL could be written "!fullname"
> 
>  v2:
>    - Added missing check for NULL return value of kmalloc_array()
> 
>  drivers/staging/lustre/lustre/llite/xattr.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
> index 6187bff..ae2efd5 100644
> --- a/drivers/staging/lustre/lustre/llite/xattr.c
> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> @@ -86,13 +86,17 @@ ll_xattr_set_common(const struct xattr_handler *handler,
>  		    const char *name, const void *value, size_t size,
>  		    int flags)
>  {
> -	char fullname[strlen(handler->prefix) + strlen(name) + 1];
> +	int fullname_len = strlen(handler->prefix) + strlen(name) + 1;
> +	char *fullname = kmalloc_array(fullname_len, sizeof(char), GFP_ATOMIC);

Using kmalloc_array() is pointless.  Everyone knows that sizeof(char) is
1 and also that 1 * x == x.  It just makes the code more confusing and
complicated for no reason.  Don't hide the allocation this declaration
block.  It should be next to the check for NULL.  But anyway, don't
bother resending, just ignore the warning.

regards,
dan carpenter

Re: [PATCH v4] staging: lustre: llite: Fix variable length array warning

[Guru Das Srinagesh]

On Tue, May 09, 2017 at 11:31:36AM +0300, Dan Carpenter wrote:
> Just ignore the warning.
> 
Thank you for your comments on this patch.