Re: [PATCH v3] ata: libata: Document when host->eh_mutex should be held

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Missing '&' operator in lock capability expression for `__acquires` and `__releases` annotations on `ata_eh_acquire` and `ata_eh_release`.
- [Low] The `ata_dummy_error_handler` function is missing the `__must_hold(&ap->host->eh_mutex)` annotation.

Pre-existing issues:
- [High] The `mv_pmp_error_handler` modifies `pp->pp_flags` and calls `ata_port_freeze(ap)` without holding the host spinlock, leading to data races with the interrupt handler and lockdep/API contract violations.
- [High] The `sil24_error_handler` clears `pp->do_port_rst` without holding the host spinlock after the port has been thawed, creating a race condition with the interrupt handler.
--

commit 5548604538b7f00a0cf37a02b72ef6176653ab1e
Author: Bart Van Assche <bvanassche@acm.org>

ata: libata: Document when host->eh_mutex should be held

Annotate various error handling functions with __must_hold(&host->eh_mutex)
to enable Clang's context analysis. This helps catch locking bugs in error
paths at compile time.

> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 9a4b67b90b17b..7b3c3e20115e5 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -469,6 +469,7 @@ static void ata_eh_clear_action(struct ata_link *link, struct ata_device *dev,
>   *	EH context.
>   */
>  void ata_eh_acquire(struct ata_port *ap)
> +	__acquires(ap->host->eh_mutex)
>  {

[Severity: Low]
Since the internal locking functions and __must_hold expect a pointer,
should this use the address-of operator like __acquires(&ap->host->eh_mutex)?
Without it, this might cause a capability mismatch in Clang's Thread Safety
Analysis and lead to build failures since CONTEXT_ANALYSIS is enabled.

[ ... ]

> @@ -486,6 +487,7 @@ void ata_eh_acquire(struct ata_port *ap)
>   *	EH context.
>   */
>  void ata_eh_release(struct ata_port *ap)
> +	__releases(ap->host->eh_mutex)
>  {

[Severity: Low]
Similarly, should this be __releases(&ap->host->eh_mutex) instead?

[ ... ]

> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index ffb396f61731f..f6942fd80bae7 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 this function modify pp->pp_flags
and call ata_port_freeze() without holding the host spinlock?

The error_handler callback is invoked with ap->host->eh_mutex held, but
without the host spinlock (ap->lock). However, mv_pmp_error_handler()
performs a read-modify-write on pp->pp_flags:

	if (pp->pp_flags & MV_PP_FLAG_DELAYED_EH) {
		pmp_map = pp->delayed_eh_pmp_map;
		pp->pp_flags &= ~MV_PP_FLAG_DELAYED_EH;

This can race with mv_err_intr() which modifies the same flags from the
interrupt handler while holding the spinlock.

Additionally, this calls ata_port_freeze(ap) directly, which requires
the host lock to be held according to its locking contract.

[ ... ]

> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> index d642ece9f07a1..89a10704e98e5 100644
> --- a/drivers/ata/sata_sil24.c
> +++ b/drivers/ata/sata_sil24.c
> @@ -1167,6 +1167,7 @@ static irqreturn_t sil24_interrupt(int irq, void *dev_instance)
>  }
>  
>  static void sil24_error_handler(struct ata_port *ap)
> +	__must_hold(&ap->host->eh_mutex)
>  {
>  	struct sil24_port_priv *pp = ap->private_data;

[Severity: High]
This is a pre-existing issue, but does clearing pp->do_port_rst here
race with the interrupt handler?

sil24_error_handler() delegates to sata_pmp_error_handler(), which recovers
the port and re-enables hardware interrupts:

	sata_pmp_error_handler(ap);
	pp->do_port_rst = 0;

If an error interrupt fires after the port is thawed but before
pp->do_port_rst = 0 executes, sil24_interrupt() could set pp->do_port_rst = 1
to request a controller reset. The EH thread would then overwrite this with 0
without holding the ap->lock spinlock, discarding the reset request.

[ ... ]

> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 5c085ef4eda7b..5f6bbe5d504fa 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);

[Severity: Low]
Does ata_dummy_error_handler() also need to be annotated with __must_hold?

Since ata_dummy_error_handler() lacks this annotation, assigning it to
ata_dummy_port_ops.error_handler will cause a type/capability mismatch in
Clang's Thread Safety Analysis and break the build.

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