can: bcm: add skb destructor

PATCH In review Oliver Hartkopp 4 participants 10 messages Jan 28, 2014 – Jan 29, 2014

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Oliver Hartkopp

author

2014-01-28 20:42 UTC #0 lore raw

PATCH 1/1 can: bcm: add skb destructor

Commit 376c7311bdb6 (net: add a temporary sanity check in skb_orphan()) leads to a BUG in can_put_echo_skb() when skb_orphan() is executed. When skbuffs created automatically in bcm_can_tx() in softirq (netrx, timer) and userspace context the precise timing has to be met. A sock wmem accouting is pointless for this use case.

This patch introduces an empty skb destructor like in commit 072017b41e49 (net: sctp: Add rudimentary infrastructure to account for control chunks) to make the cyclic transmission of CAN frames work again on real CAN netdevices. Virtual CAN interfaces do not need skb_orphan().

Signed-off-by Oliver Hartkopp <[email protected]>

---

net/can/bcm.c +6 -0

index 3fc737b..82af1a5 100644
@@ -237,6 +237,11 @@ static const struct file_operations bcm_proc_fops = {
 	.release	= single_release,
 };
 
+static void bcm_skb_destructor(struct sk_buff *skb)
+{
+	/* no accounting needed for bcm_can_tx() */
+}
+
 /*
  * bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
  *              of the given bcm tx op

@@ -267,6 +272,7 @@ static void bcm_can_tx(struct bcm_op *op)
 	memcpy(skb_put(skb, CFSIZ), cf, CFSIZ);
 
 	/* send with loopback */
+	skb->destructor = bcm_skb_destructor;
 	skb->dev = dev;
 	skb->sk = op->sk;
 	can_send(skb, 1);

Eric Dumazet

2014-01-28 22:28 UTC #1 lore raw

Quoted 33 lines · Oliver Hartkopp

Commit 376c7311bdb6 (net: add a temporary sanity check in skb_orphan()) leads to a BUG in can_put_echo_skb() when skb_orphan() is executed. When skbuffs created automatically in bcm_can_tx() in softirq (netrx, timer) and userspace context the precise timing has to be met. A sock wmem accouting is pointless for this use case.
This patch introduces an empty skb destructor like in commit 072017b41e49 (net: sctp: Add rudimentary infrastructure to account for control chunks) to make the cyclic transmission of CAN frames work again on real CAN netdevices. Virtual CAN interfaces do not need skb_orphan().
Signed-off-by Oliver Hartkopp <[email protected]>
---
net/can/bcm.c +5 -0
index 3fc737b..82af1a5 100644
@@ -237,6 +237,11 @@ static const struct file_operations bcm_proc_fops = {
 	.release	= single_release,
 };
 
+static void bcm_skb_destructor(struct sk_buff *skb)
+{
+	/* no accounting needed for bcm_can_tx() */
+}
+
 /*
  * bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
  *              of the given bcm tx op

net/can/bcm.c static void bcm_can_tx(struct bcm_op *op) · line 278

Quoted 7 lines

net/can/bcm.c +1 -0

@@ -267,6 +272,7 @@ static void bcm_can_tx(struct bcm_op *op)
 	memcpy(skb_put(skb, CFSIZ), cf, CFSIZ);
 
 	/* send with loopback */
+	skb->destructor = bcm_skb_destructor;
 	skb->dev = dev;
 	skb->sk = op->sk;
 	can_send(skb, 1);

You do not explain why its safe to keep a reference on a socket without incrementing a refcount.

Instead of understanding the issue, it seems this patch exactly shutup the useful warning.

If you set skb->sk, then you expect a future reader of skb->sk to be 100% sure the socket did not disappear.

I do not see this explained in the changelog.

Oliver Hartkopp

author

2014-01-28 22:49 UTC #2 lore raw

Quoted 35 lines · Eric Dumazet

