Re: [PATCH v4] efivarfs: add nostatfs mount option to skip QueryVariableInfo()

[Prashant Singh <singhpra@juniper.net>]

Thanks Ard and Sebastian for the comments.

>Yeah you can trim it a bit but git commit log space is cheap and
>only people that care about the patch will read it anyway.

Will do.

>As for the docs and code comment changes themselves, please make
>those a bit more to the point. People can go and find the commit
>that introduced/updated them to know more about the background.

Will do.

>>                        used/ available. Disabling it skips the EFI runtime
>>                        service call, which might block the CPU for a few milliseconds,
>
>might block all CPUs for a few milliseconds.
>
>>                        reporting 0 for used and capacity. Enabled by default on
>
>Instead, report 0/0 for used/available.

Will take care of this.

>AFAICT, that would potentially leave KCSAN instrumentation on the reboot
>path, which might trigger and interfere with the reboot. So instead,
>I'd like to put this in efi_reboot_required if we can. If it is needed
>in more places to address an actual KCSAN splat, I don't mind. If it is
>just to make Sashiko happy, then we shouldn't bother.

Could you please clarify what you mean by efi_reboot_required here? nostatfs
is only read in efivarfs_statfs(), efivarfs_show_options() and
efivarfs_init_fs_context(), none of which run on the reboot path, so I'm
not sure how it would apply.

On the annotation itself: an internal review flagged a potential KCSAN
data race rather than an observed splat -- statfs() can run concurrently
with a remount updating the flag, so it is a genuine (benign) concurrent
access. I ran concurrent statfs/remount loops on separate CPUs under
KCSAN and didn't trigger a report in a bounded run, which could be
expected given KCSAN samples accesses, so it doesn't disprove the race.
Since KCSAN only needs one side of the pair marked, data_race() on the
write covers both readers and the reads stay plain. I'm happy to drop it
entirely if you'd prefer to keep the benign race unannotated.

>Please keep this description _here_ where you have it. Once this is
>merged, you could send another patch, extending the documentation with
>the statfs option (I think the workqueue change is in).

Sure -- I'll keep it in Documentation/filesystems/efivarfs.rst for now
and send a follow-up extending Documentation/core-api/real-time/hardware.rst
once this is merged.

>You still have the problem that someone reading the variable leads to
>the same problem but this requires a privileged user. And if I am not
>mistaken, someone sent patches to have efi-runtime runtime disabled/
>enabled.

Agreed -- the variable-read path is the same, but needs a privileged
user unlike the unprivileged statfs()/df trigger.

Thanks,
Prashant