[PATCH] can-raw: Fix skb_orphan_try handling

[PATCH] can-raw: Fix skb_orphan_try handling

[Oliver Hartkopp]

Hello Eric, hello Patrick,

Commit fc6055a5ba31e2c14e36e8939f9bf2b6d586a7f5 (net: Introduce
skb_orphan_try()) allows an early orphan of the skb and takes care on
tx timestamping, which needs the sk-reference in the skb on driver level.
So does the can-raw socket, which has not been taken into account here.

The patch below adds a 'prevent_sk_orphan' bit in the skb tx shared info,
which fixes the problem discovered by Matthias Fuchs here:

      http://marc.info/?t=128030411900003&r=1&w=2

Even if it's not a primary tx timestamp topic it fits well into some skb
shared tx context. Or should be find a different place for the information to
protect the sk reference until it reaches the driver level?

Regards,
Oliver

This patch applies on net-2.6 and would be a candidate for stable 2.6.34 also.

Signed-off-by: Oliver Hartkopp <socketcan-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>

---

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index f89e7fd..eb674b7 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -169,6 +169,7 @@ struct skb_shared_hwtstamps {
  * @software:		generate software time stamp
  * @in_progress:	device driver is going to provide
  *			hardware time stamp
+ * @prevent_sk_orphan:	make sk reference available on driver level
  * @flags:		all shared_tx flags
  *
  * These flags are attached to packets as part of the
@@ -178,7 +179,8 @@ union skb_shared_tx {
 	struct {
 		__u8	hardware:1,
 			software:1,
-			in_progress:1;
+			in_progress:1,
+			prevent_sk_orphan:1;
 	};
 	__u8 flags;
 };
diff --git a/net/can/raw.c b/net/can/raw.c
index da99cf1..eedab37 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -655,6 +655,10 @@ static int raw_sendmsg(struct kiocb *iocb, struct socket *sock,
 	err = sock_tx_timestamp(msg, sk, skb_tx(skb));
 	if (err < 0)
 		goto free_skb;
+
+	/* to be able to check the received tx sock reference in raw_rcv() */
+	(skb_tx(skb))->prevent_sk_orphan = 1;
+
 	skb->dev = dev;
 	skb->sk  = sk;

Re: [PATCH] can-raw: Fix skb_orphan_try handling

[David Miller]

From: Oliver Hartkopp <socketcan@hartkopp.net>
Date: Fri, 30 Jul 2010 11:44:27 +0200

> Hello Eric, hello Patrick,
> 
> Commit fc6055a5ba31e2c14e36e8939f9bf2b6d586a7f5 (net: Introduce
> skb_orphan_try()) allows an early orphan of the skb and takes care on
> tx timestamping, which needs the sk-reference in the skb on driver level.
> So does the can-raw socket, which has not been taken into account here.
> 
> The patch below adds a 'prevent_sk_orphan' bit in the skb tx shared info,
> which fixes the problem discovered by Matthias Fuchs here:
> 
>       http://marc.info/?t=128030411900003&r=1&w=2

Your patch sets this new value, but I never see it getting tested anywhere.

How does this work?

Re: [PATCH] can-raw: Fix skb_orphan_try handling

[Oliver Hartkopp]

On 01.08.2010 10:03, David Miller wrote:
> From: Oliver Hartkopp <socketcan-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>
> Date: Fri, 30 Jul 2010 11:44:27 +0200
> 
>> Hello Eric, hello Patrick,
>>
>> Commit fc6055a5ba31e2c14e36e8939f9bf2b6d586a7f5 (net: Introduce
>> skb_orphan_try()) allows an early orphan of the skb and takes care on
>> tx timestamping, which needs the sk-reference in the skb on driver level.
>> So does the can-raw socket, which has not been taken into account here.
>>
>> The patch below adds a 'prevent_sk_orphan' bit in the skb tx shared info,
>> which fixes the problem discovered by Matthias Fuchs here:
>>
>>       http://marc.info/?t=128030411900003&r=1&w=2
> 
> Your patch sets this new value, but I never see it getting tested anywhere.
> 
> How does this work?


The flags are tested all together in skb_orphan_try() ...

See at

http://git.kernel.org/?p=linux/kernel/git/davem/net-next-2.6.git;a=commitdiff;h=fc6055a5ba31e2c14e36e8939f9bf2b6d586a7f5

+/*
+ * Try to orphan skb early, right before transmission by the device.
+ * We cannot orphan skb if tx timestamp is requested, since
+ * drivers need to call skb_tstamp_tx() to send the timestamp.
+ */
+static inline void skb_orphan_try(struct sk_buff *skb)
+{
+       if (!skb_tx(skb)->flags)
+               skb_orphan(skb);
+}

So my patch just added a new bit that's tested here but does not touch the
rest of the tx timestamp bits that are checked at this place.

Regards,
Oliver

Re: [PATCH] can-raw: Fix skb_orphan_try handling

[David Miller]

From: Oliver Hartkopp <socketcan@hartkopp.net>
Date: Sun, 01 Aug 2010 12:50:16 +0200

> On 01.08.2010 10:03, David Miller wrote:
>> From: Oliver Hartkopp <socketcan@hartkopp.net>
>> Date: Fri, 30 Jul 2010 11:44:27 +0200
>> 
>>> Hello Eric, hello Patrick,
>>>
>>> Commit fc6055a5ba31e2c14e36e8939f9bf2b6d586a7f5 (net: Introduce
>>> skb_orphan_try()) allows an early orphan of the skb and takes care on
>>> tx timestamping, which needs the sk-reference in the skb on driver level.
>>> So does the can-raw socket, which has not been taken into account here.
>>>
>>> The patch below adds a 'prevent_sk_orphan' bit in the skb tx shared info,
>>> which fixes the problem discovered by Matthias Fuchs here:
>>>
>>>       http://marc.info/?t=128030411900003&r=1&w=2
>> 
>> Your patch sets this new value, but I never see it getting tested anywhere.
>> 
>> How does this work?
> 
> 
> The flags are tested all together in skb_orphan_try() ...

This is why I hate using unions in situations like this... it makes
code impossible to audit easily.

This damn thing should just be a "u8 flags" and a bunch of bit mask
CPP macro defines for the various boolean values.

Anyways, I'll apply your patch thanks.

Re: [PATCH] can-raw: Fix skb_orphan_try handling

[Oliver Hartkopp]

On 03.08.2010 09:30, David Miller wrote:
> From: Oliver Hartkopp <socketcan-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>
> Date: Sun, 01 Aug 2010 12:50:16 +0200
> 
>> On 01.08.2010 10:03, David Miller wrote:
>>> From: Oliver Hartkopp <socketcan-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>
>>> Date: Fri, 30 Jul 2010 11:44:27 +0200
>>>
>>>> Hello Eric, hello Patrick,
>>>>
>>>> Commit fc6055a5ba31e2c14e36e8939f9bf2b6d586a7f5 (net: Introduce
>>>> skb_orphan_try()) allows an early orphan of the skb and takes care on
>>>> tx timestamping, which needs the sk-reference in the skb on driver level.
>>>> So does the can-raw socket, which has not been taken into account here.
>>>>
>>>> The patch below adds a 'prevent_sk_orphan' bit in the skb tx shared info,
>>>> which fixes the problem discovered by Matthias Fuchs here:
>>>>
>>>>       http://marc.info/?t=128030411900003&r=1&w=2
>>>
>>> Your patch sets this new value, but I never see it getting tested anywhere.
>>>
>>> How does this work?
>>
>>
>> The flags are tested all together in skb_orphan_try() ...
> 
> This is why I hate using unions in situations like this... it makes
> code impossible to audit easily.
> 
> This damn thing should just be a "u8 flags" and a bunch of bit mask
> CPP macro defines for the various boolean values.

Yep! I also felt like this.

Maybe Patrick Ohly can give some feedback, if he's ok with that kind of
change. So far there are only a few places that would need to be changed for
the flags bitops.

> Anyways, I'll apply your patch thanks.

Thanks!

Re: [PATCH] can-raw: Fix skb_orphan_try handling

[Patrick Ohly]

On Tue, 2010-08-03 at 18:22 +0300, Oliver Hartkopp wrote:
> >> The flags are tested all together in skb_orphan_try() ...
> > 
> > This is why I hate using unions in situations like this... it makes
> > code impossible to audit easily.
> > 
> > This damn thing should just be a "u8 flags" and a bunch of bit mask
> > CPP macro defines for the various boolean values.
> 
> Yep! I also felt like this.
> 
> Maybe Patrick Ohly can give some feedback, if he's ok with that kind of
> change. So far there are only a few places that would need to be changed for
> the flags bitops.

I'm fine with using a simple u8. I'm not sure where I picked up the
union thing, it's not something that I usually do in my own code.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH] can-raw: Fix skb_orphan_try handling

[Oliver Hartkopp]

On 03.08.2010 18:14, Patrick Ohly wrote:
> On Tue, 2010-08-03 at 18:22 +0300, Oliver Hartkopp wrote:
>>>> The flags are tested all together in skb_orphan_try() ...
>>>
>>> This is why I hate using unions in situations like this... it makes
>>> code impossible to audit easily.
>>>
>>> This damn thing should just be a "u8 flags" and a bunch of bit mask
>>> CPP macro defines for the various boolean values.
>>
>> Yep! I also felt like this.
>>
>> Maybe Patrick Ohly can give some feedback, if he's ok with that kind of
>> change. So far there are only a few places that would need to be changed for
>> the flags bitops.
> 
> I'm fine with using a simple u8. I'm not sure where I picked up the
> union thing, it's not something that I usually do in my own code.
> 

:-)

Im currently busy until next week, would you like to provide a patch?

Regards,
Oliver

Re: [PATCH] can-raw: Fix skb_orphan_try handling

[Patrick Ohly]

On Tue, 2010-08-03 at 10:11 -0700, Oliver Hartkopp wrote:
> > I'm fine with using a simple u8. I'm not sure where I picked up the
> > union thing, it's not something that I usually do in my own code.
> > 
> 
> :-)
> 
> Im currently busy until next week, would you like to provide a patch?

