From: David W. <dw...@in...> - 2013-09-03 16:57:03
Attachments:
smime.p7s
|
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? 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; } -- David Woodhouse Open Source Technology Centre Dav...@in... Intel Corporation |
From: David W. <dw...@in...> - 2013-09-04 20:41:38
Attachments:
smime.p7s
|
On Wed, 2013-09-04 at 14:30 -0400, David Miller wrote: > skb_realloc_headroom() should do everything you need. Great, thanks! Something like this then... ? Do I really need the truesize check? And if so, is there a better way to handle the ATM accounting? It just *happens* to be the case that the the br2684_pop() and pppoatm_pop() functions don't mind being bypassed in this fashion, and we should probably get away with tweaking the core ATM accounting directly like this. Doesn't make me happy though... diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c index 32784d1..4492c0f 100644 --- a/drivers/atm/solos-pci.c +++ b/drivers/atm/solos-pci.c @@ -1145,19 +1145,19 @@ static int psend(struct atm_vcc *vcc, struct sk_buff *skb) return 0; } - 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; - } + if (skb_headroom(skb) < sizeof(*header)) { + struct sk_buff *nskb; + + nskb = skb_realloc_headroom(skb, sizeof(*header)); + if (!nskb) { + solos_pop(vcc, skb); + return -ENOMEM; + } + if (skb->truesize != nskb->truesize) + atm_force_charge(vcc, nskb->truesize - skb->truesize); + + dev_kfree_skb_any(skb); + skb = nskb; } header = (void *)skb_push(skb, sizeof(*header)); -- dwmw2 |
From: David W. <dw...@in...> - 2015-09-15 19:11:00
Attachments:
smime.p7s
|
On Wed, 2013-09-04 at 21:41 +0100, David Woodhouse wrote: > On Wed, 2013-09-04 at 14:30 -0400, David Miller wrote: > > skb_realloc_headroom() should do everything you need. > > Great, thanks! Something like this then... ? > > diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c > index 32784d1..4492c0f 100644 > --- a/drivers/atm/solos-pci.c > +++ b/drivers/atm/solos-pci.c > @@ -1145,19 +1145,19 @@ static int psend(struct atm_vcc *vcc, struct sk_buff *skb) > > > > return 0; > > > } > > -> > 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; > -> > > } > +> > if (skb_headroom(skb) < sizeof(*header)) { > +> > > struct sk_buff *nskb; > + > +> > > nskb = skb_realloc_headroom(skb, sizeof(*header)); > +> > > if (!nskb) { > +> > > > solos_pop(vcc, skb); > +> > > > return -ENOMEM; > +> > > } > +> > > if (skb->truesize != nskb->truesize) > +> > > > atm_force_charge(vcc, nskb->truesize - skb->truesize); > + > +> > > dev_kfree_skb_any(skb); > +> > > skb = nskb; > > > } > > > > header = (void *)skb_push(skb, sizeof(*header)); Simon, did you ever test this? Can you still (tell me how to) reproduce the original problem? I think that sending on br2684 was necessary but not sufficient...? -- David Woodhouse Open Source Technology Centre Dav...@in... Intel Corporation |