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

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Missing context analysis annotation on ata_exec_internal() causing a build failure.
- [Low] Convoluted and redundant pointer traversal in lockdep_assert_held().

Pre-existing issues:
- [High] Read-modify-write race on pp->pp_flags in mv_pmp_error_handler().
--

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

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

Annotate error handler implementations and helpers with __must_hold,
__acquires, and __releases for host->eh_mutex. Enable Clang's context analysis
to prevent locking bugs in error paths.

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

> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index 20e6645ab7371..b96025abd45e3 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> +CONTEXT_ANALYSIS := y
> +

[Severity: Low]
Does enabling context analysis here cause a build failure?

ata_exec_internal() in drivers/ata/libata-core.c unconditionally calls
ata_eh_release() and ata_eh_acquire() without being annotated. Since Clang's
analysis assumes no locks are held at the start of an unannotated function,
calling ata_eh_release() triggers a compiler error for releasing a mutex that
was not held.

Could ata_exec_internal() be annotated to fix this?

[ ... ]

> diff --git a/drivers/ata/libata-pmp.c b/drivers/ata/libata-pmp.c
> index e3adc008fed1e..bf40633f8fb23 100644
> --- a/drivers/ata/libata-pmp.c
> +++ b/drivers/ata/libata-pmp.c
> @@ -750,6 +751,8 @@ static int sata_pmp_eh_recover_pmp(struct ata_port *ap,
>  	int detach = 0, rc = 0;
>  	int reval_failed = 0;
>  
> +	lockdep_assert_held(&ap->link.ap->host->eh_mutex);

[Severity: Low]
Could this pointer traversal be simplified?

Since the variable ap is already a pointer to struct ata_port, traversing
through its embedded link struct to resolve back to the exact same ap pointer
seems redundant.

Could this be simplified to lockdep_assert_held(&ap->host->eh_mutex)?

[ ... ]

> 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 have a read-modify-write
race on pp->pp_flags?

This function runs in process context (EH thread) and modifies pp->pp_flags
without holding ap->lock:

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

Concurrently, the hardware interrupt handler (mv_interrupt) can execute,
acquire ap->lock, and modify pp->pp_flags (e.g., setting
MV_PP_FLAG_FAKE_ATA_BUSY).

If the interrupt fires between the read and the write of pp->pp_flags in
mv_pmp_error_handler(), will the interrupt's modifications be silently
overwritten and lost?

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