LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] scsi: qla4xxx: add error handling for try_module_get
@ 2018-06-12  4:48 Zhouyang Jia
  2018-06-12  7:52 ` Rangankar, Manish
  2018-06-12 14:08 ` James Bottomley
  0 siblings, 2 replies; 3+ messages in thread
From: Zhouyang Jia @ 2018-06-12  4:48 UTC (permalink / raw)
  Cc: Zhouyang Jia, QLogic-Storage-Upstream, James E.J. Bottomley,
	Martin K. Petersen, linux-scsi, linux-kernel

When try_module_get fails, the lack of error-handling code may
cause unexpected results.

This patch adds error-handling code after calling try_module_get.

Signed-off-by: Zhouyang Jia <jiazhouyang09@gmail.com>
---
 drivers/scsi/qla4xxx/ql4_os.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index 0e13349..6b677ab 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -7687,7 +7687,10 @@ static int qla4xxx_sysfs_ddb_logout_sid(struct iscsi_cls_session *cls_sess)
 	 * to be seamless without actually destroying the
 	 * session
 	 **/
-	try_module_get(qla4xxx_iscsi_transport.owner);
+	if (!try_module_get(qla4xxx_iscsi_transport.owner))
+		ql4_printk(KERN_WARNING, ha,
+			"%s: cannot get module.\n", __func__);
+
 	iscsi_destroy_endpoint(ddb_entry->conn->ep);
 
 	spin_lock_irqsave(&ha->hardware_lock, flags);
@@ -8970,7 +8973,9 @@ static void qla4xxx_destroy_fw_ddb_session(struct scsi_qla_host *ha)
 			 * to be seamless without actually destroying the
 			 * session
 			 **/
-			try_module_get(qla4xxx_iscsi_transport.owner);
+			if (!try_module_get(qla4xxx_iscsi_transport.owner))
+				ql4_printk(KERN_WARNING, ha,
+					"%s: cannot get module.\n", __func__);
 			iscsi_destroy_endpoint(ddb_entry->conn->ep);
 			qla4xxx_free_ddb(ha, ddb_entry);
 			iscsi_session_teardown(ddb_entry->sess);
-- 
2.7.4


^ permalink raw reply	[flat|nested] 3+ messages in thread

* RE: [PATCH] scsi: qla4xxx: add error handling for try_module_get
  2018-06-12  4:48 [PATCH] scsi: qla4xxx: add error handling for try_module_get Zhouyang Jia
@ 2018-06-12  7:52 ` Rangankar, Manish
  2018-06-12 14:08 ` James Bottomley
  1 sibling, 0 replies; 3+ messages in thread
From: Rangankar, Manish @ 2018-06-12  7:52 UTC (permalink / raw)
  To: Zhouyang Jia
  Cc: Dept-Eng QLogic Storage Upstream, James E.J. Bottomley,
	Martin K. Petersen, linux-scsi, linux-kernel


