lkml.org 
[lkml]   [2021]   [Jun]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] xfrm: policy: Restructure RCU-read locking in xfrm_sk_policy_lookup
    On Mon, Jun 21, 2021 at 01:05:28PM +0200, Steffen Klassert wrote:
    > On Mon, Jun 21, 2021 at 11:11:18AM +0200, Varad Gautam wrote:
    > >
    > > Right, I misread the call chain - security_xfrm_policy_lookup does not reach
    > > xfrm_policy_lookup, making this patch unnecessary. The bug I have is:
    > >
    > > T1, holding hash_resize_mutex and sleeping inside synchronize_rcu:
    > >
    > > __schedule
    > > schedule
    > > schedule_timeout
    > > wait_for_completion
    > > __wait_rcu_gp
    > > synchronize_rcu
    > > xfrm_hash_resize
    > >
    > > And T2 producing RCU-stalls since it blocked on the mutex:
    > >
    > > __schedule
    > > schedule
    > > __rt_mutex_slowlock
    > > rt_mutex_slowlock_locked
    > > rt_mutex_slowlock
    > > xfrm_policy_lookup_bytype.constprop.77
    >
    > Ugh, why does xfrm_policy_lookup_bytype use a mutex? This is called
    > in the receive path inside a sofirq.
    >
    > The bug was introduced by:
    >
    > commit 77cc278f7b202e4f16f8596837219d02cb090b96
    > Author: Ahmed S. Darwish <a.darwish@linutronix.de>
    > Date: Mon Jul 20 17:55:22 2020 +0200
    >
    > xfrm: policy: Use sequence counters with associated lock
    >
    > A sequence counter write side critical section must be protected by some
    > form of locking to serialize writers. If the serialization primitive is
    > not disabling preemption implicitly, preemption has to be explicitly
    > disabled before entering the sequence counter write side critical
    > section.
    >
    > A plain seqcount_t does not contain the information of which lock must
    > be held when entering a write side critical section.
    >
    > Use the new seqcount_spinlock_t and seqcount_mutex_t data types instead,
    > which allow to associate a lock with the sequence counter. This enables
    > lockdep to verify that the lock used for writer serialization is held
    > when the write side critical section is entered.
    >
    > If lockdep is disabled this lock association is compiled out and has
    > neither storage size nor runtime overhead.
    >
    > Signed-off-by: Ahmed S. Darwish <a.darwish@linutronix.de>
    > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
    > Link: https://lkml.kernel.org/r/20200720155530.1173732-17-a.darwish@linutronix.de
    >
    > This uses a seqcount_mutex_t for xfrm_policy_hash_generation, that's
    > wrong.

    Varad, can you try to replace the seqcount_mutex_t for xfrm_policy_hash_generation
    by a seqcount_spinlock_t? I'm not familiar with that seqcount changes,
    but we should not end up with using a mutex in this codepath.

    \
     
     \ /
      Last update: 2021-06-22 13:22    [W:6.093 / U:0.600 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site