Skip to contentSkip to conversation

can-raw: Fix skb_orphan_try handling

PATCH In review Oliver Hartkopp 3 participants 10 messages Jul 30, 2010 – Aug 3, 2010 4 messages not in the archive

Subsystems (3)
Networking Network drivers CAN network layer
lore
Share & tools
lore

Oliver Hartkopp

author
#0 lore raw

PATCH 1/1 can-raw: Fix skb_orphan_try handling

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 <[email protected]>

---

include/linux/skbuff.h +3 -1
index f89e7fd..eb674b7 100644
@@ -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;
 };
net/can/raw.c +4 -0
index da99cf1..eedab37 100644
@@ -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;
maintainer
#1 lore raw

From: Oliver Hartkopp <[email protected]>
Date: Fri, 30 Jul 2010 11:44:27 +0200

Quoted 11 lines

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?

A message that is not in the archive (sent off-list or not yet received): 20100801.010337.68133932.davem-fT/PcQaiUtIeIZ0/[email protected]

author
#3 lore raw
Quoted 10 lines · David Miller

From: Oliver Hartkopp <[email protected]>
Date: Fri, 30 Jul 2010 11:44:27 +0200

Quoted 7 lines

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.

Quoted 8 lines
Quoted 4 lines

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

maintainer
#4 lore raw

From: Oliver Hartkopp <[email protected]>
Date: Sun, 01 Aug 2010 12:50:16 +0200

Quoted 14 lines · David Miller
Quoted 13 lines

From: Oliver Hartkopp <[email protected]>
Date: Fri, 30 Jul 2010 11:44:27 +0200

Quoted 10 lines

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:

Quoted 8 lines
Quoted 5 lines
Quoted 1 line
      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.

A message that is not in the archive (sent off-list or not yet received): 20100803.003056.216763358.davem-fT/PcQaiUtIeIZ0/[email protected]

author
#6 lore raw
Quoted 24 lines · David Miller

From: Oliver Hartkopp <[email protected]>
Date: Sun, 01 Aug 2010 12:50:16 +0200

Quoted 21 lines · David Miller
Quoted 18 lines

From: Oliver Hartkopp <[email protected]>
Date: Fri, 30 Jul 2010 11:44:27 +0200

Quoted 11 lines

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?

Quoted 7 lines
Quoted 1 line

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.

Quoted 1 line

Anyways, I'll apply your patch thanks.

Thanks!

#7 lore raw
Quoted 5 lines · Oliver Hartkopp
Quoted 5 lines
Quoted 1 line

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.

Quoted 8 lines
Quoted 2 lines

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.

Signature · 6 lines
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.

A message that is not in the archive (sent off-list or not yet received): 1280852081.3266.834.camel-/[email protected]

author
#9 lore raw
Quoted 9 lines · Patrick Ohly
Quoted 8 lines · Oliver Hartkopp
Quoted 7 lines
Quoted 1 line

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.

Quoted 9 lines
Quoted 5 lines

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

#10 lore raw
Quoted 7 lines · Oliver Hartkopp
Quoted 3 lines

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.

Signature · 6 lines
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.

A message that is not in the archive (sent off-list or not yet received): 1280856828.3266.839.camel-/[email protected]

author
#12 lore raw
Quoted 11 lines · Patrick Ohly
Quoted 7 lines · Oliver Hartkopp
Quoted 3 lines

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

maintainer
#13 lore raw

From: Oliver Hartkopp <[email protected]>
Date: Tue, 03 Aug 2010 19:50:27 +0200

Quoted 8 lines · Patrick Ohly
Quoted 7 lines · Oliver Hartkopp
Quoted 6 lines
Quoted 3 lines

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.

:-)

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

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.