Re: [PATCH] xfrm: policy: Restructure RCU-read locking in xfrm_sk_policy_lookup

[Frederic Weisbecker]

On Tue, Jun 22, 2021 at 01:21:59PM +0200, Steffen Klassert wrote:
> 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@xxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxx>
> >     Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> >     Link: https://lkml.kernel.org/r/20200720155530.1173732-17-a.darwish@xxxxxxxxxxxxx
> > 
> > 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.

Something like this? (beware, untested, also I don't know if the read side
should then disable bh, doesn't look necessary for PREEMPT_RT, but I may be
missing something...)

diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
index e816b6a3ef2b..9b376b87bd54 100644
--- a/include/net/netns/xfrm.h
+++ b/include/net/netns/xfrm.h
@@ -74,6 +74,7 @@ struct netns_xfrm {
 #endif
 	spinlock_t		xfrm_state_lock;
 	seqcount_spinlock_t	xfrm_state_hash_generation;
+	seqcount_spinlock_t	xfrm_policy_hash_generation;
 
 	spinlock_t xfrm_policy_lock;
 	struct mutex xfrm_cfg_mutex;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index ce500f847b99..46a6d15b66d6 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -155,7 +155,6 @@ static struct xfrm_policy_afinfo const __rcu *xfrm_policy_afinfo[AF_INET6 + 1]
 						__read_mostly;
 
 static struct kmem_cache *xfrm_dst_cache __ro_after_init;
-static __read_mostly seqcount_mutex_t xfrm_policy_hash_generation;
 
 static struct rhashtable xfrm_policy_inexact_table;
 static const struct rhashtable_params xfrm_pol_inexact_params;
@@ -585,7 +584,7 @@ static void xfrm_bydst_resize(struct net *net, int dir)
 		return;
 
 	spin_lock_bh(&net->xfrm.xfrm_policy_lock);
-	write_seqcount_begin(&xfrm_policy_hash_generation);
+	write_seqcount_begin(&net->xfrm.xfrm_policy_hash_generation);
 
 	odst = rcu_dereference_protected(net->xfrm.policy_bydst[dir].table,
 				lockdep_is_held(&net->xfrm.xfrm_policy_lock));
@@ -596,7 +595,7 @@ static void xfrm_bydst_resize(struct net *net, int dir)
 	rcu_assign_pointer(net->xfrm.policy_bydst[dir].table, ndst);
 	net->xfrm.policy_bydst[dir].hmask = nhashmask;
 
-	write_seqcount_end(&xfrm_policy_hash_generation);
+	write_seqcount_end(&net->xfrm.xfrm_policy_hash_generation);
 	spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
 
 	synchronize_rcu();
@@ -1245,7 +1244,7 @@ static void xfrm_hash_rebuild(struct work_struct *work)
 	} while (read_seqretry(&net->xfrm.policy_hthresh.lock, seq));
 
 	spin_lock_bh(&net->xfrm.xfrm_policy_lock);
-	write_seqcount_begin(&xfrm_policy_hash_generation);
+	write_seqcount_begin(&net->xfrm.xfrm_policy_hash_generation);
 
 	/* make sure that we can insert the indirect policies again before
 	 * we start with destructive action.
@@ -1354,7 +1353,7 @@ static void xfrm_hash_rebuild(struct work_struct *work)
 
 out_unlock:
 	__xfrm_policy_inexact_flush(net);
-	write_seqcount_end(&xfrm_policy_hash_generation);
+	write_seqcount_end(&net->xfrm.xfrm_policy_hash_generation);
 	spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
 
 	mutex_unlock(&hash_resize_mutex);
@@ -2095,9 +2094,9 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
 	rcu_read_lock();
  retry:
 	do {
-		sequence = read_seqcount_begin(&xfrm_policy_hash_generation);
+		sequence = read_seqcount_begin(&net->xfrm.xfrm_policy_hash_generation);
 		chain = policy_hash_direct(net, daddr, saddr, family, dir);
-	} while (read_seqcount_retry(&xfrm_policy_hash_generation, sequence));
+	} while (read_seqcount_retry(&net->xfrm.xfrm_policy_hash_generation, sequence));
 
 	ret = NULL;
 	hlist_for_each_entry_rcu(pol, chain, bydst) {
@@ -2128,7 +2127,7 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
 	}
 
 skip_inexact:
-	if (read_seqcount_retry(&xfrm_policy_hash_generation, sequence))
+	if (read_seqcount_retry(&net->xfrm.xfrm_policy_hash_generation, sequence))
 		goto retry;
 
 	if (ret && !xfrm_pol_hold_rcu(ret))
@@ -4084,6 +4083,7 @@ static int __net_init xfrm_net_init(struct net *net)
 	/* Initialize the per-net locks here */
 	spin_lock_init(&net->xfrm.xfrm_state_lock);
 	spin_lock_init(&net->xfrm.xfrm_policy_lock);
+	seqcount_spinlock_init(&net->xfrm.xfrm_policy_hash_generation, &net->xfrm.xfrm_policy_lock);
 	mutex_init(&net->xfrm.xfrm_cfg_mutex);
 
 	rv = xfrm_statistics_init(net);
@@ -4128,7 +4128,6 @@ void __init xfrm_init(void)
 {
 	register_pernet_subsys(&xfrm_net_ops);
 	xfrm_dev_init();
-	seqcount_mutex_init(&xfrm_policy_hash_generation, &hash_resize_mutex);
 	xfrm_input_init();
 
 #ifdef CONFIG_XFRM_ESPINTCP