[PATCH 0/4] libata/libsas cleanup fixes

[PATCH 0/4] libata/libsas cleanup fixes

[Niklas Cassel]

Hello there,

This series takes the patches that are -stable material from this series:
https://lore.kernel.org/linux-ide/20240626180031.4050226-15-cassel@kernel.org/

Changes since series above:
-Fixed minor review comments for the four patches included in the series.
-Picked up tags.


Kind regards,
Niklas


Niklas Cassel (4):
  ata: libata-core: Fix null pointer dereference on error
  ata,scsi: libata-core: Do not leak memory for ata_port struct members
  ata: libata-core: Fix double free on error
  ata: ahci: Clean up sysfs file on error

 drivers/ata/ahci.c                 | 17 ++++++++++++-----
 drivers/ata/libata-core.c          | 29 ++++++++++++++++++-----------
 drivers/scsi/libsas/sas_ata.c      |  2 +-
 drivers/scsi/libsas/sas_discover.c |  2 +-
 include/linux/libata.h             |  1 +
 5 files changed, 33 insertions(+), 18 deletions(-)

-- 
2.45.2

[PATCH 1/4] ata: libata-core: Fix null pointer dereference on error

[Niklas Cassel]

If the ata_port_alloc() call in ata_host_alloc() fails,
ata_host_release() will get called.

However, the code in ata_host_release() tries to free ata_port struct
members unconditionally, which can lead to the following:

BUG: unable to handle page fault for address: 0000000000003990
PGD 0 P4D 0
Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
CPU: 10 PID: 594 Comm: (udev-worker) Not tainted 6.10.0-rc5 #44
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
RIP: 0010:ata_host_release.cold+0x2f/0x6e [libata]
Code: e4 4d 63 f4 44 89 e2 48 c7 c6 90 ad 32 c0 48 c7 c7 d0 70 33 c0 49 83 c6 0e 41
RSP: 0018:ffffc90000ebb968 EFLAGS: 00010246
RAX: 0000000000000041 RBX: ffff88810fb52e78 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff88813b3218c0 RDI: ffff88813b3218c0
RBP: ffff88810fb52e40 R08: 0000000000000000 R09: 6c65725f74736f68
R10: ffffc90000ebb738 R11: 73692033203a746e R12: 0000000000000004
R13: 0000000000000000 R14: 0000000000000011 R15: 0000000000000006
FS:  00007f6cc55b9980(0000) GS:ffff88813b300000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000003990 CR3: 00000001122a2000 CR4: 0000000000750ef0
PKRU: 55555554
Call Trace:
 <TASK>
 ? __die_body.cold+0x19/0x27
 ? page_fault_oops+0x15a/0x2f0
 ? exc_page_fault+0x7e/0x180
 ? asm_exc_page_fault+0x26/0x30
 ? ata_host_release.cold+0x2f/0x6e [libata]
 ? ata_host_release.cold+0x2f/0x6e [libata]
 release_nodes+0x35/0xb0
 devres_release_group+0x113/0x140
 ata_host_alloc+0xed/0x120 [libata]
 ata_host_alloc_pinfo+0x14/0xa0 [libata]
 ahci_init_one+0x6c9/0xd20 [ahci]

Do not access ata_port struct members unconditionally.

Fixes: 633273a3ed1c ("libata-pmp: hook PMP support and enable it")
Cc: stable@vger.kernel.org
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index e1bf8a19b3c8..f47838da75d7 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5518,6 +5518,9 @@ static void ata_host_release(struct kref *kref)
 	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);
-- 
2.45.2

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

[Niklas Cassel]

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@kernel.org>
---
 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;
+
+	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);
 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,
-- 
2.45.2

[PATCH 3/4] ata: libata-core: Fix double free on error

[Niklas Cassel]

If e.g. the ata_port_alloc() call in ata_host_alloc() fails, we will jump
to the err_out label, which will call devres_release_group().
devres_release_group() will trigger a call to ata_host_release().
ata_host_release() calls kfree(host), so executing the kfree(host) in
ata_host_alloc() will lead to a double free:

