On Fri, Mar 10, 2023 at 8:47 AM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
> On Thu, Mar 9, 2023 at 4:15 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Thu, Mar 9, 2023 at 3:55 PM Stephen Smalley
> > <stephen.smalley.work@xxxxxxxxx> wrote:
> > > On Thu, Mar 9, 2023 at 3:48 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > > On Thu, Mar 9, 2023 at 2:18 PM Stephen Smalley
> > > > <stephen.smalley.work@xxxxxxxxx> wrote:
> > > > >
> > > > > Linus observed that the pervasive passing of selinux_state pointers
> > > > > introduced by me in commit aa8e712cee93 ("selinux: wrap global selinux
> > > > > state") adds overhead and complexity without providing any
> > > > > benefit. The original idea was to pave the way for SELinux namespaces
> > > > > but those have not yet been implemented and there isn't currently
> > > > > a concrete plan to do so. Remove the passing of the selinux_state
> > > > > pointers, reverting to direct use of the single global selinux_state,
> > > > > and likewise remove passing of child pointers like the selinux_avc.
> > > > > The selinux_policy pointer remains as it is needed for atomic switching
> > > > > of policies.
> > > > >
> > > > > Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > > > > Signed-off-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx>
> > > > > ---
> > > > > security/selinux/avc.c | 197 ++++-----
> > > > > security/selinux/hooks.c | 549 ++++++++++---------------
> > > > > security/selinux/ibpkey.c | 2 +-
> > > > > security/selinux/ima.c | 37 +-
> > > > > security/selinux/include/avc.h | 29 +-
> > > > > security/selinux/include/avc_ss.h | 3 +-
> > > > > security/selinux/include/conditional.h | 4 +-
> > > > > security/selinux/include/ima.h | 10 +-
> > > > > security/selinux/include/security.h | 171 +++-----
> > > > > security/selinux/netif.c | 2 +-
> > > > > security/selinux/netlabel.c | 17 +-
> > > > > security/selinux/netnode.c | 4 +-
> > > > > security/selinux/netport.c | 2 +-
> > > > > security/selinux/selinuxfs.c | 208 ++++------
> > > > > security/selinux/ss/services.c | 346 +++++++---------
> > > > > security/selinux/ss/services.h | 1 -
> > > > > security/selinux/status.c | 44 +-
> > > > > security/selinux/xfrm.c | 20 +-
> > > > > 18 files changed, 651 insertions(+), 995 deletions(-)
> > > >
> > > > It looks like this patch was a bit too big for the mailing list; I'm
> > > > trimming my reply to get this discussion on the list.
> > > >
> > > > I strongly dislike merging patches that haven't hit the list, but I do
> > > > recognize that this is a bit of an unusual case. Have you tried
> > > > breaking this up into two (three?) patches? I imagine that should be
> > > > possible, although I worry that the time required to do that would be
> > > > prohibitive given the change itself.
> > > >
> > > > If that doesn't work, an alternative might be to file a PR against our
> > > > kernel subsystem mirror on GitHub and posting a link to the PR here.
> > > > I don't want to encourage this as a general way of submitting SELinux
> > > > kernel patches, but I could make an exception here.
> > > >
> > > > https://github.com/SELinuxProject/selinux-kernel
> > >
> > > I'm open to suggestions but didn't see an obvious way to split it in a
> > > manner that keeps everything in a working state after each patch.
> > > checkpatch.pl didn't complain about the size - not sure if that is a
> > > change in policy.
> >
> > I'm not sure checkpatch.pl is the right place for that anyway as every
> > list could have a different policy.
> >
> > FWIW, many years ago I was told to keep the diffstat under a thousand
> > lines as a general rule. While I've seen patches go over that limit
> > and hit various @vger lists, I personally try to keep my patches under
> > that 1k limit, and generally that isn't too hard. This really is a
> > bit of a special case I think.
> >
> > > Created a PR here:
> > > https://github.com/SELinuxProject/selinux-kernel/pull/64
> >
> > Thanks, I'll work from that.
>
> For those following along on the mailing list, the kernel test robot
> reported a problem with the original patch when lockdep is enabled, so
> I generated a v2 patch and updated the PR (same link above).
That's interesting. Does that mean that your original patch made it
to the mailing list but somehow didn't make it to the lore archive? I
just (re)checked and there is no record of your patch in the archive
or the patchwork instance.
How did the kernel test robot get the patch to test? Does it know to
go fetch GH PR URLs?
--
paul-moore.com
