Re: [PATCH v2] ata: libata: Do not leave ata_host_stop() registered when activation fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The generic helper ata_pci_init_one() skips .host_stop cleanup on activation failure because ata_host_undo_start() destroys the devres action.
- [Medium] Missing ata_host_undo_start() on devres_open_group() failure in ata_pci_sff_activate_host() leaves the devres action registered, causing double host_stop() execution.

Pre-existing issues:
- [High] Pre-existing double-unmap and double-free in sata_fsl.c on device_create_file() failure.
--

commit 19ca4d6912533c209fda6772c9bbd7b4176f4fb0
Author: Niklas Cassel <cassel@kernel.org>

ata: libata: Do not leave ata_host_stop() registered when activation fails

ata_host_start() registers ata_host_stop() as a devres action as soon as
it has succeeded, which hands the release of the host resources over to
devres: ->port_stop() and ->host_stop() are then called by the driver
core when probe() fails.
    
ata_host_activate(), ahci_host_activate_multi_irqs() and
ata_pci_sff_activate_host() can all fail after ata_host_start() has
succeeded. A driver which releases the host resources in its probe() error
path therefore releases them twice: once itself and once through ->host_stop().

Fixes: 1896b15eddb4 ("ahci_platform: perform platform exit in host_stop() hook")
Cc: stable@vger.kernel.org
Signed-off-by: Niklas Cassel <cassel@kernel.org>

[ ... ]

> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 734e7c88439a7..faa20d817c850 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -2338,10 +2338,12 @@ int ata_pci_sff_activate_host(struct ata_host *host,
>  
>  	rc = ata_host_register(host, sht);
>  out:
> -	if (rc == 0)
> +	if (rc == 0) {
>  		devres_remove_group(dev, NULL);
> -	else
> +	} else {
>  		devres_release_group(dev, NULL);
> +		ata_host_undo_start(host);
> +	}
>  
>  	return rc;
>  }

[Severity: Medium]
When ata_host_undo_start() drops the devres action, does this cause
driver-specific teardown logic to be permanently skipped for generic callers?

For example, when pata_amd.c uses ata_pci_bmdma_init_one(), which in turn
calls ata_pci_init_one():

drivers/ata/libata-sff.c:ata_pci_init_one() {
    ...
    rc = ata_pci_sff_activate_host(host, ata_sff_interrupt, sht);
out:
    if (rc == 0)
        devres_remove_group(&pdev->dev, NULL);
    else
        devres_release_group(&pdev->dev, NULL);

    return rc;
}

Because ata_pci_init_one() relies on devres cleanup and does not manually call
host->ops->host_stop(), it appears this teardown logic will now be skipped on
activation failure.

[Severity: Medium]
Can a failure earlier in ata_pci_sff_activate_host() leak the devres action?

If devres_open_group() fails due to memory pressure, it returns directly:

drivers/ata/libata-sff.c:ata_pci_sff_activate_host() {
    ...
    rc = ata_host_start(host);
    if (rc)
        return rc;

    if (!devres_open_group(dev, NULL, GFP_KERNEL))
        return -ENOMEM;
    ...
}

This bypasses the out: label and skips ata_host_undo_start(). Since callers
like nv_init_one() have been updated by this patch to manually call
host_stop() on error:

drivers/ata/sata_nv.c:nv_init_one() {
    ...
    rc = ata_pci_sff_activate_host(host, ipriv->irq_handler, ipriv->sht);
    if (rc && host->ops->host_stop)
        host->ops->host_stop(host);
    ...
}

Will this result in a double execution of the hardware teardown logic during
driver core devres cleanup?

[ ... ]

> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index 70b210afd2911..299b2c886cbed 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -1490,8 +1490,12 @@ static int sata_fsl_probe(struct platform_device *ofdev)
>  	 * device discovery process, invoking our port_start() handler &
>  	 * error_handler() to execute a dummy Softreset EH session
>  	 */
> -	ata_host_activate(host, irq, sata_fsl_interrupt, SATA_FSL_IRQ_FLAG,
> -			  &sata_fsl_sht);
> +	retval = ata_host_activate(host, irq, sata_fsl_interrupt,
> +				   SATA_FSL_IRQ_FLAG, &sata_fsl_sht);
> +	if (retval) {
> +		sata_fsl_host_stop(host);
> +		return retval;
> +	}
>  
>  	host_priv->intr_coalescing.show = fsl_sata_intr_coalescing_show;

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under
review, but does sata_fsl_probe() suffer from a double free and double
unmap on later error paths?

If ata_host_activate() succeeds, the devres action for ata_host_stop is
successfully registered. If a later step fails (like device_create_file()),
the code jumps to error_exit_with_cleanup:

drivers/ata/sata_fsl.c:sata_fsl_probe() {
    ...
error_exit_with_cleanup:
    if (host)
        ata_host_detach(host);

    if (hcr_base)
        iounmap(hcr_base);
    kfree(host_priv);

    return retval;
}

When the driver probe fails, devres cleanup will automatically execute
sata_fsl_host_stop(), which attempts to free and unmap the exact same
resources again:

drivers/ata/sata_fsl.c:sata_fsl_host_stop() {
    struct sata_fsl_host_priv *host_priv = host->private_data;

    iounmap(host_priv->hcr_base);
    kfree(host_priv);
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909204317.5750-2-cassel@kernel.org?part=1