Re: [PATCH v11 1/1] proto: add ability to populate a filesystem from a directory

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Jul 30, 2025 at 04:40:39PM +0200, Luca Di Maio wrote:
> Thanks Darrick for the review!
> Sorry again for this indentation mess, I'm going to basically align all
> function arguments and all the top-function declarations
> I was a bit confused because elsewhere in the code is not like that so
> it's a bit difficult to infere
> 
> Offtopic: maybe we could also introduce an editorconfig setup? so that
> all various editors will be correctly set to see the tabs/spaces as
> needed (https://editorconfig.org/)

Hrmm, that /would/ be useful.

> Back on topic:
> 
> On Tue, Jul 29, 2025 at 02:43:22PM -0700, Darrick J. Wong wrote:

<snipping>

> > > +	if (!S_ISSOCK(file_stat.st_mode) &&
> > > +	    !S_ISLNK(file_stat.st_mode)  &&
> > > +	    !S_ISFIFO(file_stat.st_mode)) {
> > > +		close(fd);
> > > +		fd = openat(pathfd, entry->d_name,
> > > +			    O_NOFOLLOW | O_RDONLY | O_NOATIME);
> >
> > Just out of curiosity, does O_NOATIME not work in the previous openat?

[narrator: it doesn't]

> Actually on my test setup (mainly using docker/podman to test), opening
> with and without O_NOATIME when using O_PATH, does not change accesstime
> checking with `stat`, but also it works if I add it.
> As a precautionary measure (not sure if podman/docker is messing with
> noatime) I'll add it, as it seems to work correctly.

On second thought I think you might leave the double opens because
O_NOATIME is only allowed if the current user owns source file, or has
CAP_FOWNER.  If you're running mkfs as an unprivileged user trying to
capture a rootfs (with uid 0 files) then O_NOATIME won't be allowed.

Maybe something along the lines of:

	/*
	 * Try to open the source file noatime to avoid a flood of
	 * writes to the source fs, but we can fall back to plain
	 * readonly mode if we don't have enough permission.
	 */
	fd = openat(pathfd, entry->d_name, O_NOFOLLOW | O_RDONLY | O_NOATIME);
	if (fd < 0)
		fd = openat(pathfd, entry->d_name, O_NOFOLLOW | O_RDONLY);
	if (fd < 0)
		/* whine and exit */

Just to see if you can open the source file without touching atime?

--D

> > > +		 * this will make flistxattr() and fgetxattr() fail wil EBADF,
> >
> > "fail with EBADF"...
> >
> 
> Ack.
> 
> > > +	/*
> > > +	 * Copy over attributes.
> > > +	 */
> > > +	writeattrs(ip, path_buf, fd);
> >
> > Nothing closes fd here; does it leak?
> >
> > --D
> >
> 
> Ack.
> 
> L.