Re: [PATCH] locking/lockdep: Fix lock IRQ usage initialization bug

From: Qian Cai
Date: Mon Jul 08 2019 - 21:50:45 EST



> On Jul 8, 2019, at 9:21 PM, Yuyang Du <duyuyang@xxxxxxxxx> wrote:
>
> The problem should have been fixed with this in that pull:
>
> locking/lockdep: Move mark_lock() inside CONFIG_TRACE_IRQFLAGS &&
> CONFIG_PROVE_LOCKING
>
> and this is a better fix than mine.

I donât think so. That commit was included in todayâs linux-next, but I can still reproduce the issue there.

[ 8872.727085] DEBUG_LOCKS_WARN_ON(debug_atomic_read(nr_unused_locks) != nr_unused)
[ 8872.727113] WARNING: CPU: 60 PID: 124801 at kernel/locking/lockdep_proc.c:249 lockdep_stats_show+0xe44/0x11e0
[ 8872.727157] Modules linked in: brd ext4 crc16 mbcache jbd2 loop i2c_opal i2c_core ip_tables x_tables xfs sd_mod ahci libahci tg3 firmware_class libata libphy dm_mirror dm_region_hash dm_log dm_mod [last unloaded: dummy_del_mod]
[ 8872.727213] CPU: 60 PID: 124801 Comm: proc01 Tainted: G W O 5.2.0-next-20190708+ #1
[ 8872.727253] NIP: c0000000001a97b4 LR: c0000000001a97b0 CTR: c0000000008c4660
[ 8872.727293] REGS: c000001c2e84f8e0 TRAP: 0700 Tainted: G W O (5.2.0-next-20190708+)
[ 8872.727326] MSR: 9000000000029033 <SF,HV,EE,ME,IR,DR,RI,LE> CR: 28888402 XER: 20040000
[ 8872.727371] CFAR: c00000000011a0d4 IRQMASK: 0
GPR00: c0000000001a97b0 c000001c2e84fb70 c0000000016f8b00 0000000000000044
GPR04: c00000000172c258 c0000000001b9288 4e5241575f534b43 75626564284e4f5f
GPR08: 0000001ffdd10000 0000000000000000 0000000000000000 212029736b636f6c
GPR12: 756e755f726e203d c000001ffffce400 0000000000000000 c0000000015bd610
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000dfd
GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR24: 0000000000000000 0000000000000000 0000000000000000 c000001fb7d0f0a8
GPR28: 0000000000000000 c00000000172c278 c00000000172c4c4 c00000000172ba18
[ 8872.727698] NIP [c0000000001a97b4] lockdep_stats_show+0xe44/0x11e0
[ 8872.727742] LR [c0000000001a97b0] lockdep_stats_show+0xe40/0x11e0
[ 8872.727775] Call Trace:
[ 8872.727800] [c000001c2e84fb70] [c0000000001a97b0] lockdep_stats_show+0xe40/0x11e0 (unreliable)
[ 8872.727831] [c000001c2e84fca0] [c000000000492434] seq_read+0x1d4/0x620
[ 8872.727886] [c000001c2e84fd30] [c000000000515520] proc_reg_read+0x90/0x130
[ 8872.727938] [c000001c2e84fd60] [c000000000452d6c] __vfs_read+0x3c/0x70
[ 8872.727989] [c000001c2e84fd80] [c000000000452e5c] vfs_read+0xbc/0x1a0
[ 8872.728033] [c000001c2e84fdd0] [c0000000004532ec] ksys_read+0x7c/0x140
[ 8872.728073] [c000001c2e84fe20] [c00000000000ae08] system_call+0x5c/0x70
[ 8872.728126] Instruction dump:
[ 8872.728161] 419ef3b0 3d220003 392974fc 81290000 2f890000 409ef39c 3c82ff33 3c62ff33
[ 8872.728194] 38841af8 386308e8 4bf708c1 60000000 <0fe00000> 4bfff37c 60000000 39200000
[ 8872.728251] ---[ end trace 42d16c13415f9f32 ]---

