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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan