diff mbox series

[for-rc] IB/rdmavt: Validate remote_addr during loopback atomic tests

Message ID 1642584489-141005-1-git-send-email-mike.marciniszyn@cornelisnetworks.com (mailing list archive)
State Accepted
Delegated to: Jason Gunthorpe
Headers show
Series [for-rc] IB/rdmavt: Validate remote_addr during loopback atomic tests | expand

Commit Message

Marciniszyn, Mike Jan. 19, 2022, 9:28 a.m. UTC 
From: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>

The rdma-core test suite sends an unaligned remote address
and expects a failure.

ERROR: test_atomic_non_aligned_addr (tests.test_atomic.AtomicTest)

The qib/hfi1 rc handling validates properly, but the test has the
client and server on the same system.

The loopback of these operations is a distinct code path.

Fix by syntaxing the proposed remote address in the loopback
code path.

Fixes: 15703461533a ("IB/{hfi1, qib, rdmavt}: Move ruc_loopback to rdmavt")
Reviewed-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
Signed-off-by: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
---
 drivers/infiniband/sw/rdmavt/qp.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Leon Romanovsky Jan. 23, 2022, 10:34 a.m. UTC | #1 
On Wed, Jan 19, 2022 at 04:28:09AM -0500, mike.marciniszyn@cornelisnetworks.com wrote:
> From: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
> 
> The rdma-core test suite sends an unaligned remote address
> and expects a failure.
> 
> ERROR: test_atomic_non_aligned_addr (tests.test_atomic.AtomicTest)
> 
> The qib/hfi1 rc handling validates properly, but the test has the
> client and server on the same system.
> 
> The loopback of these operations is a distinct code path.
> 
> Fix by syntaxing the proposed remote address in the loopback
> code path.
> 
> Fixes: 15703461533a ("IB/{hfi1, qib, rdmavt}: Move ruc_loopback to rdmavt")
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
> Signed-off-by: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
> ---
>  drivers/infiniband/sw/rdmavt/qp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c
> index 3305f27..ae50b56 100644
> --- a/drivers/infiniband/sw/rdmavt/qp.c
> +++ b/drivers/infiniband/sw/rdmavt/qp.c
> @@ -3073,6 +3073,8 @@ void rvt_ruc_loopback(struct rvt_qp *sqp)
>  	case IB_WR_ATOMIC_FETCH_AND_ADD:
>  		if (unlikely(!(qp->qp_access_flags & IB_ACCESS_REMOTE_ATOMIC)))
>  			goto inv_err;
> +		if (unlikely(wqe->atomic_wr.remote_addr & (sizeof(u64) - 1)))

Isn't this "!PAGE_ALIGNED(wqe->atomic_wr.remote_addr)" check?

Thanks

