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: <20190510175538.GB13038@ziepe.ca>
Date:   Fri, 10 May 2019 14:55:38 -0300
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Santosh Shilimkar <santosh.shilimkar@...cle.com>
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 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

Jason