linux-kernel - Re: [PATCH 22/23] NFS: swap-out must always use STABLE writes.

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]

Date:   Thu, 27 Jan 2022 08:42:49 +1100
From:   "NeilBrown" <neilb@...e.de>
To:     "Trond Myklebust" <trondmy@...merspace.com>
Cc:     "mgorman@...e.de" <mgorman@...e.de>,
        "dhowells@...hat.com" <dhowells@...hat.com>,
        "hch@...radead.org" <hch@...radead.org>,
        "anna.schumaker@...app.com" <anna.schumaker@...app.com>,
        "chuck.lever@...cle.com" <chuck.lever@...cle.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 22/23] NFS: swap-out must always use STABLE writes.

On Wed, 26 Jan 2022, Trond Myklebust wrote:
> On Mon, 2022-01-24 at 14:48 +1100, NeilBrown wrote:
> > The commit handling code is not safe against memory-pressure
> > deadlocks
> > when writing to swap.  In particular, nfs_commitdata_alloc() blocks
> > indefinitely waiting for memory, and this can consume all available
> > workqueue threads.
> > 
> > swap-out most likely uses STABLE writes anyway as COND_STABLE
> > indicates
> > that a stable write should be used if the write fits in a single
> > request, and it normally does.  However if we ever swap with a small
> > wsize, or gather unusually large numbers of pages for a single write,
> > this might change.
> > 
> > For safety, make it explicit in the code that direct writes used for
> > swap
> > must always use FLUSH_COND_STABLE.
> 
> OK. Your explanation above has me extremely confused.
> 
> If you want to avoid commit, then you should be using FLUSH_STABLE,
> since that forces the writes to be synchronous. FLUSH_COND_STABLE can
> and will use unstable writes if it sees that there are more writes to
> come.
> 
> > 
> > Signed-off-by: NeilBrown <neilb@...e.de>
> > ---
> >  fs/nfs/direct.c |    7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
> > index 43a956d7fd62..29c007b2a17a 100644
> > --- a/fs/nfs/direct.c
> > +++ b/fs/nfs/direct.c
> > @@ -791,7 +791,7 @@ static const struct nfs_pgio_completion_ops
> > nfs_direct_write_completion_ops = {
> >   */
> >  static ssize_t nfs_direct_write_schedule_iovec(struct nfs_direct_req
> > *dreq,
> >                                                struct iov_iter *iter,
> > -                                              loff_t pos)
> > +                                              loff_t pos, int
> > ioflags)
> >  {
> >         struct nfs_pageio_descriptor desc;
> >         struct inode *inode = dreq->inode;
> > @@ -799,7 +799,7 @@ static ssize_t
> > nfs_direct_write_schedule_iovec(struct nfs_direct_req *dreq,
> >         size_t requested_bytes = 0;
> >         size_t wsize = max_t(size_t, NFS_SERVER(inode)->wsize,
> > PAGE_SIZE);
> >  
> > -       nfs_pageio_init_write(&desc, inode, FLUSH_COND_STABLE, false,
> > +       nfs_pageio_init_write(&desc, inode, ioflags, false,
> >                               &nfs_direct_write_completion_ops);
> >         desc.pg_dreq = dreq;
> >         get_dreq(dreq);
> > @@ -905,6 +905,7 @@ ssize_t nfs_file_direct_write(struct kiocb *iocb,
> > struct iov_iter *iter,
> >         struct nfs_direct_req *dreq;
> >         struct nfs_lock_context *l_ctx;
> >         loff_t pos, end;
> > +       int ioflags = swap ? FLUSH_COND_STABLE : FLUSH_STABLE;
> 
> This is an unacceptable change in behaviour for the non-swap case, so
> NACK.
> 

Hi Trond,
 thanks for the review.
 You are right - I had that test exactly backwards.  I've fixed for the
 next version.

Thanks,
NeilBrown