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

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

[Stanislaw Gruszka]

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);
 }
-- 
1.7.1

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

[Guy, Wey-Yi]

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

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

[Stanislaw Gruszka]

Hello

On Wed, Apr 20, 2011 at 07:05:48AM -0700, Guy, Wey-Yi wrote:
> > -	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.

get_cmd_index() is simple mathematical function of index local variable
(provided by firmware) and globally canst q->n_window, not need to be
protected.

What need to be protected is iwl_hcdm_queue_reclaim() as is touch
q->read_ptr and meta->flags to make assure is synchronized across
different cpus, when new huge command come instantly.

Note circular queue management could be done lock-less, but need
trickery described in Documentation/circular-buffers.txt to synchronize
q->read_ptr and q->write_ptr properly. What is probably too complex to
be worth to consider instead of simply using a spin lock.

Stanislaw

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

[wwguy]

On Thu, 2011-04-21 at 00:17 -0700, Stanislaw Gruszka wrote:

> > Could you elaborate a bit more, why you do not need to protect getting
> > the cmd index.
> 
> get_cmd_index() is simple mathematical function of index local variable
> (provided by firmware) and globally canst q->n_window, not need to be
> protected.
> 
> What need to be protected is iwl_hcdm_queue_reclaim() as is touch
> q->read_ptr and meta->flags to make assure is synchronized across
> different cpus, when new huge command come instantly.
> 
> Note circular queue management could be done lock-less, but need
> trickery described in Documentation/circular-buffers.txt to synchronize
> q->read_ptr and q->write_ptr properly. What is probably too complex to
> be worth to consider instead of simply using a spin lock.
> 
Thanks for the clear, explanation.

Acked-by: Wey-Yi Guy <wey-yi.w.guy@intel.com>

Wey