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

[Rich Persaud <persaur@xxxxxxxxx>]

On Jun 24, 2020, at 22:39, Jason Andryuk <jandryuk@xxxxxxxxx> wrote:
> 
> On Wed, Jun 24, 2020 at 12:19 PM Ian Jackson <ian.jackson@xxxxxxxxxx> wrote:
>> 
>> Jan Beulich writes ("Re: use of "stat -""):
>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
>>> unless you have verified the sender and know the content is safe.
>>>> On 14.05.2020 13:02, Ian Jackson wrote:
>>>>> I've read this thread.  Jan, I'm sorry that this causes you
>>>>> inconvenience.  I'm hoping it won't come down to a choice between
>>>>> supporting people who want to ship a dom0 without perl, and people who
>>>>> want a dom0 using more-than-a-decade-old coreutils.
>>>>> 
>>>>> Jan, can you tell me what the output is of this on your ancient
>>>>> system:
>>>>> 
>>>>>  $ rm -f t
>>>>>  $ >t
>>>>>  $ exec 3<t
>>>>>  $ stat -L -c '%F %i' /dev/stdin <&3
>>>>>  regular empty file 393549
>>>>>  $ rm t
>>>>>  $ stat -L -c '%F %i' /dev/stdin <&3
>>>>>  regular empty file 393549
>>>>>  $ strace -ou stat -L -c '%F %i' /dev/stdin <&3
>>>>>  $
>>> 
>>> $ rm -f t
>>> $ >t
>>> $ exec 3<t
>>> $ stat -L -c '%F %i' /dev/stdin <&3
>>> regular empty file 3380369
>>> $ rm t
>>> $ stat -L -c '%F %i' /dev/stdin <&3
>>> regular empty file 3380369
>>> $ strace -ou stat -L -c '%F %i' /dev/stdin <&3
>>> regular empty file 3380369
>>> 
>>>> Also, the contents of the file "u" afterwards, please.
>>> 
>>> Attached.
>> 
>> Thanks.
>> 
>> I think this means that `stat -' can be replaced by `stat /dev/stdin'.
>> 
>> This script is only run on Linux where /dev/stdin has existed
>> basically forever.  The strace output shows
>>  stat("/dev/stdin", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
>> and the transcript shows that your stat(1) behaves as I hope.
>> 
>> Jan, will you send a patch ?  It is best if someone else but me writes
>> it and tests it because then I am a "clean" reviewer.
>> 
>> Paul, supposing I review such a patch and say it is low risk, and we
>> commit it by Friday, can it have a release-ack ?
> 
> 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?  How
> would we prove there is no race for /dev/stdin?  And as a refresher,
> `stat -` does an fstat(0), so there is no symlink lookup.  Or is there
> no race since `stat /proc/self/fd/0` isn't a symlink lookup but just
> accessing the already open fd via the proc special file? i.e.
> equivalent to fstat.
> 
> 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

Is there any practical downside to the Qubes patch, which is already deployed 
on production systems?  The complexity of other approaches has been reflected 
in code and prior discussion.  If needed, the comments in the Qubes patch could 
be expanded, when merging that well-tested code into 4.14.

Rich