linux-kernel - Re: [PATCH v7 2/9] fs: Initial atomic write support

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]

Date: Wed, 5 Jun 2024 10:30:15 +0200
From: Christoph Hellwig <hch@....de>
To: John Garry <john.g.garry@...cle.com>
Cc: axboe@...nel.dk, kbusch@...nel.org, hch@....de, sagi@...mberg.me,
	jejb@...ux.ibm.com, martin.petersen@...cle.com, djwong@...nel.org,
	viro@...iv.linux.org.uk, brauner@...nel.org, dchinner@...hat.com,
	jack@...e.cz, linux-block@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-nvme@...ts.infradead.org,
	linux-fsdevel@...r.kernel.org, tytso@....edu, jbongio@...gle.com,
	linux-scsi@...r.kernel.org, ojaswin@...ux.ibm.com,
	linux-aio@...ck.org, linux-btrfs@...r.kernel.org,
	io-uring@...r.kernel.org, nilay@...ux.ibm.com,
	ritesh.list@...il.com, willy@...radead.org,
	Prasad Singamsetty <prasad.singamsetty@...cle.com>
Subject: Re: [PATCH v7 2/9] fs: Initial atomic write support

Highlevel question:  in a lot of the discussions we've used the
term "untorn writes" instead, which feels better than atomic to
me as atomic is a highly overloaded term.  Should we switch the
naming to that?

> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 0283cf366c2a..6cb67882bcfd 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -45,6 +45,7 @@
>  #include <linux/slab.h>
>  #include <linux/maple_tree.h>
>  #include <linux/rw_hint.h>
> +#include <linux/uio.h>

fs.h is included almost everywhere, so if we can avoid pulling in
even more dependencies that would be great.

It seems like it is pulled in just for this helper:

> +static inline
> +bool generic_atomic_write_valid(loff_t pos, struct iov_iter *iter)
> +{
> +	size_t len = iov_iter_count(iter);
> +
> +	if (!iter_is_ubuf(iter))
> +		return false;
> +
> +	if (!is_power_of_2(len))
> +		return false;
> +
> +	if (!IS_ALIGNED(pos, len))
> +		return false;
> +
> +	return true;
> +}

should that just go to uio.h instead, or move out of line?

Also the return type formatting is wrong, the two normal styles are
either:

static inline bool generic_atomic_write_valid(loff_t pos,
		struct iov_iter *iter)

or:

static inline bool
generic_atomic_write_valid(loff_t pos, struct iov_iter *iter)

(and while I'm at nitpicking, passing the pos before the iter
feels weird)

Last but not least: if READ/WRITE is passed to kiocb_set_rw_flags,
it should probably set IOCB_WRITE as well?  That might be a worthwile
prep patch on it's own.