Received-SPF: pass (google.com: domain of linux-kernel+bounces-103185-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 14 Mar 2024 20:39:38 +0900
From: Yewon Choi <woni9911@gmail.com>
To: Allison Henderson <allison.henderson@oracle.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	netdev@vger.kernel.org, linux-rdma@vger.kernel.org,
	rds-devel@oss.oracle.com, linux-kernel@vger.kernel.org,
	"Dae R. Jeong" <threeearcat@gmail.com>
Subject: Re: net/rds: Improper memory ordering semantic in release_in_xmit()
Message-ID: <ZfLh+va60YU2U86q@libra05>
References: <Zehp16cKYeGWknJs@libra05>
 <86d88699e8f22ebe0d45ffb5229fb73d78c5aae9.camel@oracle.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <86d88699e8f22ebe0d45ffb5229fb73d78c5aae9.camel@oracle.com>

On Thu, Mar 07, 2024 at 08:13:50PM +0000, Allison Henderson wrote:
> On Wed, 2024-03-06 at 22:04 +0900, Yewon Choi wrote:
> > Hello,
> > 
> > It seems to be that clear_bit() in release_in_xmit() doesn't have
> > release semantic while it works as a bit lock in rds_send_xmit().
> > Since acquire/release_in_xmit() are used in rds_send_xmit() for the 
> > serialization between callers of rds_send_xmit(), they should imply 
> > acquire/release semantics like other locks.
> > 
> > Although smp_mb__after_atomic() is placed after clear_bit(), it
> > cannot
> > prevent that instructions before clear_bit() (in critical section)
> > are
> > reordered after clear_bit().
> > As a result, mutual exclusion may not be guaranteed in specific
> > HW architectures like Arm.
> > 
> > We tested that this locking implementation doesn't guarantee the
> > atomicity of
> > critical section in Arm server. Testing was done with Arm Neoverse N1
> > cores,
> > and the testing code was generated by litmus testing tool (klitmus7).
> > 
> > Initial condition:
> > 
> > l = x = y = r0 = r1 = 0
> > 
> > Thread 0:
> > 
> > if (test_and_set_bit(0, l) == 0) {
> > ??? WRITE_ONCE(*x, 1);
> > ??? WRITE_ONCE(*y, 1);
> > ??? clear_bit(0, l);
> > ??? smp_mb__after_atomic();
> > }
> > 
> > Thread 1:
> > 
> > if (test_and_set_bit(0, l) == 0) {
> > ??? r0 = READ_ONCE(*x);
> > ??? r1 = READ_ONCE(*y);
> > ??? clear_bit(0, l);
> > ??? smp_mb__after_atomic();
> > }
> > 
> > If the implementation is correct, the value of r0 and r1 should show
> > all-or-nothing behavior (both 0 or 1). However, below test result
> > shows 
> > that atomicity violation is very rare, but exists:
> > 
> > Histogram (4 states)
> > 9673811 :>1:r0=0; 1:r1=0;
> > 5647??? :>1:r0=1; 1:r1=0; // Violate atomicity
> > 9605??? :>1:r0=0; 1:r1=1; // Violate atomicity
> > 6310937 :>1:r0=1; 1:r1=1;
> > 
> > So, we suggest introducing release semantic using clear_bit_unlock()
> > instead of clear_bit():
> > 
> > diff --git a/net/rds/send.c b/net/rds/send.c
> > index 5e57a1581dc6..65b1bb06ca71 100644
> > --- a/net/rds/send.c
> > +++ b/net/rds/send.c
> > @@ -108,7 +108,7 @@ static int acquire_in_xmit(struct rds_conn_path
> > *cp)
> > ?
> > ?static void release_in_xmit(struct rds_conn_path *cp)
> > ?{
> > -???????clear_bit(RDS_IN_XMIT, &cp->cp_flags);
> > +???????clear_bit_unlock(RDS_IN_XMIT, &cp->cp_flags);
> > ????????smp_mb__after_atomic();
> > ????????/*
> > ???????? * We don't use wait_on_bit()/wake_up_bit() because our
> > waking is in a
> > 
> > Could you check this please? If needed, we will send a patch.
> 
> Hi Yewon,
> 
> Thank you for finding this.  I had a look at the code you had
> mentioned, and while I don't see any use cases of release_in_xmit()
> that might result in an out of order read, I do think that the proposed
> change is a good clean up.  If you choose to submit a patch, please
> remove the proceeding "smp_mb__after_atomic" line as well, as it would
> no longer be needed.  Also, please update acquire_in_xmit() to use the
> corresponding test_and_set_bit_lock() call.  Thank you!
>

Thank you for examining this and giving suggestions!
I sent a patch with changes including your suggestions. If it has
problems, I will correct them as soon as possible.

Sincerely,
Yewon Choi

> Allison
> 
> 
> > 
> > Best Regards,
> > Yewon Choi
>