[PATCH stable 3.11+] can: bcm: add skb destructor

[PATCH stable 3.11+] can: bcm: add skb destructor

[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 <socketcan@hartkopp.net>

---

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 3fc737b..82af1a5 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -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);

Re: [PATCH stable 3.11+] can: bcm: add skb destructor

[Eric Dumazet]

On Tue, 2014-01-28 at 21:42 +0100, Oliver Hartkopp wrote:
> 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 <socketcan@hartkopp.net>
> 
> ---
> 
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 3fc737b..82af1a5 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -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);
> 


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.

Re: [PATCH stable 3.11+] can: bcm: add skb destructor

[Oliver Hartkopp]

On 28.01.2014 23:28, Eric Dumazet wrote:
> On Tue, 2014-01-28 at 21:42 +0100, Oliver Hartkopp wrote:
>> 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 <socketcan@hartkopp.net>
>>
>> ---
>>
>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>> index 3fc737b..82af1a5 100644
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
>> @@ -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);
>>
> 
> 
> 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

> 
> 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.

> 
> 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.

> 
> 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

Re: [PATCH stable 3.11+] can: bcm: add skb destructor

[Eric Dumazet]

On Tue, 2014-01-28 at 23:49 +0100, Oliver Hartkopp wrote:

> 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.

> 
> > 
> > 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.

> 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.
> 
> > 
> > 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.
> 
> > 
> > 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.

Re: [PATCH stable 3.11+] can: bcm: add skb destructor

[Andre Naujoks]

On 29.01.2014 00:51, schrieb Eric Dumazet:
> On Tue, 2014-01-28 at 23:49 +0100, Oliver Hartkopp wrote:
> 
>> 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.
> 
>>
>>>
>>> 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.
> 
>> 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.
>>
>>>
>>> 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.
>>
>>>
>>> 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.

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

> 
> 
> 

Re: [PATCH stable 3.11+] can: bcm: add skb destructor

[David Miller]

From: Andre Naujoks <nautsch2@gmail.com>
Date: Wed, 29 Jan 2014 08:40:03 +0100

> 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.

Re: [PATCH stable 3.11+] can: bcm: add skb destructor

[Andre Naujoks]

On 29.01.2014 08:46, schrieb David Miller:
> From: Andre Naujoks <nautsch2@gmail.com>
> Date: Wed, 29 Jan 2014 08:40:03 +0100
> 
>> 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.

"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

> 

Re: [PATCH stable 3.11+] can: bcm: add skb destructor

[Eric Dumazet]

On Wed, 2014-01-29 at 09:47 +0100, Andre Naujoks wrote:
> On 29.01.2014 08:46, schrieb David Miller:
> > From: Andre Naujoks <nautsch2@gmail.com>
> > Date: Wed, 29 Jan 2014 08:40:03 +0100
> > 
> >> 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.
> 
> "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.

Re: [PATCH stable 3.11+] can: bcm: add skb destructor

[Andre Naujoks]

On 29.01.2014 15:53, schrieb Eric Dumazet:
> On Wed, 2014-01-29 at 09:47 +0100, Andre Naujoks wrote:
>> On 29.01.2014 08:46, schrieb David Miller:
>>> From: Andre Naujoks <nautsch2@gmail.com>
>>> Date: Wed, 29 Jan 2014 08:40:03 +0100
>>>
>>>> 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.
>>
>> "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 ?

no, but thank you.

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

Totally with you on that.

> 
> 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

> 
> 

Re: [PATCH stable 3.11+] can: bcm: add skb destructor

[Eric Dumazet]

On Wed, 2014-01-29 at 16:35 +0100, Andre Naujoks wrote:

> 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.