linux-kernel - Re: [RFC PATCH rdma-next 08/10] RDMA/rxe: Implement flush execution in responder side

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: <d78ef78d-4a57-16cf-00d0-9b129f1eaad4@fujitsu.com>
Date:   Wed, 5 Jan 2022 01:43:12 +0000
From:   "lizhijian@...itsu.com" <lizhijian@...itsu.com>
To:     Tom Talpey <tom@...pey.com>,
        "lizhijian@...itsu.com" <lizhijian@...itsu.com>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "zyjzyj2000@...il.com" <zyjzyj2000@...il.com>,
        "jgg@...pe.ca" <jgg@...pe.ca>,
        "aharonl@...dia.com" <aharonl@...dia.com>,
        "leon@...nel.org" <leon@...nel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mbloch@...dia.com" <mbloch@...dia.com>,
        "liweihang@...wei.com" <liweihang@...wei.com>,
        "liangwenpeng@...wei.com" <liangwenpeng@...wei.com>,
        "yangx.jy@...itsu.com" <yangx.jy@...itsu.com>,
        "rpearsonhpe@...il.com" <rpearsonhpe@...il.com>,
        "y-goto@...itsu.com" <y-goto@...itsu.com>
Subject: Re: [RFC PATCH rdma-next 08/10] RDMA/rxe: Implement flush execution
 in responder side



On 05/01/2022 00:40, Tom Talpey wrote:
>
> On 12/28/2021 3:07 AM, Li Zhijian wrote:
>
>> +static enum resp_states process_flush(struct rxe_qp *qp,
>> +                       struct rxe_pkt_info *pkt)
>> +{
>> +    u64 length = 0, start = qp->resp.va;
>> +    u32 sel = feth_sel(pkt);
>> +    u32 plt = feth_plt(pkt);
>> +    struct rxe_mr *mr = qp->resp.mr;
>> +
>> +    if (sel == IB_EXT_SEL_MR_RANGE)
>> +        length = qp->resp.length;
>> +    else if (sel == IB_EXT_SEL_MR_WHOLE)
>> +        length = mr->cur_map_set->length;
>
> I noticed another issue in this. When the "Memory Region" flag is
> set, the RETH:VA field in the request is ignored, in addition to
> the RETH:DMALEN. Therefore, both the start and length must be set
> from the map.
Yes, that's true

Actually, i have drafted a new version last week when you first time pointed out this.
https://github.com/zhijianli88/linux/commit/a368d821b163f74836d527638625d414c7a62d00#diff-1b675c68d2f5f664abefea05cbf415f6307c9920b31a13bed41105c4a84e0259R641
the latest code is like:
     if (sel == IB_EXT_SEL_MR_RANGE) {
         start = qp->resp.va;
         length = qp->resp.length;
     } else { /* sel == IB_EXT_SEL_MR_WHOLE */
         start = mr->cur_map_set->iova;
         length = mr->cur_map_set->length;
     }


>
> Is the mr->cur_map_set[0]->addr field a virtual address, or a
> physical? 
mr->cur_map_set[0]->addr is a virtual address in kernel space.

nvdimm_flush_iova() will accept a user space address(alloc(), mmap() by applications),
this address could be transited to a kernel virtual address by lookup_iova() where it
will refer to mr->cur_map_set[n]->addr.

So i think we should use qp->resp.va and mr->cur_map_set->iova that are both user
space address.



> I can't find the cur_map_set in any patch. 
cur_map_set will be filled in rxe_mr_init_user(),
rxe_mr_init_user()
  -> ib_umem_get()
    -> pin_user_pages_fast()
  -> update cur_map_set


BTW: some of you all comments are already addressed in
https://github.com/zhijianli88/linux/tree/rdma-flush
and it may forced update irregularly.


Thanks
Zhijian

> The virtual
> (mapped) address will be needed, to pass to arch_wb_cache_pmem().
>
> Something like this?
>
>     if (sel == IB_EXT_SEL_MR_WHOLE) {
>         length = mr->cur_map_set->length;
>         start = mr->cur_map_set[0]->addr;
>     }
>
> (in addition to my other review suggestions about 0-length, etc...)
>
> Tom.