Quoted 34 lines · Oliver Hartkopp
Commit 376c7311bdb6 (net: add a temporary sanity check in skb_orphan()) leads to a BUG in can_put_echo_skb() when skb_orphan() is executed. When skbuffs created automatically in bcm_can_tx() in softirq (netrx, timer) and userspace context the precise timing has to be met. A sock wmem accouting is pointless for this use case.
This patch introduces an empty skb destructor like in commit 072017b41e49 (net: sctp: Add rudimentary infrastructure to account for control chunks) to make the cyclic transmission of CAN frames work again on real CAN netdevices. Virtual CAN interfaces do not need skb_orphan().
Signed-off-by Oliver Hartkopp <[email protected]>
---
net/can/bcm.c +5 -0
index 3fc737b..82af1a5 100644
@@ -237,6 +237,11 @@ static const struct file_operations bcm_proc_fops = {
 	.release	= single_release,
 };
 
+static void bcm_skb_destructor(struct sk_buff *skb)
+{
+	/* no accounting needed for bcm_can_tx() */
+}
+
 /*
  * bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
  *              of the given bcm tx op
@@ -267,6 +272,7 @@ static void bcm_can_tx(struct bcm_op *op)
 	memcpy(skb_put(skb, CFSIZ), cf, CFSIZ);
 

Quoted 10 lines

net/can/bcm.c static void bcm_can_tx(struct bcm_op *op) · line 278
Quoted 6 lines
net/can/bcm.c +1 -0
 	/* send with loopback */
+	skb->destructor = bcm_skb_destructor;
 	skb->dev = dev;
 	skb->sk = op->sk;
 	can_send(skb, 1);
You do not explain why its safe to keep a reference on a socket without incrementing a refcount.

The sbk->sk reference is used to make sure in AF_CAN to identify the originating socket (if any) to not deliver echoed CAN frames to the originating application.

See first check in raw_rcv() in net/can/raw.c

Quoted 3 lines

Instead of understanding the issue, it seems this patch exactly shutup the useful warning.

I would have been happy to have this a warning and not a bug as you implemented it.

I don't need this warning as I'm using skb_alloc in the cases where CAN frames are generated autonomously. They are not triggered through a direct socket write operation nor do they need to take case about any sock wmem.

The useful warning/bug might be nice for common use cases. I'm using plain skb_alloc here for fire-and-forget skbs.

So I need to shutup the useful warning or revert the two commits at skb_orphan(). I would prefer the latter.

Quoted 3 lines

If you set skb->sk, then you expect a future reader of skb->sk to be 100% sure the socket did not disappear.

It's a fire-and-forget skb. I don't need to care if the socket disappears. If it disappears no new traffic is generated. That's enough.

Quoted 3 lines

I do not see this explained in the changelog.

I hopefully was able to make it more clearly.
See Documentation/networking/can.txt

Regards,
Oliver

Eric Dumazet

2014-01-28 23:51 UTC #3 lore raw

Quoted 5 lines · Oliver Hartkopp

The sbk->sk reference is used to make sure in AF_CAN to identify the originating socket (if any) to not deliver echoed CAN frames to the originating application.
See first check in raw_rcv() in net/can/raw.c

Nice, this is buggy.

Quoted 8 lines

Quoted 4 lines
Instead of understanding the issue, it seems this patch exactly shutup the useful warning.
I would have been happy to have this a warning and not a bug as you implemented it.

Yes, I understand you are not happy of our work to discover CAN bugs.

Quoted 18 lines

I don't need this warning as I'm using skb_alloc in the cases where CAN frames are generated autonomously. They are not triggered through a direct socket write operation nor do they need to take case about any sock wmem.
The useful warning/bug might be nice for common use cases. I'm using plain skb_alloc here for fire-and-forget skbs.
So I need to shutup the useful warning or revert the two commits at skb_orphan(). I would prefer the latter.
Quoted 3 lines
If you set skb->sk, then you expect a future reader of skb->sk to be 100% sure the socket did not disappear.
It's a fire-and-forget skb. I don't need to care if the socket disappears. If it disappears no new traffic is generated. That's enough.
Quoted 1 line

