Re: [PATCH v2 8/9] ata: libata: Enable context analysis

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch introduces incomplete or mismatched thread safety annotations, which will cause compilation failures under Clang Context Analysis.
- [High] Adding `__assume_ctx_lock` inside a loop causes a lockset mismatch compilation error.

Pre-existing issues:
- [High] `ata_port_freeze()` is called locklessly from `mv_pmp_error_handler()`, which can corrupt the port's freeze state and race with other hardware state modifications.
- [High] `__ata_bmdma_stop()` and active command masks are accessed locklessly in the NVIDIA SWNCQ error handler, leading to potential hardware and state corruption.
- [High] Promise and SX4 error handlers perform a lockless TOCTOU check of the port freeze state followed by an unprotected read-modify-write (RMW) on the PDC_CTLSTAT hardware register.
--

commit 9cf4f2610433aecbb931b34a78ec27ff09ed3edb
Author: Bart Van Assche <bvanassche@acm.org>
Subject: ata: libata: Enable context analysis

This patch enables Clang's context analysis in the ATA subsystem to
catch locking bugs in error paths. It adds thread safety annotations
such as __must_hold and __assume_ctx_lock to various functions.

Link: https://lore.kernel.org/all/177926568868.711.3058599932884307249.tip-bot2@tip-bot2/

> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 7528831bb59a4..5930735bb901c 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -3877,6 +3877,9 @@ int ata_eh_recover(struct ata_port *ap, struct ata_reset_operations *reset_ops,
>  	ata_for_each_link(link, ap, EDGE) {
>  		struct ata_eh_context *ehc = &link->eh_context;
>  
> +		/* Tell the compiler that link->ap == ap. */
> +		__assume_ctx_lock(&link->ap->host->eh_mutex);

[Severity: High]
Will adding this __assume_ctx_lock() inside a loop cause a lockset
mismatch compilation error?

Clang's context analysis expects the lockset at a loop header to match the
lockset at the loop back-edge. Since we are asserting the lock state inside
the loop without releasing it before the iteration ends, could this trigger
a lockset divergence warning that breaks the build?

>  		if (!(ehc->i.action & ATA_EH_RESET))
>  			continue;
>  

[ ... ]

> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 84c1abd4d0ac4..ea10c12f449c7 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -2398,6 +2398,7 @@ static struct ata_queued_cmd *mv_get_active_qc(struct ata_port *ap)
>  }
>  
>  static void mv_pmp_error_handler(struct ata_port *ap)
> +	__must_hold(&ap->host->eh_mutex)
>  {
>  	unsigned int pmp, pmp_map;
>  	struct mv_port_priv *pp = ap->private_data;

[Severity: High]
This is a pre-existing issue, but does calling ata_port_freeze() locklessly
from mv_pmp_error_handler() risk corrupting the port's freeze state?

ata_port_freeze() modifies the port's internal state (ap->pflags) and
requires the host spinlock (ap->lock) to be held. Since
mv_pmp_error_handler() runs in the EH thread holding only the sleepable
eh_mutex, this lockless call could allow concurrent modification of
ap->pflags and command structures from interrupt handlers.

Should ata_eh_freeze_port(ap) be used here instead, as it acquires the
spinlock internally?

[ ... ]

> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
> index 841e7de2bba6e..19b9270658686 100644
> --- a/drivers/ata/sata_nv.c
> +++ b/drivers/ata/sata_nv.c
> @@ -1795,6 +1796,7 @@ static void nv_swncq_ncq_stop(struct ata_port *ap)
>  }
>  
>  static void nv_swncq_error_handler(struct ata_port *ap)
> +	__must_hold(&ap->host->eh_mutex)
>  {
>  	struct ata_eh_context *ehc = &ap->link.eh_context;
>  

[Severity: High]
This is a pre-existing issue, but are we accessing __ata_bmdma_stop() and
active command masks locklessly here, potentially causing state corruption?

nv_swncq_error_handler() executes holding only eh_mutex. It calls
nv_swncq_ncq_stop(), which modifies hardware registers via
__ata_bmdma_stop(ap) and nv_swncq_irq_clear(ap) without holding ap->lock.

Could this create a data race with nv_swncq_interrupt() running concurrently
on another CPU, which acquires ap->lock to modify DMA and IRQ states?

[ ... ]

> diff --git a/drivers/ata/sata_promise.c b/drivers/ata/sata_promise.c
> index 18d1eda69a4a8..c980e609b246e 100644
> --- a/drivers/ata/sata_promise.c
> +++ b/drivers/ata/sata_promise.c
> @@ -816,6 +816,7 @@ static int pdc_sata_hardreset(struct ata_link *link, unsigned int *class,
>  }
>  
>  static void pdc_error_handler(struct ata_port *ap)
> +	__must_hold(&ap->host->eh_mutex)
>  {
>  	if (!ata_port_is_frozen(ap))
>  		pdc_reset_port(ap);

[Severity: High]
This is a pre-existing issue, but does this pattern perform an unprotected
read-modify-write on the PDC_CTLSTAT hardware register?

We locklessly check !ata_port_is_frozen(ap) and then call pdc_reset_port(ap).
Because ap->lock is not held and we explicitly check that the port is not
frozen (meaning interrupts are not masked), an interrupt could fire
concurrently and execute pdc_freeze() or pdc_thaw().

Since those also perform read-modify-writes on PDC_CTLSTAT under ap->lock,
could this race silently overwrite hardware states and unintentionally unmask
interrupts?

[ ... ]

> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index e953a9c8e660b..bf113e66f0691 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -983,7 +983,8 @@ struct ata_port_operations {
>  	void (*thaw)(struct ata_port *ap);
>  	struct ata_reset_operations reset;
>  	struct ata_reset_operations pmp_reset;
> -	void (*error_handler)(struct ata_port *ap);
> +	void (*error_handler)(struct ata_port *ap)
> +		__must_hold(&ap->host->eh_mutex);
>  	void (*lost_interrupt)(struct ata_port *ap);
>  	void (*post_internal_cmd)(struct ata_queued_cmd *qc);
>  	void (*sched_eh)(struct ata_port *ap);

[Severity: High]
Does this struct definition miss a __must_hold(ap->lock) annotation for
sched_eh?

The assigned ata_std_sched_eh function includes __must_hold(ap->lock),
but the struct member here does not.

Additionally, will adding __must_hold() to error_handler break the build
for ata_dummy_error_handler and forward declarations (like
sil24_error_handler) if they are not also updated with these annotations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521173347.2079560-1-bvanassche@acm.org?part=8