Sorry, I have to pass. I'm busy elsewhere myself. MeeGo and
SyncEvolution plus real life consume all of my time nowadays.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH] can-raw: Fix skb_orphan_try handling

[Oliver Hartkopp]

On 03.08.2010 19:33, Patrick Ohly wrote:
> On Tue, 2010-08-03 at 10:11 -0700, Oliver Hartkopp wrote:
>>> I'm fine with using a simple u8. I'm not sure where I picked up the
>>> union thing, it's not something that I usually do in my own code.
>>>
>>
>> :-)
>>
>> Im currently busy until next week, would you like to provide a patch?
> 
> Sorry, I have to pass. I'm busy elsewhere myself. MeeGo and
> SyncEvolution plus real life consume all of my time nowadays.

Working for MeeGo is a good excuse ;-)

Will pick that task next week.

BR,
Oliver

Re: [PATCH] can-raw: Fix skb_orphan_try handling

[David Miller]

From: Oliver Hartkopp <socketcan@hartkopp.net>
Date: Tue, 03 Aug 2010 19:50:27 +0200

> On 03.08.2010 19:33, Patrick Ohly wrote:
>> On Tue, 2010-08-03 at 10:11 -0700, Oliver Hartkopp wrote:
>>>> I'm fine with using a simple u8. I'm not sure where I picked up the
>>>> union thing, it's not something that I usually do in my own code.
>>>>
>>>
>>> :-)
>>>
>>> Im currently busy until next week, would you like to provide a patch?
>> 
>> Sorry, I have to pass. I'm busy elsewhere myself. MeeGo and
>> SyncEvolution plus real life consume all of my time nowadays.
> 
> Working for MeeGo is a good excuse ;-)
> 
> Will pick that task next week.

Thanks guys.