linux-kernel - Re: [PATCH v2] seqlock,lockdep: Only check for preemption

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210513195357.xq57b2t26hhhmdn4@offworld>
Date:   Thu, 13 May 2021 12:53:57 -0700
From:   Davidlohr Bueso <dave@...olabs.net>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     a.darwish@...utronix.de, bigeasy@...utronix.de, tglx@...utronix.de,
        shung-hsi.yu@...e.com, linux-kernel@...r.kernel.org,
        Davidlohr Bueso <dbueso@...e.de>
Subject: Re: [PATCH v2] seqlock,lockdep: Only check for preemption_disabled
 in non-rt

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