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  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]
Date:   Fri, 10 May 2019 09:11:24 -0700
From:   Santosh Shilimkar <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/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.

Regards,
Santosh

Powered by blists - more mailing lists