Patchwork [net,v2] 8139cp: Add dma_mapping_error checking

login
register
mail settings
Submitter Neil Horman
Date July 26, 2013, 2:28 p.m.
Message ID <1374848918-17183-1-git-send-email-nhorman@tuxdriver.com>
Download mbox | patch
Permalink /patch/262164/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Neil Horman - July 26, 2013, 2:28 p.m.
Self explanitory dma_mapping_error addition to the 8139 driver, based on this:
https://bugzilla.redhat.com/show_bug.cgi?id=947250

It showed several backtraces arising for dma_map_* usage without checking the
return code on the mapping.  Add the check and abort the rx/tx operation if its
failed.  Untested as I have no hardware and the reporter has wandered off, but
seems pretty straightforward.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: Francois Romieu <romieu@fr.zoreil.com>

---
Change notes:

v2) Added stats update for tx_dropped as per Francois Romieu
---
 drivers/net/ethernet/realtek/8139cp.c | 37 +++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
françois romieu - July 26, 2013, 8:06 p.m.
Neil Horman <nhorman@tuxdriver.com> :
[...]
> diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
> index 0352345..ba0570a 100644
> --- a/drivers/net/ethernet/realtek/8139cp.c
> +++ b/drivers/net/ethernet/realtek/8139cp.c
> @@ -533,6 +533,11 @@ rx_status_loop:
>  
>  		mapping = dma_map_single(&cp->pdev->dev, new_skb->data, buflen,
>  					 PCI_DMA_FROMDEVICE);
> +		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> +			kfree_skb(skb);

			dev->stats.rx_dropped++;

Sorry, I did not notice that skb contained newly received data :o/

> +			break;
> +		}
> +
[...]
> @@ -749,6 +754,12 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
>  
>  		len = skb->len;
>  		mapping = dma_map_single(&cp->pdev->dev, skb->data, len, PCI_DMA_TODEVICE);
> +		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> +			kfree_skb(skb);
> +			cp->dev->stats.tx_dropped++;
> +			goto out_unlock;

These lines appear several times. You may factor them out with a
goto out_drop or such.

> @@ -799,6 +816,21 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
>  			mapping = dma_map_single(&cp->pdev->dev,
>  						 skb_frag_address(this_frag),
>  						 len, PCI_DMA_TODEVICE);
> +			if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> +				/* Unwind the mappnigs we have */
                                                  ^^

> +				for (frag = 0; frag+first_entry < entry; frag++) {
> +					cp->tx_skb[frag+first_entry] = NULL;
> +					txd = &cp->tx_ring[frag+first_entry];
> +					this_frag = &skb_shinfo(skb)->frags[frag];
> +					dma_unmap_single(&cp->pdev->dev, le64_to_cpu(txd->addr),
> +							 skb_frag_size(this_frag), PCI_DMA_TODEVICE);
> +				}

Imho, even with local &cp->pdev->dev and removal of whitespaces, it
calls for a separate xmit_frag helper or an out of line unwinder.
Neil Horman - July 26, 2013, 8:42 p.m.
On Fri, Jul 26, 2013 at 10:06:30PM +0200, Francois Romieu wrote:
> Neil Horman <nhorman@tuxdriver.com> :
> [...]
> > diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
> > index 0352345..ba0570a 100644
> > --- a/drivers/net/ethernet/realtek/8139cp.c
> > +++ b/drivers/net/ethernet/realtek/8139cp.c
> > @@ -533,6 +533,11 @@ rx_status_loop:
> >  
> >  		mapping = dma_map_single(&cp->pdev->dev, new_skb->data, buflen,
> >  					 PCI_DMA_FROMDEVICE);
> > +		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> > +			kfree_skb(skb);
> 
> 			dev->stats.rx_dropped++;
> 
> Sorry, I did not notice that skb contained newly received data :o/
> 
Yeah, this isn't needed.  The skb being mapped in is newly allocated, and
contains no data.  We haven't dropped anything here, so theres no need for the
stats bump.

