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

[Stanislaw Gruszka <sgruszka@redhat.com>]

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