Re: [PATCH v2] seqlock,lockdep: Only check for preemption_disabled in non-rt

[Davidlohr Bueso]

On Wed, 12 May 2021, Peter Zijlstra wrote:

> I'm confused, and the Changelog is useless. The code you actually
> changed is for seqcount_t, which doesn't have an associated LOCK. If

Hmm it was never my intention to touch seqcount_t, I now see the error of
my ways.

> there is a lock, the code should be changed to use the appropriate
> seqcount_LOCKNAME_t and the assertion will change into the one found in
> __seqprop_##lockname##_assert(), namely:
>
>  lockdep_assert_held(lockmember)
>
>
> But as is, seqcount_t usage relies on being non-preemptible, even for
> PREEMPT_RT, and this is a good thing. Please describe the site that goes
> boom and explain things..

So the splat is:
    WARNING: CPU: 0 PID: 15 at kernel/locking/lockdep.c:5363 lockdep_assert_preemption_disabled+0x7a/0xa0
    CPU: 0 PID: 15 Comm: kworker/0:1 Tainted: G            E      5.3.18-rt_syzkaller #1
    Workqueue: events xfrm_hash_resize
    RIP: 0010:lockdep_assert_preemption_disabled+0x7a/0xa0
    Code: 09 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 0f b6 04 02 84 c0 74 04 3c 03 7e 1c 8b 83 c8 09 00 00 85 c0 74 02 <0f> 0b 5b c3 48 c7 c7 54 39 ce 83 e8 c6 0d 43 00 eb 9f e8 bf 0d 43
    RSP: 0018:ffff888118497ca0 EFLAGS: 00010202
    RAX: 0000000000000001 RBX: ffff88811847ce40 RCX: 1ffffffff079c72a
    RDX: 1ffff1102308fb01 RSI: 0000000000000022 RDI: ffff88811847d808
    RBP: ffffffff83b9ebb0 R08: 0000000000000001 R09: ffff888118497bd8
    R10: ffff888118497c47 R11: 0000000000000001 R12: ffff88811b232200
    R13: ffff888118497dc0 R14: 0000000000000010 R15: ffff88811847ce40
     xfrm_hash_resize+0xd7/0x1490
     process_one_work+0x78e/0x16e0
     ? pwq_dec_nr_in_flight+0x2e0/0x2e0
     ? do_raw_spin_lock+0x11a/0x250
     ? _raw_spin_lock_irq+0xa/0x40
     worker_thread+0x5f5/0x1080
     ? process_one_work+0x16e0/0x16e0
     kthread+0x401/0x4f0
     ? __kthread_parkme+0x290/0x290
     ret_from_fork+0x24/0x30

I was initially chasing (and hence why the preemption check wasn't making sense):

	seqcount_mutex_init(&xfrm_policy_hash_generation, &hash_resize_mutex);

But there are actually two xfrm_hash_resize() calls (*sigh*). And the other
one, the right one, is/was indeed seqcount_t xfrm_state_hash_generation:

xfrm_hash_resize() // kworker callback, task context
    spin_lock_bh(&net->xfrm.xfrm_policy_lock); // disables softirq, preemption still enabled
    write_seqcount_begin(&xfrm_state_hash_generation);
	__seqprop_assert() <-- boom

And therefore converting it to an associated spinlock would avoid the preemption
check, which is exactly what Ahmed has already done:

bc8e0adff34 (net: xfrm: Use sequence counter with associated spinlock)
e88add19f68 (net: xfrm: Localize sequence counter per network namespace)

Sorry for the noise.

Thanks,
Davidlohr