Re: [PATCH v3 09/14] vfs: add O_CREAT|O_DIRECTORY to open*(2)

[Christian Brauner]

> Currently there is no way to race-freely create and open a directory.
> For regular files we have open(O_CREAT) for creating a new file inode,
> and returning a pinning fd to it. The lack of such functionality for
> directories means that when populating a directory tree there's always
> a race involved: the inodes first need to be created, and then opened
> to adjust their permissions/ownership/labels/timestamps/acls/xattrs/...,
> but in the time window between the creation and the opening they might
> be replaced by something else.
> 
> Addressing this race without proper APIs is possible (by immediately
> fstat()ing what was opened, to verify that it has the right inode type),
> but difficult to get right. Hence, adding support for a new flag combo

Eh, but that still means someone could've raced you and actually created
that directory. When you bring in a shared filesysem state with joint
cleanup responsibilities things aren't easily solved by fstat()ing.

> O_CREAT|O_DIRECTORY to open*(2) that creates a directory (if it does not
> exist already) and returns an O_DIRECTORY fd is very useful.
> 
> Historically, the O_CREAT|O_DIRECTORY behaviour was to return ENOTDIR if
> a regular file exists at the open path; EISDIR if a directory exists at
> the path; and to create a regular file if no file exists at the path.
> This behaviour changed accidentally with 973d4b73fbaf ("do_last(): rejoin
> the common path even earlier in FMODE_{OPENED,CREATED} case") causing
> ENOTDIR to return in the last case while still creating the file. As
> this change was not detected for a long time, Brauner proposed to adopt
> the more consistent NetBSD behaviour, i.e. to return EINVAL on the the
> O_CREAT|O_DIRECTORY combination. This change was applied in 43b450632676
> ("open: return EINVAL for O_DIRECTORY | O_CREAT") in March, 2023. As
> the EINVAL behaviour has been in the kernel for about 3 year now, no
> rollback is expected as a result of userspace reliance on old
> behaviour, leaving us free to reassign the O_CREAT|O_DIRECTORY semantics.
> 
> O_CREAT|O_DIRECTORY is made to reduce to a lookup on ->atomic_open()
> filesystems. This is implemented by stripping the O_CREAT bit. The other
> option of simply returning -EINVAL leads to inconsistent behaviour:

I think this doesn't spell out why that's done: afaict, netfses can't
currently deal with O_CREAT | O_DIRECTORY without protocol extensions.
So that means you force it into a fallback mode where it manages to open
an existing directory but fails with ENOENT otherwise. This should carry
a brief comment why you force that type of fallback.

Also, how do you deal with O_EXCL? If you strip O_CREAT but leave O_EXCL
depending on where exactly you do it, might this risk opening a
directory giving userspace the impression they exclusively created it?

-- 
Christian Brauner <brauner@xxxxxxxxxx>