kernel BUG at mm/slub.c:553!
Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
CPU: 11 PID: 599 Comm: (udev-worker) Not tainted 6.10.0-rc5 #47
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
RIP: 0010:kfree+0x2cf/0x2f0
Code: 5d 41 5e 41 5f 5d e9 80 d6 ff ff 4d 89 f1 41 b8 01 00 00 00 48 89 d9 48 89 da
RSP: 0018:ffffc90000f377f0 EFLAGS: 00010246
RAX: ffff888112b1f2c0 RBX: ffff888112b1f2c0 RCX: ffff888112b1f320
RDX: 000000000000400b RSI: ffffffffc02c9de5 RDI: ffff888112b1f2c0
RBP: ffffc90000f37830 R08: 0000000000000000 R09: 0000000000000000
R10: ffffc90000f37610 R11: 617461203a736b6e R12: ffffea00044ac780
R13: ffff888100046400 R14: ffffffffc02c9de5 R15: 0000000000000006
FS:  00007f2f1cabe980(0000) GS:ffff88813b380000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f2f1c3acf75 CR3: 0000000111724000 CR4: 0000000000750ef0
PKRU: 55555554
Call Trace:
 <TASK>
 ? __die_body.cold+0x19/0x27
 ? die+0x2e/0x50
 ? do_trap+0xca/0x110
 ? do_error_trap+0x6a/0x90
 ? kfree+0x2cf/0x2f0
 ? exc_invalid_op+0x50/0x70
 ? kfree+0x2cf/0x2f0
 ? asm_exc_invalid_op+0x1a/0x20
 ? ata_host_alloc+0xf5/0x120 [libata]
 ? ata_host_alloc+0xf5/0x120 [libata]
 ? kfree+0x2cf/0x2f0
 ata_host_alloc+0xf5/0x120 [libata]
 ata_host_alloc_pinfo+0x14/0xa0 [libata]
 ahci_init_one+0x6c9/0xd20 [ahci]

Ensure that we will not call kfree(host) twice, by performing the kfree()
only if the devres_open_group() call failed.

Fixes: dafd6c496381 ("libata: ensure host is free'd on error exit paths")
Cc: stable@vger.kernel.org
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 481baa55ebfc..e0455a182af7 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5578,8 +5578,10 @@ struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
 	if (!host)
 		return NULL;
 
-	if (!devres_open_group(dev, NULL, GFP_KERNEL))
-		goto err_free;
+	if (!devres_open_group(dev, NULL, GFP_KERNEL)) {
+		kfree(host);
+		return NULL;
+	}
 
 	dr = devres_alloc(ata_devres_release, 0, GFP_KERNEL);
 	if (!dr)
@@ -5611,8 +5613,6 @@ struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
 
  err_out:
 	devres_release_group(dev, NULL);
- err_free:
-	kfree(host);
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(ata_host_alloc);
-- 
2.45.2

[PATCH 4/4] ata: ahci: Clean up sysfs file on error

[Niklas Cassel]

.probe() (ahci_init_one()) calls sysfs_add_file_to_group(), however,
if probe() fails after this call, we currently never call
sysfs_remove_file_from_group().

(The sysfs_remove_file_from_group() call in .remove() (ahci_remove_one())
does not help, as .remove() is not called on .probe() error.)

Thus, if probe() fails after the sysfs_add_file_to_group() call, the next
time we insmod the module we will get:

sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:04.0/remapped_nvme'
CPU: 11 PID: 954 Comm: modprobe Not tainted 6.10.0-rc5 #43
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
Call Trace:
 <TASK>
 dump_stack_lvl+0x5d/0x80
 sysfs_warn_dup.cold+0x17/0x23
 sysfs_add_file_mode_ns+0x11a/0x130
 sysfs_add_file_to_group+0x7e/0xc0
 ahci_init_one+0x31f/0xd40 [ahci]

Fixes: 894fba7f434a ("ata: ahci: Add sysfs attribute to show remapped NVMe device count")
Cc: stable@vger.kernel.org
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 5eb38fbbbecd..fc6fd583faf8 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1975,8 +1975,10 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
 
 	host = ata_host_alloc_pinfo(&pdev->dev, ppi, n_ports);
-	if (!host)
-		return -ENOMEM;
+	if (!host) {
+		rc = -ENOMEM;
+		goto err_rm_sysfs_file;
+	}
 	host->private_data = hpriv;
 
 	if (ahci_init_msi(pdev, n_ports, hpriv) < 0) {
@@ -2031,11 +2033,11 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* initialize adapter */
 	rc = ahci_configure_dma_masks(pdev, hpriv);
 	if (rc)
-		return rc;
+		goto err_rm_sysfs_file;
 
 	rc = ahci_pci_reset_controller(host);
 	if (rc)
-		return rc;
+		goto err_rm_sysfs_file;
 
 	ahci_pci_init_controller(host);
 	ahci_pci_print_info(host);
@@ -2044,10 +2046,15 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	rc = ahci_host_activate(host, &ahci_sht);
 	if (rc)
-		return rc;
+		goto err_rm_sysfs_file;
 
 	pm_runtime_put_noidle(&pdev->dev);
 	return 0;
+
+err_rm_sysfs_file:
+	sysfs_remove_file_from_group(&pdev->dev.kobj,
+				     &dev_attr_remapped_nvme.attr, NULL);
+	return rc;
 }
 
 static void ahci_shutdown_one(struct pci_dev *pdev)
