netdev - Re: [net-next][PATCH v2 1/2] rds: handle unsupported rdma request to fs dax memory

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: <9b0e6760-b684-c1ce-6bf5-738eff325240@oracle.com>
Date:   Fri, 10 May 2019 11:02:35 -0700
From:   "santosh.shilimkar@...cle.com" <santosh.shilimkar@...cle.com>
To:     Jason Gunthorpe <jgg@...pe.ca>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        Hans Westgaard Ry <hans.westgaard.ry@...cle.com>
Subject: Re: [net-next][PATCH v2 1/2] rds: handle unsupported rdma request to
 fs dax memory



On 5/10/19 10:55 AM, Jason Gunthorpe wrote:
> On Fri, May 10, 2019 at 09:11:24AM -0700, Santosh Shilimkar wrote:
>> On 5/10/2019 5:54 AM, Jason Gunthorpe wrote:
>>> On Mon, Apr 29, 2019 at 04:37:19PM -0700, Santosh Shilimkar wrote:
>>>> From: Hans Westgaard Ry <hans.westgaard.ry@...cle.com>
>>>>
>>>> RDS doesn't support RDMA on memory apertures that require On Demand
>>>> Paging (ODP), such as FS DAX memory. User applications can try to use
>>>> RDS to perform RDMA over such memories and since it doesn't report any
>>>> failure, it can lead to unexpected issues like memory corruption when
>>>> a couple of out of sync file system operations like ftruncate etc. are
>>>> performed.
>>>
>>> This comment doesn't make any sense..
>>>
>>>> The patch adds a check so that such an attempt to RDMA to/from memory
>>>> apertures requiring ODP will fail.
>>>>
>>>> Reviewed-by: Håkon Bugge <haakon.bugge@...cle.com>
>>>> Reviewed-tested-by: Zhu Yanjun <yanjun.zhu@...cle.com>
>>>> Signed-off-by: Hans Westgaard Ry <hans.westgaard.ry@...cle.com>
>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@...cle.com>
>>>>    net/rds/rdma.c | 5 +++--
>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/net/rds/rdma.c b/net/rds/rdma.c
>>>> index 182ab84..e0a6b72 100644
>>>> +++ b/net/rds/rdma.c
>>>> @@ -158,8 +158,9 @@ static int rds_pin_pages(unsigned long user_addr, unsigned int nr_pages,
>>>>    {
>>>>    	int ret;
>>>> -	ret = get_user_pages_fast(user_addr, nr_pages, write, pages);
>>>> -
>>>> +	/* get_user_pages return -EOPNOTSUPP for fs_dax memory */
>>>> +	ret = get_user_pages_longterm(user_addr, nr_pages,
>>>> +				      write, pages, NULL);
>>>
>>> GUP is supposed to fully work on DAX filesystems.
>>>
>> Above comment has typo. Should have been
>> get_user_pages_longterm return -EOPNOTSUPP.
>>
>>> You only need to switch to the long term version if the duration of
>>> the GUP is under control of user space - ie it may last forever.
>>>
>>> Short duration pins in the kernel do not need long term.
>>>
>> Thats true but the intention here is to use the long term version
>> which does check for the FS DAX memory. Instead of calling direct
>> accessor to check DAX memory region, longterm version of the API
>> is used
>>
>>> At a minimum the commit message needs re-writing to properly explain
>>> the motivation here.
>>>
>> Commit is actually trying to describe the motivation describing more of
>> issues of not making the call fail. The code comment typo was
>> misleading.
> 
> Every single sentence in the commit message is wrong
> 
I will rewrite commit message but can you please comment on other
questions above. GUP long term was used to detect whether its
fs_dax memory which could be misleading since the RDS MRs are
short lived. Do you want us to use accessor instead to check
if its FS DAX memory?

Regards,
Santosh