Messages in this thread | | | Date | Tue, 9 Jan 2024 10:40:03 -0500 | Subject | Re: [PATCH v2] lockdep: Add missing graph_unlock in check_prev_add | From | 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@redhat.com> 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@unisoc.com> >>> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com> >>> --- >>> 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
| |