| Message ID | 1321870915.2552.22.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2011-11-21 at 11:21 +0100, Eric Dumazet wrote: > Le lundi 21 novembre 2011 à 11:15 +0100, Eric Dumazet a écrit : > > > > > Hmm, trinity tries to crash decnet ;) > > > > Maybe we should remove this decnet stuff for good instead of tracking > > all bugs just for the record. Is there anybody still using decnet ? > > > > For example dn_start_slow_timer() starts a timer without holding a > > reference on struct sock, this is highly suspect. > > > > [PATCH] decnet: proper socket refcounting > > > > Better use sk_reset_timer() / sk_stop_timer() helpers to make sure we > > dont access already freed/reused memory later. > > > > Reported-by: Sasha Levin <levinsasha928@gmail.com> > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > > Hmm, I forgot to remove the sock_hold(sk) call from dn_slow_timer(), > here is V2 : > > [PATCH] decnet: proper socket refcounting > > Better use sk_reset_timer() / sk_stop_timer() helpers to make sure we > dont access already freed/reused memory later. > > Reported-by: Sasha Levin <levinsasha928@gmail.com> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > --- [snip] Applied locally and running same tests as before, will update with results.
Hi, On Mon, 2011-11-21 at 11:21 +0100, Eric Dumazet wrote: > Le lundi 21 novembre 2011 à 11:15 +0100, Eric Dumazet a écrit : > > > > > Hmm, trinity tries to crash decnet ;) > > > > Maybe we should remove this decnet stuff for good instead of tracking > > all bugs just for the record. Is there anybody still using decnet ? > > The best place to ask that question is on the decnet mailing list: linux-decnet-user@lists.sourceforge.net. I've BCC'd this message since that list requires you to be subscribed in order to post there. I have to say that I've been wondering lately whether it has got to the point where it is no longer useful. Has anybody actually tested it lately against "real" DEC implementations? Steve. > > > For example dn_start_slow_timer() starts a timer without holding a > > reference on struct sock, this is highly suspect. > > > > [PATCH] decnet: proper socket refcounting > > > > Better use sk_reset_timer() / sk_stop_timer() helpers to make sure we > > dont access already freed/reused memory later. > > > > Reported-by: Sasha Levin <levinsasha928@gmail.com> > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > > Hmm, I forgot to remove the sock_hold(sk) call from dn_slow_timer(), > here is V2 : > > [PATCH] decnet: proper socket refcounting > > Better use sk_reset_timer() / sk_stop_timer() helpers to make sure we > dont access already freed/reused memory later. > > Reported-by: Sasha Levin <levinsasha928@gmail.com> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > --- > V2: remove sock_hold(sk) call from dn_slow_timer() > > net/decnet/dn_timer.c | 17 +++++------------ > 1 file changed, 5 insertions(+), 12 deletions(-) > > diff --git a/net/decnet/dn_timer.c b/net/decnet/dn_timer.c > index 67f691b..d9c150c 100644 > --- a/net/decnet/dn_timer.c > +++ b/net/decnet/dn_timer.c > @@ -36,16 +36,13 @@ static void dn_slow_timer(unsigned long arg); > > void dn_start_slow_timer(struct sock *sk) > { > - sk->sk_timer.expires = jiffies + SLOW_INTERVAL; > - sk->sk_timer.function = dn_slow_timer; > - sk->sk_timer.data = (unsigned long)sk; > - > - add_timer(&sk->sk_timer); > + setup_timer(&sk->sk_timer, dn_slow_timer, (unsigned long)sk); > + sk_reset_timer(sk, &sk->sk_timer, jiffies + SLOW_INTERVAL); > } > > void dn_stop_slow_timer(struct sock *sk) > { > - del_timer(&sk->sk_timer); > + sk_stop_timer(sk, &sk->sk_timer); > } > > static void dn_slow_timer(unsigned long arg) > @@ -53,12 +50,10 @@ static void dn_slow_timer(unsigned long arg) > struct sock *sk = (struct sock *)arg; > struct dn_scp *scp = DN_SK(sk); > > - sock_hold(sk); > bh_lock_sock(sk); > > if (sock_owned_by_user(sk)) { > - sk->sk_timer.expires = jiffies + HZ / 10; > - add_timer(&sk->sk_timer); > + sk_reset_timer(sk, &sk->sk_timer, jiffies + HZ / 10); > goto out; > } > > @@ -100,9 +95,7 @@ static void dn_slow_timer(unsigned long arg) > scp->keepalive_fxn(sk); > } > > - sk->sk_timer.expires = jiffies + SLOW_INTERVAL; > - > - add_timer(&sk->sk_timer); > + sk_reset_timer(sk, &sk->sk_timer, jiffies + SLOW_INTERVAL); > out: > bh_unlock_sock(sk); > sock_put(sk); > > > -- > 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 -- 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 Mon, 2011-11-21 at 12:22 +0200, Sasha Levin wrote: > On Mon, 2011-11-21 at 11:21 +0100, Eric Dumazet wrote: > > Le lundi 21 novembre 2011 à 11:15 +0100, Eric Dumazet a écrit : > > > > > > > > Hmm, trinity tries to crash decnet ;) > > > > > > Maybe we should remove this decnet stuff for good instead of tracking > > > all bugs just for the record. Is there anybody still using decnet ? > > > > > > For example dn_start_slow_timer() starts a timer without holding a > > > reference on struct sock, this is highly suspect. > > > > > > [PATCH] decnet: proper socket refcounting > > > > > > Better use sk_reset_timer() / sk_stop_timer() helpers to make sure we > > > dont access already freed/reused memory later. > > > > > > Reported-by: Sasha Levin <levinsasha928@gmail.com> > > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > > > > Hmm, I forgot to remove the sock_hold(sk) call from dn_slow_timer(), > > here is V2 : > > > > [PATCH] decnet: proper socket refcounting > > > > Better use sk_reset_timer() / sk_stop_timer() helpers to make sure we > > dont access already freed/reused memory later. > > > > Reported-by: Sasha Levin <levinsasha928@gmail.com> > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > > --- > > [snip] > > Applied locally and running same tests as before, will update with > results. > Looks ok after a couple days of testing. Tested-by: Sasha Levin <levinsasha928@gmail.com>
Le samedi 26 novembre 2011 à 12:54 +0200, Sasha Levin a écrit : > > On Mon, 2011-11-21 at 11:21 +0100, Eric Dumazet wrote: > > > > > > Hmm, I forgot to remove the sock_hold(sk) call from dn_slow_timer(), > > > here is V2 : > > > > > > [PATCH] decnet: proper socket refcounting > > > > > > Better use sk_reset_timer() / sk_stop_timer() helpers to make sure we > > > dont access already freed/reused memory later. > > > > > > Reported-by: Sasha Levin <levinsasha928@gmail.com> > > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > > > --- > > > > > > Applied locally and running same tests as before, will update with > > results. > > > > Looks ok after a couple days of testing. > > Tested-by: Sasha Levin <levinsasha928@gmail.com> > Thanks Sasha ! -- 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: Eric Dumazet <eric.dumazet@gmail.com> Date: Sat, 26 Nov 2011 11:59:22 +0100 > Le samedi 26 novembre 2011 à 12:54 +0200, Sasha Levin a écrit : >> > On Mon, 2011-11-21 at 11:21 +0100, Eric Dumazet wrote: >> > > >> > > Hmm, I forgot to remove the sock_hold(sk) call from dn_slow_timer(), >> > > here is V2 : >> > > >> > > [PATCH] decnet: proper socket refcounting >> > > >> > > Better use sk_reset_timer() / sk_stop_timer() helpers to make sure we >> > > dont access already freed/reused memory later. >> > > >> > > Reported-by: Sasha Levin <levinsasha928@gmail.com> >> > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> >> > > --- >> > >> > >> > Applied locally and running same tests as before, will update with >> > results. >> > >> >> Looks ok after a couple days of testing. >> >> Tested-by: Sasha Levin <levinsasha928@gmail.com> >> > > Thanks Sasha ! Applied and queued up for -stable, thanks everyone. -- 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: Steven Whitehouse <swhiteho@redhat.com> Date: Mon, 21 Nov 2011 10:58:30 +0000 > I have to say that I've been wondering lately whether it has got to the > point where it is no longer useful. Has anybody actually tested it > lately against "real" DEC implementations? I doubt it :-) If we can't think of any real reason to keep it around, let's try to reach a quirk consensus and I'll toss it from the net-next tree. 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 26/11/11 20:50, David Miller wrote: > From: Steven Whitehouse<swhiteho@redhat.com> > Date: Mon, 21 Nov 2011 10:58:30 +0000 > >> I have to say that I've been wondering lately whether it has got to the >> point where it is no longer useful. Has anybody actually tested it >> lately against "real" DEC implementations? > > I doubt it :-) > DECnet is in use against real DEC implementations - I have checked it quite recently against a VAX running OpenVMS. How many people are actually using it for real work is a different question though. It's also true that it's not really supported by anyone as I orphaned it some time ago and nobody else seems to care enough to take it over. So if it's becoming a burden on people doing real kernel work then I don't think many tears will be wept for its removal. Chrissie -- 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
Hi, On Mon, 2011-11-28 at 09:58 +0000, Christine Caulfield wrote: > On 26/11/11 20:50, David Miller wrote: > > From: Steven Whitehouse<swhiteho@redhat.com> > > Date: Mon, 21 Nov 2011 10:58:30 +0000 > > > >> I have to say that I've been wondering lately whether it has got to the > >> point where it is no longer useful. Has anybody actually tested it > >> lately against "real" DEC implementations? > > > > I doubt it :-) > > > > DECnet is in use against real DEC implementations - I have checked it > quite recently against a VAX running OpenVMS. How many people are > actually using it for real work is a different question though. > Ok, thats useful info. > It's also true that it's not really supported by anyone as I orphaned it > some time ago and nobody else seems to care enough to take it over. So > if it's becoming a burden on people doing real kernel work then I don't > think many tears will be wept for its removal. > > Chrissie Really the only issue with keeping it around is the maintenance burden I think. It doesn't look like anybody wants to take it on, but maybe we should give it another few days for someone to speak up, just in case they are on holiday or something at the moment. Also, I've updated the subject of the thread, to make it more obvious what is being discussed, as well as bcc'ing it again to the DECnet list, Steve. -- 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
reflum, On Tue, 2011-11-29 at 15:34 +0100, Steven Whitehouse wrote: > Has anybody actually tested it > > >> lately against "real" DEC implementations? > > > I doubt it :-) > > DECnet is in use against real DEC implementations - I have checked it > > quite recently against a VAX running OpenVMS. How many people are > > actually using it for real work is a different question though. > > > Ok, thats useful info. I confirmed parts of it with tcpdump and the specs some weeks ago. The parts I worked on passed :) I also considered to send the tcpdump upstream a patch for protocol decoding. > > It's also true that it's not really supported by anyone as I orphaned it > > some time ago and nobody else seems to care enough to take it over. So > > if it's becoming a burden on people doing real kernel work then I don't > > think many tears will be wept for its removal. > > Chrissie > > Really the only issue with keeping it around is the maintenance burden I > think. It doesn't look like anybody wants to take it on, but maybe we > should give it another few days for someone to speak up, just in case > they are on holiday or something at the moment. > > Also, I've updated the subject of the thread, to make it more obvious > what is being discussed, as well as bcc'ing it again to the DECnet list, I'm very interested in the module. However my problem is that I had nothing to do with kernel coding yet. However I'm currently searching a new maintainer for it (I got info about this thread by today). If somebody is interested in this and only needs some "motivation" or maybe someone would like to get me into kernel coding, please just reply :)
> Philipp Schafft wrote: >I'm very interested in the module. Sorry to butt in - I got this message via Philipp and the linux-decnet-user list - but I can also confirm that the linux DECnet implementation does work against real DEC hardware and software. The guys over in the HECnet group http://www.update.uu.se/~bqt/hecnet.html use it all the time. If you want access to real hardware and/or real software to test it against, contact me and I can set something up. Bob Armstrong -- 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/net/decnet/dn_timer.c b/net/decnet/dn_timer.c index 67f691b..d9c150c 100644 --- a/net/decnet/dn_timer.c +++ b/net/decnet/dn_timer.c @@ -36,16 +36,13 @@ static void dn_slow_timer(unsigned long arg); void dn_start_slow_timer(struct sock *sk) { - sk->sk_timer.expires = jiffies + SLOW_INTERVAL; - sk->sk_timer.function = dn_slow_timer; - sk->sk_timer.data = (unsigned long)sk; - - add_timer(&sk->sk_timer); + setup_timer(&sk->sk_timer, dn_slow_timer, (unsigned long)sk); + sk_reset_timer(sk, &sk->sk_timer, jiffies + SLOW_INTERVAL); } void dn_stop_slow_timer(struct sock *sk) { - del_timer(&sk->sk_timer); + sk_stop_timer(sk, &sk->sk_timer); } static void dn_slow_timer(unsigned long arg) @@ -53,12 +50,10 @@ static void dn_slow_timer(unsigned long arg) struct sock *sk = (struct sock *)arg; struct dn_scp *scp = DN_SK(sk); - sock_hold(sk); bh_lock_sock(sk); if (sock_owned_by_user(sk)) { - sk->sk_timer.expires = jiffies + HZ / 10; - add_timer(&sk->sk_timer); + sk_reset_timer(sk, &sk->sk_timer, jiffies + HZ / 10); goto out; } @@ -100,9 +95,7 @@ static void dn_slow_timer(unsigned long arg) scp->keepalive_fxn(sk); } - sk->sk_timer.expires = jiffies + SLOW_INTERVAL; - - add_timer(&sk->sk_timer); + sk_reset_timer(sk, &sk->sk_timer, jiffies + SLOW_INTERVAL); out: bh_unlock_sock(sk); sock_put(sk);