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: <20201222175518.GD3248@redhat.com>
Date:   Tue, 22 Dec 2020 12:55:18 -0500
From:   Vivek Goyal <vgoyal@...hat.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-unionfs@...r.kernel.org, jlayton@...nel.org,
        amir73il@...il.com, sargun@...gun.me, miklos@...redi.hu,
        jack@...e.cz, neilb@...e.com, viro@...iv.linux.org.uk, hch@....de
Subject: Re: [PATCH 3/3] overlayfs: Report writeback errors on upper

On Tue, Dec 22, 2020 at 05:46:37PM +0000, Matthew Wilcox wrote:
> On Tue, Dec 22, 2020 at 11:29:25AM -0500, Vivek Goyal wrote:
> > On Tue, Dec 22, 2020 at 04:20:27PM +0000, Matthew Wilcox wrote:
> > > On Mon, Dec 21, 2020 at 02:50:55PM -0500, Vivek Goyal wrote:
> > > > +static int ovl_errseq_check_advance(struct super_block *sb, struct file *file)
> > > > +{
> > > > +	struct ovl_fs *ofs = sb->s_fs_info;
> > > > +	struct super_block *upper_sb;
> > > > +	int ret;
> > > > +
> > > > +	if (!ovl_upper_mnt(ofs))
> > > > +		return 0;
> > > > +
> > > > +	upper_sb = ovl_upper_mnt(ofs)->mnt_sb;
> > > > +
> > > > +	if (!errseq_check(&upper_sb->s_wb_err, file->f_sb_err))
> > > > +		return 0;
> > > > +
> > > > +	/* Something changed, must use slow path */
> > > > +	spin_lock(&file->f_lock);
> > > > +	ret = errseq_check_and_advance(&upper_sb->s_wb_err, &file->f_sb_err);
> > > > +	spin_unlock(&file->f_lock);
> > > 
> > > Why are you microoptimising syncfs()?  Are there really applications which
> > > call syncfs() in a massively parallel manner on the same file descriptor?
> > 
> > This is atleast theoritical race. I am not aware which application can
> > trigger this race. So to me it makes sense to fix the race.
> > 
> > Jeff Layton also posted a fix for syncfs().
> > 
> > https://lore.kernel.org/linux-fsdevel/20201219134804.20034-1-jlayton@kernel.org/
> > 
> > To me it makes sense to fix the race irrespective of the fact if somebody
> > hit it or not. People end up copying code in other parts of kernel and
> > and they will atleast copy race free code.
> 
> Let me try again.  "Why are you trying to avoid taking the spinlock?"

Aha.., sorry, I misunderstood your question. I don't have a good answer.
I just copied the code from Jeff Layton's patch.

Agreed that cost of taking spin lock will not be significant until
syncfs() is called at high frequency. Having said that, most of the
time taking spin lock will not be needed, so avoiding it with
a simple call to errseq_check() sounds reasonable too.

I don't have any strong opinions here. I am fine with any of the
implementation people like.

Vivek

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