diff mbox series

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

Message ID 20230806-resolve_cached-o_tmpfile-v2-2-058bff24fb16@cyphar.com
State New
Headers
Series open: make RESOLVE_CACHED correctly test for O_TMPFILE |

Commit Message

Aleksa Sarai Aug. 5, 2023, 10:48 p.m. UTC 
  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(-)

Comments

Jens Axboe Aug. 6, 2023, 12:29 a.m. UTC | #1 
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?
Aleksa Sarai Aug. 6, 2023, 6:42 a.m. UTC | #2 
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.
Jens Axboe Aug. 6, 2023, 4:41 p.m. UTC | #3 
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.
diff mbox series

Patch

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)