linux-ext4 - Re: [PATCH] ext4: use directio end_io error status to finish unwritten aio dio correctly

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: <20160219131829.GA30166@quack.suse.cz>
Date:	Fri, 19 Feb 2016 14:18:29 +0100
From:	Jan Kara <jack@...e.cz>
To:	Dave Chinner <david@...morbit.com>
Cc:	Christoph Hellwig <hch@...radead.org>,
	"Darrick J. Wong" <darrick.wong@...cle.com>,
	Theodore Ts'o <tytso@....edu>,
	linux-ext4 <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] ext4: use directio end_io error status to finish
 unwritten aio dio correctly

On Fri 19-02-16 09:02:32, Dave Chinner wrote:
> On Wed, Feb 17, 2016 at 10:01:48PM -0800, Christoph Hellwig wrote:
> > Might help to tell that this is on top of a direct-io.c patch from the
> > XFS tree.
> > 
> > I don't think clearing any flags is the right thing - now that we
> > always call ->end_io the code dealing with it in ext4_ext_direct_IO
> > can simply be moved to the ->end_io handler.
> > 
> > Something like the untested patch below:
> > 
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index 9db04dd..b741c79 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -3166,23 +3166,25 @@ static int ext4_end_io_dio(struct kiocb *iocb, loff_t offset,
> >  {
> >          ext4_io_end_t *io_end = iocb->private;
> >  
> > -	if (size <= 0)
> > -		return 0;
> > -
> >  	/* if not async direct IO just return */
> >  	if (!io_end)
> >  		return 0;
> >  
> > +	if (size <= 0) {
> > +		WARN_ON(io_end->flag & EXT4_IO_END_UNWRITTEN);
> > +		goto out;
> > +	}
> 
> That will still issue a warning when an I/O error occurs on an
> unwritten extent.

Ah, correct.

> > +
> >  	ext_debug("ext4_end_io_dio(): io_end 0x%p "
> >  		  "for inode %lu, iocb 0x%p, offset %llu, size %zd\n",
> >   		  iocb->private, io_end->inode->i_ino, iocb, offset,
> >  		  size);
> >  
> > -	iocb->private = NULL;
> >  	io_end->offset = offset;
> >  	io_end->size = size;
> > +out:
> >  	ext4_put_io_end(io_end);
> 
> Won't that now call ext4_put_io_end() ->
> ext4_convert_unwritten_extents() with an uninitialised offset and
> size?
> 
> i.e. I don't think this prevents warnings, and may make things
> worse when real errors occur....

Yeah, if IO error occurs while writing to unwritten extent we need to just
destroy the IO end without doing the extent conversion (since we don't know
how much got written). Attached patch should fix the issue - full xfstests
run is in progress but a quick check using generic/299 has passed.

How do we merge this? It depends on the changes in Dave's tree so do we
merge it via that? I have other ext4 changes pending in this area so Ted
would then have to pull some branch from Dave's tree. Guys?

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

View attachment "0001-ext4-Fix-data-exposure-after-failed-AIO-DIO.patch" of type "text/x-patch" (4359 bytes)