* [PATCH RFC] solos-pci: Fix BUG() with shared skb
@ 2013-09-03 15:45 David Woodhouse
2013-09-04 18:30 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: David Woodhouse @ 2013-09-03 15:45 UTC (permalink / raw)
To: netdev; +Cc: Simon Arlott, linux-atm-general, nathan
[-- Attachment #1: Type: text/plain, Size: 2625 bytes --]
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
David.Woodhouse@intel.com Intel Corporation
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] solos-pci: Fix BUG() with shared skb
2013-09-03 15:45 [PATCH RFC] solos-pci: Fix BUG() with shared skb David Woodhouse
@ 2013-09-04 18:30 ` David Miller
2013-09-04 20:41 ` David Woodhouse
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2013-09-04 18:30 UTC (permalink / raw)
To: dwmw2; +Cc: netdev, simon, linux-atm-general, nathan
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] solos-pci: Fix BUG() with shared skb
2013-09-04 18:30 ` David Miller
@ 2013-09-04 20:41 ` David Woodhouse
2013-09-04 21:51 ` David Miller
2015-09-15 19:10 ` David Woodhouse
0 siblings, 2 replies; 6+ messages in thread
From: David Woodhouse @ 2013-09-04 20:41 UTC (permalink / raw)
To: David Miller; +Cc: netdev, simon, linux-atm-general, nathan
[-- Attachment #1: Type: text/plain, Size: 1591 bytes --]
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
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] solos-pci: Fix BUG() with shared skb
2013-09-04 20:41 ` David Woodhouse
@ 2013-09-04 21:51 ` David Miller
2015-09-15 19:10 ` David Woodhouse
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2013-09-04 21:51 UTC (permalink / raw)
To: dwmw2; +Cc: netdev, simon, linux-atm-general, nathan
From: David Woodhouse <dwmw2@infradead.org>
Date: Wed, 04 Sep 2013 21:41:15 +0100
> 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...
...
> + 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);
My understanding is that truesize will not be changed by calls to
skb_realloc_headroom(), because if it did then every ethernet driver
would be screwed as the skb->truesize adjustment would corrupt socket
memory accounting.
The only thing you need to be mindful is that after the
skb_realloc_headroom() call all skb data pointers change, and
therefore things like packet pointers are now stale and need to be
recalculated.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] solos-pci: Fix BUG() with shared skb
2013-09-04 20:41 ` David Woodhouse
2013-09-04 21:51 ` David Miller
@ 2015-09-15 19:10 ` David Woodhouse
2015-09-16 11:32 ` Simon Arlott
1 sibling, 1 reply; 6+ messages in thread
From: David Woodhouse @ 2015-09-15 19:10 UTC (permalink / raw)
To: simon; +Cc: netdev, linux-atm-general
[-- Attachment #1: Type: text/plain, Size: 1840 bytes --]
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
David.Woodhouse@intel.com Intel Corporation
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC] solos-pci: Fix BUG() with shared skb
2015-09-15 19:10 ` David Woodhouse
@ 2015-09-16 11:32 ` Simon Arlott
0 siblings, 0 replies; 6+ messages in thread
From: Simon Arlott @ 2015-09-16 11:32 UTC (permalink / raw)
To: David Woodhouse; +Cc: netdev, linux-atm-general
On Tue, September 15, 2015 20:10, David Woodhouse wrote:
> On Wed, 2013-09-04 at 21:41 +0100, David Woodhouse wrote:
>> +++ b/drivers/atm/solos-pci.c
>> @@ -1145,19 +1145,19 @@ static int psend(struct atm_vcc *vcc, struct sk_buff *skb)
>> +> > 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;
>> > > }
>
> 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...?
I'm currently using this but without the call to atm_force_charge().
I don't know how to reproduce the BUG() but it hasn't happened again.
--
Simon Arlott
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-09-16 11:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-03 15:45 [PATCH RFC] solos-pci: Fix BUG() with shared skb David Woodhouse
2013-09-04 18:30 ` David Miller
2013-09-04 20:41 ` David Woodhouse
2013-09-04 21:51 ` David Miller
2015-09-15 19:10 ` David Woodhouse
2015-09-16 11:32 ` Simon Arlott
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).