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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Mon, Dec 04, 2023 at 02:39:24PM +0100, Philippe Mathieu-Daudé wrote:
> 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?

Please assign release blocker issues to the 8.2 milestone so that are
tracked:
https://gitlab.com/qemu-project/qemu/-/milestones/10

Thanks,
Stefan

> 
> > 
> >      > ---
> >      >   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
> > 
> 

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