[lustre-devel] [PATCH 5/7] lustre/libcfs: discard cfs_trace_allocate_string_buffer()

[Andreas Dilger]

> On Jul 29, 2018, at 21:49, NeilBrown <neilb at suse.com> wrote:
> 
> cfs_trace_allocate_string_buffer() is a simple wrapper
> around kzalloc() that adds little value.  The code is
> clearer if we perform the test and the allocation
> directly where needed.
> 
> Also change the test from '>' to '>=' to ensure we
> never try to allocate more than 2 pages, as that seems
> to be the intent.
> 
> Signed-off-by: NeilBrown <neilb at suse.com>

I was going to say that we probably don't have even a single debug message that is larger than PAGE_SIZE, but I suspect in some cases, printing a message with a PATH_MAX-sized filename may result in a buffer that is larger than PAGE_SIZE, though smaller than 2x PAGE_SIZE.

It isn't clear that returning an error for = 2 * PAGE_SIZE makes sense, but I also don't think this corner case is critical (and it leaves a byte for a NUL
terminator in the buffer).

Reviewed-by: Andreas Dilger <adilger at whamcloud.com>

> ---
> drivers/staging/lustre/lnet/libcfs/module.c    |    6 +++--
> drivers/staging/lustre/lnet/libcfs/tracefile.c |   28 +++++++++---------------
> drivers/staging/lustre/lnet/libcfs/tracefile.h |    1 -
> 3 files changed, 13 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lnet/libcfs/module.c b/drivers/staging/lustre/lnet/libcfs/module.c
> index 5d2be941777e..1de83b1997c6 100644
> --- a/drivers/staging/lustre/lnet/libcfs/module.c
> +++ b/drivers/staging/lustre/lnet/libcfs/module.c
> @@ -305,9 +305,9 @@ static int proc_dobitmasks(struct ctl_table *table, int write,
> 	int is_subsys = (mask == &libcfs_subsystem_debug) ? 1 : 0;
> 	int is_printk = (mask == &libcfs_printk) ? 1 : 0;
> 
> -	rc = cfs_trace_allocate_string_buffer(&tmpstr, tmpstrlen);
> -	if (rc < 0)
> -		return rc;
> +	tmpstr = kzalloc(tmpstrlen, GFP_KERNEL);
> +	if (!tmpstr)
> +		return -ENOMEM;
> 
> 	if (!write) {
> 		libcfs_debug_mask2str(tmpstr, tmpstrlen, *mask, is_subsys);
> diff --git a/drivers/staging/lustre/lnet/libcfs/tracefile.c b/drivers/staging/lustre/lnet/libcfs/tracefile.c
> index 40048165fc16..b273107b3815 100644
> --- a/drivers/staging/lustre/lnet/libcfs/tracefile.c
> +++ b/drivers/staging/lustre/lnet/libcfs/tracefile.c
> @@ -963,26 +963,16 @@ int cfs_trace_copyout_string(char __user *usr_buffer, int usr_buffer_nob,
> }
> EXPORT_SYMBOL(cfs_trace_copyout_string);
> 
> -int cfs_trace_allocate_string_buffer(char **str, int nob)
> -{
> -	if (nob > 2 * PAGE_SIZE)	    /* string must be "sensible" */
> -		return -EINVAL;
> -
> -	*str = kmalloc(nob, GFP_KERNEL | __GFP_ZERO);
> -	if (!*str)
> -		return -ENOMEM;
> -
> -	return 0;
> -}
> -
> int cfs_trace_dump_debug_buffer_usrstr(void __user *usr_str, int usr_str_nob)
> {
> 	char *str;
> 	int rc;
> 
> -	rc = cfs_trace_allocate_string_buffer(&str, usr_str_nob + 1);
> -	if (rc)
> -		return rc;
> +	if (usr_str_nob >= 2 * PAGE_SIZE)
> +		return -EINVAL;
> +	str = kzalloc(usr_str_nob + 1, GFP_KERNEL);
> +	if (!str)
> +		return -ENOMEM;
> 
> 	rc = cfs_trace_copyin_string(str, usr_str_nob + 1,
> 				     usr_str, usr_str_nob);
> @@ -1044,9 +1034,11 @@ int cfs_trace_daemon_command_usrstr(void __user *usr_str, int usr_str_nob)
> 	char *str;
> 	int rc;
> 
> -	rc = cfs_trace_allocate_string_buffer(&str, usr_str_nob + 1);
> -	if (rc)
> -		return rc;
> +	if (usr_str_nob >= 2 * PAGE_SIZE)
> +		return -EINVAL;
> +	str = kzalloc(usr_str_nob + 1, GFP_KERNEL);
> +	if (!str)
> +		return -ENOMEM;
> 
> 	rc = cfs_trace_copyin_string(str, usr_str_nob + 1,
> 				     usr_str, usr_str_nob);
> diff --git a/drivers/staging/lustre/lnet/libcfs/tracefile.h b/drivers/staging/lustre/lnet/libcfs/tracefile.h
> index 82f090fd8dfa..2134549bb3d7 100644
> --- a/drivers/staging/lustre/lnet/libcfs/tracefile.h
> +++ b/drivers/staging/lustre/lnet/libcfs/tracefile.h
> @@ -63,7 +63,6 @@ int cfs_trace_copyin_string(char *knl_buffer, int knl_buffer_nob,
> 			    const char __user *usr_buffer, int usr_buffer_nob);
> int cfs_trace_copyout_string(char __user *usr_buffer, int usr_buffer_nob,
> 			     const char *knl_str, char *append);
> -int cfs_trace_allocate_string_buffer(char **str, int nob);
> int cfs_trace_dump_debug_buffer_usrstr(void __user *usr_str, int usr_str_nob);
> int cfs_trace_daemon_command(char *str);
> int cfs_trace_daemon_command_usrstr(void __user *usr_str, int usr_str_nob);
> 
> 

Cheers, Andreas
---
Andreas Dilger
CTO Whamcloud




-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 235 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20180730/7ec4de1e/attachment.sig>