Re: [PATCH v2] lockdep: Add missing graph_unlock in check_prev_add

[Waiman Long]

On 1/9/24 00:11, Xuewen Yan wrote:
> Hi Waiman
>
>
> On Tue, Jan 9, 2024 at 11:51 AM Waiman Long <longman@xxxxxxxxxx> wrote:
> > On 1/5/24 01:04, Xuewen Yan wrote:
> > > The check_prev_add() is held graph_lock, and it should unlock
> > > the graph_lock before return 0.
> > > But there is one condition where it will return 0 without unlock,
> > > that is:
> > >
> > > /* <prev> is not found in <next>::locks_before */
> > >        return 0;
> > >
> > > So add graph_unlock before return 0.
> > >
> > > Fixes: 3454a36d6a39 ("lockdep: Introduce lock_list::dep")
> > > Signed-off-by: Xuewen Yan <xuewen.yan@xxxxxxxxxx>
> > > Signed-off-by: Zhiguo Niu <zhiguo.niu@xxxxxxxxxx>
> > > ---
> > > Change in V2:
> > > -move the graph_unlock to check_prev_add from validate_chain(Boqun)
> > > -Add fix tag
> > > ---
> > > ---
> > >    kernel/locking/lockdep.c | 1 +
> > >    1 file changed, 1 insertion(+)
> > >
> > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > > index 151bd3de5936..c8602a251bec 100644
> > > --- a/kernel/locking/lockdep.c
> > > +++ b/kernel/locking/lockdep.c
> > > @@ -3178,6 +3178,7 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,
> > >                        }
> > >
> > >                        /* <prev> is not found in <next>::locks_before */
> > > +                     graph_unlock();
> > >                        return 0;
> > >                }
> > >        }
> > There are multiple places in check_prev_add() that will return 0. It
> > will be odd to have just one of them has a graph_unlock(). It makes the
> > code hard to understand. You should insert graph_unlock() in a place
> > that matches the other places where graph_unlock() will be called. My
> > suggestion is as follows:
> >
> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index 151bd3de5936..d9f2df36332c 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -3252,7 +3252,7 @@ check_prevs_add(struct task_struct *curr, struct
> > held_loc>
> >                   if (hlock->check) {
> >                           int ret = check_prev_add(curr, hlock, next,
> > distance, &>
> >                           if (!ret)
> > -                               return 0;
> > +                               goto out_bug;
> >
> >                           /*
> >                            * Stop after the first non-trylock entry,
> As you say, there are multiple places in check_prev_add() that will
> return 0, and some cases had unlocked the lock, if all goto the
> out_bug, would it cause double unlock?
> Maybe as follows?
> ---
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 151bd3de5936..8b665ba90ad0 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -3178,7 +3178,7 @@ check_prev_add(struct task_struct *curr, struct
> held_lock *prev,
>                          }
>
>                          /* <prev> is not found in <next>::locks_before */
> -                       return 0;
> +                       goto list_err;
>                  }
>          }
>
> @@ -3215,6 +3215,11 @@ check_prev_add(struct task_struct *curr, struct
> held_lock *prev,
>                  return 0;
>
>          return 2;
> +
> +list_err:
> +       /* still get graph_lock, unlock it before return*/
> +       graph_unlock();
> +       return 0;
>   }

I see. the graph_unlock() is called before any error message is printed. 
I understand the reason why this is done this way, but it does make it 
easy to re-introduce this kind of error when the lockdep code is 
changed. We need a better system to track the state of the graph_lock 
and do an unlock if necessary.

Cheers,
Longman