| Message ID | 1544566543-3395-2-git-send-email-pkaustub@cisco.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | IB/usnic: usnic bug fixes | expand |
On Tue, Dec 11, 2018 at 02:15:41PM -0800, Parvi Kaustubhi wrote: > From: Govindarajulu Varadarajan <gvaradar@cisco.com> > > There is a dead lock in usnic ib_register() and netdev_notify() path. > > usnic_ib_discover_pf() > | mutex_lock(&usnic_ib_ibdev_list_lock); > | usnic_ib_device_add(); > | ib_register_device() > | usnic_ib_query_port() > | mutex_lock(&us_ibdev->usdev_lock); > | ib_get_eth_speed() > | rtnl_lock() > > Order of locking: &usnic_ib_ibdev_list_lock -> usdev_lock -> rtnl_lock > > rtnl_lock() > | usnic_ib_netdevice_event() > | mutex_lock(&usnic_ib_ibdev_list_lock); > > Order of locking: rtnl_lock -> &usnic_ib_ibdev_list_lock > > The solution is to not handle usnic_ib_netdevice_event() with the rtnl > lock held. > This commit creates a single threaded workqueue to defer the handling of > netdev/inet events. > > Signed-off-by: Govindarajulu Varadarajan <gvaradar@cisco.com> > Signed-off-by: Parvi Kaustubhi <pkaustub@cisco.com> > --- > drivers/infiniband/hw/usnic/usnic_ib.h | 6 +++ > drivers/infiniband/hw/usnic/usnic_ib_main.c | 60 ++++++++++++++++++++++++----- > 2 files changed, 56 insertions(+), 10 deletions(-) I'm really getting sick of seeing this same code pattern in the ethernet connecting drivers.. I think this is the 3rd time now I've seen a fix for this same rtnl locking bug in a driver. Does anyone want to make a proper driver core based struct device_driver for this discovery model??? Also, this surely has the same bugs as rxe and bnxt, see those recent threads too. Jason
On Tue, Dec 11, 2018 at 11:06:49PM +0000, Jason Gunthorpe wrote: > On Tue, Dec 11, 2018 at 02:15:41PM -0800, Parvi Kaustubhi wrote: > > From: Govindarajulu Varadarajan <gvaradar@cisco.com> > > > > There is a dead lock in usnic ib_register() and netdev_notify() path. > > > > usnic_ib_discover_pf() > > | mutex_lock(&usnic_ib_ibdev_list_lock); > > | usnic_ib_device_add(); > > | ib_register_device() > > | usnic_ib_query_port() > > | mutex_lock(&us_ibdev->usdev_lock); > > | ib_get_eth_speed() > > | rtnl_lock() > > > > Order of locking: &usnic_ib_ibdev_list_lock -> usdev_lock -> rtnl_lock > > > > rtnl_lock() > > | usnic_ib_netdevice_event() > > | mutex_lock(&usnic_ib_ibdev_list_lock); > > > > Order of locking: rtnl_lock -> &usnic_ib_ibdev_list_lock > > > > The solution is to not handle usnic_ib_netdevice_event() with the rtnl > > lock held. > > This commit creates a single threaded workqueue to defer the handling of > > netdev/inet events. > > > > Signed-off-by: Govindarajulu Varadarajan <gvaradar@cisco.com> > > Signed-off-by: Parvi Kaustubhi <pkaustub@cisco.com> > > --- > > drivers/infiniband/hw/usnic/usnic_ib.h | 6 +++ > > drivers/infiniband/hw/usnic/usnic_ib_main.c | 60 ++++++++++++++++++++++++----- > > 2 files changed, 56 insertions(+), 10 deletions(-) > > I'm really getting sick of seeing this same code pattern in the > ethernet connecting drivers.. I think this is the 3rd time now I've > seen a fix for this same rtnl locking bug in a driver. I had the same feeling while I read the patch. Thanks
On Tue, 2018-12-11 at 23:06 +0000, Jason Gunthorpe wrote: > On Tue, Dec 11, 2018 at 02:15:41PM -0800, Parvi Kaustubhi wrote: > > From: Govindarajulu Varadarajan <gvaradar@cisco.com> > > > > There is a dead lock in usnic ib_register() and netdev_notify() path. > > > > usnic_ib_discover_pf() > > > mutex_lock(&usnic_ib_ibdev_list_lock); > > | usnic_ib_device_add(); > > | ib_register_device() > > | usnic_ib_query_port() > > | mutex_lock(&us_ibdev->usdev_lock); > > | ib_get_eth_speed() > > | rtnl_lock() > > > > Order of locking: &usnic_ib_ibdev_list_lock -> usdev_lock -> rtnl_lock > > > > rtnl_lock() > > > usnic_ib_netdevice_event() > > | mutex_lock(&usnic_ib_ibdev_list_lock); > > > > Order of locking: rtnl_lock -> &usnic_ib_ibdev_list_lock > > > > The solution is to not handle usnic_ib_netdevice_event() with the rtnl > > lock held. > > This commit creates a single threaded workqueue to defer the handling of > > netdev/inet events. > > > > Signed-off-by: Govindarajulu Varadarajan <gvaradar@cisco.com> > > Signed-off-by: Parvi Kaustubhi <pkaustub@cisco.com> > > --- > > drivers/infiniband/hw/usnic/usnic_ib.h | 6 +++ > > drivers/infiniband/hw/usnic/usnic_ib_main.c | 60 ++++++++++++++++++++++++--- > > -- > > 2 files changed, 56 insertions(+), 10 deletions(-) > > I'm really getting sick of seeing this same code pattern in the > ethernet connecting drivers.. I think this is the 3rd time now I've > seen a fix for this same rtnl locking bug in a driver. > Is the issue here using a workqueue to handle notify? > Does anyone want to make a proper driver core based struct > device_driver for this discovery model??? > Can you elaborate on this? I do not follow. > Also, this surely has the same bugs as rxe and bnxt, see those recent > threads too. Removing usnic_verbs modules works fine. I do not see issue. I will have to try this with lockdep on.
On Fri, Dec 14, 2018 at 02:53:00AM +0000, Govindarajulu Varadarajan (gvaradar) wrote: > On Tue, 2018-12-11 at 23:06 +0000, Jason Gunthorpe wrote: > > On Tue, Dec 11, 2018 at 02:15:41PM -0800, Parvi Kaustubhi wrote: > > > From: Govindarajulu Varadarajan <gvaradar@cisco.com> > > > > > > There is a dead lock in usnic ib_register() and netdev_notify() path. > > > > > > usnic_ib_discover_pf() > > > > mutex_lock(&usnic_ib_ibdev_list_lock); > > > | usnic_ib_device_add(); > > > | ib_register_device() > > > | usnic_ib_query_port() > > > | mutex_lock(&us_ibdev->usdev_lock); > > > | ib_get_eth_speed() > > > | rtnl_lock() > > > > > > Order of locking: &usnic_ib_ibdev_list_lock -> usdev_lock -> rtnl_lock > > > > > > rtnl_lock() > > > > usnic_ib_netdevice_event() > > > | mutex_lock(&usnic_ib_ibdev_list_lock); > > > > > > Order of locking: rtnl_lock -> &usnic_ib_ibdev_list_lock > > > > > > The solution is to not handle usnic_ib_netdevice_event() with the rtnl > > > lock held. > > > This commit creates a single threaded workqueue to defer the handling of > > > netdev/inet events. > > > > > > Signed-off-by: Govindarajulu Varadarajan <gvaradar@cisco.com> > > > Signed-off-by: Parvi Kaustubhi <pkaustub@cisco.com> > > > drivers/infiniband/hw/usnic/usnic_ib.h | 6 +++ > > > drivers/infiniband/hw/usnic/usnic_ib_main.c | 60 ++++++++++++++++++++++++--- > > > > I'm really getting sick of seeing this same code pattern in the > > ethernet connecting drivers.. I think this is the 3rd time now I've > > seen a fix for this same rtnl locking bug in a driver. > > Is the issue here using a workqueue to handle notify? The issue is all the code duplication.. Jason
diff --git a/drivers/infiniband/hw/usnic/usnic_ib.h b/drivers/infiniband/hw/usnic/usnic_ib.h index 525bf27..aff4eb4 100644 --- a/drivers/infiniband/hw/usnic/usnic_ib.h +++ b/drivers/infiniband/hw/usnic/usnic_ib.h @@ -93,6 +93,12 @@ struct usnic_ib_vf { struct list_head link; }; +struct usnic_work { + struct work_struct work; + unsigned long event; + void *ptr; +}; + static inline struct usnic_ib_dev *to_usdev(struct ib_device *ibdev) { diff --git a/drivers/infiniband/hw/usnic/usnic_ib_main.c b/drivers/infiniband/hw/usnic/usnic_ib_main.c index 413fa57..dd28442 100644 --- a/drivers/infiniband/hw/usnic/usnic_ib_main.c +++ b/drivers/infiniband/hw/usnic/usnic_ib_main.c @@ -71,6 +71,7 @@ static const char usnic_version[] = static DEFINE_MUTEX(usnic_ib_ibdev_list_lock); static LIST_HEAD(usnic_ib_ibdev_list); +static struct workqueue_struct *usnic_notify_work; /* Callback dump funcs */ static int usnic_ib_dump_vf_hdr(void *obj, char *buf, int buf_sz) @@ -212,21 +213,35 @@ static void usnic_ib_handle_usdev_event(struct usnic_ib_dev *us_ibdev, mutex_unlock(&us_ibdev->usdev_lock); } -static int usnic_ib_netdevice_event(struct notifier_block *notifier, - unsigned long event, void *ptr) +static void usnic_ib_netdevice_work(struct work_struct *work) { + struct usnic_work *uswork = container_of(work, struct usnic_work, work); + struct net_device *netdev = netdev_notifier_info_to_dev(uswork->ptr); struct usnic_ib_dev *us_ibdev; - struct net_device *netdev = netdev_notifier_info_to_dev(ptr); - mutex_lock(&usnic_ib_ibdev_list_lock); list_for_each_entry(us_ibdev, &usnic_ib_ibdev_list, ib_dev_link) { if (us_ibdev->netdev == netdev) { - usnic_ib_handle_usdev_event(us_ibdev, event); + usnic_ib_handle_usdev_event(us_ibdev, uswork->event); break; } } mutex_unlock(&usnic_ib_ibdev_list_lock); + kfree(uswork); +} + +static int usnic_ib_netdevice_event(struct notifier_block *nblock, + unsigned long event, void *ptr) +{ + struct usnic_work *uswork; + + uswork = kzalloc(sizeof(*uswork), GFP_ATOMIC); + if (uswork) { + uswork->event = event; + uswork->ptr = ptr; + INIT_WORK(&uswork->work, usnic_ib_netdevice_work); + queue_work(usnic_notify_work, &uswork->work); + } return NOTIFY_DONE; } @@ -276,24 +291,42 @@ static int usnic_ib_handle_inet_event(struct usnic_ib_dev *us_ibdev, return NOTIFY_DONE; } -static int usnic_ib_inetaddr_event(struct notifier_block *notifier, - unsigned long event, void *ptr) +static void usnic_ib_inetaddr_work(struct work_struct *work) { + struct usnic_work *uswork = container_of(work, struct usnic_work, work); struct usnic_ib_dev *us_ibdev; - struct in_ifaddr *ifa = ptr; + struct in_ifaddr *ifa = uswork->ptr; struct net_device *netdev = ifa->ifa_dev->dev; mutex_lock(&usnic_ib_ibdev_list_lock); list_for_each_entry(us_ibdev, &usnic_ib_ibdev_list, ib_dev_link) { if (us_ibdev->netdev == netdev) { - usnic_ib_handle_inet_event(us_ibdev, event, ptr); + usnic_ib_handle_inet_event(us_ibdev, uswork->event, + uswork->ptr); break; } } mutex_unlock(&usnic_ib_ibdev_list_lock); + kfree(uswork); +} + +static int usnic_ib_inetaddr_event(struct notifier_block *nblock, + unsigned long event, void *ptr) +{ + struct usnic_work *uswork; + + uswork = kzalloc(sizeof(*uswork), GFP_ATOMIC); + + if (uswork) { + uswork->ptr = ptr; + uswork->event = event; + INIT_WORK(&uswork->work, usnic_ib_inetaddr_work); + queue_work(usnic_notify_work, &uswork->work); + } return NOTIFY_DONE; } + static struct notifier_block usnic_ib_inetaddr_notifier = { .notifier_call = usnic_ib_inetaddr_event }; @@ -653,10 +686,15 @@ static int __init usnic_ib_init(void) return err; } + usnic_notify_work = create_singlethread_workqueue("usnic_notify_work"); + if (!usnic_notify_work) { + usnic_err("Failed to create notify workqueue"); + goto out_umem_fini; + } err = pci_register_driver(&usnic_ib_pci_driver); if (err) { usnic_err("Unable to register with PCI\n"); - goto out_umem_fini; + goto out_notify_work; } err = register_netdevice_notifier(&usnic_ib_netdevice_notifier); @@ -687,6 +725,8 @@ static int __init usnic_ib_init(void) unregister_netdevice_notifier(&usnic_ib_netdevice_notifier); out_pci_unreg: pci_unregister_driver(&usnic_ib_pci_driver); +out_notify_work: + destroy_workqueue(usnic_notify_work); out_umem_fini: usnic_uiom_fini();