[PATCH v2 01/13] ata: libata-core: Fix null pointer dereference on error

[PATCH v2 01/13] 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
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index e1bf8a19b3c8..88e32f638f33 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5518,10 +5518,12 @@ static void ata_host_release(struct kref *kref)
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 
-		kfree(ap->pmp_link);
-		kfree(ap->slave_link);
-		kfree(ap->ncq_sense_buf);
-		kfree(ap);
+		if (ap) {
+			kfree(ap->pmp_link);
+			kfree(ap->slave_link);
+			kfree(ap->ncq_sense_buf);
+			kfree(ap);
+		}
 		host->ports[i] = NULL;
 	}
 	kfree(host);
-- 
2.45.2

Re: [PATCH v2 01/13] ata: libata-core: Fix null pointer dereference on error

[Damien Le Moal]

On 6/27/24 03:00, Niklas Cassel wrote:
> 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
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good, with a nit below. This should be queued as soon as possible as a
6.10 fix patch.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

> ---
>  drivers/ata/libata-core.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index e1bf8a19b3c8..88e32f638f33 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5518,10 +5518,12 @@ static void ata_host_release(struct kref *kref)
>  	for (i = 0; i < host->n_ports; i++) {
>  		struct ata_port *ap = host->ports[i];
>  
> -		kfree(ap->pmp_link);
> -		kfree(ap->slave_link);
> -		kfree(ap->ncq_sense_buf);
> -		kfree(ap);
> +		if (ap) {
> +			kfree(ap->pmp_link);
> +			kfree(ap->slave_link);
> +			kfree(ap->ncq_sense_buf);
> +			kfree(ap);
> +		}
>  		host->ports[i] = NULL;

Nit: this line can go inside the if as well. Or even better: reverse the if
condition and continue to ignore NULL ports.

	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);
		host->ports[i] = NULL;
	}

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 01/13] ata: libata-core: Fix null pointer dereference on error

[Hannes Reinecke]

On 6/26/24 20:00, Niklas Cassel wrote:
> 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
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/ata/libata-core.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index e1bf8a19b3c8..88e32f638f33 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5518,10 +5518,12 @@ static void ata_host_release(struct kref *kref)
>   	for (i = 0; i < host->n_ports; i++) {
>   		struct ata_port *ap = host->ports[i];
>   
> -		kfree(ap->pmp_link);
> -		kfree(ap->slave_link);
> -		kfree(ap->ncq_sense_buf);
> -		kfree(ap);
> +		if (ap) {
> +			kfree(ap->pmp_link);
> +			kfree(ap->slave_link);
> +			kfree(ap->ncq_sense_buf);
> +			kfree(ap);
> +		}
>   		host->ports[i] = NULL;
>   	}
>   	kfree(host);
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

[PATCH v2 02/13] 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
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 88e32f638f33..c916cbe3e099 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5573,8 +5573,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)
@@ -5606,8 +5608,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

Re: [PATCH v2 02/13] ata: libata-core: Fix double free on error

[Damien Le Moal]

On 6/27/24 03:00, Niklas Cassel wrote:
> 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
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 02/13] ata: libata-core: Fix double free on error

[Hannes Reinecke]

On 6/26/24 20:00, Niklas Cassel wrote:
> 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
> 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 88e32f638f33..c916cbe3e099 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5573,8 +5573,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)
> @@ -5606,8 +5608,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);
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

[PATCH v2 03/13] 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, we 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
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 v2 03/13] ata: ahci: Clean up sysfs file on error

[Niklas Cassel]

On Wed, Jun 26, 2024 at 08:00:33PM +0200, Niklas Cassel wrote:
> .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, we get:

Nit:
s/we get/the next time we insmod the module we will get/

Re: [PATCH v2 03/13] ata: ahci: Clean up sysfs file on error

[Damien Le Moal]

On 6/27/24 03:00, Niklas Cassel wrote:
> .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, we 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
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good. And I think same as patch 1 and 2: let's send this out as a 6.10 fix.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 03/13] ata: ahci: Clean up sysfs file on error

[Hannes Reinecke]

On 6/26/24 20:00, Niklas Cassel wrote:
> .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, we 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
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/ata/ahci.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 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