[RFC][PATCH] rcu in drivers/net/hamradio

[RFC][PATCH] rcu in drivers/net/hamradio

[Suzanne Wood]

In a prior submittal [RFC][PATCH] identify rcu-protected ptr
posted Thu Oct 06 2005 - 01:08:25 EST, the usage of RCU in 
drivers/net/hamradio/bpqether.c was addressed in

> Please consider the effective addition of an
> rcu_dereference() in bpq_get_ax25_dev() in recognition 
> of each use being nested in an rcu critical section.

> diff a/drivers/net/hamradio/bpqether.c b/drivers/net/hamradio/bpqether.c
> --- a/drivers/net/hamradio/bpqether.c 2005-09-30 14:17:35.000000000 -0700
> +++ b/drivers/net/hamradio/bpqether.c 2005-10-05 22:32:53.000000000 -0700
. . .
> - list_for_each_entry(bpq, &bpq_devices, bpq_list) {
> + list_for_each_entry_rcu(bpq, &bpq_devices, bpq_list) {

Please let me know if any assumptions below are false.
Thank you.

The similarity to drivers/net/wan/lapbether.c supports 
the suggestion above, but the reason used earlier fails 
because I suggest now that the rcu_read_lock()/unlock() 
in bpq_device_event() should be moved due to the 
following found by considering the cases of the 
switch statement: 

(1) bpq_new_device() calls list_add_rcu() labeled as 
"list protected by RTNL."  The comment may need to go 
since the only apparent rtnl_lock()/unlock() pair encloses 
the call to bpq_free_device() in bpq_cleanup_driver()
called upon module_exit().

(2) dev_close() as defined in net/core/dev.c 
employs a memory barrier.

(3) bpq_free_device() calls list_del_rcu() which, according 
to list.h, requires synchronize_rcu() which can block or 
call_rcu() or call_rcu_bh() which cannot block. 
None of these is called anywhere in the directory drivers/net,
so synchronize_irq() may address this.  
(synchronize_sched() is called in drivers/net/sis190.c and 
r8169.c with FIXME comment about synchronize_irq().)

Because the rcu read-side critical section marked by 
rcu_read_lock()/unlock() disables preemption, it is not 
suitable in bpq_device_event() in the context of update.

If RCU is indeed appropriate in bpq_new_device() and 
bpq_free_device() of bpqether.c, then, substituting 
list_for_each_entry_rcu() for list_for_each_entry()
introduces an rcu_dereference on bpq.  This requires the 
marking of the read-side critical section, raising the 
question of the rcu_assign_pointer(), but the list_add_rcu()
introduces the corresponding write memory barrier.

We consider the statements in bpq_rcv()
	rcu_read_lock();
	dev = bpq_get_ax25_dev(dev);

and see that a result of the rcu_dereference() in
list_for_each_entry_rcu(bpq, &bpq_devices, bpq_list)
is that the future dereference of the pointer to the bpqdev 
struct bpq is rcu-protected.  But bpq_get_ax25_dev() 
returns bpq->axdev, a pointer to a net_device struct.  The 
rcu_read_lock() in bpq_rcv() likely implies that is another 
pointer to receive rcu-protected dereference.

As a starting point, please consider the following patch.
Thank you.

-------

 bpqether.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

-------


--- src/linux-2.6.14-rc4/drivers/net/hamradio/bpqether.c	2005-10-10 18:19:19.000000000 -0700
+++ patch/linux-2.6.14-rc4/drivers/net/hamradio/bpqether.c	2005-10-14 00:18:14.000000000 -0700
@@ -144,10 +144,13 @@ static inline struct net_device *bpq_get
 {
 	struct bpqdev *bpq;
 
-	list_for_each_entry(bpq, &bpq_devices, bpq_list) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(bpq, &bpq_devices, bpq_list) {
 		if (bpq->ethdev == dev)
+			rcu_read_unlock();
 			return bpq->axdev;
 	}
+	rcu_read_unlock();
 	return NULL;
 }
 
@@ -179,7 +182,7 @@ static int bpq_rcv(struct sk_buff *skb, 
 		goto drop;
 
 	rcu_read_lock();
-	dev = bpq_get_ax25_dev(dev);
+	dev = rcu_dereference(bpq_get_ax25_dev(dev));
 
 	if (dev == NULL || !netif_running(dev)) 
 		goto drop_unlock;
@@ -530,7 +533,6 @@ static int bpq_new_device(struct net_dev
 	if (err)
 		goto error;
 
-	/* List protected by RTNL */
 	list_add_rcu(&bpq->bpq_list, &bpq_devices);
 	return 0;
 
@@ -561,8 +563,6 @@ static int bpq_device_event(struct notif
 	if (!dev_is_ethdev(dev))
 		return NOTIFY_DONE;
 
-	rcu_read_lock();
-
 	switch (event) {
 	case NETDEV_UP:		/* new ethernet device -> new BPQ interface */
 		if (bpq_get_ax25_dev(dev) == NULL)
@@ -581,7 +581,6 @@ static int bpq_device_event(struct notif
 	default:
 		break;
 	}
-	rcu_read_unlock();
 
 	return NOTIFY_DONE;
 }

Re: [RFC][PATCH] rcu in drivers/net/hamradio

[Herbert Xu]

Suzanne Wood <suzannew@cs.pdx.edu> wrote:
>
> (1) bpq_new_device() calls list_add_rcu() labeled as 
> "list protected by RTNL."  The comment may need to go 
> since the only apparent rtnl_lock()/unlock() pair encloses 
> the call to bpq_free_device() in bpq_cleanup_driver()
> called upon module_exit().

The RTNL comment is correct actually.

bpq_new_device can only be called from bpq_device_event which
is called from a netdev event handler.  All netdev event handlers
must be called uner the RTNL.

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

Re: [RFC][PATCH] rcu in drivers/net/hamradio

[Francois Romieu]

Suzanne Wood <suzannew@cs.pdx.edu> :
[...]
> (3) bpq_free_device() calls list_del_rcu() which, according 
> to list.h, requires synchronize_rcu() which can block or 
> call_rcu() or call_rcu_bh() which cannot block. 
> None of these is called anywhere in the directory drivers/net,
> so synchronize_irq() may address this.  
> (synchronize_sched() is called in drivers/net/sis190.c and 
> r8169.c with FIXME comment about synchronize_irq().)

Same author for both. The code (driver specific) can be issued from
userspace and it needs to wait for running hard_start_xmit to
complete. Afaik synchronize_irq() is not adequate and the FIXME
should go.

--
Ueimor

Re: [RFC][PATCH] rcu in drivers/net/hamradio

[Herbert Xu]

Suzanne Wood <suzannew@cs.pdx.edu> wrote:
> 
> (3) bpq_free_device() calls list_del_rcu() which, according 
> to list.h, requires synchronize_rcu() which can block or 
> call_rcu() or call_rcu_bh() which cannot block. 
> None of these is called anywhere in the directory drivers/net,
> so synchronize_irq() may address this.  
> (synchronize_sched() is called in drivers/net/sis190.c and 
> r8169.c with FIXME comment about synchronize_irq().)

The synchronisation is carried out by unregister_netdevice.

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

Re: [RFC][PATCH] rcu in drivers/net/hamradio

[Suzanne Wood]

  > Date: Sun, 16 Oct 2005 17:21:36 +1000
  > From: Herbert Xu <herbert@gondor.apana.org.au>

  > On Sat, Oct 15, 2005 at 03:24:57PM -0700, Suzanne Wood wrote:
  > > 
  > > Please find attached a patch to drivers/net/hamradio/bpqether.c
  > > that might finally merit being
  > > Signed-off-by: suzannew@cs.pdx.edu

  > Looks good to me.

  > Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

  > You might want to send this patch to ralf@linux-mips.org who
  > is the current maintainer.

Thank you for providing the next step, too.

-------

ChangeLog: clarify RCU implementation in 
drivers/net/hamradio/bpqether.c 

Because bpq_new_device() calls list_add_rcu()
and bpq_free_device() calls list_del_rcu(),
substitute list_for_each_entry_rcu() for 
list_for_each_entry() in bpq_get_ax25_dev()
and in bpq_seq_start().

Add rcu dereference protection in bpq_seq_next().

The rcu_read_lock()/unlock() in bpq_device_event() 
are removed because netdev event handlers are called 
with RTNL locking in place.

FYI: bpq_free_device() calls list_del_rcu() which, per 
list.h, requires synchronize_rcu() which can block or 
call_rcu() or call_rcu_bh() which cannot block. 
Herbert Xu notes that synchronization is done here by 
unregister_netdevice().  This calls synchronize_net()
which in turn uses synchronize_rcu().

-------

 bpqether.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

-------

--- src/linux-2.6.14-rc4/drivers/net/hamradio/bpqether.c	2005-10-10 18:19:19.000000000 -0700
+++ patch/linux-2.6.14-rc4/drivers/net/hamradio/bpqether.c	2005-10-15 15:12:15.000000000 -0700
@@ -144,7 +144,7 @@ static inline struct net_device *bpq_get
 {
 	struct bpqdev *bpq;
 
-	list_for_each_entry(bpq, &bpq_devices, bpq_list) {
+	list_for_each_entry_rcu(bpq, &bpq_devices, bpq_list) {
 		if (bpq->ethdev == dev)
 			return bpq->axdev;
 	}
@@ -399,7 +399,7 @@ static void *bpq_seq_start(struct seq_fi
 	if (*pos == 0)
 		return SEQ_START_TOKEN;
 	
-	list_for_each_entry(bpqdev, &bpq_devices, bpq_list) {
+	list_for_each_entry_rcu(bpqdev, &bpq_devices, bpq_list) {
 		if (i == *pos)
 			return bpqdev;
 	}
@@ -418,7 +418,7 @@ static void *bpq_seq_next(struct seq_fil
 		p = ((struct bpqdev *)v)->bpq_list.next;
 
 	return (p == &bpq_devices) ? NULL 
-		: list_entry(p, struct bpqdev, bpq_list);
+		: rcu_dereference(list_entry(p, struct bpqdev, bpq_list));
 }
 
 static void bpq_seq_stop(struct seq_file *seq, void *v)
@@ -561,8 +561,6 @@ static int bpq_device_event(struct notif
 	if (!dev_is_ethdev(dev))
 		return NOTIFY_DONE;
 
-	rcu_read_lock();
-
 	switch (event) {
 	case NETDEV_UP:		/* new ethernet device -> new BPQ interface */
 		if (bpq_get_ax25_dev(dev) == NULL)
@@ -581,7 +579,6 @@ static int bpq_device_event(struct notif
 	default:
 		break;
 	}
-	rcu_read_unlock();
 
 	return NOTIFY_DONE;
 }

Re: [RFC][PATCH] rcu in drivers/net/hamradio

[Herbert Xu]

On Sat, Oct 15, 2005 at 03:24:57PM -0700, Suzanne Wood wrote:
> 
> Please find attached a patch to drivers/net/hamradio/bpqether.c
> that might finally merit being
> Signed-off-by: suzannew@cs.pdx.edu

Looks good to me.

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

You might want to send this patch to ralf@linux-mips.org who
is the current maintainer.

Thanks,
-- 
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

Re: [RFC][PATCH] rcu in drivers/net/hamradio

[Suzanne Wood]

Many thanks for your direction and insights.

  > From herbert@gondor.apana.org.au  Sat Oct 15 01:05:52 2005

  > On Sat, Oct 15, 2005 at 12:57:23AM -0700, Suzanne Wood wrote:
  > > 
  > > Another list_for_each_entry() in bpq_seq_start() in a marked rcu 
  > > read-side critical section becomes the rcu version.

  > You might want to add an rcu_dereference in bpq_seq_next
  > as well.

Please find attached a patch to drivers/net/hamradio/bpqether.c
that might finally merit being
Signed-off-by: suzannew@cs.pdx.edu

-------

ChangeLog: clarify RCU implementation in 
drivers/net/hamradio/bpqether.c 

Because bpq_new_device() calls list_add_rcu()
and bpq_free_device() calls list_del_rcu(),
substitute list_for_each_entry_rcu() for 
list_for_each_entry() in bpq_get_ax25_dev()
and in bpq_seq_start().

Add rcu dereference protection in bpq_seq_next().

The rcu_read_lock()/unlock() in bpq_device_event() 
are removed because netdev event handlers are called 
with RTNL locking in place.

FYI: bpq_free_device() calls list_del_rcu() which, per 
list.h, requires synchronize_rcu() which can block or 
call_rcu() or call_rcu_bh() which cannot block. 
Herbert Xu notes that synchronization is done here by 
unregister_netdevice().  This calls synchronize_net()
which in turn uses synchronize_rcu().

-------

 bpqether.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

-------

--- src/linux-2.6.14-rc4/drivers/net/hamradio/bpqether.c	2005-10-10 18:19:19.000000000 -0700
+++ patch/linux-2.6.14-rc4/drivers/net/hamradio/bpqether.c	2005-10-15 15:12:15.000000000 -0700
@@ -144,7 +144,7 @@ static inline struct net_device *bpq_get
 {
 	struct bpqdev *bpq;
 
-	list_for_each_entry(bpq, &bpq_devices, bpq_list) {
+	list_for_each_entry_rcu(bpq, &bpq_devices, bpq_list) {
 		if (bpq->ethdev == dev)
 			return bpq->axdev;
 	}
@@ -399,7 +399,7 @@ static void *bpq_seq_start(struct seq_fi
 	if (*pos == 0)
 		return SEQ_START_TOKEN;
 	
-	list_for_each_entry(bpqdev, &bpq_devices, bpq_list) {
+	list_for_each_entry_rcu(bpqdev, &bpq_devices, bpq_list) {
 		if (i == *pos)
 			return bpqdev;
 	}
@@ -418,7 +418,7 @@ static void *bpq_seq_next(struct seq_fil
 		p = ((struct bpqdev *)v)->bpq_list.next;
 
 	return (p == &bpq_devices) ? NULL 
-		: list_entry(p, struct bpqdev, bpq_list);
+		: rcu_dereference(list_entry(p, struct bpqdev, bpq_list));
 }
 
 static void bpq_seq_stop(struct seq_file *seq, void *v)
@@ -561,8 +561,6 @@ static int bpq_device_event(struct notif
 	if (!dev_is_ethdev(dev))
 		return NOTIFY_DONE;
 
-	rcu_read_lock();
-
 	switch (event) {
 	case NETDEV_UP:		/* new ethernet device -> new BPQ interface */
 		if (bpq_get_ax25_dev(dev) == NULL)
@@ -581,7 +579,6 @@ static int bpq_device_event(struct notif
 	default:
 		break;
 	}
-	rcu_read_unlock();
 
 	return NOTIFY_DONE;
 }

Re: [RFC][PATCH] rcu in drivers/net/hamradio

[Herbert Xu]

On Sat, Oct 15, 2005 at 12:57:23AM -0700, Suzanne Wood wrote:
> 
> Another list_for_each_entry() in bpq_seq_start() in a marked rcu 
> read-side critical section becomes the rcu version.

Good catch.  You might want to add an rcu_dereference in bpq_seq_next
as well.

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

Re: [RFC][PATCH] rcu in drivers/net/hamradio

[Suzanne Wood]

Thank you and sorry if I'm belaboring this.

  > From herbert@gondor.apana.org.au  Fri Oct 14 15:36:36 2005

  > On Fri, Oct 14, 2005 at 09:38:54AM -0700, Suzanne Wood wrote:
  > > 
  > > ChangeLog:
  > > Because bpq_new_device() calls list_add_rcu()
  > > and bpq_free_device() calls list_del_rcu(),
  > > substitute list_for_each_entry_rcu() for 
  > > list_for_each_entry() in bpq_get_ax25_dev().
  > > This requires the insertion of rcu_read_lock()/unlock().

  > The rcu_read_lock/unlock is unnecessary because the only caller that
  > doesn't hold RTNL (bpq_rcv) already takes that lock.  In fact it's
  > better to take it there since you need to hold the lock for the duration
  > of the use of the device.

  > > A consequence of list_for_each_entry_rcu(bpq, &bpq_devices, bpq_list)
  > > is that the future dereference of the pointer to the bpqdev 
  > > struct bpq is rcu-protected.  But bpq_get_ax25_dev() 
  > > returns bpq->axdev, a pointer to a net_device struct.  The 
  > > rcu_read_lock() in bpq_rcv() likely implies that is another 
  > > pointer to receive rcu-protected dereference.

Thought this was an example of protecting a pointer dereference of an object
and then wanting deferred destruction to extend to a field of that object 
which is pointer to a different object, similar to traversing a list.
So I guess the dereference being protected is bpq->axdev and not a deref
of that pointer to the net_device struct, since we see the fields of dev
being assigned within the rcu read-side critical section.  Thank you for
correcting me on this.

  > The rcu_dereference should be provided by list_for_each_entry_rcu.
  > In fact there is a bug in the list_*_rcu macros where the first
  > element is not put through rcu_dereference.  I'll fix that up.

This is very significant.

  > > The rcu_read_lock()/unlock() in bpq_device_event() 
  > > are removed due to the following found by considering 
  > > the cases of the switch statement: 

  > Agreed.

Another list_for_each_entry() in bpq_seq_start() in a marked rcu 
read-side critical section becomes the rcu version.

Please consider a corrected patch attached.
Thank you.


-------

 bpqether.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

-------

--- src/linux-2.6.14-rc4/drivers/net/hamradio/bpqether.c	2005-10-10 18:19:19.000000000 -0700
+++ patch/linux-2.6.14-rc4/drivers/net/hamradio/bpqether.c	2005-10-15 00:42:14.000000000 -0700
@@ -144,7 +144,7 @@ static inline struct net_device *bpq_get
 {
 	struct bpqdev *bpq;
 
-	list_for_each_entry(bpq, &bpq_devices, bpq_list) {
+	list_for_each_entry_rcu(bpq, &bpq_devices, bpq_list) {
 		if (bpq->ethdev == dev)
 			return bpq->axdev;
 	}
@@ -399,7 +399,7 @@ static void *bpq_seq_start(struct seq_fi
 	if (*pos == 0)
 		return SEQ_START_TOKEN;
 	
-	list_for_each_entry(bpqdev, &bpq_devices, bpq_list) {
+	list_for_each_entry_rcu(bpqdev, &bpq_devices, bpq_list) {
 		if (i == *pos)
 			return bpqdev;
 	}
@@ -561,8 +561,6 @@ static int bpq_device_event(struct notif
 	if (!dev_is_ethdev(dev))
 		return NOTIFY_DONE;
 
-	rcu_read_lock();
-
 	switch (event) {
 	case NETDEV_UP:		/* new ethernet device -> new BPQ interface */
 		if (bpq_get_ax25_dev(dev) == NULL)
@@ -581,7 +579,6 @@ static int bpq_device_event(struct notif
 	default:
 		break;
 	}
-	rcu_read_unlock();
 
 	return NOTIFY_DONE;
 }

Re: [RFC][PATCH] rcu in drivers/net/hamradio

[Herbert Xu]

On Fri, Oct 14, 2005 at 09:38:54AM -0700, Suzanne Wood wrote:
> 
> ChangeLog:
> Because bpq_new_device() calls list_add_rcu()
> and bpq_free_device() calls list_del_rcu(),
> substitute list_for_each_entry_rcu() for 
> list_for_each_entry() in bpq_get_ax25_dev().
> This requires the insertion of rcu_read_lock()/unlock().

The rcu_read_lock/unlock is unnecessary because the only caller that
doesn't hold RTNL (bpq_rcv) already takes that lock.  In fact it's
better to take it there since you need to hold the lock for the duration
of the use of the device.

> A consequence of list_for_each_entry_rcu(bpq, &bpq_devices, bpq_list)
> is that the future dereference of the pointer to the bpqdev 
> struct bpq is rcu-protected.  But bpq_get_ax25_dev() 
> returns bpq->axdev, a pointer to a net_device struct.  The 
> rcu_read_lock() in bpq_rcv() likely implies that is another 
> pointer to receive rcu-protected dereference.

The rcu_dereference should be provided by list_for_each_entry_rcu.
In fact there is a bug in the list_*_rcu macros where the first
element is not put through rcu_dereference.  I'll fix that up.

> The rcu_read_lock()/unlock() in bpq_device_event() 
> are removed due to the following found by considering 
> the cases of the switch statement: 

Agreed.

Thanks,
-- 
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

Re: [RFC][PATCH] rcu in drivers/net/hamradio

[Suzanne Wood]

Thank you and please consider the following patch to drivers/net/hamradio/bpqether.c.

> From: "Herbert Xu" 
> Sent: Friday, October 14, 2005 6:37 AM
>
> Suzanne Wood <suzannew@cs.pdx.edu> wrote:
>>
>> (1) bpq_new_device() calls list_add_rcu() labeled as 
>> "list protected by RTNL."  The comment may need to go 
>> since the only apparent rtnl_lock()/unlock() pair encloses 
>> the call to bpq_free_device() in bpq_cleanup_driver()
>> called upon module_exit().
> 
> The RTNL comment is correct actually.
> 
> bpq_new_device can only be called from bpq_device_event which
> is called from a netdev event handler.  All netdev event handlers
> must be called uner the RTNL.

Thank you very much.  I'll reinstate the comment and  
submit this corrected patch
Signed-off-by: suzannew@cs.pdx.edu

ChangeLog:
Because bpq_new_device() calls list_add_rcu()
and bpq_free_device() calls list_del_rcu(),
substitute list_for_each_entry_rcu() for 
list_for_each_entry() in bpq_get_ax25_dev().
This requires the insertion of rcu_read_lock()/unlock().

A consequence of list_for_each_entry_rcu(bpq, &bpq_devices, bpq_list)
is that the future dereference of the pointer to the bpqdev 
struct bpq is rcu-protected.  But bpq_get_ax25_dev() 
returns bpq->axdev, a pointer to a net_device struct.  The 
rcu_read_lock() in bpq_rcv() likely implies that is another 
pointer to receive rcu-protected dereference.

The rcu_read_lock()/unlock() in bpq_device_event() 
are removed due to the following found by considering 
the cases of the switch statement: 

(1) bpq_new_device() calls list_add_rcu() labeled as 
"list protected by RTNL."  

(2) dev_close() as defined in net/core/dev.c 
employs a memory barrier.

(3) bpq_free_device() calls list_del_rcu() which, according 
to list.h, requires synchronize_rcu() which can block or 
call_rcu() or call_rcu_bh() which cannot block. 
None of these is called anywhere in the directory drivers/net,
so synchronize_irq() might address this.  

-------

 bpqether.c | 11 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-------

--- src/linux-2.6.14-rc4/drivers/net/hamradio/bpqether.c	2005-10-10 18:19:19.000000000 -0700
+++ patch/linux-2.6.14-rc4/drivers/net/hamradio/bpqether.c	2005-10-14 08:56:12.000000000 -0700
@@ -144,10 +144,13 @@ static inline struct net_device *bpq_get
 {
 	struct bpqdev *bpq;
 
-	list_for_each_entry(bpq, &bpq_devices, bpq_list) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(bpq, &bpq_devices, bpq_list) {
 		if (bpq->ethdev == dev)
+			rcu_read_unlock();
 			return bpq->axdev;
 	}
+	rcu_read_unlock();
 	return NULL;
 }
 
@@ -179,7 +182,7 @@ static int bpq_rcv(struct sk_buff *skb, 
 		goto drop;
 
 	rcu_read_lock();
-	dev = bpq_get_ax25_dev(dev);
+	dev = rcu_dereference(bpq_get_ax25_dev(dev));
 
 	if (dev == NULL || !netif_running(dev)) 
 		goto drop_unlock;
@@ -561,8 +564,6 @@ static int bpq_device_event(struct notif
 	if (!dev_is_ethdev(dev))
 		return NOTIFY_DONE;
 
-	rcu_read_lock();
-
 	switch (event) {
 	case NETDEV_UP:		/* new ethernet device -> new BPQ interface */
 		if (bpq_get_ax25_dev(dev) == NULL)
@@ -581,7 +582,6 @@ static int bpq_device_event(struct notif
 	default:
 		break;
 	}
-	rcu_read_unlock();
 
 	return NOTIFY_DONE;
 }