Quoted 6 lines

Quoted 2 lines
I do not see this explained in the changelog.
I hopefully was able to make it more clearly.
See Documentation/networking/can.txt

Just take a reference on the damn socket, and we do not have to worry.

bcm_tx_send() suffers from the same problem

can_send() is buggy as well :

newskb->sk = skb->sk; // line 293

dev_queue_xmit() can queue a packet a long time, and some packet qdisc even look at skb->sk.

So this is really wrong to assume only net/can can assume things about skb->sk, and not care of net/core or net/sched users.

I absolutely disagree with your patch. You need quite different _real_ fixes.

Andre Naujoks

2014-01-29 07:40 UTC #4 lore raw

On 29.01.2014 00:51, schrieb Eric Dumazet:

Quoted 55 lines · Oliver Hartkopp

Quoted 6 lines
The sbk->sk reference is used to make sure in AF_CAN to identify the originating socket (if any) to not deliver echoed CAN frames to the originating application.
See first check in raw_rcv() in net/can/raw.c
Nice, this is buggy.
Quoted 8 lines
Quoted 4 lines
Instead of understanding the issue, it seems this patch exactly shutup the useful warning.
I would have been happy to have this a warning and not a bug as you implemented it.
Yes, I understand you are not happy of our work to discover CAN bugs.
Quoted 18 lines
I don't need this warning as I'm using skb_alloc in the cases where CAN frames are generated autonomously. They are not triggered through a direct socket write operation nor do they need to take case about any sock wmem.
The useful warning/bug might be nice for common use cases. I'm using plain skb_alloc here for fire-and-forget skbs.
So I need to shutup the useful warning or revert the two commits at skb_orphan(). I would prefer the latter.
Quoted 3 lines
If you set skb->sk, then you expect a future reader of skb->sk to be 100% sure the socket did not disappear.
It's a fire-and-forget skb. I don't need to care if the socket disappears. If it disappears no new traffic is generated. That's enough.
Quoted 1 line
Quoted 6 lines
Quoted 2 lines
I do not see this explained in the changelog.
I hopefully was able to make it more clearly.
See Documentation/networking/can.txt
Just take a reference on the damn socket, and we do not have to worry.
bcm_tx_send() suffers from the same problem
can_send() is buggy as well :
newskb->sk = skb->sk; // line 293

Quoted 8 lines

dev_queue_xmit() can queue a packet a long time, and some packet qdisc even look at skb->sk.
So this is really wrong to assume only net/can can assume things about skb->sk, and not care of net/core or net/sched users.
I absolutely disagree with your patch. You need quite different _real_ fixes.

Hi.

Even if this is a bug in the CAN BCM implementation. Your "fix" just enabled a user space application to shut down any machine with a kernel containing the BUG_ON patch.

If the BCM implementation is broken, it needs to be fixed. But this is a regression that causes Kernel crashes, where there were none before.

As I am using the BCM, I would rather have a flawed but working implementation than an unusable one. If the empty socket destructor enables the system to work again, then I would like to see it. But, like Oliver, I would prefer the BUG_ON patch reverted at least until this issue is resolved.

Regards
Andre

Quoted 3 lines

David Miller

maintainer

2014-01-29 07:46 UTC #5 lore raw

From: Andre Naujoks <[email protected]>
Date: Wed, 29 Jan 2014 08:40:03 +0100

Quoted 3 lines

Even if this is a bug in the CAN BCM implementation. Your "fix" just enabled a user space application to shut down any machine with a kernel containing the BUG_ON patch.

Rather, he detected a potential stray pointer reference to freed data that was caused by the CAN code which would difficult if not impossible to detect otherwise.

That's even more dangerous, and you should be thanking him.

Andre Naujoks

2014-01-29 08:47 UTC #6 lore raw

On 29.01.2014 08:46, schrieb David Miller:

Quoted 7 lines

