[v4,0/1] linux-user: add openat2 support in linux-user

Message

Michael Vogt Sept. 6, 2024, 7:39 a.m. UTC

Hi,

This is v4 of the openat2 support in linux-user. Thanks again for the
excellent second round of feedback from Richard Henderson.

The code is identical to the previous v3 and I only fixed two typos in
the commit message. I'm sending v4 because in v3 I forgot to add
"--threaded" when generating the coverletter/patch which makes it a bit
awkward to review and it does not show up properly on
e.g. https://patchew.org/QEMU/. My apologies for this mistake.

This version tries to be closer to the kernels behavior, i.e. now
do_openat2() uses a new copy_struct_from_user() helper that is very
similar to the kernels. This lead me to also drop incuding openat2.h
(as was originally suggested in the v1 review). It now contains it as
a copy named `struct open_how_ver0` and with that we can LOG_UNIMP if
the struct ever grows and qemu-user needs updating.

To answer the question why "maybe_do_fake_open()" uses a
"use_returned_fd" bool instead of just returning "-1": I wanted to be
as close as possible to the previous behavior and maybe_fake_open()
could in theory return "-1" for failures in memfd_create() or
mkstemp() or fake_open->fill(). In those cases the old code in
do_guest_openat() failed and returned the error but the new code would
just see a -1 and continue trying to open a special file that should
have been faked. Maybe I did overthink this as it's very
corner-case-y. Advise is welcome here, happy to change back or
simplify in other ways.

Thanks again,
 Michael

v3 -> v4:
- fix typos in the commit message

v2 -> v3:
- fix coding style (braches)
- improve argument args/naming in do_openat2()
- merge do_openat2/do_guest_openat2
- do size checks first in do_openat2
- add "copy_struct_from_user" and use in "do_openat2()"
- drop using openat2.h and create "struct open_how_v0"
- log if open_how guest struct is bigger than our supported struct

v1 -> v2:
- do not include <sys/syscall.h>
- drop do_guest_openat2 from qemu.h and make static
- drop "safe" from do_guest_openat2
- ensure maybe_do_fake_open() is correct about when the result should
  be used or not
- Extract do_openat2() helper from do_syscall1()
- Call user_unlock* if a lock call fails
- Fix silly incorrect use of "target_open_how" when "open_how" is required
- Fix coding style comments
- Fix validation of arg4 in openat2
- Fix missing zero initialization of open_how
- Define target_open_how with abi_* types
- Warn about unimplemented size if "size" of openat2 is bigger than
  target_open_how


Michael Vogt (1):
  linux-user: add openat2 support in linux-user

 linux-user/syscall.c      | 116 ++++++++++++++++++++++++++++++++++++--
 linux-user/syscall_defs.h |   7 +++
 2 files changed, 119 insertions(+), 4 deletions(-)

Comments

Michael Vogt Sept. 17, 2024, 1:39 p.m. UTC | #1

friendly ping (see also
https://patchew.org/QEMU/[e-mail]/)

Please let me know if there is anything I can do to make this easier to
review or if I should split or help otherwise.

On Fri, Sep 6, 2024 at 9:39 AM Michael Vogt <[e-mail]> wrote:

> Hi,
>
> This is v4 of the openat2 support in linux-user. Thanks again for the
> excellent second round of feedback from Richard Henderson.
>
> The code is identical to the previous v3 and I only fixed two typos in
> the commit message. I'm sending v4 because in v3 I forgot to add
> "--threaded" when generating the coverletter/patch which makes it a bit
> awkward to review and it does not show up properly on
> e.g. https://patchew.org/QEMU/. My apologies for this mistake.
>
> This version tries to be closer to the kernels behavior, i.e. now
> do_openat2() uses a new copy_struct_from_user() helper that is very
> similar to the kernels. This lead me to also drop incuding openat2.h
> (as was originally suggested in the v1 review). It now contains it as
> a copy named `struct open_how_ver0` and with that we can LOG_UNIMP if
> the struct ever grows and qemu-user needs updating.
>
> To answer the question why "maybe_do_fake_open()" uses a
> "use_returned_fd" bool instead of just returning "-1": I wanted to be
> as close as possible to the previous behavior and maybe_fake_open()
> could in theory return "-1" for failures in memfd_create() or
> mkstemp() or fake_open->fill(). In those cases the old code in
> do_guest_openat() failed and returned the error but the new code would
> just see a -1 and continue trying to open a special file that should
> have been faked. Maybe I did overthink this as it's very
> corner-case-y. Advise is welcome here, happy to change back or
> simplify in other ways.
>
> Thanks again,
>  Michael
>
> v3 -> v4:
> - fix typos in the commit message
>
> v2 -> v3:
> - fix coding style (braches)
> - improve argument args/naming in do_openat2()
> - merge do_openat2/do_guest_openat2
> - do size checks first in do_openat2
> - add "copy_struct_from_user" and use in "do_openat2()"
> - drop using openat2.h and create "struct open_how_v0"
> - log if open_how guest struct is bigger than our supported struct
>
> v1 -> v2:
> - do not include <sys/syscall.h>
> - drop do_guest_openat2 from qemu.h and make static
> - drop "safe" from do_guest_openat2
> - ensure maybe_do_fake_open() is correct about when the result should
>   be used or not
> - Extract do_openat2() helper from do_syscall1()
> - Call user_unlock* if a lock call fails
> - Fix silly incorrect use of "target_open_how" when "open_how" is required
> - Fix coding style comments
> - Fix validation of arg4 in openat2
> - Fix missing zero initialization of open_how
> - Define target_open_how with abi_* types
> - Warn about unimplemented size if "size" of openat2 is bigger than
>   target_open_how
>
>
> Michael Vogt (1):
>   linux-user: add openat2 support in linux-user
>
>  linux-user/syscall.c      | 116 ++++++++++++++++++++++++++++++++++++--
>  linux-user/syscall_defs.h |   7 +++
>  2 files changed, 119 insertions(+), 4 deletions(-)
>
> --
> 2.45.2
>
>