RE: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu

Hello Eric Dumazet,

Like Fan Du, even I could not see the bug in the current code. Can you please elaborate.

Thanks
Priyanka

-----Original Message-----
From: Fan Du [mailto:fan.du@windriver.com] 
Sent: Wednesday, August 08, 2012 12:14 PM
To: Eric Dumazet
Cc: Jain Priyanka-B32167; netdev@vger.kernel.org
Subject: Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu

First, sorry to jump in.

On 2012年08月08日 14:25, Eric Dumazet wrote:

On Tue, 2012-08-07 at 10:51 +0530, Priyanka Jain wrote:

quoted

xfrm_policy_afinfo is read mosly data structure.
Write on xfrm_policy_afinfo is done only at the time of 
configuration.
So rwlocks can be safely replaced with RCU.

RCUs usage optimizes the performance.

quoted

  static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned 
short family) @@ -2530,16 +2535,16 @@ static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
  	struct xfrm_policy_afinfo *afinfo;
  	if (unlikely(family>= NPROTO))
  		return NULL;
-	read_lock(&xfrm_policy_afinfo_lock);
-	afinfo = xfrm_policy_afinfo[family];
+	rcu_read_lock();
+	afinfo = rcu_dereference(xfrm_policy_afinfo[family]);
  	if (unlikely(!afinfo))
-		read_unlock(&xfrm_policy_afinfo_lock);
+		rcu_read_unlock();

This makes no sense to me : We cant safely return afinfo here.

Note the current code is buggy as well, this is worrying.

As soon as we exit from xfrm_policy_get_afinfo(), pointer might be 
invalid.

Yes, it might be invalid, but all the callers have checked the return value, thus use it in a sane way.
So I don't follow "Note the current code is buggy as well".

Am I missing something here?

Really, RCU conversion should be the right moment to spot those bugs 
and first fix them (for stable trees)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in 
the body of a message to majordomo@vger.kernel.org More majordomo info 
at  http://vger.kernel.org/majordomo-info.html

-- 

Love each day!
--fan