On Tue, Jun 27, 2023 at 10:34 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > On Fri, Jun 9, 2023 at 4:24 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > > > Hello all, > > > > Over the past few weeks, as time allowed, I've been working on adding > > some automation to my Linux Kernel workflows. Most of this has been > > scripting various maintainer tasks, e.g. reviewing and merging > > patches, which I expect is not very interesting to most people reading > > this, but I think there is one aspect of these automations that might > > be interesting for a wider audience: basic patch sanity/verification > > tests. These tests are intended to mimic the sanity tests I perform > > when reviewing patch submissions, things like "run checkpatch", "make > > sure it builds without error", "ensure the style/formatting is > > reasonable", etc. For those that are curious, you can see the current > > tests in the repo below, but I will caution you that there are surely > > problems with the scripts, they are still very new and barely tested; > > expect changes. I should also note that I haven't published the > > tool/framework which I use to run these tests just yet, but the tests > > are intended to be standalone so there should still be value as-is. > > My thinking is that by sharing these scripts, and keeping them > > updated, it will help keep the developers and reviewers sync'd as to > > what is expected from a SELinux kernel patch. Much like checkpatch, I > > don't expect these scripts to represent a perfect, ideal standard but > > I think they represent a "good enough" example where the accepted > > verification failures should be relatively few. > > > > * https://github.com/pcmoore/git-verification_scripts > > This looks like a nice step forward. Thanks. My hope is that we can continue to grow/refine as we go along. > Are you also looking into > integrating these checks into patchwork? I noticed that e.g. in the > netdev project patches get tagged with similar sanity checks (netdev) > and even test results (bpf). I just noticed that a few weeks ago too. I took a look at the documentation (there isn't much) and it looks like I should be able to mark/set the "checks" using pwclient; I'll need to do some extra work to filter out the non-kernel patches but that shouldn't be too hard. For a while now I've had hopes to automatically run each posted patchset through a full build/test cycle similar to what happens with kernel-secnext and updates to the XXX/next branches, and getting it working with the sanity tests above is a good first step towards making that happen. > Perhaps you could even reuse some of the tooling/infra they use. As far as tooling/infrastructure is concerned, I've got my own hacky little setup that I started putting together back when I started the kernel-secnext automated builds/testing, I'd just assume use that as it is already in place and works well for my needs. I can post a link to the tooling I use if anyone is interested, but I'm not sure I would recommend anyone use it ;) > > With all of the above in mind, I wanted to get everyone's opinions on > > the style/formatting suggested by the scripts above. As anyone who > > has looked at the SELinux kernel code knows, the style is somewhat > > inconsistent, both with respect to the SELinux subsystem, and the > > kernel as a whole. That unfortunately means that if we want to be > > able to start vetting the style of new code changes, we really need to > > properly (re)format the existing code first. Before I went too far > > with this I wanted to see what it might look like when applied to one > > of our ugliest source files, security/selinux/hooks.c; you can see the > > results in the commit below: > > > > * https://github.com/pcmoore/misc-linux_kernel/commit/3f94fd77b46522a038eb6771b63d0a6d36ca3547 > > > > I'd like to hear what everyone thinks about making a change like this. > > I personally think it is a positive step forward, but I do acknowledge > > that those who have to do backports will likely feel some occasional > > pain. As the backport pain will eventually subside, and the benefits > > of nicer-looking code and improved/shared patch verification will > > continue, I'm leaning towards reformatting the code, but I do *really* > > want to hear what people have to say before we do this. > > Regarding the impact on backports: yes, this will be potentially > *very* painful for us (Red Hat engineering), depending on how it's > done. What would help is if you could split the change into many small > commits - ideally something close to per-function (with some closely > related functions/declarations/definitions maybe combined together). Thanks for the input and the suggestions. I'm not sure I have the time/patience to do this on a per-function level across all of security/selinux, but I understand your point about breaking it up to allow for easier cherry-picking. Let me see what I can do. > From a pure upstream perspective I > definitely support the change, so hopefully we can come to a > compromise such that we are both happy with the result :) :) > As for the proposed style, it looks mostly good to me. One minor > concern I have is that it removes some visually helpful alignment > after '='/':' in selinux_sb_clone_mnt_opts(), selinux_syslog(), > selinux_fs_parameters, and selinux_nf_ops. Not something I would lose > sleep over, but would be nice if it can be somehow preserved. I'm not sure if there is a clang-format config setting for that, but if I'm honest I kinda prefer it without the alignment. If you can find a clang-format config that would preserve the alignment without affecting anything else we can consider it, but I might pull a "maintainer's prerogative" card on that ;) -- paul-moore.com
