LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH-next] scsi: libsas: dynamically allocate and free ata host
@ 2018-05-07  2:11 Jason Yan
  2018-05-07  9:16 ` kbuild test robot
  0 siblings, 1 reply; 2+ messages in thread
From: Jason Yan @ 2018-05-07  2:11 UTC (permalink / raw)
  To: martin.petersen, jejb
  Cc: linux-scsi, linux-kernel, john.garry, zhaohongjiang,
	dan.j.williams, Jason Yan

Now the ata host for libsas is embedded in domain_device, and the ->kref
member is not initialized. Afer we add ata transport class,
ata_host_get() will be called when adding transport ATA port and a
warning will be triggered as below:

refcount_t: increment on 0; use-after-free.
WARNING: CPU: 2 PID: 103 at lib/refcount.c:153 refcount_inc+0x40/0x48
......
Call trace:
 refcount_inc+0x40/0x48
 ata_host_get+0x10/0x18
 ata_tport_add+0x40/0x120
 ata_sas_tport_add+0xc/0x14
 sas_ata_init+0x7c/0xc8
 sas_discover_domain+0x380/0x53c
 process_one_work+0x12c/0x288
 worker_thread+0x58/0x3f0
 kthread+0xfc/0x128
 ret_from_fork+0x10/0x18

And also when removing transport ATA port ata_host_put() will be called
and another similar warning will be triggered. If the refcount decreased
to zero, the ata host will be freed. But this ata host is only part of
domain_device, it cannot be freed directly.

So we have to change this embedded static ata host to a dynamically
allocated ata host and initialize the ->kref member. To use
ata_host_get() and ata_host_put() in libsas, we need to move the
declaration of these functions to the public libata.h.

Fixes: b6240a4df018 ("scsi: libsas: add transport class for ATA devices")
Signed-off-by: Jason Yan <yanaijie@huawei.com>
CC: John Garry <john.garry@huawei.com>
---
 drivers/ata/libata-core.c          |  1 +
 drivers/ata/libata.h               |  2 --
 drivers/scsi/libsas/sas_ata.c      | 40 +++++++++++++++++++++++++-------------
 drivers/scsi/libsas/sas_discover.c |  2 ++
 include/linux/libata.h             |  2 ++
 include/scsi/libsas.h              |  2 +-
 6 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 68596bd4cf06..33c2dfbbc02d 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6424,6 +6424,7 @@ void ata_host_init(struct ata_host *host, struct device *dev,
 	host->n_tags = ATA_MAX_QUEUE - 1;
 	host->dev = dev;
 	host->ops = ops;
+	kref_init(&host->kref);
 }
 
 void __ata_port_probe(struct ata_port *ap)
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 9e21c49cf6be..f953cb4bb1ba 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -100,8 +100,6 @@ extern int ata_port_probe(struct ata_port *ap);
 extern void __ata_port_probe(struct ata_port *ap);
 extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 				      u8 page, void *buf, unsigned int sectors);
-extern void ata_host_get(struct ata_host *host);
-extern void ata_host_put(struct ata_host *host);
 
 #define to_ata_port(d) container_of(d, struct ata_port, tdev)
 
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index ff1d612f6fb9..41cdda7a926b 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -557,34 +557,46 @@ int sas_ata_init(struct domain_device *found_dev)
 {
 	struct sas_ha_struct *ha = found_dev->port->ha;
 	struct Scsi_Host *shost = ha->core.shost;
+	struct ata_host *ata_host;
 	struct ata_port *ap;
 	int rc;
 
-	ata_host_init(&found_dev->sata_dev.ata_host, ha->dev, &sas_sata_ops);
-	ap = ata_sas_port_alloc(&found_dev->sata_dev.ata_host,
-				&sata_port_info,
-				shost);
+	ata_host = kzalloc(sizeof(*ata_host), GFP_KERNEL);
+	if (!ata_host)	{
+		SAS_DPRINTK("ata host alloc failed.\n");
+		return -ENOMEM;
+	}
+
+	ata_host_init(ata_host, ha->dev, &sas_sata_ops);
+
+	ap = ata_sas_port_alloc(ata_host, &sata_port_info, shost);
 	if (!ap) {
 		SAS_DPRINTK("ata_sas_port_alloc failed.\n");
-		return -ENODEV;
+		rc = -ENODEV;
+		goto free_host;
 	}
 
 	ap->private_data = found_dev;
 	ap->cbl = ATA_CBL_SATA;
 	ap->scsi_host = shost;
 	rc = ata_sas_port_init(ap);
-	if (rc) {
-		ata_sas_port_destroy(ap);
-		return rc;
-	}
-	rc = ata_sas_tport_add(found_dev->sata_dev.ata_host.dev, ap);
-	if (rc) {
-		ata_sas_port_destroy(ap);
-		return rc;
-	}
+	if (rc)
+		goto destroy_port;
+
+	rc = ata_sas_tport_add(ata_host->dev, ap);
+	if (rc)
+		goto destroy_port;
+
+	found_dev->sata_dev.ata_host = ata_host;
 	found_dev->sata_dev.ap = ap;
 
 	return 0;
+
+destroy_port:
+	ata_sas_port_destroy(ap);
+free_host:
+	ata_host_put(ata_host);
+	return rc;
 }
 
 void sas_ata_task_abort(struct sas_task *task)
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 1ffca28fe6a8..0148ae62a52a 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -316,6 +316,8 @@ void sas_free_device(struct kref *kref)
 	if (dev_is_sata(dev) && dev->sata_dev.ap) {
 		ata_sas_tport_delete(dev->sata_dev.ap);
 		ata_sas_port_destroy(dev->sata_dev.ap);
+		ata_host_put(dev->sata_dev.ata_host);
+		dev->sata_dev.ata_host = NULL;
 		dev->sata_dev.ap = NULL;
 	}
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 0619ebf4d475..852cdb2fc692 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1110,6 +1110,8 @@ extern struct ata_host *ata_host_alloc(struct device *dev, int max_ports);
 extern struct ata_host *ata_host_alloc_pinfo(struct device *dev,
 			const struct ata_port_info * const * ppi, int n_ports);
 extern int ata_slave_link_init(struct ata_port *ap);
+extern void ata_host_get(struct ata_host *host);
+extern void ata_host_put(struct ata_host *host);
 extern int ata_host_start(struct ata_host *host);
 extern int ata_host_register(struct ata_host *host,
 			     struct scsi_host_template *sht);
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 225ab7783dfd..3de3b10da19a 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -161,7 +161,7 @@ struct sata_device {
 	u8     port_no;        /* port number, if this is a PM (Port) */
 
 	struct ata_port *ap;
-	struct ata_host ata_host;
+	struct ata_host *ata_host;
 	struct smp_resp rps_resp ____cacheline_aligned; /* report_phy_sata_resp */
 	u8     fis[ATA_RESP_FIS_SIZE];
 };
-- 
2.13.6

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

* Re: [PATCH-next] scsi: libsas: dynamically allocate and free ata host
  2018-05-07  2:11 [PATCH-next] scsi: libsas: dynamically allocate and free ata host Jason Yan
@ 2018-05-07  9:16 ` kbuild test robot
  0 siblings, 0 replies; 2+ messages in thread
From: kbuild test robot @ 2018-05-07  9:16 UTC (permalink / raw)
  To: linux-kernel-owner
  Cc: kbuild-all, martin.petersen, jejb, linux-scsi, linux-kernel,
	john.garry, zhaohongjiang, dan.j.williams, Jason Yan

[-- Attachment #1: Type: text/plain, Size: 949 bytes --]

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on next-20180504]
[cannot apply to v4.17-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/linux-kernel-owner-vger-kernel-org/scsi-libsas-dynamically-allocate-and-free-ata-host/20180507-105656
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-federa-25 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "ata_host_put" [drivers/scsi/libsas/libsas.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 47763 bytes --]

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

end of thread, other threads:[~2018-05-07  9:17 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-07  2:11 [PATCH-next] scsi: libsas: dynamically allocate and free ata host Jason Yan
2018-05-07  9:16 ` kbuild test robot

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).