* dn_route.c momentarily exiting RCU read-side critical section
@ 2007-10-29 21:15 Paul E. McKenney
2007-10-30 8:10 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Paul E. McKenney @ 2007-10-29 21:15 UTC (permalink / raw)
To: linux-kernel, netdev; +Cc: SteveW, davem, dipankar
Hello!
net/decnet/dn_route.c in dn_rt_cache_get_next() is as follows:
static struct dn_route *dn_rt_cache_get_next(struct seq_file *seq, struct dn_route *rt)
{
struct dn_rt_cache_iter_state *s = rcu_dereference(seq->private);
rt = rt->u.dst.dn_next;
while(!rt) {
rcu_read_unlock_bh();
if (--s->bucket < 0)
break;
... But what happens if seq->private is freed up right here?
... Or what prevents this from happening?
rcu_read_lock_bh();
rt = dn_rt_hash_table[s->bucket].chain;
}
return rt;
}
Similar code is in rt_cache_get_next().
So, what am I missing here?
Thanx, Paul
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dn_route.c momentarily exiting RCU read-side critical section
2007-10-29 21:15 dn_route.c momentarily exiting RCU read-side critical section Paul E. McKenney
@ 2007-10-30 8:10 ` David Miller
2007-10-30 15:12 ` Paul E. McKenney
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2007-10-30 8:10 UTC (permalink / raw)
To: paulmck; +Cc: linux-kernel, netdev, SteveW, dipankar
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date: Mon, 29 Oct 2007 14:15:40 -0700
> net/decnet/dn_route.c in dn_rt_cache_get_next() is as follows:
>
> static struct dn_route *dn_rt_cache_get_next(struct seq_file *seq, struct dn_route *rt)
> {
> struct dn_rt_cache_iter_state *s = rcu_dereference(seq->private);
>
> rt = rt->u.dst.dn_next;
> while(!rt) {
> rcu_read_unlock_bh();
> if (--s->bucket < 0)
> break;
>
> ... But what happens if seq->private is freed up right here?
> ... Or what prevents this from happening?
...
> Similar code is in rt_cache_get_next().
>
> So, what am I missing here?
seq->private is allocated on file open (here via seq_open_private()),
and freed up on file close (via seq_release_private).
So it cannot be freed up in the middle of an iteration.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dn_route.c momentarily exiting RCU read-side critical section
2007-10-30 8:10 ` David Miller
@ 2007-10-30 15:12 ` Paul E. McKenney
2007-11-05 11:53 ` Herbert Xu
0 siblings, 1 reply; 6+ messages in thread
From: Paul E. McKenney @ 2007-10-30 15:12 UTC (permalink / raw)
To: David Miller; +Cc: linux-kernel, netdev, SteveW, dipankar
On Tue, Oct 30, 2007 at 01:10:36AM -0700, David Miller wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Date: Mon, 29 Oct 2007 14:15:40 -0700
>
> > net/decnet/dn_route.c in dn_rt_cache_get_next() is as follows:
> >
> > static struct dn_route *dn_rt_cache_get_next(struct seq_file *seq, struct dn_route *rt)
> > {
> > struct dn_rt_cache_iter_state *s = rcu_dereference(seq->private);
> >
> > rt = rt->u.dst.dn_next;
> > while(!rt) {
> > rcu_read_unlock_bh();
> > if (--s->bucket < 0)
> > break;
> >
> > ... But what happens if seq->private is freed up right here?
> > ... Or what prevents this from happening?
> ...
> > Similar code is in rt_cache_get_next().
> >
> > So, what am I missing here?
>
> seq->private is allocated on file open (here via seq_open_private()),
> and freed up on file close (via seq_release_private).
>
> So it cannot be freed up in the middle of an iteration.
Thank you for the info!!!
OK, for my next stupid question: why is the rcu_dereference(seq->private)
required, as opposed to simply seq->private?
Thanx, Paul
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dn_route.c momentarily exiting RCU read-side critical section
2007-10-30 15:12 ` Paul E. McKenney
@ 2007-11-05 11:53 ` Herbert Xu
2007-11-05 16:12 ` Paul E. McKenney
0 siblings, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2007-11-05 11:53 UTC (permalink / raw)
To: paulmck; +Cc: davem, linux-kernel, netdev, SteveW, dipankar
Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
>>
>> > net/decnet/dn_route.c in dn_rt_cache_get_next() is as follows:
>> >
>> > static struct dn_route *dn_rt_cache_get_next(struct seq_file *seq, struct dn_route *rt)
>> > {
>> > struct dn_rt_cache_iter_state *s = rcu_dereference(seq->private);
>> >
>> > rt = rt->u.dst.dn_next;
>> > while(!rt) {
>> > rcu_read_unlock_bh();
>> > if (--s->bucket < 0)
>> > break;
>
> OK, for my next stupid question: why is the rcu_dereference(seq->private)
> required, as opposed to simply seq->private?
It was put there by someone who went through the code converting
all occurances of smp_read_barrier_depends to rcu_dereference.
In this instance the rcu_dereference conversion doesn't make much
sense so we should probably just revert it.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dn_route.c momentarily exiting RCU read-side critical section
2007-11-05 11:53 ` Herbert Xu
@ 2007-11-05 16:12 ` Paul E. McKenney
2007-11-05 23:51 ` Herbert Xu
0 siblings, 1 reply; 6+ messages in thread
From: Paul E. McKenney @ 2007-11-05 16:12 UTC (permalink / raw)
To: Herbert Xu; +Cc: davem, linux-kernel, netdev, SteveW, dipankar
On Mon, Nov 05, 2007 at 07:53:04PM +0800, Herbert Xu wrote:
> Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> >>
> >> > net/decnet/dn_route.c in dn_rt_cache_get_next() is as follows:
> >> >
> >> > static struct dn_route *dn_rt_cache_get_next(struct seq_file *seq, struct dn_route *rt)
> >> > {
> >> > struct dn_rt_cache_iter_state *s = rcu_dereference(seq->private);
> >> >
> >> > rt = rt->u.dst.dn_next;
> >> > while(!rt) {
> >> > rcu_read_unlock_bh();
> >> > if (--s->bucket < 0)
> >> > break;
> >
> > OK, for my next stupid question: why is the rcu_dereference(seq->private)
> > required, as opposed to simply seq->private?
>
> It was put there by someone who went through the code converting
> all occurances of smp_read_barrier_depends to rcu_dereference.
> In this instance the rcu_dereference conversion doesn't make much
> sense so we should probably just revert it.
Thank you for the info! Stupid question #3: what sequence of events
would the smp_read_barrier_depends() be defending against?
Thanx, Paul
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dn_route.c momentarily exiting RCU read-side critical section
2007-11-05 16:12 ` Paul E. McKenney
@ 2007-11-05 23:51 ` Herbert Xu
0 siblings, 0 replies; 6+ messages in thread
From: Herbert Xu @ 2007-11-05 23:51 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: davem, linux-kernel, netdev, SteveW, dipankar
On Mon, Nov 05, 2007 at 08:12:03AM -0800, Paul E. McKenney wrote:
> > >>
> > >> > net/decnet/dn_route.c in dn_rt_cache_get_next() is as follows:
> > >> >
> > >> > static struct dn_route *dn_rt_cache_get_next(struct seq_file *seq, struct dn_route *rt)
> > >> > {
> > >> > struct dn_rt_cache_iter_state *s = rcu_dereference(seq->private);
> > >> >
> > >> > rt = rt->u.dst.dn_next;
> > >> > while(!rt) {
> > >> > rcu_read_unlock_bh();
> > >> > if (--s->bucket < 0)
> > >> > break;
>
> Thank you for the info! Stupid question #3: what sequence of events
> would the smp_read_barrier_depends() be defending against?
The reading of rt from the hash bucket and the dereferencing above.
We need to make sure that we see the initialised rt.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2007-11-05 23:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-10-29 21:15 dn_route.c momentarily exiting RCU read-side critical section Paul E. McKenney
2007-10-30 8:10 ` David Miller
2007-10-30 15:12 ` Paul E. McKenney
2007-11-05 11:53 ` Herbert Xu
2007-11-05 16:12 ` Paul E. McKenney
2007-11-05 23:51 ` Herbert Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).