netdev - Re: net/rds: Improper memory ordering semantic in release_in

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <86d88699e8f22ebe0d45ffb5229fb73d78c5aae9.camel@oracle.com>
Date: Thu, 7 Mar 2024 20:13:50 +0000
From: Allison Henderson <allison.henderson@...cle.com>
To: "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "rds-devel@....oracle.com"
	<rds-devel@....oracle.com>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "woni9911@...il.com" <woni9911@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "kuba@...nel.org"
	<kuba@...nel.org>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "pabeni@...hat.com" <pabeni@...hat.com>
CC: "threeearcat@...il.com" <threeearcat@...il.com>
Subject: Re: net/rds: Improper memory ordering semantic in release_in_xmit()

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