Lost locking in fl6_sock_lookup
Subsystems (4)
PATCH 2/3 Lost locking in fl6_sock_lookup
This routine scans the ipv6_fl_list whose update is protected with the socket lock and the ip6_sk_fl_lock.
Since the socket lock is not taken in the lookup, use the other one.
---
net/ipv6/ip6_flowlabel.c +3 -0
@@ -190,14 +190,17 @@ struct ip6_flowlabel * fl6_sock_lookup(struct sock *sk, __be32 label) label &=IPV6_FLOWLABEL_MASK ; +read_lock_bh (&ip6_sk_fl_lock);for (sfl=np->ipv6_fl_list; sfl; sfl = sfl->next) {struct ip6_flowlabel *fl = sfl->fl;if (fl->label == label) { +read_unlock_bh (&ip6_sk_fl_lock); fl->lastuse = jiffies;atomic_inc (&fl->users);return fl; } } +read_unlock_bh (&ip6_sk_fl_lock);return NULL ; }
In article <[email protected]> (at Thu, 18 Oct 2007 15:53:52 +0400), Pavel Emelyanov <[email protected]> says:
net/ipv6/ip6_flowlabel.c struct ip6_flowlabel * fl6_sock_lookup(struct sock *sk, __be32 label) · line 200
Quoted 9 lines
This routine scans the
ipv6_fl_listwhose update is protected with the socket lock and theip6_sk_fl_lock.net/ipv6/ip6_flowlabel.c +1
-0struct ip6_flowlabel *fl = sfl->fl;if (fl->label == label) { +read_unlock_bh (&ip6_sk_fl_lock); fl->lastuse = jiffies;atomic_inc (&fl->users);return fl;
We should increment fl->users within the critical section, shouldn't we?
--yoshfuji
Quoted 5 lines · YOSHIFUJI Hideaki
In article <[email protected]> (at Thu, 18 Oct 2007 15:53:52 +0400), Pavel Emelyanov <[email protected]> says:
Quoted 2 lines
This routine scans the
ipv6_fl_listwhose update is protected with the socket lock and theip6_sk_fl_lock.
Quoted 8 lines
net/ipv6/ip6_flowlabel.c struct ip6_flowlabel * fl6_sock_lookup(struct sock *sk, __be32 label) · line 200
Quoted 6 lines
net/ipv6/ip6_flowlabel.c +1
-0struct ip6_flowlabel *fl = sfl->fl;if (fl->label == label) { +read_unlock_bh (&ip6_sk_fl_lock); fl->lastuse = jiffies;atomic_inc (&fl->users);return fl;We should increment
fl->userswithin the critical section, shouldn't we?
Not necessary. The users is more than zero (because it is linked in the sock's list) so garbage collector won't catch it in any way.
Thanks,
Pavel
Quoted 6 lines
--yoshfuji
-
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller
maintainerFrom: Pavel Emelyanov <[email protected]>
Date: Thu, 18 Oct 2007 16:11:58 +0400
Quoted 15 lines
Quoted 5 lines · YOSHIFUJI Hideaki
In article <[email protected]> (at Thu, 18 Oct 2007 15:53:52 +0400), Pavel Emelyanov <[email protected]> says:
Quoted 2 lines
This routine scans the
ipv6_fl_listwhose update is protected with the socket lock and theip6_sk_fl_lock.Quoted 8 lines
net/ipv6/ip6_flowlabel.c struct ip6_flowlabel * fl6_sock_lookup(struct sock *sk, __be32 label) · line 200
Quoted 6 lines
net/ipv6/ip6_flowlabel.c +1
-0struct ip6_flowlabel *fl = sfl->fl;if (fl->label == label) { +read_unlock_bh (&ip6_sk_fl_lock); fl->lastuse = jiffies;atomic_inc (&fl->users);return fl;We should increment
fl->userswithin the critical section, shouldn't we?
Quoted 3 lines
Not necessary. The users is more than zero (because it is linked in the sock's list) so garbage collector won't catch it in any way.
Right, we're grabbing an "extra" reference here and only someone who gets the socket lock (which we have) can unlink it and thus potentially drop the count to zero.
From: Pavel Emelyanov <[email protected]>
Date: Thu, 18 Oct 2007 15:53:52 +0400
Quoted 7 lines
This routine scans the
ipv6_fl_listwhose update is protected with the socket lock and theip6_sk_fl_lock.Since the socket lock is not taken in the lookup, use the other one.
Signed-off-by Pavel Emelyanov <[email protected]>
Applied.
But I notice that I was wrong in my email, we don't hold the socket lock here.
What prevents an unlink from the socket's list and thus a reference count of zero occurring for a brief moment?
Quoted 11 lines · David Miller
From: Pavel Emelyanov <[email protected]>
Date: Thu, 18 Oct 2007 15:53:52 +0400Quoted 7 lines
This routine scans the
ipv6_fl_listwhose update is protected with the socket lock and theip6_sk_fl_lock.Since the socket lock is not taken in the lookup, use the other one.
Signed-off-by Pavel Emelyanov <[email protected]>
Quoted 8 lines
Applied.
But I notice that I was wrong in my email, we don't hold the socket lock here.
What prevents an unlink from the socket's list and thus a reference count of zero occurring for a brief moment?
Oops. You're right here :( I looked at the ip6_fl_lock and messed it with the ip6_sk_fl_lock.
Should I resend the whole patch, or just make an
incremental one?
Thanks,
Pavel
David Miller
maintainerFrom: Pavel Emelyanov <[email protected]>
Date: Thu, 18 Oct 2007 16:22:49 +0400
Quoted 5 lines
Oops. You're right here :( I looked at the
ip6_fl_lockand messed it with theip6_sk_fl_lock.Should I resend the whole patch, or just make an
incremental one?
Please make an incremental one.
And hurry, I'm trying to go to bed :-)))