Re: [XEN RFC for-4.14] Re: use of "stat -"

[Jason Andryuk <jandryuk@gmail.com>]

On Thu, Jun 25, 2020 at 10:08 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 25.06.2020 15:27, Ian Jackson wrote:
> > Jason Andryuk writes ("Re: [XEN RFC for-4.14] Re: use of "stat -""):
> >> I was going to just write a patch to replace - with /dev/stdin and ask
> >> Jan to test it.  When I opened the script, this comment was staring at
> >> me:
> >>         # We can't just stat /dev/stdin or /proc/self/fd/$_lockfd or
> >>         # use bash's test -ef because those all go through what is
> >>         # actually a synthetic symlink in /proc and we aren't
> >>         # guaranteed that our stat(2) won't lose the race with an
> >>         # rm(1) between reading the synthetic link and traversing the
> >>         # file system to find the inum.
> >>
> >> On my system:
> >> $ ls -l /dev/stdin
> >> lrwxrwxrwx 1 root root 15 Jun 24 21:13 /dev/stdin -> /proc/self/fd/0
> >> $ ls -l /proc/self/fd/0 0<lockfile
> >> lrwx------ 1 jason jason 64 Jun 24 21:26 /proc/self/fd/0 -> /home/jason/lockfile
> >>
> >> stat /dev/stdin will work around the lack of `stat -` support, but it
> >> doesn't address the race in the comment.  Is the comment valid?
> >
> > Thanks, but:
> >
> > The tests in my transcript show that the comment (which I wrote) is
> > false.  It shows that stat /dev/stdin works on deleted files, and
> > stats the right file even if the name has been rused.
> >
> >>  How would we prove there is no race for /dev/stdin?
> >
> > It is easy to create the "bad" situation by hand, without racing.
> >
> > The transcript shows that the output from readlink(2) is a fiction and
> > that stat works to stat the actual open-file.
> >
> >> I've mentioned it before, but maybe we should just use the Qubes
> >> patch.  It leaves the lockfile even when no-one is holding the lock,
> >> but it simplifies the code and sidesteps the issues we are discussing
> >> here.
> >> https://github.com/QubesOS/qubes-vmm-xen/blob/xen-4.13/patch-tools-hotplug-drop-perl-usage-in-locking-mechanism.patch
> >
> > I don't like that because this locking code might be reused (or maybe
> > already is used) in contexts with a varying lockfile filename, leaving
> > many lockfiles.  And because having lockfiles lying about might
> > confuse sysadmins who are used to programs which use (the broken)
> > LOCK_EX-style locking paradigm.
> >
> > So tl;dr: yes, we need that patch to replace - with /dev/stdin.
>
> I'm about to test this then, but to be honest I have no idea what
> to do with the comment. I don't think I could properly justify its
> deletion in the description (beyond saying it's not really true),
> nor would I be certain whether to e.g. leave the test -ef part
> there.

Yes, this is what made me pause yesterday.  Also, I'm not sure how
test -ef would be used to check the file & FD.

> Also is there any reason to go through two symlinks then, rather
> than using /proc/self/fd/$_lockfd directly?

I think /proc/self/fd/$_lockfd should just be used to avoid playing
unnecessary FD games.

Regards,
Jason