netdev - Re: [PATCH 3/3] net: stmmac: Prefer kcalloc() over kmalloc

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] [day] [month] [year] [list]

Message-ID: <f39eac0b-9859-dc07-979f-9571f3f64dc1@axis.com>
Date:   Wed, 29 Mar 2017 10:51:06 +0200
From:   Niklas Cassel <niklas.cassel@...s.com>
To:     Thierry Reding <thierry.reding@...il.com>
CC:     Joao Pinto <Joao.Pinto@...opsys.com>,
        David Miller <davem@...emloft.net>,
        <clabbe.montjoie@...il.com>, <peppe.cavallaro@...com>,
        <alexandre.torgue@...com>, <sergei.shtylyov@...entembedded.com>,
        <f.fainelli@...il.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH 3/3] net: stmmac: Prefer kcalloc() over kmalloc_array()

(resending mail without SPAM header)

Hi Thierry

Sorry that I missed your previous email,
for some reason it got stuck in the spam filter.

Really good catch. This patch fixes the random
RX brokenness for me. Thanks!

It would be nice with a Fixes tag though,
but lately there's been so many changes,
so it might be hard to point to a single commit.

Nevertheless:

Tested-by: Niklas Cassel <niklas.cassel@...s.com>

On 03/28/2017 03:57 PM, Thierry Reding wrote:
> From: Thierry Reding <treding@...dia.com>
> 
> Some of the data in the new queue structures seems to not be properly
> initialized, causing undefined behaviour (networking will work about 2
> out of 10 tries). kcalloc() will zero the allocated memory and results
> in 10 out of 10 successful boots.
> 
> Signed-off-by: Thierry Reding <treding@...dia.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 37 +++++++++++------------
>  1 file changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ec5bba85c529..845320bc3333 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1412,13 +1412,12 @@ static void free_dma_desc_resources(struct stmmac_priv *priv)
>  static int alloc_rx_dma_desc_resources(struct stmmac_priv *priv)
>  {
>  	u32 rx_count = priv->plat->rx_queues_to_use;
> +	struct stmmac_rx_queue *rx_q;
>  	int ret = -ENOMEM;
>  	u32 queue = 0;
>  
>  	/* Allocate RX queues array */
> -	priv->rx_queue = kmalloc_array(rx_count,
> -				       sizeof(struct stmmac_rx_queue),
> -				       GFP_KERNEL);
> +	priv->rx_queue = kcalloc(rx_count, sizeof(*rx_q), GFP_KERNEL);
>  	if (!priv->rx_queue) {
>  		kfree(priv->rx_queue);
>  		return -ENOMEM;
> @@ -1426,20 +1425,19 @@ static int alloc_rx_dma_desc_resources(struct stmmac_priv *priv)
>  
>  	/* RX queues buffers and DMA */
>  	for (queue = 0; queue < rx_count; queue++) {
> -		struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
> +		rx_q = &priv->rx_queue[queue];
>  
>  		rx_q->queue_index = queue;
>  		rx_q->priv_data = priv;
>  
> -		rx_q->rx_skbuff_dma = kmalloc_array(DMA_RX_SIZE,
> -							sizeof(dma_addr_t),
> -							GFP_KERNEL);
> +		rx_q->rx_skbuff_dma = kcalloc(DMA_RX_SIZE, sizeof(dma_addr_t),
> +					      GFP_KERNEL);
>  		if (!rx_q->rx_skbuff_dma)
>  			goto err_dma_buffers;
>  
> -		rx_q->rx_skbuff = kmalloc_array(DMA_RX_SIZE,
> -						    sizeof(struct sk_buff *),
> -						    GFP_KERNEL);
> +		rx_q->rx_skbuff = kcalloc(DMA_RX_SIZE,
> +					  sizeof(struct sk_buff *),
> +					  GFP_KERNEL);
>  		if (!rx_q->rx_skbuff)
>  			goto err_dma_buffers;
>  
> @@ -1477,33 +1475,32 @@ static int alloc_rx_dma_desc_resources(struct stmmac_priv *priv)
>  static int alloc_tx_dma_desc_resources(struct stmmac_priv *priv)
>  {
>  	u32 tx_count = priv->plat->tx_queues_to_use;
> +	struct stmmac_tx_queue *tx_q;
>  	int ret = -ENOMEM;
>  	u32 queue = 0;
>  
>  	/* Allocate TX queues array */
> -	priv->tx_queue = kmalloc_array(tx_count,
> -				       sizeof(struct stmmac_tx_queue),
> -				       GFP_KERNEL);
> +	priv->tx_queue = kcalloc(tx_count, sizeof(*tx_q), GFP_KERNEL);
>  	if (!priv->tx_queue)
>  		return -ENOMEM;
>  
>  	/* TX queues buffers and DMA */
>  	for (queue = 0; queue < tx_count; queue++) {
> -		struct stmmac_tx_queue *tx_q = &priv->tx_queue[queue];
> +		tx_q = &priv->tx_queue[queue];
>  
>  		tx_q->queue_index = queue;
>  		tx_q->priv_data = priv;
>  
> -		tx_q->tx_skbuff_dma = kmalloc_array(DMA_TX_SIZE,
> -					  sizeof(struct stmmac_tx_info),
> -					  GFP_KERNEL);
> +		tx_q->tx_skbuff_dma = kcalloc(DMA_TX_SIZE,
> +					      sizeof(struct stmmac_tx_info),
> +					      GFP_KERNEL);
>  
>  		if (!tx_q->tx_skbuff_dma)
>  			goto err_dma_buffers;
>  
> -		tx_q->tx_skbuff = kmalloc_array(DMA_TX_SIZE,
> -						    sizeof(struct sk_buff *),
> -						    GFP_KERNEL);
> +		tx_q->tx_skbuff = kcalloc(DMA_TX_SIZE,
> +					  sizeof(struct sk_buff *),
> +					  GFP_KERNEL);
>  		if (!tx_q->tx_skbuff)
>  			goto err_dma_buffers;
>  
>