linux-kernel - Re: [RFC PATCH rdma-next 05/10] RDMA/rxe: Allow registering persistent flag for pmem MR only

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: <45bfd837-a784-5ea2-7ae0-46e7e557b030@talpey.com>
Date:   Thu, 30 Dec 2021 17:25:01 -0500
From:   Tom Talpey <tom@...pey.com>
To:     Li Zhijian <lizhijian@...fujitsu.com>, linux-rdma@...r.kernel.org,
        zyjzyj2000@...il.com, jgg@...pe.ca, aharonl@...dia.com,
        leon@...nel.org
Cc:     linux-kernel@...r.kernel.org, mbloch@...dia.com,
        liweihang@...wei.com, liangwenpeng@...wei.com,
        yangx.jy@...fujitsu.com, rpearsonhpe@...il.com, y-goto@...itsu.com
Subject: Re: [RFC PATCH rdma-next 05/10] RDMA/rxe: Allow registering
 persistent flag for pmem MR only

On 12/28/2021 3:07 AM, Li Zhijian wrote:
> Memory region should support 2 placement types: IB_ACCESS_FLUSH_PERSISTENT
> and IB_ACCESS_FLUSH_GLOBAL_VISIBILITY, and only pmem/nvdimm has ability to
> persist data(IB_ACCESS_FLUSH_PERSISTENT).
> 
> Signed-off-by: Li Zhijian <lizhijian@...fujitsu.com>
> ---
>   drivers/infiniband/sw/rxe/rxe_mr.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
> index bcd5e7afa475..21616d058f29 100644
> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> @@ -206,6 +206,11 @@ static bool iova_in_pmem(struct rxe_mr *mr, u64 iova, int length)
>   	return page_in_dev_pagemap(page);
>   }
>   
> +static bool ib_check_flush_access_flags(struct ib_mr *mr, u32 flags)
> +{
> +	return mr->is_pmem || !(flags & IB_ACCESS_FLUSH_PERSISTENT);
> +}

It is perfectly allowed to flush ordinary memory, persistence is
another matter entirely. Is this subroutine checking for flush,
or persistence? Its name is confusing and needs to be clarified.

> +
>   int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova,
>   		     int access, struct rxe_mr *mr)
>   {
> @@ -282,6 +287,13 @@ int rxe_mr_init_user(struct rxe_pd *pd, u64 start, u64 length, u64 iova,
>   
>   	// iova_in_pmem must be called after set is updated
>   	mr->ibmr.is_pmem = iova_in_pmem(mr, iova, length);
> +	if (!ib_check_flush_access_flags(&mr->ibmr, access)) {
> +		pr_err("Cannot set IB_ACCESS_FLUSH_PERSISTENT for non-pmem memory\n");
> +		mr->state = RXE_MR_STATE_INVALID;
> +		mr->umem = NULL;
> +		err = -EINVAL;
> +		goto err_release_umem;
> +	}

Setting is_pmem is reasonable, but again, this is confusing with respect
to the region being flushable. In general, all memory is flushable,
provided the platform supports any kind of cache flush (i.e. all of them).

Tom.