Skip to contentSkip to conversation

libata/libsas cleanup fixes

PATCH Applied Niklas Cassel 4 participants 12 messages Jun 29, 2024 – Jul 3, 2024

Subsystems (4)
lore
Share & tools
lore

Niklas Cassel

author maintainer
#0 lore raw

cover libata/libsas cleanup fixes

Hello there,

This series takes the patches that are -stable material from this series:
https://lore.kernel.org/linux-ide/[email protected]/

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(-)
Signature · 1 line
2.45.2
author maintainer
#1 lore raw

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

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")
---
 drivers/ata/libata-core.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/ata/libata-core.c +3 -0
index e1bf8a19b3c8..f47838da75d7 100644
@@ -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);
Signature · 1 line
2.45.2
author maintainer
#3 lore raw

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

In reply to 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")
---
 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(-)
drivers/ata/libata-core.c +14 -10
index f47838da75d7..481baa55ebfc 100644
@@ -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++) {
drivers/scsi/libsas/sas_ata.c +1 -1
index 4c69fc63c119..1f247a8cd185 100644
@@ -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;
drivers/scsi/libsas/sas_discover.c +1 -1
index 8fb7c41c0962..48d975c6dbf2 100644
@@ -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;
include/linux/libata.h +1 -0
index 13fb41d25da6..7d3bd7c9664a 100644
@@ -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,
Signature · 1 line
2.45.2
reviewer Reviewed-by on v1 2/4
#4 lore raw

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

Quoted 10 lines · 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")

Apart from a couple of nitpicks:

Quoted 12 lines
---
  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(-)
drivers/ata/libata-core.c +0 -0
index f47838da75d7..481baa55ebfc 100644

drivers/ata/libata-core.c struct ata_port *ata_port_alloc(struct ata_host *host) · line 5496

Quoted 7 lines
drivers/ata/libata-core.c +4 -0
@@ -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.

Quoted 42 lines
drivers/ata/libata-core.c +10 -10
+
+	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++) {
drivers/scsi/libsas/sas_ata.c +0 -0
index 4c69fc63c119..1f247a8cd185 100644

drivers/scsi/libsas/sas_ata.c int sas_ata_init(struct domain_device *found_dev) · line 621

Quoted 5 lines
drivers/scsi/libsas/sas_ata.c +1 -1
@@ -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.

Quoted 28 lines
drivers/scsi/libsas/sas_ata.c +0 -0
  free_host:
  	ata_host_put(ata_host);
  	return rc;
drivers/scsi/libsas/sas_discover.c +1 -1
index 8fb7c41c0962..48d975c6dbf2 100644
@@ -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;
include/linux/libata.h +1 -0
index 13fb41d25da6..7d3bd7c9664a 100644
@@ -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,
author maintainer
#5 lore raw

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

Quoted 23 lines · John Garry
Quoted 10 lines · 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")

Apart from a couple of nitpicks:

Quoted 7 lines
---
  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(-)
Quoted 14 lines

drivers/ata/libata-core.c struct ata_port *ata_port_alloc(struct ata_host *host) · line 5495

Quoted 11 lines
drivers/ata/libata-core.c +4 -0
index f47838da75d7..481baa55ebfc 100644
@@ -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.

Quoted 40 lines
Quoted 40 lines
drivers/ata/libata-core.c +10 -10
+
+	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++) {
drivers/scsi/libsas/sas_ata.c +0 -0
index 4c69fc63c119..1f247a8cd185 100644
Quoted 8 lines

drivers/scsi/libsas/sas_ata.c int sas_ata_init(struct domain_device *found_dev) · line 620

Quoted 5 lines
drivers/scsi/libsas/sas_ata.c +1 -1
@@ -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.

Quoted 29 lines
Quoted 28 lines
drivers/scsi/libsas/sas_ata.c +0 -0
  free_host:
  	ata_host_put(ata_host);
  	return rc;
drivers/scsi/libsas/sas_discover.c +1 -1
index 8fb7c41c0962..48d975c6dbf2 100644
@@ -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;
include/linux/libata.h +1 -0
index 13fb41d25da6..7d3bd7c9664a 100644
@@ -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,
Reviewed-by on v1 2/4
#6 lore raw

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

In reply to Niklas Cassel
Quoted 8 lines · 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.

Quoted 9 lines
Fixes 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL commands using policy 0xD")
---
  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(-)

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +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
author maintainer
#7 lore raw

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

In reply to 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")
---
 drivers/ata/libata-core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/ata/libata-core.c +4 -4
index 481baa55ebfc..e0455a182af7 100644
@@ -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);
Signature · 1 line
2.45.2
author maintainer
#8 lore raw

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

In reply to 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")
---
 drivers/ata/ahci.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
drivers/ata/ahci.c +12 -5
index 5eb38fbbbecd..fc6fd583faf8 100644
@@ -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)
Signature · 1 line
2.45.2
author maintainer
#9 lore raw
In reply to Niklas Cassel
Quoted 20 lines · Niklas Cassel

Hello there,

This series takes the patches that are -stable material from this series:
https://lore.kernel.org/linux-ide/[email protected]/

Changes since series above:

(quoted diff) +0 -2
-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

Quoted 6 lines
 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

#10 lore raw

Niklas,

Quoted 3 lines

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!

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
author maintainer Applied
#11 lore raw
In reply to Niklas Cassel

Review

Quoted 8 lines · Niklas Cassel

This series takes the patches that are -stable material from this series:
https://lore.kernel.org/linux-ide/[email protected]/

Changes since series above:

(quoted diff) +0 -2
-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