linux-kernel - Re: [PATCH -next,v2] fuse: return the more nuanced writeback error on close()

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: <9915b7b556106d2a525941141755adcca9e50163.camel@kernel.org>
Date:   Mon, 30 May 2022 10:02:06 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     Miklos Szeredi <miklos@...redi.hu>,
        Al Viro <viro@...iv.linux.org.uk>
Cc:     ChenXiaoSong <chenxiaosong2@...wei.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        liuyongqiang13@...wei.com, "zhangyi (F)" <yi.zhang@...wei.com>,
        zhangxiaoxu5@...wei.com, Steve French <smfrench@...il.com>,
        NeilBrown <neilb@...e.de>
Subject: Re: [PATCH -next,v2] fuse: return the more nuanced writeback error
 on close()

On Mon, 2022-05-30 at 14:13 +0200, Miklos Szeredi wrote:
> On Mon, 23 May 2022 at 03:35, ChenXiaoSong <chenxiaosong2@...wei.com> wrote:
> > 
> > As filemap_check_errors() only report -EIO or -ENOSPC, we return more nuanced
> > writeback error -(file->f_mapping->wb_err & MAX_ERRNO).
> > 
> >   filemap_write_and_wait
> >     filemap_write_and_wait_range
> >       filemap_check_errors
> >         -ENOSPC or -EIO
> >   filemap_check_wb_err
> >     errseq_check
> >       return -(file->f_mapping->wb_err & MAX_ERRNO)
> > 
> > Signed-off-by: ChenXiaoSong <chenxiaosong2@...wei.com>
> > ---
> >  fs/fuse/file.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index f18d14d5fea1..9917bc2795e6 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -488,10 +488,10 @@ static int fuse_flush(struct file *file, fl_owner_t id)
> >         inode_unlock(inode);
> > 
> >         err = filemap_check_errors(file->f_mapping);
> > +       /* return more nuanced writeback errors */
> >         if (err)
> > -               return err;
> > +               return filemap_check_wb_err(file->f_mapping, 0);
> 
> I'm wondering if this should be file_check_and_advance_wb_err() instead.
> 

I think that it probably shouldn't be, actually. Reason below...

> Is there a difference between ->flush() and ->fsync()?
> 
> Jeff, can you please help?
> 
> 

The main difference is that ->flush is called from filp_close, so it's
called when a file descriptor (or equivalent) is being torn down out,
whereas ->fsync is (obviously) called from the fsync codepath.

We _must_ report writeback errors on fsync, but reporting them on the
close() syscall is less clear. The thing about close() is that it's
going be successful no matter what is returned. The file descriptor will
no longer work afterward regardless.

fsync also must also initiate writeback of all the buffered data, but
it's not required for filesystems to do that on close() (and in fact,
there are good reasons not to if you can). A successful close() tells
you nothing about whether your data made it to the backing store. It
might just not have been synced out yet.

Personally, I think it's probably best to _not_ return writeback errors
on close at all. The only "legitimate" error on close is -EBADF.
Arguably, we should make ->flush be void return. Note that most
filp_close callers ignore the error anyway, so it's not much of a
stretch.

In any case, if you do decide to return errors in fuse_flush, then
advancing the cursor would also have the effect of masking writeback
errors on dup()'ed file descriptors, and I don't think you want to do
that.
-- 
Jeff Layton <jlayton@...nel.org>