diff mbox

[RFC] solos-pci: Fix BUG() with shared skb

Message ID 1378223125.4210.11.camel@i7.infradead.org
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

David Woodhouse Sept. 3, 2013, 3:45 p.m. UTC 
Simon reported this BUG():

 kernel BUG at net/core/skbuff.c:1065!
 Call Trace:
  [<f9b7c12c>] ? pppoatm_send+0x3f/0x1a0 [pppoatm]
  [<f8751797>] psend+0xa9/0x14a [solos_pci]
  [<f9b7c248>] pppoatm_send+0x15b/0x1a0 [pppoatm]
  [<f8a2f77d>] ppp_push+0x76/0x533 [ppp_generic]

(Rest of backtrace at http://s85.org/mn0aOxMN — the skb appears to be
IPv6, forwarded from another interface over PPPoATM.)

I wasn't expecting to see shared skbs in the ATM driver's ->send()
function. Is this the right fix?

I've included the whole function in the patch context... I appear to be
jumping through a lot of hand-coded hoops here, just to ensure that I
can safely prepend my own device's hardware header to the skb. This
makes me think that I'm doing something severely wrong. There ought to
be an easier way of doing this, surely? What am I missing?

Comments

David Miller Sept. 4, 2013, 6:30 p.m. UTC | #1 
From: David Woodhouse <dwmw2@infradead.org>
Date: Tue, 03 Sep 2013 16:45:25 +0100

> Simon reported this BUG():
> 
>  kernel BUG at net/core/skbuff.c:1065!
>  Call Trace:
>   [<f9b7c12c>] ? pppoatm_send+0x3f/0x1a0 [pppoatm]
>   [<f8751797>] psend+0xa9/0x14a [solos_pci]
>   [<f9b7c248>] pppoatm_send+0x15b/0x1a0 [pppoatm]
>   [<f8a2f77d>] ppp_push+0x76/0x533 [ppp_generic]
> 
> (Rest of backtrace at http://s85.org/mn0aOxMN ― the skb appears to be
> IPv6, forwarded from another interface over PPPoATM.)
> 
> I wasn't expecting to see shared skbs in the ATM driver's ->send()
> function. Is this the right fix?

skb_realloc_headroom() should do everything you need.

This is what ethernet drivers do to prepend custom headers
when skb_headroom() is not large enough.

For example, see drivers/net/ethernet/sun/niu.c:niu_start_xmit().
There, the driver is attempting to prepend a TX descriptor to the
SKB.

If the SKB is shared, skb_realloc_headroom() will do the clone
for you if necessary.
--
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
diff mbox

Patch

diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
index 32784d1..b15e475 100644
--- a/drivers/atm/solos-pci.c
+++ b/drivers/atm/solos-pci.c
@@ -1135,40 +1135,50 @@  static uint32_t fpga_tx(struct solos_card *card)
 static int psend(struct atm_vcc *vcc, struct sk_buff *skb)
 {
 	struct solos_card *card = vcc->dev->dev_data;
 	struct pkt_hdr *header;
 	int pktlen;
 
 	pktlen = skb->len;
 	if (pktlen > (BUF_SIZE - sizeof(*header))) {
 		dev_warn(&card->dev->dev, "Length of PDU is too large. Dropping PDU.\n");
 		solos_pop(vcc, skb);
 		return 0;
 	}
 
+	/* This is skb_share_check() except it uses solos_pop() instead of kfree_skb() */
+	if (skb_shared(skb)) {
+		struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
+		if (unlikely(!nskb)) {
+			solos_pop(vcc, skb);
+			return 0;
+		}
+		consume_skb(skb);
+		skb = nskb;
+	}
 	if (!skb_clone_writable(skb, sizeof(*header))) {
 		int expand_by = 0;
 		int ret;
 
 		if (skb_headroom(skb) < sizeof(*header))
 			expand_by = sizeof(*header) - skb_headroom(skb);
 
 		ret = pskb_expand_head(skb, expand_by, 0, GFP_ATOMIC);
 		if (ret) {
 			dev_warn(&card->dev->dev, "pskb_expand_head failed.\n");
 			solos_pop(vcc, skb);
 			return ret;
 		}
 	}
 
 	header = (void *)skb_push(skb, sizeof(*header));
 
 	/* This does _not_ include the size of the header */
 	header->size = cpu_to_le16(pktlen);
 	header->vpi = cpu_to_le16(vcc->vpi);
 	header->vci = cpu_to_le16(vcc->vci);
 	header->type = cpu_to_le16(PKT_DATA);
 
 	fpga_queue(card, SOLOS_CHAN(vcc->dev), skb, vcc);
 
 	return 0;
 }