2012-05-03 21:16:13

by Wey-Yi Guy

Subject: [PATCH 3.4] iwlwifi: fix a potential race in receive buffer allocation

From: Meenakshi Venkataraman <[email protected]>

The driver can potentially unmap pages that
have not been mapped yet. Fix this race
condition.

Cc: [email protected]
Reported-by: Emmanuel Grumbach <[email protected]>
Signed-off-by: Meenakshi Venkataraman <[email protected]>
Signed-off-by: Wey-Yi Guy <[email protected]>
---
this patch will be also available from wireless branch on
git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi.git

drivers/net/wireless/iwlwifi/iwl-trans-pcie-rx.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl-trans-pcie-rx.c b/drivers/net/wireless/iwlwifi/iwl-trans-pcie-rx.c
index aa7aea1..173275f 100644
--- a/drivers/net/wireless/iwlwifi/iwl-trans-pcie-rx.c
+++ b/drivers/net/wireless/iwlwifi/iwl-trans-pcie-rx.c
@@ -310,7 +310,6 @@ static void iwlagn_rx_allocate(struct iwl_trans *trans, gfp_t priority)
spin_unlock_irqrestore(&rxq->lock, flags);

BUG_ON(rxb->page);
- rxb->page = page;
/* Get physical address of the RB */
rxb->page_dma = dma_map_page(trans->dev, page, 0,
PAGE_SIZE << hw_params(trans).rx_page_order,
@@ -320,6 +319,9 @@ static void iwlagn_rx_allocate(struct iwl_trans *trans, gfp_t priority)
/* and also 256 byte aligned! */
BUG_ON(rxb->page_dma & DMA_BIT_MASK(8));

+ /* Page *must* be mapped before before updating the rxb. */
+ rxb->page = page;
+
spin_lock_irqsave(&rxq->lock, flags);

list_add_tail(&rxb->list, &rxq->rx_free);
--
1.7.0.4

2012-05-08 18:34:07

by Venkataraman, Meenakshi

Subject: RE: [PATCH 3.4] iwlwifi: fix a potential race in receive buffer allocation

Hi guys,

>> This patch make no sense. Nothing stops compiler or CPU to write
>> ->page into memory before ->page_dma .
>>
>
>I thought that too,but the call to dma_map... is a call to functon.
>Shouldn't force the compiler the make things in order. Or maybe the
>assignment to rxb->page_dma can be reordered... I am not really sure.

[MV] In that case I'll explicitly add barriers here and elsewhere...there is a race condition here and it needs to be resolved.

Thanks,
Meenakshi

2012-05-08 17:00:30

by Emmanuel Grumbach

Subject: Re: [PATCH 3.4] iwlwifi: fix a potential race in receive buffer allocation

>
> This patch make no sense. Nothing stops compiler or CPU to write
> ->page into memory before ->page_dma .
>

I thought that too,but the call to dma_map... is a call to functon.
Shouldn't force the compiler the make things in order. Or maybe the
assignment to rxb->page_dma can be reordered... I am not really sure.

2012-05-09 07:53:35

by Stanislaw Gruszka

Subject: Re: [PATCH 3.4] iwlwifi: fix a potential race in receive buffer allocation

On Tue, May 08, 2012 at 08:00:29PM +0300, Emmanuel Grumbach wrote:
> >
> > This patch make no sense. Nothing stops compiler or CPU to write
> > ->page into memory before ->page_dma .
> >
>
> I thought that too,but the call to dma_map... is a call to functon.
> Shouldn't force the compiler the make things in order. Or maybe the
> assignment to rxb->page_dma can be reordered... I am not really sure.

Calling a non lock/unlock function does not make any guarantees about
ordering.

Stanislaw

2012-05-08 14:05:53

by Stanislaw Gruszka

Subject: Re: [PATCH 3.4] iwlwifi: fix a potential race in receive buffer allocation

On Thu, May 03, 2012 at 02:09:24PM -0700, Wey-Yi Guy wrote:
> From: Meenakshi Venkataraman <[email protected]>
>
> The driver can potentially unmap pages that
> have not been mapped yet. Fix this race
> condition.
>
> Cc: [email protected]
> Reported-by: Emmanuel Grumbach <[email protected]>
> Signed-off-by: Meenakshi Venkataraman <[email protected]>
> Signed-off-by: Wey-Yi Guy <[email protected]>
> ---
> this patch will be also available from wireless branch on
> git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi.git
>
> drivers/net/wireless/iwlwifi/iwl-trans-pcie-rx.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wireless/iwlwifi/iwl-trans-pcie-rx.c b/drivers/net/wireless/iwlwifi/iwl-trans-pcie-rx.c
> index aa7aea1..173275f 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-trans-pcie-rx.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-trans-pcie-rx.c
> @@ -310,7 +310,6 @@ static void iwlagn_rx_allocate(struct iwl_trans *trans, gfp_t priority)
> spin_unlock_irqrestore(&rxq->lock, flags);
>
> BUG_ON(rxb->page);
> - rxb->page = page;
> /* Get physical address of the RB */
> rxb->page_dma = dma_map_page(trans->dev, page, 0,
> PAGE_SIZE << hw_params(trans).rx_page_order,
> @@ -320,6 +319,9 @@ static void iwlagn_rx_allocate(struct iwl_trans *trans, gfp_t priority)
> /* and also 256 byte aligned! */
> BUG_ON(rxb->page_dma & DMA_BIT_MASK(8));
>
> + /* Page *must* be mapped before before updating the rxb. */
> + rxb->page = page;
> +
> spin_lock_irqsave(&rxq->lock, flags);
>
> list_add_tail(&rxb->list, &rxq->rx_free);

This patch make no sense. Nothing stops compiler or CPU to write
->page into memory before ->page_dma .

Stanislaw