> -----Original Message-----
> From: Zhouyang Jia <jiazhouyang09@gmail.com>
> Sent: Tuesday, June 12, 2018 10:18 AM
> Cc: Zhouyang Jia <jiazhouyang09@gmail.com>; Dept-Eng QLogic Storage
> Upstream <QLogic-Storage-Upstream@cavium.com>; James E.J. Bottomley
> <jejb@linux.vnet.ibm.com>; Martin K. Petersen <martin.petersen@oracle.com>;
> linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH] scsi: qla4xxx: add error handling for try_module_get
> 
> When try_module_get fails, the lack of error-handling code may cause
> unexpected results.
> 
> This patch adds error-handling code after calling try_module_get.
> 
> Signed-off-by: Zhouyang Jia <jiazhouyang09@gmail.com>
> ---
>  drivers/scsi/qla4xxx/ql4_os.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c index
> 0e13349..6b677ab 100644
> --- a/drivers/scsi/qla4xxx/ql4_os.c
> +++ b/drivers/scsi/qla4xxx/ql4_os.c
> @@ -7687,7 +7687,10 @@ static int qla4xxx_sysfs_ddb_logout_sid(struct
> iscsi_cls_session *cls_sess)
>  	 * to be seamless without actually destroying the
>  	 * session
>  	 **/
> -	try_module_get(qla4xxx_iscsi_transport.owner);
> +	if (!try_module_get(qla4xxx_iscsi_transport.owner))
> +		ql4_printk(KERN_WARNING, ha,
> +			"%s: cannot get module.\n", __func__);
> +
>  	iscsi_destroy_endpoint(ddb_entry->conn->ep);
> 
>  	spin_lock_irqsave(&ha->hardware_lock, flags); @@ -8970,7 +8973,9
> @@ static void qla4xxx_destroy_fw_ddb_session(struct scsi_qla_host *ha)
>  			 * to be seamless without actually destroying the
>  			 * session
>  			 **/
> -			try_module_get(qla4xxx_iscsi_transport.owner);
> +			if (!try_module_get(qla4xxx_iscsi_transport.owner))
> +				ql4_printk(KERN_WARNING, ha,
> +					"%s: cannot get module.\n",
> __func__);
>  			iscsi_destroy_endpoint(ddb_entry->conn->ep);
>  			qla4xxx_free_ddb(ha, ddb_entry);
>  			iscsi_session_teardown(ddb_entry->sess);
> --
> 2.7.4

Thanks,

Acked-by: Manish Rangankar <Manish.Rangankar@cavium.com>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] scsi: qla4xxx: add error handling for try_module_get
  2018-06-12  4:48 [PATCH] scsi: qla4xxx: add error handling for try_module_get Zhouyang Jia
  2018-06-12  7:52 ` Rangankar, Manish
@ 2018-06-12 14:08 ` James Bottomley
  1 sibling, 0 replies; 3+ messages in thread
From: James Bottomley @ 2018-06-12 14:08 UTC (permalink / raw)
  To: Zhouyang Jia
  Cc: QLogic-Storage-Upstream, Martin K. Petersen, linux-scsi, linux-kernel

On Tue, 2018-06-12 at 12:48 +0800, Zhouyang Jia wrote:
> When try_module_get fails, the lack of error-handling code may
> cause unexpected results.
> 
> This patch adds error-handling code after calling try_module_get.
> 
> Signed-off-by: Zhouyang Jia <jiazhouyang09@gmail.com>
> ---
>  drivers/scsi/qla4xxx/ql4_os.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/qla4xxx/ql4_os.c
> b/drivers/scsi/qla4xxx/ql4_os.c
> index 0e13349..6b677ab 100644
> --- a/drivers/scsi/qla4xxx/ql4_os.c
> +++ b/drivers/scsi/qla4xxx/ql4_os.c
> @@ -7687,7 +7687,10 @@ static int qla4xxx_sysfs_ddb_logout_sid(struct
> iscsi_cls_session *cls_sess)
>  	 * to be seamless without actually destroying the
>  	 * session
>  	 **/
> -	try_module_get(qla4xxx_iscsi_transport.owner);
> +	if (!try_module_get(qla4xxx_iscsi_transport.owner))
> +		ql4_printk(KERN_WARNING, ha,
> +			"%s: cannot get module.\n", __func__);
> +

This isn't error handling at all.  If try_module_get() fails it means
you're about to get the text segment freed from underneath you if the
code can be executed concurrently with the module_exit function.  The
comment implies that qla4xxx operates with a zero module use count even
when there are logged in sessions, so module_exit can race with logout
(or indeed any other userspace initiated function).  This means that
the entire module reference counting of qla4xxx looks to be racy and
wrong.  Can we get a description of what the expected theoretical model
is so we can validate (or invalidate) this theory?

James




^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-06-12 14:09 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-12  4:48 [PATCH] scsi: qla4xxx: add error handling for try_module_get Zhouyang Jia
2018-06-12  7:52 ` Rangankar, Manish
2018-06-12 14:08 ` James Bottomley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).