>
> Thanks,
> Yuyang
>
> On Tue, 9 Jul 2019 at 01:32, Qian Cai <cai@xxxxxx> wrote:
>>
>> I saw Ingo send a pull request to Linus for 5.3 [1] includes the offensive
>> commit "locking/lockdep: Consolidate lock usage bit initialization" but did not
>> include this patch.
>>
>> [1] https://lore.kernel.org/lkml/20190708093516.GA57558@xxxxxxxxx/
>>
>> On Mon, 2019-06-10 at 13:52 +0800, Yuyang Du wrote:
>>> The commit:
>>>
>>> 091806515124b20 ("locking/lockdep: Consolidate lock usage bit
>>> initialization")
>>>
>>> misses marking LOCK_USED flag at IRQ usage initialization when
>>> CONFIG_TRACE_IRQFLAGS
>>> or CONFIG_PROVE_LOCKING is not defined. Fix it.
>>>
>>> Reported-by: Qian Cai <cai@xxxxxx>
>>> Signed-off-by: Yuyang Du <duyuyang@xxxxxxxxx>
>>> ---
>>> kernel/locking/lockdep.c | 110 +++++++++++++++++++++++-----------------------
>>> -
>>> 1 file changed, 53 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
>>> index 48a840a..c3db987 100644
>>> --- a/kernel/locking/lockdep.c
>>> +++ b/kernel/locking/lockdep.c
>>> @@ -3460,9 +3460,61 @@ void trace_softirqs_off(unsigned long ip)
>>> debug_atomic_inc(redundant_softirqs_off);
>>> }
>>>
>>> +static inline unsigned int task_irq_context(struct task_struct *task)
>>> +{
>>> + return 2 * !!task->hardirq_context + !!task->softirq_context;
>>> +}
>>> +
>>> +static int separate_irq_context(struct task_struct *curr,
>>> + struct held_lock *hlock)
>>> +{
>>> + unsigned int depth = curr->lockdep_depth;
>>> +
>>> + /*
>>> + * Keep track of points where we cross into an interrupt context:
>>> + */
>>> + if (depth) {
>>> + struct held_lock *prev_hlock;
>>> +
>>> + prev_hlock = curr->held_locks + depth-1;
>>> + /*
>>> + * If we cross into another context, reset the
>>> + * hash key (this also prevents the checking and the
>>> + * adding of the dependency to 'prev'):
>>> + */
>>> + if (prev_hlock->irq_context != hlock->irq_context)
>>> + return 1;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +#else /* defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_PROVE_LOCKING) */
>>> +
>>> +static inline
>>> +int mark_lock_irq(struct task_struct *curr, struct held_lock *this,
>>> + enum lock_usage_bit new_bit)
>>> +{
>>> + WARN_ON(1); /* Impossible innit? when we don't have TRACE_IRQFLAG */
>>> + return 1;
>>> +}
>>> +
>>> +static inline unsigned int task_irq_context(struct task_struct *task)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static inline int separate_irq_context(struct task_struct *curr,
>>> + struct held_lock *hlock)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +#endif /* defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_PROVE_LOCKING) */
>>> +
>>> static int
>>> mark_usage(struct task_struct *curr, struct held_lock *hlock, int check)
>>> {
>>> +#if defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_PROVE_LOCKING)
>>> if (!check)
>>> goto lock_used;
>>>
>>> @@ -3510,6 +3562,7 @@ void trace_softirqs_off(unsigned long ip)
>>> }
>>>
>>> lock_used:
>>> +#endif
>>> /* mark it as used: */
>>> if (!mark_lock(curr, hlock, LOCK_USED))
>>> return 0;
>>> @@ -3517,63 +3570,6 @@ void trace_softirqs_off(unsigned long ip)
>>> return 1;
>>> }
>>>
>>> -static inline unsigned int task_irq_context(struct task_struct *task)
>>> -{
>>> - return 2 * !!task->hardirq_context + !!task->softirq_context;
>>> -}
>>> -
>>> -static int separate_irq_context(struct task_struct *curr,
>>> - struct held_lock *hlock)
>>> -{
>>> - unsigned int depth = curr->lockdep_depth;
>>> -
>>> - /*
>>> - * Keep track of points where we cross into an interrupt context:
>>> - */
>>> - if (depth) {
>>> - struct held_lock *prev_hlock;
>>> -
>>> - prev_hlock = curr->held_locks + depth-1;
>>> - /*
>>> - * If we cross into another context, reset the
>>> - * hash key (this also prevents the checking and the
>>> - * adding of the dependency to 'prev'):
>>> - */
>>> - if (prev_hlock->irq_context != hlock->irq_context)
>>> - return 1;
>>> - }
>>> - return 0;
>>> -}
>>> -
>>> -#else /* defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_PROVE_LOCKING) */
>>> -
>>> -static inline
>>> -int mark_lock_irq(struct task_struct *curr, struct held_lock *this,
>>> - enum lock_usage_bit new_bit)
>>> -{
>>> - WARN_ON(1); /* Impossible innit? when we don't have TRACE_IRQFLAG */
>>> - return 1;
>>> -}
>>> -
>>> -static inline int
>>> -mark_usage(struct task_struct *curr, struct held_lock *hlock, int check)
>>> -{
>>> - return 1;
>>> -}
>>> -
>>> -static inline unsigned int task_irq_context(struct task_struct *task)
>>> -{
>>> - return 0;
>>> -}
>>> -
>>> -static inline int separate_irq_context(struct task_struct *curr,
>>> - struct held_lock *hlock)
>>> -{
>>> - return 0;
>>> -}
>>> -
>>> -#endif /* defined(CONFIG_TRACE_IRQFLAGS) && defined(CONFIG_PROVE_LOCKING) */
>>> -
>>> /*
>>> * Mark a lock with a usage bit, and validate the state transition:
>>> */