stat(2): Zero stat struct before copyout

[obiwac]

Summary

Not all fo_stat implementations set all fields of struct stat before
copying out to userspace (e.g. DMA-BUF file operations in dma_buf_stat),
leading to kernel stack leaks, so zero out the struct before calling
fo_stat.

Test Plan

Stating a DMA-BUF plane previously gave a garbage st_ino value, but now
it gives 0. In the end our DRM implementation should set this inode
"correctly" (not sure what that means yet) because some programs rely
on this, but this patch already helps by not causing DMA-BUFs to (sometimes)
be considered disjointed by e.g. the wlroots Vulkan renderer, on top of
not leaking kernel stack memory of course.

[kib]

stat(2) was micro-optimized to the death. Do not introduce second write to the same memory (not to mention that the next two lines write to some fields third time).

Whatever the DMA-BUF code is, do the bzero() there.

[obiwac]

In D54556#1246415, @kib wrote:

stat(2) was micro-optimized to the death. Do not introduce second write to the same memory (not to mention that the next two lines write to some fields third time).

FWIW I did microbenchmark it and the code generated with the bzero was actually consistently ever so slightly faster on 20 run averages running fstat 10 million times on a tmpfs file. But almost the same. I kept the other two lines because I thought they could have some semantic meaning.

Whatever the DMA-BUF code is, do the bzero() there.

That is a possibility, most other fo_stats seem to follow the bzero before setting fields pattern already. Will patch drm-kmod like this already.

[markj]

That is a possibility, most other fo_stats seem to follow the bzero before setting fields pattern already.

Including vn_statfile(). I think I agree that for now we should patch dma-buf instead, but in the longer term we perhaps should consolidate the zeroing here.

BTW, if you haven't tried it already, the GENERIC-KMSAN kernel should catch bugs like this very quickly. See kmsan(9) for more details. Note that to use it, the kernel and all kernel modules must all be compiled with options KMSAN enabled in the kernel config, and you might run into false positives--it's been a while since I tried booting a laptop with KMSAN configured. But you might find other fun bugs too.

[obiwac]

In D54556#1246429, @markj wrote:

That is a possibility, most other fo_stats seem to follow the bzero before setting fields pattern already.

Including vn_statfile(). I think I agree that for now we should patch dma-buf instead, but in the longer term we perhaps should consolidate the zeroing here.

Got it, I've opened an equivalent PR in our drm-kmod. Abandoning this revision for now.

BTW, if you haven't tried it already, the GENERIC-KMSAN kernel should catch bugs like this very quickly. See kmsan(9) for more details. Note that to use it, the kernel and all kernel modules must all be compiled with options KMSAN enabled in the kernel config, and you might run into false positives--it's been a while since I tried booting a laptop with KMSAN configured. But you might find other fun bugs too.

Thanks for the tip! I knew about KASAN but not KMSAN.

[kib]

In D54556#1246427, @obiwac wrote:

In D54556#1246415, @kib wrote:

stat(2) was micro-optimized to the death. Do not introduce second write to the same memory (not to mention that the next two lines write to some fields third time).

FWIW I did microbenchmark it and the code generated with the bzero was actually consistently ever so slightly faster on 20 run averages running fstat 10 million times on a tmpfs file. But almost the same. I kept the other two lines because I thought they could have some semantic meaning.

I suspect that bzero worked as a prefetch for write on your CPU, putting the cache line into the exclusive ownership mode. You might look at Intel' PREFETCHW. Our sys/kern_prefetch.h is strange starting even with the naming.