Re: [PATCH] iwlwifi: fix possible data overwrite in hcmd callback

["Guy, Wey-Yi" <wey-yi.w.guy@intel.com>]

Hi Stanislaw,

On Wed, 2011-04-20 at 07:02 -0700, Stanislaw Gruszka wrote:
> My commit 3598e1774c94e55c71b585340e7dc4538f310e3f
> "iwlwifi: fix enqueue hcmd race conditions" move hcmd callback after
> command queue reclaim, to avoid call it with hcmd_lock. But since
> queue read index was updated, cmd data can be overwritten. Fix problem
> by calling callback before taking hcmd_lock and queue reclaim.
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  drivers/net/wireless/iwlwifi/iwl-tx.c |   15 ++++-----------
>  1 files changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/wireless/iwlwifi/iwl-tx.c b/drivers/net/wireless/iwlwifi/iwl-tx.c
> index 3732380..efff189 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-tx.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-tx.c
> @@ -607,9 +607,6 @@ void iwl_tx_cmd_complete(struct iwl_priv *priv, struct iwl_rx_mem_buffer *rxb)
>  	struct iwl_cmd_meta *meta;
>  	struct iwl_tx_queue *txq = &priv->txq[priv->cmd_queue];
>  	unsigned long flags;
> -	void (*callback) (struct iwl_priv *priv, struct iwl_device_cmd *cmd,
> -			  struct iwl_rx_packet *pkt);
> -
>  
>  	/* If a Tx command is being handled and it isn't in the actual
>  	 * command queue then there a command routing bug has been introduced
> @@ -623,8 +620,6 @@ void iwl_tx_cmd_complete(struct iwl_priv *priv, struct iwl_rx_mem_buffer *rxb)
>  		return;
>  	}
>  
> -	spin_lock_irqsave(&priv->hcmd_lock, flags);
> -
>  	cmd_index = get_cmd_index(&txq->q, index, huge);
>  	cmd = txq->cmd[cmd_index];
>  	meta = &txq->meta[cmd_index];
> @@ -634,13 +629,14 @@ void iwl_tx_cmd_complete(struct iwl_priv *priv, struct iwl_rx_mem_buffer *rxb)
>  			 dma_unmap_len(meta, len),
>  			 PCI_DMA_BIDIRECTIONAL);
>  
> -	callback = NULL;
>  	/* Input error checking is done when commands are added to queue. */
>  	if (meta->flags & CMD_WANT_SKB) {
>  		meta->source->reply_page = (unsigned long)rxb_addr(rxb);
>  		rxb->page = NULL;
> -	} else
> -		callback = meta->callback;
> +	} else if (meta->callback)
> +		meta->callback(priv, cmd, pkt);
> +
> +	spin_lock_irqsave(&priv->hcmd_lock, flags);
>  
>  	iwl_hcmd_queue_reclaim(priv, txq_id, index, cmd_index);
>  
> @@ -655,7 +651,4 @@ void iwl_tx_cmd_complete(struct iwl_priv *priv, struct iwl_rx_mem_buffer *rxb)
>  	meta->flags = 0;
>  
>  	spin_unlock_irqrestore(&priv->hcmd_lock, flags);
> -
> -	if (callback)
> -		callback(priv, cmd, pkt);
>  }

Could you elaborate a bit more, why you do not need to protect getting
the cmd index.

Wey