On Tue, Dec 19, 2023 at 4:11 AM Alfred Piccioni alpic@google.com wrote:
Thanks for taking the time to review! Apologies for the number of small mistakes.
NP.
Also, IIRC, Paul prefers putting a pair of parentheses after function names to distinguish them, so in the subject line and description it should be security_file_ioctl_compat() and security_file_ioctl(), and you should put a patch version in the [PATCH] prefix e.g. [PATCH v3] to make clear that it is a later version, and usually one doesn't capitalize SELinux or the leading verb in the subject line (just "selinux: introduce").
Changed title to lower-case, prefixed with security, changed slightly to fit in summary with new parentheses. Added [PATCH V3] to the subject.
Patch description still doesn't include the parentheses after each function name but probably not worth re-spinning unless Paul says to do so. I don't see the v3 in the subject line. Seemingly that in combination with the fact that you replied to the original thread confuses the b4 tool (b4.docs.kernel.org) such that b4 mbox/am/shazam ends up selecting the v2 patch instead by default.
Actually, since this spans more than just SELinux, the prefix likely needs to reflect that (e.g. security: introduce ...) and the patch should go to the linux-security-module mailing list too and perhaps linux-fsdevel for the ioctl change.
Added cc 'selinux@vger.kernel.org' and cc 'linux-kernel@vger.kernel.org'. Thanks!
Just FYI, scripts/get_maintainer.pl /path/to/patch will provide an over-approximation of who to include on the distribution for patches based on MAINTAINERS and recent committers. That said, I generally prune the set it provides. More art than science.
I didn't do an audit but does anything need to be updated for the BPF LSM or does it auto-magically pick up new hooks?
I'm unsure. I looked through the BPF LSM and I can't see any way it's picking up the file_ioctl hook to begin with. It appears to me skimming through the code that it automagically picks it up, but I'm not willing to bet the kernel on it.
Do you know who would be a good person to ask about this to make sure?
Looks like it inherited it via the lsm_hook_defs.h. $ nm security/bpf/hooks.o | grep ioctl U bpf_lsm_file_ioctl U bpf_lsm_file_ioctl_compat