linux-ext4 - Re: [PATCH 2/2] ext4: dax: keep orphan list before truncate overflow allocated blocks

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] [day] [month] [year] [list]
Message-ID: <20240828092742.t6kbk7sky5ime7iq@quack3>
Date: Wed, 28 Aug 2024 11:27:42 +0200
From: Jan Kara <jack@...e.cz>
To: yangerkun <yangerkun@...weicloud.com>
Cc: Jan Kara <jack@...e.cz>, tytso@....edu, adilger.kernel@...ger.ca,
	linux-ext4@...r.kernel.org, yangerkun@...wei.com,
	chengzhihao1@...wei.com
Subject: Re: [PATCH 2/2] ext4: dax: keep orphan list before truncate overflow
 allocated blocks

On Wed 28-08-24 10:06:46, yangerkun wrote:
> 
> 
> 在 2024/8/28 1:08, Jan Kara 写道:
> > On Tue 20-08-24 22:06:57, yangerkun wrote:
> > > From: yangerkun <yangerkun@...wei.com>
> > 
> > Thanks for debugging this. Couple of spelling fixes first:
> 
> Hi,
> 
> Thank you for your patient review!
> 
> > > Any extended write for ext4 requires the inode to be placed on the
> >        ^^^ extending
> > 
> > > orphan list before the actual write. In addition, the inode can be
> > > actually removed from the orphan list only after all writes are
> > > completed. Otherwise, those overcommitted blocks (If the allocated
> > 	     ^^ I'd phrase this: Otherwise we'd leave allocated blocks
> > beyond i_disksize if we could not copy all the data into allocated block
> > and e2fsck would complain.
> > 
> > > blocks are not written due to certain reasons, the inode size does not
> > > exceed the offset of these blocks) The leak status is always retained,
> > > and fsck reports an alarm for this scenario.
> > > 
> > > Currently, the dio and buffer IO comply with this logic. However, the
> > 			 ^^ buffered
> > 
> > BTW: The only reason why direct IO doesn't have this problem is because
> > we don't do short writes for direct IO. We either submit all or we return
> > error.
> 
> Yeah. In fact, the first version in my mind is same as this, don't do
> short write for dax too. But thinking deeper, it seems better to keep
> the blocks that has been successfully written...
> 
> 
> > 
> > > dax write will removed the inode from orphan list since
> > 		  ^^^ remove           ^ the orphan ...
> > 
> > > ext4_handle_inode_extension is unconditionally called during extend
> > 								^^ extending
> > 
> > > write. Fix it with this patch. We open the code from
> > > ext4_handle_inode_extension since we want to keep the blocks valid
> > > has been allocated and write success.
> > > 
> > > Signed-off-by: yangerkun <yangerkun@...wei.com>
> > > ---
> > >   fs/ext4/file.c | 35 +++++++++++++++++++++++++++++++----
> > >   1 file changed, 31 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> > > index be061bb64067..fd8597eef75e 100644
> > > --- a/fs/ext4/file.c
> > > +++ b/fs/ext4/file.c
> > > @@ -628,11 +628,12 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
> > >   static ssize_t
> > >   ext4_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
> > >   {
> > > -	ssize_t ret;
> > > +	ssize_t ret, written;
> > >   	size_t count;
> > >   	loff_t offset;
> > >   	handle_t *handle;
> > >   	bool extend = false;
> > > +	bool need_trunc = true;
> > >   	struct inode *inode = file_inode(iocb->ki_filp);
> > >   	if (iocb->ki_flags & IOCB_NOWAIT) {
> > > @@ -668,10 +669,36 @@ ext4_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
> > >   	ret = dax_iomap_rw(iocb, from, &ext4_iomap_ops);
> > > -	if (extend) {
> > > -		ret = ext4_handle_inode_extension(inode, offset, ret);
> > > -		ext4_inode_extension_cleanup(inode, ret < (ssize_t)count);
> > > +	if (!extend)
> > > +		goto out;
> > > +
> > > +	if (ret <= 0)
> > > +		goto err_trunc;
> > > +
> > > +	written = ret;
> > > +	handle = ext4_journal_start(inode, EXT4_HT_INODE, 2);
> > > +	if (IS_ERR(handle)) {
> > > +		ret = PTR_ERR(handle);
> > > +		goto err_trunc;
> > >   	}
> > > +
> > > +	if (ext4_update_inode_size(inode, offset + written)) {
> > > +		ret = ext4_mark_inode_dirty(handle, inode);
> > > +		if (unlikely(ret)) {
> > > +			ext4_journal_stop(handle);
> > > +			goto err_trunc;
> > > +		}
> > > +	}
> > > +
> > > +	if (written == count)
> > > +		need_trunc = false;
> > > +
> > > +	if (inode->i_nlink)
> > > +		ext4_orphan_del(handle, inode);
> > 
> > Why did you keep ext4_orphan_del() here? I thought the whole point of this
> 
> Sorry, I make a mistake here, there should be a truncate before. Thanks
> for point out this!
> 
> > patch is to avoid it? In fact, rather then opencoding
> > ext4_handle_inode_extension() I'd add argument to
> > ext4_handle_inode_extension() like:
> > 
> > ext4_handle_inode_extension(inode, pos, written, allocated)
> > 
> > and remove inode from the orphan list only if written == allocated. The
> > call site in ext4_dio_write_end_io() would call:
> > 
> > 	/*
> > 	 * For DIO we don't do partial writes so we must have submitted all
> > 	 * that was allocated.
> > 	 */
> > 	return ext4_handle_inode_extension(inode, pos, size, size);
> > 
> > and the call site in ext4_dax_write_iter() would call:
> > 
> > 	ret = ext4_handle_inode_extension(inode, offset, ret, count);
> > 
> > What do you think?
> 
> Great! This seems more clearly and I think it should works too. Whould I
> send a v2 patch for this?

Yes, please! If you could send a fix, that would be great.

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