-- 
2.45.2

Re: [PATCH 1/4] ata: libata-core: Fix null pointer dereference on error

[John Garry]

> 
> Fixes: 633273a3ed1c ("libata-pmp: hook PMP support and enable it")
> Cc:stable@vger.kernel.org
> Reviewed-by: Damien Le Moal<dlemoal@kernel.org>
> Reviewed-by: Hannes Reinecke<hare@suse.de>
> Signed-off-by: Niklas Cassel<cassel@kernel.org>
> ---

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

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

[John Garry]

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@kernel.org>

Apart from a couple of nitpicks:

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

> ---
>   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,

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

[Niklas Cassel]

On Sun, Jun 30, 2024 at 10:42:45AM +0100, John Garry wrote:
> 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@kernel.org>
> 
> Apart from a couple of nitpicks:
> 
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> 
> > ---
> >   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.

People are used to free() family of functions handling NULL,
so I think it is wise to keep it as is.


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

Sure, will fixup when applying.


> 
> >   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,
> 

Re: [PATCH 0/4] libata/libsas cleanup fixes

[Niklas Cassel]

On Sat, Jun 29, 2024 at 02:42:10PM +0200, Niklas Cassel wrote:
> Hello there,
> 
> This series takes the patches that are -stable material from this series:
> https://lore.kernel.org/linux-ide/20240626180031.4050226-15-cassel@kernel.org/
> 
> Changes since series above:
> -Fixed minor review comments for the four patches included in the series.
> -Picked up tags.
> 
> 
> Kind regards,
> Niklas
> 
> 
> Niklas Cassel (4):
>   ata: libata-core: Fix null pointer dereference on error
>   ata,scsi: libata-core: Do not leak memory for ata_port struct members
>   ata: libata-core: Fix double free on error
>   ata: ahci: Clean up sysfs file on error
> 
>  drivers/ata/ahci.c                 | 17 ++++++++++++-----
>  drivers/ata/libata-core.c          | 29 ++++++++++++++++++-----------
>  drivers/scsi/libsas/sas_ata.c      |  2 +-
>  drivers/scsi/libsas/sas_discover.c |  2 +-
>  include/linux/libata.h             |  1 +
>  5 files changed, 33 insertions(+), 18 deletions(-)

Martin, James,

considering that the libsas change is extremely trivial,
and that the libsas maintainer (John) has added his R-b tag,
I will simply take this via the libata tree.

Please tell me if you have a problem with this, and it will
never happen again. (I would never do this for something that
wasn't extremely trivial.)


Kind regards,
Niklas

Re: [PATCH 0/4] libata/libsas cleanup fixes

[Niklas Cassel]

On Sat, 29 Jun 2024 14:42:10 +0200, Niklas Cassel wrote:
> This series takes the patches that are -stable material from this series:
> https://lore.kernel.org/linux-ide/20240626180031.4050226-15-cassel@kernel.org/
> 
> Changes since series above:
> -Fixed minor review comments for the four patches included in the series.
> -Picked up tags.
> 
> [...]

Applied to libata/linux.git (for-6.10-fixes), thanks!

[1/4] ata: libata-core: Fix null pointer dereference on error
      https://git.kernel.org/libata/linux/c/5d92c7c5
[2/4] ata,scsi: libata-core: Do not leak memory for ata_port struct members
      https://git.kernel.org/libata/linux/c/f6549f53
[3/4] ata: libata-core: Fix double free on error
      https://git.kernel.org/libata/linux/c/ab9e0c52
[4/4] ata: ahci: Clean up sysfs file on error
      https://git.kernel.org/libata/linux/c/eeb25a09

Kind regards,
Niklas

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

[Hannes Reinecke]

On 6/29/24 14: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@kernel.org>
> ---
>   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(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 0/4] libata/libsas cleanup fixes

[Martin K. Petersen]

Niklas,

> considering that the libsas change is extremely trivial, and that the
> libsas maintainer (John) has added his R-b tag, I will simply take
> this via the libata tree.

That's fine!

-- 
Martin K. Petersen	Oracle Linux Engineering