Re: [PATCH 2/2] linux-user: Fix openat() emulation to not modify atime

[Philippe Mathieu-Daudé]

Hi Laurent, Helge, Richard,

On 1/12/23 19:51, Shu-Chun Weng wrote:
> On Fri, Dec 1, 2023 at 4:42 AM Philippe Mathieu-Daudé <philmd@linaro.org 
> <mailto:philmd@linaro.org>> wrote:
>
>     Hi Shu-Chun,
>
>     On 1/12/23 04:21, Shu-Chun Weng wrote:
>      > Commit b8002058 strengthened openat()'s /proc detection by calling
>      > realpath(3) on the given path, which allows various paths and
>     symlinks
>      > that points to the /proc file system to be intercepted correctly.
>      >
>      > Using realpath(3), though, has a side effect that it reads the
>     symlinks
>      > along the way, and thus changes their atime. The results in the
>      > following code snippet already get ~now instead of the real atime:
>      >
>      >    int fd = open("/path/to/a/symlink", O_PATH | O_NOFOLLOW);
>      >    struct stat st;
>      >    fstat(fd, st);
>      >    return st.st_atime;
>      >
>      > This change opens a path that doesn't appear to be part of /proc
>      > directly and checks the destination of /proc/self/fd/n to
>     determine if
>      > it actually refers to a file in /proc.
>      >
>      > Neither this nor the existing code works with symlinks or
>     indirect paths
>      > (e.g.  /tmp/../proc/self/exe) that points to /proc/self/exe
>     because it
>      > is itself a symlink, and both realpath(3) and /proc/self/fd/n will
>      > resolve into the location of QEMU.
>
>     Does this fix any of the following issues?
>     https://gitlab.com/qemu-project/qemu/-/issues/829
>     <https://gitlab.com/qemu-project/qemu/-/issues/829>
>
>
> Not this one -- this is purely in the logic of util/path.c, which we do 
> see and carry an internal patch. It's quite a behavior change so we 
> never upstreamed it.
>
>     https://gitlab.com/qemu-project/qemu/-/issues/927
>     <https://gitlab.com/qemu-project/qemu/-/issues/927>
>
>
> No, either. This patch only touches the path handling, not how files are 
> opened.
>
>     https://gitlab.com/qemu-project/qemu/-/issues/2004
>     <https://gitlab.com/qemu-project/qemu/-/issues/2004>
>
>
> Yes! Though I don't have a toolchain for HPPA or any of the 
> architectures intercepting /proc/cpuinfo handy, I hacked the condition 
> and confirmed that on 7.1 and 8.2, test.c as attached in the bug prints 
> out the host cpuinfo while with this patch, it prints out the content 
> generated by `open_cpuinfo()`.
>
>
>
>      > Signed-off-by: Shu-Chun Weng <scw@google.com <mailto:scw@google.com>>
>
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2004 
> <https://gitlab.com/qemu-project/qemu/-/issues/2004>

Do we need to merge this for 8.2?

>      > ---
>      >   linux-user/syscall.c | 42
>     +++++++++++++++++++++++++++++++++---------
>      >   1 file changed, 33 insertions(+), 9 deletions(-)
>
>
> On Fri, Dec 1, 2023 at 9:09 AM Helge Deller <deller@gmx.de 
> <mailto:deller@gmx.de>> wrote:
>
>     On 12/1/23 04:21, Shu-Chun Weng wrote:
>      > Commit b8002058 strengthened openat()'s /proc detection by calling
>      > realpath(3) on the given path, which allows various paths and
>     symlinks
>      > that points to the /proc file system to be intercepted correctly.
>      >
>      > Using realpath(3), though, has a side effect that it reads the
>     symlinks
>      > along the way, and thus changes their atime.
>
>     Ah, ok. I didn't thought of that side effect when I came up with the
>     patch.
>     Does the updated atimes trigger some real case issue ?
>
>
> We have an internal library shimming the underlying filesystem that uses 
> the `open(O_PATH|O_NOFOLLOW)`+`fstat()` pattern for all file stats. 
> Checking symlink atime is in one of the unittests, though I don't know 
> if production ever uses it.
>
>
>     Helge