Re: [PATCH 2/4] ata,scsi: libata-core: Do not leak memory for ata_port struct members

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 29/06/2024 13:42, Niklas Cassel wrote:
libsas is currently not freeing all the struct ata_port struct members,
e.g. ncq_sense_buf for a driver supporting Command Duration Limits (CDL).

Add a function, ata_port_free(), that is used to free a ata_port,
including its struct members. It makes sense to keep the code related to
freeing a ata_port in its own function, which will also free all the
struct members of struct ata_port.

Fixes: 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL commands using policy 0xD")
Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>

Apart from a couple of nitpicks:

Reviewed-by: John Garry <john.g.garry@xxxxxxxxxx>

---
  drivers/ata/libata-core.c          | 24 ++++++++++++++----------
  drivers/scsi/libsas/sas_ata.c      |  2 +-
  drivers/scsi/libsas/sas_discover.c |  2 +-
  include/linux/libata.h             |  1 +
  4 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index f47838da75d7..481baa55ebfc 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5490,6 +5490,18 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
  	return ap;
  }
+void ata_port_free(struct ata_port *ap)
+{
+	if (!ap)
+		return;

nit: personally I'd have the caller check this. The main reason for that is that often it seems an unnecessary check.

+
+	kfree(ap->pmp_link);
+	kfree(ap->slave_link);
+	kfree(ap->ncq_sense_buf);
+	kfree(ap);
+}
+EXPORT_SYMBOL_GPL(ata_port_free);
+
  static void ata_devres_release(struct device *gendev, void *res)
  {
  	struct ata_host *host = dev_get_drvdata(gendev);
@@ -5516,15 +5528,7 @@ static void ata_host_release(struct kref *kref)
  	int i;
for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap = host->ports[i];
-
-		if (!ap)
-			continue;
-
-		kfree(ap->pmp_link);
-		kfree(ap->slave_link);
-		kfree(ap->ncq_sense_buf);
-		kfree(ap);
+		ata_port_free(host->ports[i]);
  		host->ports[i] = NULL;
  	}
  	kfree(host);
@@ -5907,7 +5911,7 @@ int ata_host_register(struct ata_host *host, const struct scsi_host_template *sh
  	 * allocation time.
  	 */
  	for (i = host->n_ports; host->ports[i]; i++)
-		kfree(host->ports[i]);
+		ata_port_free(host->ports[i]);
/* give ports names and add SCSI hosts */
  	for (i = 0; i < host->n_ports; i++) {
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 4c69fc63c119..1f247a8cd185 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -618,7 +618,7 @@ int sas_ata_init(struct domain_device *found_dev)
  	return 0;
destroy_port:
-	kfree(ap);
+	ata_port_free(ap);

nit: If not a nuisance, could we change the label name to free_port, like free_host, below. No big deal.

  free_host:
  	ata_host_put(ata_host);
  	return rc;
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 8fb7c41c0962..48d975c6dbf2 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -301,7 +301,7 @@ void sas_free_device(struct kref *kref)
if (dev_is_sata(dev) && dev->sata_dev.ap) {
  		ata_sas_tport_delete(dev->sata_dev.ap);
-		kfree(dev->sata_dev.ap);
+		ata_port_free(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 13fb41d25da6..7d3bd7c9664a 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1249,6 +1249,7 @@ extern int ata_slave_link_init(struct ata_port *ap);
  extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
  					   struct ata_port_info *, struct Scsi_Host *);
  extern void ata_port_probe(struct ata_port *ap);
+extern void ata_port_free(struct ata_port *ap);
  extern int ata_sas_tport_add(struct device *parent, struct ata_port *ap);
  extern void ata_sas_tport_delete(struct ata_port *ap);
  int ata_sas_device_configure(struct scsi_device *sdev, struct queue_limits *lim,





[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux