[PATCH v2 0/2] open: make RESOLVE_CACHED correctly test for O_TMPFILE

[PATCH v2 0/2] open: make RESOLVE_CACHED correctly test for O_TMPFILE

[Aleksa Sarai]

There were a few places that were incorrectly testing for whether an
open(2) operation was O_TMPFILE by doing (flags & O_TMPFILE). As
O_TMPFILE is defined as __O_TMPFILE|O_DIRECTORY, this would cause the
code to assume that O_DIRECTORY is equivalent to O_TMPFILE.

The only places where this happened were in RESOLVE_CACHED and
io_uring's checking related to RESOLVE_CACHED, so the only bug this
really fixes is that now O_DIRECTORY will no longer cause RESOLVE_CACHED
to always fail with -EAGAIN (and io_uring will thus be faster when doing
O_DIRECTORY opens).

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
Changes in v2:
- fix io_uring's io_openat_force_async as well.
- v1: <https://lore.kernel.org/r/20230806-resolve_cached-o_tmpfile-v1-1-7ba16308465e@cyphar.com>

---
Aleksa Sarai (2):
      open: make RESOLVE_CACHED correctly test for O_TMPFILE
      io_uring: correct check for O_TMPFILE

 fs/open.c            | 2 +-
 io_uring/openclose.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
---
base-commit: bf5ad7af0516cb47121dae1b1c160e4385615274
change-id: 20230806-resolve_cached-o_tmpfile-978cb238bd68

Best regards,
-- 
Aleksa Sarai <cyphar@cyphar.com>

[PATCH v2 1/2] open: make RESOLVE_CACHED correctly test for O_TMPFILE

[Aleksa Sarai]

O_TMPFILE is actually __O_TMPFILE|O_DIRECTORY. This means that the old
fast-path check for RESOLVE_CACHED would reject all users passing
O_DIRECTORY with -EAGAIN, when in fact the intended test was to check
for __O_TMPFILE.

Cc: stable@vger.kernel.org # v5.12+
Fixes: 99668f618062 ("fs: expose LOOKUP_CACHED through openat2() RESOLVE_CACHED")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 fs/open.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/open.c b/fs/open.c
index b8883ec286f5..2634047c9e9f 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1337,7 +1337,7 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
 		lookup_flags |= LOOKUP_IN_ROOT;
 	if (how->resolve & RESOLVE_CACHED) {
 		/* Don't bother even trying for create/truncate/tmpfile open */
-		if (flags & (O_TRUNC | O_CREAT | O_TMPFILE))
+		if (flags & (O_TRUNC | O_CREAT | __O_TMPFILE))
 			return -EAGAIN;
 		lookup_flags |= LOOKUP_CACHED;
 	}

-- 
2.41.0

[PATCH v2 2/2] io_uring: correct check for O_TMPFILE

[Aleksa Sarai]

O_TMPFILE is actually __O_TMPFILE|O_DIRECTORY. This means that the old
check for whether RESOLVE_CACHED can be used would incorrectly think
that O_DIRECTORY could not be used with RESOLVE_CACHED.

Cc: stable@vger.kernel.org # v5.12+
Fixes: 3a81fd02045c ("io_uring: enable LOOKUP_CACHED path resolution for filename lookups")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 io_uring/openclose.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/io_uring/openclose.c b/io_uring/openclose.c
index 10ca57f5bd24..a029c230119f 100644
--- a/io_uring/openclose.c
+++ b/io_uring/openclose.c
@@ -35,9 +35,9 @@ static bool io_openat_force_async(struct io_open *open)
 {
 	/*
 	 * Don't bother trying for O_TRUNC, O_CREAT, or O_TMPFILE open,
-	 * it'll always -EAGAIN
+	 * it'll always -EAGAIN.
 	 */
-	return open->how.flags & (O_TRUNC | O_CREAT | O_TMPFILE);
+	return open->how.flags & (O_TRUNC | O_CREAT | __O_TMPFILE);
 }
 
 static int __io_openat_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)

-- 
2.41.0

Re: [PATCH v2 2/2] io_uring: correct check for O_TMPFILE

[Jens Axboe]

