linux-kernel - Re: [PATCH 1/4] iomap: Lift blocksize restriction on atomic writes

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: <20241205063029.GB7820@frogsfrogsfrogs>
Date: Wed, 4 Dec 2024 22:30:29 -0800
From: "Darrick J. Wong" <djwong@...nel.org>
To: Dave Chinner <david@...morbit.com>
Cc: John Garry <john.g.garry@...cle.com>, brauner@...nel.org,
	cem@...nel.org, dchinner@...hat.com, hch@....de,
	ritesh.list@...il.com, linux-xfs@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	martin.petersen@...cle.com
Subject: Re: [PATCH 1/4] iomap: Lift blocksize restriction on atomic writes

On Thu, Dec 05, 2024 at 07:35:45AM +1100, Dave Chinner wrote:
> On Wed, Dec 04, 2024 at 03:43:41PM +0000, John Garry wrote:
> > From: "Ritesh Harjani (IBM)" <ritesh.list@...il.com>
> > 
> > Filesystems like ext4 can submit writes in multiples of blocksizes.
> > But we still can't allow the writes to be split into multiple BIOs. Hence
> > let's check if the iomap_length() is same as iter->len or not.
> > 
> > It is the responsibility of userspace to ensure that a write does not span
> > mixed unwritten and mapped extents (which would lead to multiple BIOs).
> 
> How is "userspace" supposed to do this?
> 
> No existing utility in userspace is aware of atomic write limits or
> rtextsize configs, so how does "userspace" ensure everything is
> laid out in a manner compatible with atomic writes?
> 
> e.g. restoring a backup (or other disaster recovery procedures) is
> going to have to lay the files out correctly for atomic writes.
> backup tools often sparsify the data set and so what gets restored
> will not have the same layout as the original data set...
> 
> Where's the documentation that outlines all the restrictions on
> userspace behaviour to prevent this sort of problem being triggered?
> Common operations such as truncate, hole punch, buffered writes,
> reflinks, etc will trip over this, so application developers, users
> and admins really need to know what they should be doing to avoid
> stepping on this landmine...

I'm kinda assuming that this requires forcealign to get the extent
alignments correct, and writing zeroes non-atomically if the extent
state gets mixed up before retrying the untorn write.  John?

> Further to that, what is the correction process for users to get rid
> of this error? They'll need some help from an atomic write
> constraint aware utility that can resilver the file such that these
> failures do not occur again. Can xfs_fsr do this? Or maybe the new
> exchangr-range code? Or does none of this infrastructure yet exist?

TBH aside from databases doing pure overwrites to storage hardware, I
think everyone else would be better served by start_commit /
commit_range since it's /much/ more flexible.

> > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@...il.com>
> > jpg: tweak commit message
> > Signed-off-by: John Garry <john.g.garry@...cle.com>
> > ---
> >  fs/iomap/direct-io.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> > index b521eb15759e..3dd883dd77d2 100644
> > --- a/fs/iomap/direct-io.c
> > +++ b/fs/iomap/direct-io.c
> > @@ -306,7 +306,7 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
> >  	size_t copied = 0;
> >  	size_t orig_count;
> >  
> > -	if (atomic && length != fs_block_size)
> > +	if (atomic && length != iter->len)
> >  		return -EINVAL;
> 
> Given this is now rejecting IOs that are otherwise well formed from
> userspace, this situation needs to have a different errno now. The
> userspace application has not provided an invalid set of
> IO parameters for this IO - the IO has been rejected because
> the previously set up persistent file layout was screwed up by
> something in the past.
> 
> i.e. This is not an application IO submission error anymore, hence
> EINVAL is the wrong error to be returning to userspace here.

Admittedly it would be useful to add at least a couple of new errnos for
alignment issues and incorrect file storage mapping states.  How
difficult is it to get a new errno code added to uapi and then plumbed
into glibc?  Are new errno additions to libc still gate-kept by Ulrich
or is my knowlege 15 years out of date?

--D

> -Dave.
> -- 
> Dave Chinner
> david@...morbit.com
>