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

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():

net/rds/send.c +0 -0

index 5e57a1581dc6..65b1bb06ca71 100644
@@ -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.

Best Regards,
Yewon Choi

Quoted 74 lines · Yewon Choi

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():

net/rds/send.c +0 -0

index 5e57a1581dc6..65b1bb06ca71 100644
@@ -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

Quoted 1 line

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

Quoted 3 lines

Best Regards,
Yewon Choi