> > +			break;
> > +		}
> > +
> [...]
> > @@ -749,6 +754,12 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
> >  
> >  		len = skb->len;
> >  		mapping = dma_map_single(&cp->pdev->dev, skb->data, len, PCI_DMA_TODEVICE);
> > +		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> > +			kfree_skb(skb);
> > +			cp->dev->stats.tx_dropped++;
> > +			goto out_unlock;
> 
> These lines appear several times. You may factor them out with a
> goto out_drop or such.
> 
I may, I'm not sure its worth it.  If the item below results in a respin, then
I'l work this in as well.

> > @@ -799,6 +816,21 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
> >  			mapping = dma_map_single(&cp->pdev->dev,
> >  						 skb_frag_address(this_frag),
> >  						 len, PCI_DMA_TODEVICE);
> > +			if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> > +				/* Unwind the mappnigs we have */
>                                                   ^^
> 
> > +				for (frag = 0; frag+first_entry < entry; frag++) {
> > +					cp->tx_skb[frag+first_entry] = NULL;
> > +					txd = &cp->tx_ring[frag+first_entry];
> > +					this_frag = &skb_shinfo(skb)->frags[frag];
> > +					dma_unmap_single(&cp->pdev->dev, le64_to_cpu(txd->addr),
> > +							 skb_frag_size(this_frag), PCI_DMA_TODEVICE);
> > +				}
> 
> Imho, even with local &cp->pdev->dev and removal of whitespaces, it
> calls for a separate xmit_frag helper or an out of line unwinder.
> 
I'm not 100% sure its actually going to be more readable with an extra function
there.  We still have to pass in all the start points and descriptor counts, do
the machinations of iterating over two separately biased lists, and undo what
was done in this function.  I honestly kind of like it better in line, as you
can see where the values originated from, and the mapping/unmapping looks
balanced.

Thoughts?
Neil

> -- 
> Ueimor
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
françois romieu - July 26, 2013, 9:24 p.m.
Neil Horman <nhorman@tuxdriver.com> :
> On Fri, Jul 26, 2013 at 10:06:30PM +0200, Francois Romieu wrote:
> > Neil Horman <nhorman@tuxdriver.com> :
[...]
> > > @@ -533,6 +533,11 @@ rx_status_loop:
> > >  
> > >  		mapping = dma_map_single(&cp->pdev->dev, new_skb->data, buflen,
> > >  					 PCI_DMA_FROMDEVICE);
> > > +		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> > > +			kfree_skb(skb);
> > 
> > 			dev->stats.rx_dropped++;
> > 
> > Sorry, I did not notice that skb contained newly received data :o/
> > 
> Yeah, this isn't needed.  The skb being mapped in is newly allocated, and
> contains no data.  We haven't dropped anything here, so theres no need for the
> stats bump.

Huh ?

[...error path...]
		kfree(skb);
[...normal path...]
	cp_rx_skb(cp, skb, desc);

Afaiks it's the same skb. We drop data and a stats bump is needed.

[...]
> Thoughts?

Nevermind. cp_start_xmit is a piece of it and it isn't your work to
turn it into something sensible.
Neil Horman - July 28, 2013, 10:40 a.m.
On Fri, Jul 26, 2013 at 11:24:03PM +0200, Francois Romieu wrote:
> Neil Horman <nhorman@tuxdriver.com> :
> > On Fri, Jul 26, 2013 at 10:06:30PM +0200, Francois Romieu wrote:
> > > Neil Horman <nhorman@tuxdriver.com> :
> [...]
> > > > @@ -533,6 +533,11 @@ rx_status_loop:
> > > >  
> > > >  		mapping = dma_map_single(&cp->pdev->dev, new_skb->data, buflen,
> > > >  					 PCI_DMA_FROMDEVICE);
> > > > +		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
> > > > +			kfree_skb(skb);
> > > 
> > > 			dev->stats.rx_dropped++;
> > > 
> > > Sorry, I did not notice that skb contained newly received data :o/
> > > 
> > Yeah, this isn't needed.  The skb being mapped in is newly allocated, and
> > contains no data.  We haven't dropped anything here, so theres no need for the
> > stats bump.
> 
> Huh ?
> 
> [...error path...]
> 		kfree(skb);
> [...normal path...]
> 	cp_rx_skb(cp, skb, desc);
> 
> Afaiks it's the same skb. We drop data and a stats bump is needed.
> 
> [...]
> > Thoughts?
> 
> Nevermind. cp_start_xmit is a piece of it and it isn't your work to
> turn it into something sensible.
> 
So you agree with the patch as it stands?
Neil