From: Andre Naujoks <[email protected]>
Date: Wed, 29 Jan 2014 08:40:03 +0100
Quoted 3 lines
Even if this is a bug in the CAN BCM implementation. Your "fix" just enabled a user space application to shut down any machine with a kernel containing the BUG_ON patch.

Quoted 5 lines

Rather, he detected a potential stray pointer reference to freed data that was caused by the CAN code which would difficult if not impossible to detect otherwise.
That's even more dangerous, and you should be thanking him.

"potential" is the keyword here. But its a definite kernel crash as it is right now with a standard use case for the BCM.

Don't get me wrong. If there are bugs in the code, they should be fixed, but I don't think breaking a working (even if flawed) part of the kernel is the right thing to do here.

Regards
Andre

Quoted 1 line

Eric Dumazet

2014-01-29 14:53 UTC #7 lore raw

Quoted 12 lines · Andre Naujoks

On 29.01.2014 08:46, schrieb David Miller:
Quoted 11 lines
From: Andre Naujoks <[email protected]>
Date: Wed, 29 Jan 2014 08:40:03 +0100
Quoted 3 lines
Even if this is a bug in the CAN BCM implementation. Your "fix" just enabled a user space application to shut down any machine with a kernel containing the BUG_ON patch.
Rather, he detected a potential stray pointer reference to freed data that was caused by the CAN code which would difficult if not impossible to detect otherwise.

Quoted 8 lines

Quoted 1 line
That's even more dangerous, and you should be thanking him.
"potential" is the keyword here. But its a definite kernel crash as it is right now with a standard use case for the BCM.
Don't get me wrong. If there are bugs in the code, they should be fixed, but I don't think breaking a working (even if flawed) part of the kernel is the right thing to do here.

Shall I remember you this patch was suggested by David Miller, our beloved network maintainer ?

Really this is quite silly, I'll tell you.

I can send a patch to mark CAN as BROKEN if you want, or you can send an appropriate patch.

Your resistance is futile.

Andre Naujoks

2014-01-29 15:35 UTC #8 lore raw

On 29.01.2014 15:53, schrieb Eric Dumazet:

Quoted 18 lines · Andre Naujoks

Quoted 17 lines
On 29.01.2014 08:46, schrieb David Miller:
Quoted 7 lines
From: Andre Naujoks <[email protected]>
Date: Wed, 29 Jan 2014 08:40:03 +0100
Quoted 3 lines
Even if this is a bug in the CAN BCM implementation. Your "fix" just enabled a user space application to shut down any machine with a kernel containing the BUG_ON patch.
Quoted 5 lines
Rather, he detected a potential stray pointer reference to freed data that was caused by the CAN code which would difficult if not impossible to detect otherwise.
That's even more dangerous, and you should be thanking him.
"potential" is the keyword here. But its a definite kernel crash as it is right now with a standard use case for the BCM.

Quoted 6 lines

Quoted 3 lines
Don't get me wrong. If there are bugs in the code, they should be fixed, but I don't think breaking a working (even if flawed) part of the kernel is the right thing to do here.
Shall I remember you this patch was suggested by David Miller, our beloved network maintainer ?

no, but thank you.

Quoted 2 lines

Really this is quite silly, I'll tell you.

Totally with you on that.

Quoted 5 lines

I can send a patch to mark CAN as BROKEN if you want, or you can send an appropriate patch.
Your resistance is futile.

I am not resisting to anything. I was just *irritated* about the way this was handled. Since Oliver is already trying to fix this, any further discussion here is meaningless anyway.

Regards
Andre

Quoted 2 lines

Eric Dumazet

2014-01-29 15:48 UTC #9 lore raw

Quoted 3 lines · Andre Naujoks

I am not resisting to anything. I was just *irritated* about the way this was handled. Since Oliver is already trying to fix this, any further discussion here is meaningless anyway.

I sent a patch, because I thought the fix was obvious, but last attempt from Oliver was way too complex for a backport.