linux-kernel - Re: [KJ] kmalloc to kzalloc patches for drivers/block [sane version]

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20060921072017.GA27798@us.ibm.com>
Date:	Thu, 21 Sep 2006 00:20:17 -0700
From:	Nishanth Aravamudan <nacc@...ibm.com>
To:	Om Narasimhan <om.turyx@...il.com>
Cc:	linux-kernel@...r.kernel.org, kernel-janitors@...ts.osdl.org
Subject: Re: [KJ] kmalloc to kzalloc patches for drivers/block [sane version]

On 20.09.2006 [23:11:25 -0700], Om Narasimhan wrote:
> This patch changes the kmalloc() calls followed by memset(,0,) to kzalloc.
> 
>     cciss.c : Changed the kmalloc/memset pair to kzalloc
>     cpqarray.c : km2zalloc conversion and code size reduction by
> changing multiple cleanup calls and returns of the function
> getgeometry() by adding a label.
>     loop.c : km2zalloc converion
> 
> 
> Signed off by Om Narasimhan <om.turyx@...il.com>

This is not the canonical format, per SubmittingPatches. It should be:

Signed-off-by: Random J Developer <random@...eloper.example.org>

>  drivers/block/cciss.c    |    4 +--
>  drivers/block/cpqarray.c |   72 +++++++++++++++-------------------------------
>  drivers/block/loop.c     |    4 +--
>  3 files changed, 25 insertions(+), 55 deletions(-)

Your diffstat should have indicated to you that this should be split up
better. Please (re-)read SubmittingPatches. *One* logical change per
patch, most importantly.

> 
> diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
> index 2cd3391..a800a69 100644
> --- a/drivers/block/cciss.c
> +++ b/drivers/block/cciss.c
> @@ -900,7 +900,7 @@ #if 0				/* 'buf_size' member is 16-bits
>  				return -EINVAL;
>  #endif
>  			if (iocommand.buf_size > 0) {
> -				buff = kmalloc(iocommand.buf_size, GFP_KERNEL);
> +				buff = kzalloc(iocommand.buf_size, GFP_KERNEL);
>  				if (buff == NULL)
>  					return -EFAULT;
>  			}
> @@ -911,8 +911,6 @@ #endif
>  					kfree(buff);
>  					return -EFAULT;
>  				}
> -			} else {
> -				memset(buff, 0, iocommand.buf_size);
>  			}
>  			if ((c = cmd_alloc(host, 0)) == NULL) {
>  				kfree(buff);

This changes performance potentially, no? The memset before was
conditional upon (iocommand.Request.Type.Direction == XFER_WRITE) and
now the memory will always be zero'd.

> diff --git a/drivers/block/cpqarray.c b/drivers/block/cpqarray.c
> index 78082ed..8a697c7 100644
> --- a/drivers/block/cpqarray.c
> +++ b/drivers/block/cpqarray.c
> @@ -1642,58 +1639,46 @@ static void start_fwbk(int ctlr)
>      It is used only at init time.
>  *****************************************************************/
>  static void getgeometry(int ctlr)
> -{				
> -	id_log_drv_t *id_ldrive;
> -	id_ctlr_t *id_ctlr_buf;
> -	sense_log_drv_stat_t *id_lstatus_buf;
> -	config_t *sense_config_buf;
> +{

Unrelated whitespace change.

> +	id_log_drv_t *id_ldrive = NULL;
> +	id_ctlr_t *id_ctlr_buf = NULL;
> +	sense_log_drv_stat_t *id_lstatus_buf = NULL;
> +	config_t *sense_config_buf = NULL;

Why initialize if you're going to immediately assign the return of
kzalloc()?

>  	unsigned int log_unit, log_index;
>  	int ret_code, size;
> -	drv_info_t *drv;
> +	drv_info_t *drv = NULL;

What does this do? Seems unnecessary and unrelated.

<snip>

> -		kfree(id_ctlr_buf);
> -		kfree(id_ldrive);
>  		printk( KERN_ERR "cpqarray:  out of memory.\n");
> -		return;
> +		goto end;

All of this rearrangement needs to be a separate patch.

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@...ibm.com>
IBM Linux Technology Center
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/