Re: net/rds: Improper memory ordering semantic in release_in_xmit()

[Yewon Choi <woni9911@gmail.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
>