Re: [PATCH v2 9/9] ata: Annotate the code that uses the host lock

[Niklas Cassel <cassel@kernel.org>]

On Wed, May 27, 2026 at 12:40:24AM +0200, Marco Elver wrote:
> On Tue, 26 May 2026 at 23:33, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> Then the assumption is checked at runtime when running with lockdep.
> This is by design, because it goes both ways: every time you now add
> __assume_ctx_lock(), someone just has to move that code around and
> suddenly the assumption no longer holds and you have a bug. If you
> were using lockdep_assert_held() you're at least ensuring that someone
> running with lockdep (e.g. syzkaller) will catch the bad assumption.

My biggest worry/issue with this series is all the __assume_ctx_lock()
annotations it adds. Because __assume_ctx_lock() seems to tell the compiler:
"trust me, I have taken this lock". It seems quite fragile, and if someone
refactors the code, the annotation might no longer be valid.

I suggested something similar to what Bart suggested, a:
__objects_are_equal(dev->link->ap->lock, ap->lock) annotation.
But as Bart said, that would probably require additions to Clang.

But here Marco says that pointer-equality is unsound, since it can't be
verified statically and would have to be verified at rutime...


> 
> > Whether lockdep_assert_held() or __assume_ctx_lock() is used to help the
> > compiler alias analysis, there is a risk that these annotations are
> > incorrect. My preferred solution is that a new macro would be added that
> > supports expressing pointer equality in a direct way to the compiler
> > alias analysis engine and also that evaluates pointer equality at
> > runtime. However, I'm afraid this requires compiler changes and changing
> > Clang is out-of-scope for me.
> 
> Telling the compiler about pointer-equality is also unsound, because
> there's no way to verify our assumption statically. The only way to
> recover soundness is to again introduce runtime checks of some sort.
> 
> > I chose __assume_ctx_lock() because it is evaluated only at compile time
> > and a compile time annotation is sufficient in this context.
> 
> There's a real risk that your assumption may become stale, and if it
> were my codebase, I'd prefer lockdep_assert_held() just to make sure
> we check our assumption at runtime. This is not my subsystem -- just
> my 2c.

Right now, I think my ideal solution would be to, to the furthest extent
possible, try to avoid adding __assume_ctx_lock().
Sure, a few special cases might be acceptable, if there are no reasonable
way to avoid it.

And if there is no reasonable way to avoid it, at least with the current
behavior in linux-next/master, I would prefer a lockdep_assert_held() over
a __assume_ctx_lock().


Kind regards,
Niklas