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

[Allison Henderson <allison.henderson@oracle.com>]

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!

Allison


> 
> Best Regards,
> Yewon Choi