On 8/5/23 4:48?PM, Aleksa Sarai wrote:
> O_TMPFILE is actually __O_TMPFILE|O_DIRECTORY. This means that the old
> check for whether RESOLVE_CACHED can be used would incorrectly think
> that O_DIRECTORY could not be used with RESOLVE_CACHED.
> 
> Cc: stable@vger.kernel.org # v5.12+
> Fixes: 3a81fd02045c ("io_uring: enable LOOKUP_CACHED path resolution for filename lookups")
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
>  io_uring/openclose.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/io_uring/openclose.c b/io_uring/openclose.c
> index 10ca57f5bd24..a029c230119f 100644
> --- a/io_uring/openclose.c
> +++ b/io_uring/openclose.c
> @@ -35,9 +35,9 @@ static bool io_openat_force_async(struct io_open *open)
>  {
>  	/*
>  	 * Don't bother trying for O_TRUNC, O_CREAT, or O_TMPFILE open,
> -	 * it'll always -EAGAIN
> +	 * it'll always -EAGAIN.

Please don't make this change, it just detracts from the actual change.
And if we are making changes in there, why not change O_TMPFILE as well
since this is what the change is about?

-- 
Jens Axboe

Re: [PATCH v2 2/2] io_uring: correct check for O_TMPFILE

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 1538 bytes --]

On 2023-08-05, Jens Axboe <axboe@kernel.dk> wrote:
> On 8/5/23 4:48?PM, Aleksa Sarai wrote:
> > O_TMPFILE is actually __O_TMPFILE|O_DIRECTORY. This means that the old
> > check for whether RESOLVE_CACHED can be used would incorrectly think
> > that O_DIRECTORY could not be used with RESOLVE_CACHED.
> > 
> > Cc: stable@vger.kernel.org # v5.12+
> > Fixes: 3a81fd02045c ("io_uring: enable LOOKUP_CACHED path resolution for filename lookups")
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > ---
> >  io_uring/openclose.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/io_uring/openclose.c b/io_uring/openclose.c
> > index 10ca57f5bd24..a029c230119f 100644
> > --- a/io_uring/openclose.c
> > +++ b/io_uring/openclose.c
> > @@ -35,9 +35,9 @@ static bool io_openat_force_async(struct io_open *open)
> >  {
> >  	/*
> >  	 * Don't bother trying for O_TRUNC, O_CREAT, or O_TMPFILE open,
> > -	 * it'll always -EAGAIN
> > +	 * it'll always -EAGAIN.
> 
> Please don't make this change, it just detracts from the actual change.
> And if we are making changes in there, why not change O_TMPFILE as well
> since this is what the change is about?

Userspace can't pass just __O_TMPFILE, so to me "__O_TMPFILE open"
sounds strange. The intention is to detect open(O_TMPFILE), it just so
happens that the correct check is __O_TMPFILE.

But I can change it if you prefer.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 2/2] io_uring: correct check for O_TMPFILE

[Jens Axboe]

On 8/6/23 12:42?AM, Aleksa Sarai wrote:
> On 2023-08-05, Jens Axboe <axboe@kernel.dk> wrote:
>> On 8/5/23 4:48?PM, Aleksa Sarai wrote:
>>> O_TMPFILE is actually __O_TMPFILE|O_DIRECTORY. This means that the old
>>> check for whether RESOLVE_CACHED can be used would incorrectly think
>>> that O_DIRECTORY could not be used with RESOLVE_CACHED.
>>>
>>> Cc: stable@vger.kernel.org # v5.12+
>>> Fixes: 3a81fd02045c ("io_uring: enable LOOKUP_CACHED path resolution for filename lookups")
>>> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
>>> ---
>>>  io_uring/openclose.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/io_uring/openclose.c b/io_uring/openclose.c
>>> index 10ca57f5bd24..a029c230119f 100644
>>> --- a/io_uring/openclose.c
>>> +++ b/io_uring/openclose.c
>>> @@ -35,9 +35,9 @@ static bool io_openat_force_async(struct io_open *open)
>>>  {
>>>  	/*
>>>  	 * Don't bother trying for O_TRUNC, O_CREAT, or O_TMPFILE open,
>>> -	 * it'll always -EAGAIN
>>> +	 * it'll always -EAGAIN.
>>
>> Please don't make this change, it just detracts from the actual change.
>> And if we are making changes in there, why not change O_TMPFILE as well
>> since this is what the change is about?
> 
> Userspace can't pass just __O_TMPFILE, so to me "__O_TMPFILE open"
> sounds strange. The intention is to detect open(O_TMPFILE), it just so
> happens that the correct check is __O_TMPFILE.

Right, but it's confusing now as the comment refers to O_TMPFILE but
__O_TMPFILE is being used. I'd include a comment in there on why it's
__O_TMPFILE and not O_TMPFILE, that's the interesting bit. As it stands,
you'd read the comment and look at the code and need to figure that on
your own. Hence it deserves a comment.

-- 
Jens Axboe