[PATCH scsi] cxgb4i : Avoid holding mutex in interrupt context

[PATCH scsi] cxgb4i : Avoid holding mutex in interrupt context

[Anish Bhatt]

cxgbi_inet6addr_handler() can be called in interrupt context, so use rcu
protected list while finding netdev

Applies on top of core-for-3.18

Signed-off-by: Anish Bhatt <anish@chelsio.com>
Signed-off-by: Karen Xie <kxie@chelsio.com>

Fixes: fc8d0590d914 ("libcxgbi: Add ipv6 api to driver")
Fixes: 759a0cc5a3e1 ("cxgb4i: Add ipv6 code to driver, call into libcxgbi ipv6
api")
---
 drivers/scsi/cxgbi/cxgb4i/cxgb4i.c |  2 +-
 drivers/scsi/cxgbi/libcxgbi.c      | 57 ++++++++++++++++++++++++++++++++++----
 drivers/scsi/cxgbi/libcxgbi.h      |  3 ++
 3 files changed, 55 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
index 79788a1..02e69e7 100644
--- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
+++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
@@ -1647,7 +1647,7 @@ static int cxgbi_inet6addr_handler(struct notifier_block *this,
 	if (event_dev->priv_flags & IFF_802_1Q_VLAN)
 		event_dev = vlan_dev_real_dev(event_dev);
 
-	cdev = cxgbi_device_find_by_netdev(event_dev, NULL);
+	cdev = cxgbi_device_find_by_netdev_rcu(event_dev, NULL);
 
 	if (!cdev)
 		return ret;
diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
index d65df6d..addd1dd 100644
--- a/drivers/scsi/cxgbi/libcxgbi.c
+++ b/drivers/scsi/cxgbi/libcxgbi.c
@@ -57,6 +57,9 @@ MODULE_PARM_DESC(dbg_level, "libiscsi debug level (default=0)");
 static LIST_HEAD(cdev_list);
 static DEFINE_MUTEX(cdev_mutex);
 
+static LIST_HEAD(cdev_rcu_list);
+static DEFINE_SPINLOCK(cdev_rcu_lock);
+
 int cxgbi_device_portmap_create(struct cxgbi_device *cdev, unsigned int base,
 				unsigned int max_conn)
 {
@@ -142,6 +145,10 @@ struct cxgbi_device *cxgbi_device_register(unsigned int extra,
 	list_add_tail(&cdev->list_head, &cdev_list);
 	mutex_unlock(&cdev_mutex);
 
+	spin_lock(&cdev_rcu_lock);
+	list_add_tail_rcu(&cdev->rcu_node, &cdev_rcu_list);
+	spin_unlock(&cdev_rcu_lock);
+
 	log_debug(1 << CXGBI_DBG_DEV,
 		"cdev 0x%p, p# %u.\n", cdev, nports);
 	return cdev;
@@ -153,9 +160,16 @@ void cxgbi_device_unregister(struct cxgbi_device *cdev)
 	log_debug(1 << CXGBI_DBG_DEV,
 		"cdev 0x%p, p# %u,%s.\n",
 		cdev, cdev->nports, cdev->nports ? cdev->ports[0]->name : "");
+
 	mutex_lock(&cdev_mutex);
 	list_del(&cdev->list_head);
 	mutex_unlock(&cdev_mutex);
+
+	spin_lock(&cdev_rcu_lock);
+	list_del_rcu(&cdev->rcu_node);
+	spin_unlock(&cdev_rcu_lock);
+	synchronize_rcu();
+
 	cxgbi_device_destroy(cdev);
 }
 EXPORT_SYMBOL_GPL(cxgbi_device_unregister);
@@ -167,12 +181,9 @@ void cxgbi_device_unregister_all(unsigned int flag)
 	mutex_lock(&cdev_mutex);
 	list_for_each_entry_safe(cdev, tmp, &cdev_list, list_head) {
 		if ((cdev->flags & flag) == flag) {
-			log_debug(1 << CXGBI_DBG_DEV,
-				"cdev 0x%p, p# %u,%s.\n",
-				cdev, cdev->nports, cdev->nports ?
-				 cdev->ports[0]->name : "");
-			list_del(&cdev->list_head);
-			cxgbi_device_destroy(cdev);
+			mutex_unlock(&cdev_mutex);
+			cxgbi_device_unregister(cdev);
+			mutex_lock(&cdev_mutex);
 		}
 	}
 	mutex_unlock(&cdev_mutex);
@@ -191,6 +202,7 @@ struct cxgbi_device *cxgbi_device_find_by_lldev(void *lldev)
 		}
 	}
 	mutex_unlock(&cdev_mutex);
+
 	log_debug(1 << CXGBI_DBG_DEV,
 		"lldev 0x%p, NO match found.\n", lldev);
 	return NULL;
@@ -230,6 +242,39 @@ struct cxgbi_device *cxgbi_device_find_by_netdev(struct net_device *ndev,
 }
 EXPORT_SYMBOL_GPL(cxgbi_device_find_by_netdev);
 
+struct cxgbi_device *cxgbi_device_find_by_netdev_rcu(struct net_device *ndev,
+						     int *port)
+{
+	struct net_device *vdev = NULL;
+	struct cxgbi_device *cdev;
+	int i;
+
+	if (ndev->priv_flags & IFF_802_1Q_VLAN) {
+		vdev = ndev;
+		ndev = vlan_dev_real_dev(ndev);
+		pr_info("vlan dev %s -> %s.\n", vdev->name, ndev->name);
+	}
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(cdev, &cdev_rcu_list, rcu_node) {
+		for (i = 0; i < cdev->nports; i++) {
+			if (ndev == cdev->ports[i]) {
+				cdev->hbas[i]->vdev = vdev;
+				rcu_read_unlock();
+				if (port)
+					*port = i;
+				return cdev;
+			}
+		}
+	}
+	rcu_read_unlock();
+
+	log_debug(1 << CXGBI_DBG_DEV,
+		  "ndev 0x%p, %s, NO match found.\n", ndev, ndev->name);
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(cxgbi_device_find_by_netdev_rcu);
+
 static struct cxgbi_device *cxgbi_device_find_by_mac(struct net_device *ndev,
 						     int *port)
 {
diff --git a/drivers/scsi/cxgbi/libcxgbi.h b/drivers/scsi/cxgbi/libcxgbi.h
index b3e6e75..1d98fad 100644
--- a/drivers/scsi/cxgbi/libcxgbi.h
+++ b/drivers/scsi/cxgbi/libcxgbi.h
@@ -527,6 +527,7 @@ struct cxgbi_ports_map {
 #define CXGBI_FLAG_IPV4_SET		0x10
 struct cxgbi_device {
 	struct list_head list_head;
+	struct list_head rcu_node;
 	unsigned int flags;
 	struct net_device **ports;
 	void *lldev;
@@ -709,6 +710,8 @@ void cxgbi_device_unregister(struct cxgbi_device *);
 void cxgbi_device_unregister_all(unsigned int flag);
 struct cxgbi_device *cxgbi_device_find_by_lldev(void *);
 struct cxgbi_device *cxgbi_device_find_by_netdev(struct net_device *, int *);
+struct cxgbi_device *cxgbi_device_find_by_netdev_rcu(struct net_device *,
+						     int *);
 int cxgbi_hbas_add(struct cxgbi_device *, u64, unsigned int,
 			struct scsi_host_template *,
 			struct scsi_transport_template *);
-- 
2.1.0

RE: [PATCH scsi] cxgb4i : Avoid holding mutex in interrupt context

[Anish Bhatt]

Pinging for visibility
-Anish
________________________________________
From: Anish Bhatt [anish@chelsio.com]
Sent: Monday, September 15, 2014 5:44 PM
To: linux-scsi@vger.kernel.org
Cc: JBottomley@parallels.com; hch@infradead.org; Karen Xie; Manoj Malviya; Anish Bhatt
Subject: [PATCH scsi] cxgb4i : Avoid holding mutex in interrupt context

cxgbi_inet6addr_handler() can be called in interrupt context, so use rcu
protected list while finding netdev

Applies on top of core-for-3.18

Signed-off-by: Anish Bhatt <anish@chelsio.com>
Signed-off-by: Karen Xie <kxie@chelsio.com>

Fixes: fc8d0590d914 ("libcxgbi: Add ipv6 api to driver")
Fixes: 759a0cc5a3e1 ("cxgb4i: Add ipv6 code to driver, call into libcxgbi ipv6
api")
---
 drivers/scsi/cxgbi/cxgb4i/cxgb4i.c |  2 +-
 drivers/scsi/cxgbi/libcxgbi.c      | 57 ++++++++++++++++++++++++++++++++++----
 drivers/scsi/cxgbi/libcxgbi.h      |  3 ++
 3 files changed, 55 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
index 79788a1..02e69e7 100644
--- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
+++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
@@ -1647,7 +1647,7 @@ static int cxgbi_inet6addr_handler(struct notifier_block *this,
        if (event_dev->priv_flags & IFF_802_1Q_VLAN)
                event_dev = vlan_dev_real_dev(event_dev);

-       cdev = cxgbi_device_find_by_netdev(event_dev, NULL);
+       cdev = cxgbi_device_find_by_netdev_rcu(event_dev, NULL);

        if (!cdev)
                return ret;
diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
index d65df6d..addd1dd 100644
--- a/drivers/scsi/cxgbi/libcxgbi.c
+++ b/drivers/scsi/cxgbi/libcxgbi.c
@@ -57,6 +57,9 @@ MODULE_PARM_DESC(dbg_level, "libiscsi debug level (default=0)");
 static LIST_HEAD(cdev_list);
 static DEFINE_MUTEX(cdev_mutex);

+static LIST_HEAD(cdev_rcu_list);
+static DEFINE_SPINLOCK(cdev_rcu_lock);
+
 int cxgbi_device_portmap_create(struct cxgbi_device *cdev, unsigned int base,
                                unsigned int max_conn)
 {
@@ -142,6 +145,10 @@ struct cxgbi_device *cxgbi_device_register(unsigned int extra,
        list_add_tail(&cdev->list_head, &cdev_list);
        mutex_unlock(&cdev_mutex);

+       spin_lock(&cdev_rcu_lock);
+       list_add_tail_rcu(&cdev->rcu_node, &cdev_rcu_list);
+       spin_unlock(&cdev_rcu_lock);
+
        log_debug(1 << CXGBI_DBG_DEV,
                "cdev 0x%p, p# %u.\n", cdev, nports);
        return cdev;
@@ -153,9 +160,16 @@ void cxgbi_device_unregister(struct cxgbi_device *cdev)
        log_debug(1 << CXGBI_DBG_DEV,
                "cdev 0x%p, p# %u,%s.\n",
                cdev, cdev->nports, cdev->nports ? cdev->ports[0]->name : "");
+
        mutex_lock(&cdev_mutex);
        list_del(&cdev->list_head);
        mutex_unlock(&cdev_mutex);
+
+       spin_lock(&cdev_rcu_lock);
+       list_del_rcu(&cdev->rcu_node);
+       spin_unlock(&cdev_rcu_lock);
+       synchronize_rcu();
+
        cxgbi_device_destroy(cdev);
 }
 EXPORT_SYMBOL_GPL(cxgbi_device_unregister);
@@ -167,12 +181,9 @@ void cxgbi_device_unregister_all(unsigned int flag)
        mutex_lock(&cdev_mutex);
        list_for_each_entry_safe(cdev, tmp, &cdev_list, list_head) {
                if ((cdev->flags & flag) == flag) {
-                       log_debug(1 << CXGBI_DBG_DEV,
-                               "cdev 0x%p, p# %u,%s.\n",
-                               cdev, cdev->nports, cdev->nports ?
-                                cdev->ports[0]->name : "");
-                       list_del(&cdev->list_head);
-                       cxgbi_device_destroy(cdev);
+                       mutex_unlock(&cdev_mutex);
+                       cxgbi_device_unregister(cdev);
+                       mutex_lock(&cdev_mutex);
                }
        }
        mutex_unlock(&cdev_mutex);
@@ -191,6 +202,7 @@ struct cxgbi_device *cxgbi_device_find_by_lldev(void *lldev)
                }
        }
        mutex_unlock(&cdev_mutex);
+
        log_debug(1 << CXGBI_DBG_DEV,
                "lldev 0x%p, NO match found.\n", lldev);
        return NULL;
@@ -230,6 +242,39 @@ struct cxgbi_device *cxgbi_device_find_by_netdev(struct net_device *ndev,
 }
 EXPORT_SYMBOL_GPL(cxgbi_device_find_by_netdev);

+struct cxgbi_device *cxgbi_device_find_by_netdev_rcu(struct net_device *ndev,
+                                                    int *port)
+{
+       struct net_device *vdev = NULL;
+       struct cxgbi_device *cdev;
+       int i;
+
+       if (ndev->priv_flags & IFF_802_1Q_VLAN) {
+               vdev = ndev;
+               ndev = vlan_dev_real_dev(ndev);
+               pr_info("vlan dev %s -> %s.\n", vdev->name, ndev->name);
+       }
+
+       rcu_read_lock();
+       list_for_each_entry_rcu(cdev, &cdev_rcu_list, rcu_node) {
+               for (i = 0; i < cdev->nports; i++) {
+                       if (ndev == cdev->ports[i]) {
+                               cdev->hbas[i]->vdev = vdev;
+                               rcu_read_unlock();
+                               if (port)
+                                       *port = i;
+                               return cdev;
+                       }
+               }
+       }
+       rcu_read_unlock();
+
+       log_debug(1 << CXGBI_DBG_DEV,
+                 "ndev 0x%p, %s, NO match found.\n", ndev, ndev->name);
+       return NULL;
+}
+EXPORT_SYMBOL_GPL(cxgbi_device_find_by_netdev_rcu);
+
 static struct cxgbi_device *cxgbi_device_find_by_mac(struct net_device *ndev,
                                                     int *port)
 {
diff --git a/drivers/scsi/cxgbi/libcxgbi.h b/drivers/scsi/cxgbi/libcxgbi.h
index b3e6e75..1d98fad 100644
--- a/drivers/scsi/cxgbi/libcxgbi.h
+++ b/drivers/scsi/cxgbi/libcxgbi.h
@@ -527,6 +527,7 @@ struct cxgbi_ports_map {
 #define CXGBI_FLAG_IPV4_SET            0x10
 struct cxgbi_device {
        struct list_head list_head;
+       struct list_head rcu_node;
        unsigned int flags;
        struct net_device **ports;
        void *lldev;
@@ -709,6 +710,8 @@ void cxgbi_device_unregister(struct cxgbi_device *);
 void cxgbi_device_unregister_all(unsigned int flag);
 struct cxgbi_device *cxgbi_device_find_by_lldev(void *);
 struct cxgbi_device *cxgbi_device_find_by_netdev(struct net_device *, int *);
+struct cxgbi_device *cxgbi_device_find_by_netdev_rcu(struct net_device *,
+                                                    int *);
 int cxgbi_hbas_add(struct cxgbi_device *, u64, unsigned int,
                        struct scsi_host_template *,
                        struct scsi_transport_template *);
--
2.1.0

Re: [PATCH scsi] cxgb4i : Avoid holding mutex in interrupt context

[Christoph Hellwig]

Thanks,

applied.

RE: [PATCH scsi] cxgb4i : Avoid holding mutex in interrupt context

[Anish Bhatt]

Little late, but I was hoping this would be pushed as part of fixes to 3.17. 
Is it too late for that ?
-Anish
________________________________________
From: Christoph Hellwig [hch@infradead.org]
Sent: Monday, September 22, 2014 7:45 AM
To: Anish Bhatt
Cc: linux-scsi@vger.kernel.org; JBottomley@parallels.com; Karen Xie; Manoj Malviya
Subject: Re: [PATCH scsi] cxgb4i : Avoid holding mutex in interrupt context

Thanks,

applied.

Re: [PATCH scsi] cxgb4i : Avoid holding mutex in interrupt context

[Christoph Hellwig]

On Wed, Oct 01, 2014 at 05:22:59AM +0000, Anish Bhatt wrote:
> Little late, but I was hoping this would be pushed as part of fixes to 3.17. 
> Is it too late for that ?

It's in the drivers-for-3.18 tree as there was no indication you wanted
it for 3.17.  In fact the description said "Applies on top of
core-for-3.18".  You can try to persuade James to cherry pick if for
3.17, but given how late we are in the cycle that might need very good
argument.  You can always send it to stable@vger.kernel.org as soon as
Linus pulls the SCSI update for 3.18, though.

RE: [PATCH scsi] cxgb4i : Avoid holding mutex in interrupt context

[Anish Bhatt]

James, 
    This is bugfix  and prevents a "scheduling while atomic" bug that can be
hit when using an ipv6 address with cxgb4i, and can possibly be seen without
Chelsio hardware. Could this be cherry picked for 3.17 please ?

Christoph,
   My bad, this is a bugfix for code that was added for 3.17, and I assumed the
bugfixes would automatically be picked for the next rc. If James doesn't pick 
this up, I'll send it to stable when appropriate.
-Anish
________________________________________
From: Christoph Hellwig [hch@infradead.org]
Sent: Wednesday, October 01, 2014 12:49 AM
To: Anish Bhatt
Cc: Christoph Hellwig; linux-scsi@vger.kernel.org; JBottomley@parallels.com; Karen Xie; Manoj Malviya
Subject: Re: [PATCH scsi] cxgb4i : Avoid holding mutex in interrupt context

On Wed, Oct 01, 2014 at 05:22:59AM +0000, Anish Bhatt wrote:
> Little late, but I was hoping this would be pushed as part of fixes to 3.17.
> Is it too late for that ?

It's in the drivers-for-3.18 tree as there was no indication you wanted
it for 3.17.  In fact the description said "Applies on top of
core-for-3.18".  You can try to persuade James to cherry pick if for
3.17, but given how late we are in the cycle that might need very good
argument.  You can always send it to stable@vger.kernel.org as soon as
Linus pulls the SCSI update for 3.18, though.

Re: [PATCH scsi] cxgb4i : Avoid holding mutex in interrupt context

[James Bottomley]

On Wed, 2014-10-01 at 08:08 +0000, Anish Bhatt wrote:
> James, 
>     This is bugfix  and prevents a "scheduling while atomic" bug that can be
> hit when using an ipv6 address with cxgb4i, and can possibly be seen without
> Chelsio hardware. Could this be cherry picked for 3.17 please ?

Can you describe the bug it's fixing (what are the consequences) and how
often is it seen?

If it's not something everyone hits, it may be better to go via stable,
because the code change is quite large, so validating that nothing else
breaks with it at this stage (a week away from release) is quite
important.

> Christoph,
>    My bad, this is a bugfix for code that was added for 3.17, and I assumed the
> bugfixes would automatically be picked for the next rc. If James doesn't pick 
> this up, I'll send it to stable when appropriate.

You said in your change log:

        Applies on top of core-for-3.18

So the natural assumption would be this is for the merge window queue.

James

RE: [PATCH scsi] cxgb4i : Avoid holding mutex in interrupt context

[Karen Xie]

James, the bug is that with iscsi running, if ipv6 is provisioned on system, the kernel would oops on "scheduling while atomic". So it could be hit easily if ipv6 is used.

The changeset does look big, but it really just split a list original protected by mutex lock into two list, The 2nd list uses rcu lock.

Karen

________________________________________
From: James Bottomley [James.Bottomley@HansenPartnership.com]
Sent: Wednesday, October 01, 2014 4:36 AM
To: Anish Bhatt
Cc: Christoph Hellwig; linux-scsi@vger.kernel.org; Karen Xie; Manoj Malviya
Subject: Re: [PATCH scsi] cxgb4i : Avoid holding mutex in interrupt context

On Wed, 2014-10-01 at 08:08 +0000, Anish Bhatt wrote:
> James,
>     This is bugfix  and prevents a "scheduling while atomic" bug that can be
> hit when using an ipv6 address with cxgb4i, and can possibly be seen without
> Chelsio hardware. Could this be cherry picked for 3.17 please ?

Can you describe the bug it's fixing (what are the consequences) and how
often is it seen?

If it's not something everyone hits, it may be better to go via stable,
because the code change is quite large, so validating that nothing else
breaks with it at this stage (a week away from release) is quite
important.

> Christoph,
>    My bad, this is a bugfix for code that was added for 3.17, and I assumed the
> bugfixes would automatically be picked for the next rc. If James doesn't pick
> this up, I'll send it to stable when appropriate.

You said in your change log:

        Applies on top of core-for-3.18

So the natural assumption would be this is for the merge window queue.

James