linux-kernel - Re: [PATCH] RDMA/siw: Reuse value read using READ

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: <2d508574-c2b8-489b-a26d-71b1c36961cf@linux.dev>
Date: Sun, 10 Mar 2024 05:53:26 +0100
From: Zhu Yanjun <yanjun.zhu@...ux.dev>
To: linke li <lilinke99@...com>
Cc: Bernard Metzler <bmt@...ich.ibm.com>, Jason Gunthorpe <jgg@...pe.ca>,
 Leon Romanovsky <leon@...nel.org>, linux-rdma@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] RDMA/siw: Reuse value read using READ_ONCE instead of
 re-reading it

在 2024/3/9 13:27, linke li 写道:
> In siw_orqe_start_rx, the orqe's flag in the if condition is read using
> READ_ONCE, checked, and then re-read, voiding all guarantees of the
> checks. Reuse the value that was read by READ_ONCE to ensure the
> consistency of the flags throughout the function.
> 
> Signed-off-by: linke li <lilinke99@...com>
> ---
>   drivers/infiniband/sw/siw/siw_qp_rx.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/siw/siw_qp_rx.c b/drivers/infiniband/sw/siw/siw_qp_rx.c
> index ed4fc39718b4..f5f69de56882 100644
> --- a/drivers/infiniband/sw/siw/siw_qp_rx.c
> +++ b/drivers/infiniband/sw/siw/siw_qp_rx.c
> @@ -740,6 +740,7 @@ static int siw_orqe_start_rx(struct siw_qp *qp)
>   {
>   	struct siw_sqe *orqe;
>   	struct siw_wqe *wqe = NULL;
> +	u16 orqe_flags;
>   
>   	if (unlikely(!qp->attrs.orq_size))
>   		return -EPROTO;
> @@ -748,7 +749,8 @@ static int siw_orqe_start_rx(struct siw_qp *qp)
>   	smp_mb();
>   
>   	orqe = orq_get_current(qp);
> -	if (READ_ONCE(orqe->flags) & SIW_WQE_VALID) {

In this if test, READ_ONCE is needed to read orqe->flags. But in this 
commit, this READ_ONCE is moved to other places.

In a complicated environment, for example, this function is called many 
times at the same time and orqe->flags is changed at the same time, I am 
not sure if this will introduce risks or not.

if you need to ensure the consistency of the flags throughout the 
function, not sure if the following is better or not.

if (((orqe_flags=READ_ONCE(orqe->flags))) & SIW_WQE_VALID) {

Thanks,
Zhu Yanjun

> +	orqe_flags = READ_ONCE(orqe->flags);
> +	if (orqe_flags & SIW_WQE_VALID) {
>   		/* RRESP is a TAGGED RDMAP operation */
>   		wqe = rx_wqe(&qp->rx_tagged);
>   		wqe->sqe.id = orqe->id;
> @@ -756,7 +758,7 @@ static int siw_orqe_start_rx(struct siw_qp *qp)
>   		wqe->sqe.sge[0].laddr = orqe->sge[0].laddr;
>   		wqe->sqe.sge[0].lkey = orqe->sge[0].lkey;
>   		wqe->sqe.sge[0].length = orqe->sge[0].length;
> -		wqe->sqe.flags = orqe->flags;
> +		wqe->sqe.flags = orqe_flags;
>   		wqe->sqe.num_sge = 1;
>   		wqe->bytes = orqe->sge[0].length;
>   		wqe->processed = 0;