Messages in this thread Patch in this message | | | Date | Fri, 14 Oct 2005 09:38:54 -0700 (PDT) | From | Suzanne Wood <> | Subject | Re: [RFC][PATCH] rcu in drivers/net/hamradio |
| |
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; } - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |