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;
}
Suzanne Wood <[email protected]> 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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]> 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: [email protected]
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;
}
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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Thank you and sorry if I'm belaboring this.
> From [email protected] 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;
}
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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Suzanne Wood <[email protected]> :
[...]
> (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
Suzanne Wood <[email protected]> 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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Many thanks for your direction and insights.
> From [email protected] 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: [email protected]
-------
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;
}
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: [email protected]
Looks good to me.
Acked-by: Herbert Xu <[email protected]>
You might want to send this patch to [email protected] who
is the current maintainer.
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> Date: Sun, 16 Oct 2005 17:21:36 +1000
> From: Herbert Xu <[email protected]>
> 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: [email protected]
> Looks good to me.
> Acked-by: Herbert Xu <[email protected]>
> You might want to send this patch to [email protected] 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;
}