[PATCH V3 for 3.19] rtlwifi: Fix error when accessing unmapped memory in skb

[PATCH V3 for 3.19] rtlwifi: Fix error when accessing unmapped memory in skb

[Larry Finger]

These drivers use 9100-byte receive buffers, thus allocating an skb requires
an O(3) memory allocation. Under heavy memory loads and fragmentation, such
a request can fail. Previous versions of the driver have dropped the packet
and reused the old buffer; however, the new version introduced a bug in that
it released the old buffer before trying to allocate a new one. The previous
method is implemented here. The skb is unmapped before any attempt is made to
allocate another.

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Stable <stable@vger.kernel.org>  [v3.18]
Reported-by: Eric Biggers <ebiggers3@gmail.com>
Cc: Eric Biggers <ebiggers3@gmail.com>
---

V2 - Fixes an error in the logic of V1. Realtek is working on a change to
     the RX buffer allocation, but that is likely to be too invasive for
     a fix to -rc or stable. In the meantime, this will help.
v3 - Unmap skb before trying to allocate a new one so as to not leak mapping.

Larry
---

 drivers/net/wireless/rtlwifi/pci.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

Index: wireless-drivers/drivers/net/wireless/rtlwifi/pci.c
===================================================================
--- wireless-drivers.orig/drivers/net/wireless/rtlwifi/pci.c
+++ wireless-drivers/drivers/net/wireless/rtlwifi/pci.c
@@ -666,7 +666,8 @@ tx_status_ok:
 }
 
 static int _rtl_pci_init_one_rxdesc(struct ieee80211_hw *hw,
-				    u8 *entry, int rxring_idx, int desc_idx)
+				    struct sk_buff *new_skb, u8 *entry,
+				    int rxring_idx, int desc_idx)
 {
 	struct rtl_priv *rtlpriv = rtl_priv(hw);
 	struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
@@ -674,11 +675,15 @@ static int _rtl_pci_init_one_rxdesc(stru
 	u8 tmp_one = 1;
 	struct sk_buff *skb;
 
+	if (likely(new_skb)) {
+		skb = new_skb;
+		goto remap;
+	}
 	skb = dev_alloc_skb(rtlpci->rxbuffersize);
 	if (!skb)
 		return 0;
-	rtlpci->rx_ring[rxring_idx].rx_buf[desc_idx] = skb;
 
+remap:
 	/* just set skb->cb to mapping addr for pci_unmap_single use */
 	*((dma_addr_t *)skb->cb) =
 		pci_map_single(rtlpci->pdev, skb_tail_pointer(skb),
@@ -686,6 +691,7 @@ static int _rtl_pci_init_one_rxdesc(stru
 	bufferaddress = *((dma_addr_t *)skb->cb);
 	if (pci_dma_mapping_error(rtlpci->pdev, bufferaddress))
 		return 0;
+	rtlpci->rx_ring[rxring_idx].rx_buf[desc_idx] = skb;
 	if (rtlpriv->use_new_trx_flow) {
 		rtlpriv->cfg->ops->set_desc(hw, (u8 *)entry, false,
 					    HW_DESC_RX_PREPARE,
@@ -781,6 +787,7 @@ static void _rtl_pci_rx_interrupt(struct
 		/*rx pkt */
 		struct sk_buff *skb = rtlpci->rx_ring[rxring_idx].rx_buf[
 				      rtlpci->rx_ring[rxring_idx].idx];
+		struct sk_buff *new_skb;
 
 		if (rtlpriv->use_new_trx_flow) {
 			rx_remained_cnt =
@@ -807,6 +814,13 @@ static void _rtl_pci_rx_interrupt(struct
 		pci_unmap_single(rtlpci->pdev, *((dma_addr_t *)skb->cb),
 				 rtlpci->rxbuffersize, PCI_DMA_FROMDEVICE);
 
+		/* get a new skb - if fail, old one will be reused */
+		new_skb = dev_alloc_skb(rtlpci->rxbuffersize);
+		if (unlikely(!new_skb)) {
+			pr_err("Allocation of new skb failed in %s\n",
+			       __func__);
+			goto no_new;
+		}
 		if (rtlpriv->use_new_trx_flow) {
 			buffer_desc =
 			  &rtlpci->rx_ring[rxring_idx].buffer_desc
@@ -911,14 +925,16 @@ static void _rtl_pci_rx_interrupt(struct
 			schedule_work(&rtlpriv->works.lps_change_work);
 		}
 end:
+		skb = new_skb;
+no_new:
 		if (rtlpriv->use_new_trx_flow) {
-			_rtl_pci_init_one_rxdesc(hw, (u8 *)buffer_desc,
+			_rtl_pci_init_one_rxdesc(hw, skb, (u8 *)buffer_desc,
 						 rxring_idx,
-					       rtlpci->rx_ring[rxring_idx].idx);
+						 rtlpci->rx_ring[rxring_idx].idx);
 		} else {
-			_rtl_pci_init_one_rxdesc(hw, (u8 *)pdesc, rxring_idx,
+			_rtl_pci_init_one_rxdesc(hw, skb, (u8 *)pdesc,
+						 rxring_idx,
 						 rtlpci->rx_ring[rxring_idx].idx);
-
 			if (rtlpci->rx_ring[rxring_idx].idx ==
 			    rtlpci->rxringcount - 1)
 				rtlpriv->cfg->ops->set_desc(hw, (u8 *)pdesc,
@@ -1307,7 +1323,7 @@ static int _rtl_pci_init_rx_ring(struct
 		rtlpci->rx_ring[rxring_idx].idx = 0;
 		for (i = 0; i < rtlpci->rxringcount; i++) {
 			entry = &rtlpci->rx_ring[rxring_idx].buffer_desc[i];
-			if (!_rtl_pci_init_one_rxdesc(hw, (u8 *)entry,
+			if (!_rtl_pci_init_one_rxdesc(hw, NULL, (u8 *)entry,
 						      rxring_idx, i))
 				return -ENOMEM;
 		}
@@ -1332,7 +1348,7 @@ static int _rtl_pci_init_rx_ring(struct
 
 		for (i = 0; i < rtlpci->rxringcount; i++) {
 			entry = &rtlpci->rx_ring[rxring_idx].desc[i];
-			if (!_rtl_pci_init_one_rxdesc(hw, (u8 *)entry,
+			if (!_rtl_pci_init_one_rxdesc(hw, NULL, (u8 *)entry,
 						      rxring_idx, i))
 				return -ENOMEM;
 		}

Re: [PATCH V3 for 3.19] rtlwifi: Fix error when accessing unmapped memory in skb

[Eric Biggers]

On Tue, Dec 30, 2014 at 09:33:07PM -0600, Larry Finger wrote:
> v3 - Unmap skb before trying to allocate a new one so as to not leak mapping.

Looks good to me, although I'm not sure about the handling of DMA mapping errors
(perhaps that's something that drivers typically don't even try to handle?).
Anyway, the skb allocation issue appears to be resolved now.  I am running your
patch with an extra hack to inject some occasional skb allocation failures, and
I haven't noticed any problems except dropped packets.

Eric

Re: [PATCH V3 for 3.19] rtlwifi: Fix error when accessing unmapped memory in skb

[Larry Finger]

On 12/30/2014 11:07 PM, Eric Biggers wrote:
> On Tue, Dec 30, 2014 at 09:33:07PM -0600, Larry Finger wrote:
>> v3 - Unmap skb before trying to allocate a new one so as to not leak mapping.
>
> Looks good to me, although I'm not sure about the handling of DMA mapping errors
> (perhaps that's something that drivers typically don't even try to handle?).
> Anyway, the skb allocation issue appears to be resolved now.  I am running your
> patch with an extra hack to inject some occasional skb allocation failures, and
> I haven't noticed any problems except dropped packets.

The last time I saw any DMA mapping errors were for some early BCM43xx cards 
that only had 20 bits of DMA addressing space. These Realtek devices have a full 
32 bits of addressing, thus any physical address in the first 4GB of RAM will be 
OK. I suppose that it might be possible to get a physical address outside this 
range for machines with a lot of RAM, but they are unlikely to have wifi interfaces.

Thanks for the testing. The Realtek engineer told me that they are looking at 
this section, and may do a rewrite. I'm waiting to see what happens there before 
considering alternatives. If the number of packets dropped due to skb allocation 
failures is small, then the current code is likely OK.

Larry

Re: [PATCH V3 for 3.19] rtlwifi: Fix error when accessing unmapped memory in skb

[Kalle Valo]

Larry Finger <Larry.Finger@lwfinger.net> writes:

> These drivers use 9100-byte receive buffers, thus allocating an skb requires
> an O(3) memory allocation. Under heavy memory loads and fragmentation, such
> a request can fail. Previous versions of the driver have dropped the packet
> and reused the old buffer; however, the new version introduced a bug in that
> it released the old buffer before trying to allocate a new one. The previous
> method is implemented here. The skb is unmapped before any attempt is made to
> allocate another.
>
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: Stable <stable@vger.kernel.org>  [v3.18]
> Reported-by: Eric Biggers <ebiggers3@gmail.com>
> Cc: Eric Biggers <ebiggers3@gmail.com>

Thanks, applied to wireless-drivers.git.

-- 
Kalle Valo

Re: [PATCH V3 for 3.19] rtlwifi: Fix error when accessing unmapped memory in skb

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1407 bytes --]

On Wed, 2014-12-31 at 15:10 -0600, Larry Finger wrote:
> On 12/30/2014 11:07 PM, Eric Biggers wrote:
> > On Tue, Dec 30, 2014 at 09:33:07PM -0600, Larry Finger wrote:
> >> v3 - Unmap skb before trying to allocate a new one so as to not leak mapping.
> >
> > Looks good to me, although I'm not sure about the handling of DMA mapping errors
> > (perhaps that's something that drivers typically don't even try to handle?).
> > Anyway, the skb allocation issue appears to be resolved now.  I am running your
> > patch with an extra hack to inject some occasional skb allocation failures, and
> > I haven't noticed any problems except dropped packets.
> 
> The last time I saw any DMA mapping errors were for some early BCM43xx cards 
> that only had 20 bits of DMA addressing space. These Realtek devices have a full 
> 32 bits of addressing, thus any physical address in the first 4GB of RAM will be 
> OK. I suppose that it might be possible to get a physical address outside this 
> range for machines with a lot of RAM, but they are unlikely to have wifi interfaces.
[...]

Really, you don't think laptops and desktops come with 4GB RAM or more?
Besides that, DMA mapping may involve calling an IOMMU driver that may
fail for some reason.

Drivers need to check for DMA mapping errors, it is not optional.

Ben.

-- 
Ben Hutchings
The world is coming to an end.	Please log off.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]