| Message ID | 4C529EFB.4090601@hartkopp.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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() ... 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03.08.2010 09:30, David Miller wrote: > 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. 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! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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@hartkopp.net> --- -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html