> +			goto inv_err;
>  		if (unlikely(!rvt_rkey_ok(qp, &qp->r_sge.sge, sizeof(u64),
>  					  wqe->atomic_wr.remote_addr,
>  					  wqe->atomic_wr.rkey,
> -- 
> 1.8.3.1
>
Haakon Bugge Jan. 24, 2022, 12:25 p.m. UTC | #2 
> On 23 Jan 2022, at 11:34, Leon Romanovsky <leon@kernel.org> wrote:
> 
> On Wed, Jan 19, 2022 at 04:28:09AM -0500, mike.marciniszyn@cornelisnetworks.com wrote:
>> From: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
>> 
>> The rdma-core test suite sends an unaligned remote address
>> and expects a failure.
>> 
>> ERROR: test_atomic_non_aligned_addr (tests.test_atomic.AtomicTest)
>> 
>> The qib/hfi1 rc handling validates properly, but the test has the
>> client and server on the same system.
>> 
>> The loopback of these operations is a distinct code path.
>> 
>> Fix by syntaxing the proposed remote address in the loopback
>> code path.
>> 
>> Fixes: 15703461533a ("IB/{hfi1, qib, rdmavt}: Move ruc_loopback to rdmavt")
>> Reviewed-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
>> Signed-off-by: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
>> ---
>> drivers/infiniband/sw/rdmavt/qp.c | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c
>> index 3305f27..ae50b56 100644
>> --- a/drivers/infiniband/sw/rdmavt/qp.c
>> +++ b/drivers/infiniband/sw/rdmavt/qp.c
>> @@ -3073,6 +3073,8 @@ void rvt_ruc_loopback(struct rvt_qp *sqp)
>> 	case IB_WR_ATOMIC_FETCH_AND_ADD:
>> 		if (unlikely(!(qp->qp_access_flags & IB_ACCESS_REMOTE_ATOMIC)))
>> 			goto inv_err;
>> +		if (unlikely(wqe->atomic_wr.remote_addr & (sizeof(u64) - 1)))
> 
> Isn't this "!PAGE_ALIGNED(wqe->atomic_wr.remote_addr)" check?

No, it checks that the address is natural aligned, in this case the three LSBs must be zero. As per IBTA:

<quote>
The virtual address in the ATOMIC Command Request packet shall be naturally aligned to an 8 byte boundary.
</quote>


Thxs, Håkon

> 
> Thanks
> 
>> +			goto inv_err;
>> 		if (unlikely(!rvt_rkey_ok(qp, &qp->r_sge.sge, sizeof(u64),
>> 					  wqe->atomic_wr.remote_addr,
>> 					  wqe->atomic_wr.rkey,
>> -- 
>> 1.8.3.1
Leon Romanovsky Jan. 24, 2022, 12:33 p.m. UTC | #3 
On Mon, Jan 24, 2022 at 12:25:32PM +0000, Haakon Bugge wrote:
> 
> 
> > On 23 Jan 2022, at 11:34, Leon Romanovsky <leon@kernel.org> wrote:
> > 
> > On Wed, Jan 19, 2022 at 04:28:09AM -0500, mike.marciniszyn@cornelisnetworks.com wrote:
> >> From: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
> >> 
> >> The rdma-core test suite sends an unaligned remote address
> >> and expects a failure.
> >> 
> >> ERROR: test_atomic_non_aligned_addr (tests.test_atomic.AtomicTest)
> >> 
> >> The qib/hfi1 rc handling validates properly, but the test has the
> >> client and server on the same system.
> >> 
> >> The loopback of these operations is a distinct code path.
> >> 
> >> Fix by syntaxing the proposed remote address in the loopback
> >> code path.
> >> 
> >> Fixes: 15703461533a ("IB/{hfi1, qib, rdmavt}: Move ruc_loopback to rdmavt")
> >> Reviewed-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
> >> Signed-off-by: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
> >> ---
> >> drivers/infiniband/sw/rdmavt/qp.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >> 
> >> diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c
> >> index 3305f27..ae50b56 100644
> >> --- a/drivers/infiniband/sw/rdmavt/qp.c
> >> +++ b/drivers/infiniband/sw/rdmavt/qp.c
> >> @@ -3073,6 +3073,8 @@ void rvt_ruc_loopback(struct rvt_qp *sqp)
> >> 	case IB_WR_ATOMIC_FETCH_AND_ADD:
> >> 		if (unlikely(!(qp->qp_access_flags & IB_ACCESS_REMOTE_ATOMIC)))
> >> 			goto inv_err;
> >> +		if (unlikely(wqe->atomic_wr.remote_addr & (sizeof(u64) - 1)))
> > 
> > Isn't this "!PAGE_ALIGNED(wqe->atomic_wr.remote_addr)" check?
> 
> No, it checks that the address is natural aligned, in this case the three LSBs must be zero. As per IBTA:
> 
> <quote>
> The virtual address in the ATOMIC Command Request packet shall be naturally aligned to an 8 byte boundary.
> </quote>

And is IBTA restriction applicable to hfi1?

Thanks
> 
> 
> Thxs, Håkon
> 
> > 
> > Thanks
> > 
> >> +			goto inv_err;
> >> 		if (unlikely(!rvt_rkey_ok(qp, &qp->r_sge.sge, sizeof(u64),
> >> 					  wqe->atomic_wr.remote_addr,
> >> 					  wqe->atomic_wr.rkey,
> >> -- 
> >> 1.8.3.1
>
Haakon Bugge Jan. 24, 2022, 12:43 p.m. UTC | #4 
> On 24 Jan 2022, at 13:33, Leon Romanovsky <leon@kernel.org> wrote:
> 
> On Mon, Jan 24, 2022 at 12:25:32PM +0000, Haakon Bugge wrote:
>> 
>> 
>>> On 23 Jan 2022, at 11:34, Leon Romanovsky <leon@kernel.org> wrote:
>>> 
>>> On Wed, Jan 19, 2022 at 04:28:09AM -0500, mike.marciniszyn@cornelisnetworks.com wrote:
>>>> From: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
>>>> 
>>>> The rdma-core test suite sends an unaligned remote address
>>>> and expects a failure.
>>>> 
>>>> ERROR: test_atomic_non_aligned_addr (tests.test_atomic.AtomicTest)
>>>> 
>>>> The qib/hfi1 rc handling validates properly, but the test has the
>>>> client and server on the same system.
>>>> 
>>>> The loopback of these operations is a distinct code path.
>>>> 
>>>> Fix by syntaxing the proposed remote address in the loopback
>>>> code path.
>>>> 
>>>> Fixes: 15703461533a ("IB/{hfi1, qib, rdmavt}: Move ruc_loopback to rdmavt")
>>>> Reviewed-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
>>>> Signed-off-by: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
>>>> ---
>>>> drivers/infiniband/sw/rdmavt/qp.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>> 
>>>> diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c
>>>> index 3305f27..ae50b56 100644
>>>> --- a/drivers/infiniband/sw/rdmavt/qp.c
>>>> +++ b/drivers/infiniband/sw/rdmavt/qp.c
>>>> @@ -3073,6 +3073,8 @@ void rvt_ruc_loopback(struct rvt_qp *sqp)
>>>> 	case IB_WR_ATOMIC_FETCH_AND_ADD:
>>>> 		if (unlikely(!(qp->qp_access_flags & IB_ACCESS_REMOTE_ATOMIC)))
>>>> 			goto inv_err;
>>>> +		if (unlikely(wqe->atomic_wr.remote_addr & (sizeof(u64) - 1)))
>>> 
>>> Isn't this "!PAGE_ALIGNED(wqe->atomic_wr.remote_addr)" check?
>> 
>> No, it checks that the address is natural aligned, in this case the three LSBs must be zero. As per IBTA:
>> 
>> <quote>
>> The virtual address in the ATOMIC Command Request packet shall be naturally aligned to an 8 byte boundary.
>> </quote>
> 
> And is IBTA restriction applicable to hfi1?

For hfi1, I do not know. But this fix was in drivers/infiniband/sw/rdmavt, for which the first commit message states:

 This patch introduces the basics for a new module called rdma_vt. This new
    driver is a software implementation of the InfiniBand verbs...


More importantly, the check we discuss is not about being page-aligned, but about being naturally aligned, right?


Thxs, Håkon

> 
> Thanks
>> 
>> 
>> Thxs, Håkon
>> 
>>> 
>>> Thanks
>>> 
>>>> +			goto inv_err;
>>>> 		if (unlikely(!rvt_rkey_ok(qp, &qp->r_sge.sge, sizeof(u64),
>>>> 					  wqe->atomic_wr.remote_addr,
>>>> 					  wqe->atomic_wr.rkey,
>>>> -- 
>>>> 1.8.3.1
Marciniszyn, Mike Jan. 24, 2022, 1:20 p.m. UTC | #5 
> From: Haakon Bugge <haakon.bugge@oracle.com>
> Sent: Monday, January 24, 2022 7:44 AM
> To: Leon Romanovsky <leon@kernel.org>
> Cc: Marciniszyn, Mike <mike.marciniszyn@cornelisnetworks.com>;
> jgg@ziepe.ca; OFED mailing list <linux-rdma@vger.kernel.org>
> Subject: Re: [PATCH for-rc] IB/rdmavt: Validate remote_addr during
> loopback atomic tests
> > And is IBTA restriction applicable to hfi1?
> 
> For hfi1, I do not know. But this fix was in drivers/infiniband/sw/rdmavt, for
> which the first commit message states:
> 
>  This patch introduces the basics for a new module called rdma_vt. This new
>     driver is a software implementation of the InfiniBand verbs...
> 
> 
> More importantly, the check we discuss is not about being page-aligned, but
> about being naturally aligned, right?
> 

Hardware supported by rdamvt strives to be 100% compatible with the verbs API.

And yes, the natural alignment is the objective for the test.

According to the IB Spec (V1-Rel1.2.1 section 9.4.5 ATOMIC OPERATIONS):
        "The virtual address in the ATOMIC Command Request packet shall
        be naturally aligned to an 8 byte boundary. The responding CA
        checks this and returns an Invalid Request NAK if it is not naturally
        aligned."

The recent additions to the rdma-core test suite caught this issue.

The test is consistent with input packet parsing:

        case OP(COMPARE_SWAP):
        case OP(FETCH_ADD): {
<snip>
                if (unlikely(vaddr & (sizeof(u64) - 1)))
                        goto nack_inv_unlck;
Leon Romanovsky Jan. 25, 2022, 7:34 a.m. UTC | #6 
On Mon, Jan 24, 2022 at 01:20:56PM +0000, Marciniszyn, Mike wrote:
> > From: Haakon Bugge <haakon.bugge@oracle.com>
> > Sent: Monday, January 24, 2022 7:44 AM
> > To: Leon Romanovsky <leon@kernel.org>
> > Cc: Marciniszyn, Mike <mike.marciniszyn@cornelisnetworks.com>;
> > jgg@ziepe.ca; OFED mailing list <linux-rdma@vger.kernel.org>
> > Subject: Re: [PATCH for-rc] IB/rdmavt: Validate remote_addr during
> > loopback atomic tests
> > > And is IBTA restriction applicable to hfi1?
> > 
> > For hfi1, I do not know. But this fix was in drivers/infiniband/sw/rdmavt, for
> > which the first commit message states:
> > 
> >  This patch introduces the basics for a new module called rdma_vt. This new
> >     driver is a software implementation of the InfiniBand verbs...
> > 
> > 
> > More importantly, the check we discuss is not about being page-aligned, but
> > about being naturally aligned, right?
> > 
> 
> Hardware supported by rdamvt strives to be 100% compatible with the verbs API.
> 
> And yes, the natural alignment is the objective for the test.
> 
> According to the IB Spec (V1-Rel1.2.1 section 9.4.5 ATOMIC OPERATIONS):
>         "The virtual address in the ATOMIC Command Request packet shall
>         be naturally aligned to an 8 byte boundary. The responding CA
>         checks this and returns an Invalid Request NAK if it is not naturally
>         aligned."
> 
> The recent additions to the rdma-core test suite caught this issue.
> 
> The test is consistent with input packet parsing:
> 
>         case OP(COMPARE_SWAP):
>         case OP(FETCH_ADD): {
> <snip>
>                 if (unlikely(vaddr & (sizeof(u64) - 1)))
>                         goto nack_inv_unlck;

I see, thanks Mike and Haakon.
Jason Gunthorpe Jan. 28, 2022, 4:51 p.m. UTC | #7 
On Wed, Jan 19, 2022 at 04:28:09AM -0500, mike.marciniszyn@cornelisnetworks.com wrote:
> From: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
> 
> The rdma-core test suite sends an unaligned remote address
> and expects a failure.
> 
> ERROR: test_atomic_non_aligned_addr (tests.test_atomic.AtomicTest)
> 
> The qib/hfi1 rc handling validates properly, but the test has the
> client and server on the same system.
> 
> The loopback of these operations is a distinct code path.
> 
> Fix by syntaxing the proposed remote address in the loopback
> code path.
> 
> Fixes: 15703461533a ("IB/{hfi1, qib, rdmavt}: Move ruc_loopback to rdmavt")
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
> Signed-off-by: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>
> ---
>  drivers/infiniband/sw/rdmavt/qp.c | 2 ++
>  1 file changed, 2 insertions(+)

Applied to for-rc, thanks

Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c
index 3305f27..ae50b56 100644
--- a/drivers/infiniband/sw/rdmavt/qp.c
+++ b/drivers/infiniband/sw/rdmavt/qp.c
@@ -3073,6 +3073,8 @@  void rvt_ruc_loopback(struct rvt_qp *sqp)
 	case IB_WR_ATOMIC_FETCH_AND_ADD:
 		if (unlikely(!(qp->qp_access_flags & IB_ACCESS_REMOTE_ATOMIC)))
 			goto inv_err;
+		if (unlikely(wqe->atomic_wr.remote_addr & (sizeof(u64) - 1)))
+			goto inv_err;
 		if (unlikely(!rvt_rkey_ok(qp, &qp->r_sge.sge, sizeof(u64),
 					  wqe->atomic_wr.remote_addr,
 					  wqe->atomic_wr.rkey,