> -- 
> Ueimor
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
françois romieu - July 28, 2013, 9:34 p.m.
Neil Horman <nhorman@tuxdriver.com> :
[...]
> So you agree with the patch as it stands?

This part, yes.

However I still see a need for a rx_dropped increase.
Neil Horman - July 28, 2013, 11:34 p.m.
On Sun, Jul 28, 2013 at 11:34:19PM +0200, Francois Romieu wrote:
> Neil Horman <nhorman@tuxdriver.com> :
> [...]
> > So you agree with the patch as it stands?
> 
> This part, yes.
> 
> However I still see a need for a rx_dropped increase.
> 
Very well, I'll take another look tomorrow morning
Neil

> -- 
> Ueimor
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 0352345..ba0570a 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -533,6 +533,11 @@  rx_status_loop:
 
 		mapping = dma_map_single(&cp->pdev->dev, new_skb->data, buflen,
 					 PCI_DMA_FROMDEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
+			kfree_skb(skb);
+			break;
+		}
+
 		cp->rx_skb[rx_tail] = new_skb;
 
 		cp_rx_skb(cp, skb, desc);
@@ -749,6 +754,12 @@  static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 
 		len = skb->len;
 		mapping = dma_map_single(&cp->pdev->dev, skb->data, len, PCI_DMA_TODEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
+			kfree_skb(skb);
+			cp->dev->stats.tx_dropped++;
+			goto out_unlock;
+		}
+
 		txd->opts2 = opts2;
 		txd->addr = cpu_to_le64(mapping);
 		wmb();
@@ -786,6 +797,12 @@  static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		first_len = skb_headlen(skb);
 		first_mapping = dma_map_single(&cp->pdev->dev, skb->data,
 					       first_len, PCI_DMA_TODEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, first_mapping)) {
+			kfree_skb(skb);
+			cp->dev->stats.tx_dropped++;
+			goto out_unlock;
+		}
+
 		cp->tx_skb[entry] = skb;
 		entry = NEXT_TX(entry);
 
@@ -799,6 +816,21 @@  static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 			mapping = dma_map_single(&cp->pdev->dev,
 						 skb_frag_address(this_frag),
 						 len, PCI_DMA_TODEVICE);
+			if (dma_mapping_error(&cp->pdev->dev, mapping)) {
+				/* Unwind the mappnigs we have */
+				for (frag = 0; frag+first_entry < entry; frag++) {
+					cp->tx_skb[frag+first_entry] = NULL;
+					txd = &cp->tx_ring[frag+first_entry];
+					this_frag = &skb_shinfo(skb)->frags[frag];
+					dma_unmap_single(&cp->pdev->dev, le64_to_cpu(txd->addr),
+							 skb_frag_size(this_frag), PCI_DMA_TODEVICE);
+				}
+
+				cp->dev->stats.tx_dropped++;
+				kfree_skb(skb);
+				goto out_unlock;
+			}
+
 			eor = (entry == (CP_TX_RING_SIZE - 1)) ? RingEnd : 0;
 
 			ctrl = eor | len | DescOwn;
@@ -859,6 +891,7 @@  static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 	if (TX_BUFFS_AVAIL(cp) <= (MAX_SKB_FRAGS + 1))
 		netif_stop_queue(dev);
 
+out_unlock:
 	spin_unlock_irqrestore(&cp->lock, intr_flags);
 
 	cpw8(TxPoll, NormalTxPoll);
@@ -1054,6 +1087,10 @@  static int cp_refill_rx(struct cp_private *cp)
 
 		mapping = dma_map_single(&cp->pdev->dev, skb->data,
 					 cp->rx_buf_sz, PCI_DMA_FROMDEVICE);
+		if (dma_mapping_error(&cp->pdev->dev, mapping)) {
+			kfree_skb(skb);
+			goto err_out;
+		}
 		cp->rx_skb[i] = skb;
 
 		cp->rx_ring[i].opts2 = 0;