linux-kernel - Re: [PATCH] vfs, syncfs: Do not ignore return code from ->sync

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: <b7ac787b170e18b620bfb6879eaf567831a0ae34.camel@kernel.org>
Date:   Wed, 16 Dec 2020 10:44:49 -0500
From:   Jeff Layton <jlayton@...nel.org>
To:     Vivek Goyal <vgoyal@...hat.com>
Cc:     Linux fsdevel mailing list <linux-fsdevel@...r.kernel.org>,
        linux-unionfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        viro@...iv.linux.org.uk, miklos@...redi.hu, amir73il@...il.com,
        willy@...radead.org, jack@...e.cz, sargun@...gun.me
Subject: Re: [PATCH] vfs, syncfs: Do not ignore return code from ->sync_fs()

On Wed, 2020-12-16 at 10:14 -0500, Vivek Goyal wrote:
> On Wed, Dec 16, 2020 at 09:57:49AM -0500, Jeff Layton wrote:
> > On Wed, 2020-12-16 at 09:38 -0500, Vivek Goyal wrote:
> > > I see that current implementation of __sync_filesystem() ignores the
> > > return code from ->sync_fs(). I am not sure why that's the case.
> > > 
> > > Ignoring ->sync_fs() return code is problematic for overlayfs where
> > > it can return error if sync_filesystem() on upper super block failed.
> > > That error will simply be lost and sycnfs(overlay_fd), will get
> > > success (despite the fact it failed).
> > > 
> > > I am assuming that we want to continue to call __sync_blockdev()
> > > despite the fact that there have been errors reported from
> > > ->sync_fs(). So I wrote this simple patch which captures the
> > > error from ->sync_fs() but continues to call __sync_blockdev()
> > > and returns error from sync_fs() if there is one.
> > > 
> > > There might be some very good reasons to not capture ->sync_fs()
> > > return code, I don't know. Hence thought of proposing this patch.
> > > Atleast I will get to know the reason. I still need to figure
> > > a way out how to propagate overlay sync_fs() errors to user
> > > space.
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@...hat.com>
> > > ---
> > >  fs/sync.c |    8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > Index: redhat-linux/fs/sync.c
> > > ===================================================================
> > > --- redhat-linux.orig/fs/sync.c	2020-12-16 09:15:49.831565653 -0500
> > > +++ redhat-linux/fs/sync.c	2020-12-16 09:23:42.499853207 -0500
> > > @@ -30,14 +30,18 @@
> > >   */
> > >  static int __sync_filesystem(struct super_block *sb, int wait)
> > >  {
> > > +	int ret, ret2;
> > > +
> > >  	if (wait)
> > >  		sync_inodes_sb(sb);
> > >  	else
> > >  		writeback_inodes_sb(sb, WB_REASON_SYNC);
> > >  
> > > 
> > >  	if (sb->s_op->sync_fs)
> > > -		sb->s_op->sync_fs(sb, wait);
> > > -	return __sync_blockdev(sb->s_bdev, wait);
> > > +		ret = sb->s_op->sync_fs(sb, wait);
> > > +	ret2 = __sync_blockdev(sb->s_bdev, wait);
> > > +
> > > +	return ret ? ret : ret2;
> > >  }
> > >  
> > > 
> > >  /*
> > > 
> > 
> > I posted a patchset that took a similar approach a couple of years ago,
> > and we decided not to go with it [1].
> > 
> > While it's not ideal to ignore the error here, I think this is likely to
> > break stuff.
> 
> So one side affect I see is that syncfs() might start returning errors
> in some cases which were not reported at all. I am wondering will that
> count as breakage.
> 
> > What may be better is to just make sync_fs void return, so
> > people don't think that returned errors there mean anything.
> 
> May be. 
> 
> But then question remains that how do we return error to user space
> in syncfs(fd) for overlayfs. I will not be surprised if other
> filesystems want to return errors as well.
> 
> Shall I create new helpers and call these in case of syncfs(). But
> that too will start returning new errors on syncfs(). So it does
> not solve that problem (if it is a problem).
> 
> Or we can define a new super block op say ->sync_fs2() and call that
> first and in that case capture return code. That way it will not
> impact existing cases and overlayfs can possibly make use of
> ->sync_fs2() and return error. IOW, impact will be limited to
> only file systems which chose to implement ->sync_fs2().
> 
> Thanks
> Vivek
> 

Sure, it's possible to add a sb->sync_fs2, but the problem is that
sync_fs is a superblock op, and is missing a lot of important context
about how it got called.

syncfs(2) syscall takes a file descriptor argument. I'd add a new f_op-
>syncfs vector and turn most of the current guts of the syncfs syscall
into a generic_syncfs() that gets called when f_op->syncfs isn't
defined.

Overlayfs could then add a ->syncfs op that would give it control over
what error gets returned. With that, you could basically leave the old
sb->sync_fs routine alone.

I think that's probably the safest approach for allowing overlayfs to
propagate syncfs errors from the upper layer to the overlay.

-- 
Jeff Layton <jlayton@...nel.org>