Re: [PATCH v19 29/40] dept: make dept aware of lockdep_set_lock_cmp_fn() annotation

On Tue, Jul 07, 2026 at 07:33:45AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] DEPT falsely reports a deadlock when a task sequentially acquires three or more locks of the same class (or interleaves them with other locks), because the `lockdep_cmp_fn` bypass is improperly restricted to only the immediately preceding lock.
> --
> 
> commit d03c5b926a945b89f5a90fd98986386e113df5af
> Author: Byungchul Park <[email protected]>
> 
> dept: make dept aware of lockdep_set_lock_cmp_fn() annotation
> 
> This commit updates the DEPT (Dependency Tracker) subsystem to respect the
> lockdep_set_lock_cmp_fn() annotation. It allows DEPT to avoid false positive
> deadlock reports by using the custom comparison function when evaluating
> dependencies between locks of the same class.
> 
> > diff --git a/kernel/dependency/dept.c b/kernel/dependency/dept.c
> > --- a/kernel/dependency/dept.c
> > +++ b/kernel/dependency/dept.c
> 
> [ ... ]
> 
> > @@ -1658,8 +1688,13 @@ static void add_wait(struct dept_class *c, unsigned long ip,
> >               if (!eh->ecxt)
> >                       continue;
> >
> > -             if (eh->ecxt->class != c || eh->sub_l == sub_l)
> > -                     add_dep(eh->ecxt, w);
> > +             if (eh->ecxt->class == c && eh->sub_l != sub_l)
> > +                     continue;
> > +
> > +             if (i == dt->ecxt_held_pos - 1 && lockdep_cmp_fn(eh->map, m))
> > +                     continue;
> 
> [Severity: Medium]
> Will this condition incorrectly bypass the comparison for earlier locks in
> the chain?
> 
> By restricting the lockdep_cmp_fn() check to only the most recently
> acquired lock (i == dt->ecxt_held_pos - 1), it seems we might miss valid
> comparisons for older contexts.

No.

It's how lockdep works and it's intended by lockdep.  The temporary code
just follows the rule.

> For example, if a task acquires lock A1, then lock B, and then lock A2
> (where A1 and A2 are the same class), the check against A1 will be skipped
> because A1 is no longer at dt->ecxt_held_pos - 1.
> 
> This would result in a call to add_dep(A1, A2), creating a
> self-referential class dependency that triggers a false positive deadlock
> report.

No.

It's a real deadlock.

> Should lockdep_cmp_fn() be allowed to evaluate against any previously held
> lock in the chain, rather than just the immediate predecessor?

No.

Check the lockdep code first.  See check_deadlock() and check_prev_add().

	Byungchul

> > +
> > +             add_dep(eh->ecxt, w);
> >       }
> >
> >       wg = next_wgen();
> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=29