diff mbox series

[v8,5/5] security: Add CONFIG_SECURITY_HOOK_LIKELY

Message ID 20231110222038.1450156-6-kpsingh@kernel.org (mailing list archive)
State Not Applicable
Headers show
Series Reduce overhead of LSMs with static calls | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-3 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-llvm-16 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-llvm-16 / build / build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-23 fail Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-llvm-16 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-16 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-16 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-16 / veristat
bpf/vmtest-bpf-next-VM_Test-17 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-16 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-10 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc

Commit Message

KP Singh Nov. 10, 2023, 10:20 p.m. UTC 
This config influences the nature of the static key that guards the
static call for LSM hooks.

When enabled, it indicates that an LSM static call slot is more likely
to be initialized. When disabled, it optimizes for the case when static
call slot is more likely to be not initialized.

When a major LSM like (SELinux, AppArmor, Smack etc) is active on a
system the system would benefit from enabling the config. However there
are other cases which would benefit from the config being disabled
(e.g. a system with a BPF LSM with no hooks enabled by default, or an
LSM like loadpin / yama). Ultimately, there is no one-size fits all
solution.

with CONFIG_SECURITY_HOOK_LIKELY enabled, the inactive /
uninitialized case is penalized with a direct jmp (still better than
an indirect jmp):

function security_file_ioctl:
   0xffffffff818f0c80 <+0>:	endbr64
   0xffffffff818f0c84 <+4>:	nopl   0x0(%rax,%rax,1)
   0xffffffff818f0c89 <+9>:	push   %rbp
   0xffffffff818f0c8a <+10>:	push   %r14
   0xffffffff818f0c8c <+12>:	push   %rbx
   0xffffffff818f0c8d <+13>:	mov    %rdx,%rbx
   0xffffffff818f0c90 <+16>:	mov    %esi,%ebp
   0xffffffff818f0c92 <+18>:	mov    %rdi,%r14
   0xffffffff818f0c95 <+21>:	jmp    0xffffffff818f0ca8 <security_file_ioctl+40>

   jump to skip the inactive BPF LSM hook.

   0xffffffff818f0c97 <+23>:	mov    %r14,%rdi
   0xffffffff818f0c9a <+26>:	mov    %ebp,%esi
   0xffffffff818f0c9c <+28>:	mov    %rbx,%rdx
   0xffffffff818f0c9f <+31>:	call   0xffffffff8141e3b0 <bpf_lsm_file_ioctl>
   0xffffffff818f0ca4 <+36>:	test   %eax,%eax
   0xffffffff818f0ca6 <+38>:	jne    0xffffffff818f0cbf <security_file_ioctl+63>
   0xffffffff818f0ca8 <+40>:	endbr64
   0xffffffff818f0cac <+44>:	jmp    0xffffffff818f0ccd <security_file_ioctl+77>

   jump to skip the empty slot.

   0xffffffff818f0cae <+46>:	mov    %r14,%rdi
   0xffffffff818f0cb1 <+49>:	mov    %ebp,%esi
   0xffffffff818f0cb3 <+51>:	mov    %rbx,%rdx
   0xffffffff818f0cb6 <+54>:	nopl   0x0(%rax,%rax,1)
  				^^^^^^^^^^^^^^^^^^^^^^^
				Empty slot

   0xffffffff818f0cbb <+59>:	test   %eax,%eax
   0xffffffff818f0cbd <+61>:	je     0xffffffff818f0ccd <security_file_ioctl+77>
   0xffffffff818f0cbf <+63>:	endbr64
   0xffffffff818f0cc3 <+67>:	pop    %rbx
   0xffffffff818f0cc4 <+68>:	pop    %r14
   0xffffffff818f0cc6 <+70>:	pop    %rbp
   0xffffffff818f0cc7 <+71>:	cs jmp 0xffffffff82c00000 <__x86_return_thunk>
   0xffffffff818f0ccd <+77>:	endbr64
   0xffffffff818f0cd1 <+81>:	xor    %eax,%eax
   0xffffffff818f0cd3 <+83>:	jmp    0xffffffff818f0cbf <security_file_ioctl+63>
   0xffffffff818f0cd5 <+85>:	mov    %r14,%rdi
   0xffffffff818f0cd8 <+88>:	mov    %ebp,%esi
   0xffffffff818f0cda <+90>:	mov    %rbx,%rdx
   0xffffffff818f0cdd <+93>:	pop    %rbx
   0xffffffff818f0cde <+94>:	pop    %r14
   0xffffffff818f0ce0 <+96>:	pop    %rbp
   0xffffffff818f0ce1 <+97>:	ret

When the config is disabled, the case optimizes the scenario above.

security_file_ioctl:
   0xffffffff818f0e30 <+0>:	endbr64
   0xffffffff818f0e34 <+4>:	nopl   0x0(%rax,%rax,1)
   0xffffffff818f0e39 <+9>:	push   %rbp
   0xffffffff818f0e3a <+10>:	push   %r14
   0xffffffff818f0e3c <+12>:	push   %rbx
   0xffffffff818f0e3d <+13>:	mov    %rdx,%rbx
   0xffffffff818f0e40 <+16>:	mov    %esi,%ebp
   0xffffffff818f0e42 <+18>:	mov    %rdi,%r14
   0xffffffff818f0e45 <+21>:	xchg   %ax,%ax
   0xffffffff818f0e47 <+23>:	xchg   %ax,%ax

   The static keys in their disabled state do not create jumps leading
   to faster code.

   0xffffffff818f0e49 <+25>:	xor    %eax,%eax
   0xffffffff818f0e4b <+27>:	xchg   %ax,%ax
   0xffffffff818f0e4d <+29>:	pop    %rbx
   0xffffffff818f0e4e <+30>:	pop    %r14
   0xffffffff818f0e50 <+32>:	pop    %rbp
   0xffffffff818f0e51 <+33>:	cs jmp 0xffffffff82c00000 <__x86_return_thunk>
   0xffffffff818f0e57 <+39>:	endbr64
   0xffffffff818f0e5b <+43>:	mov    %r14,%rdi
   0xffffffff818f0e5e <+46>:	mov    %ebp,%esi
   0xffffffff818f0e60 <+48>:	mov    %rbx,%rdx
   0xffffffff818f0e63 <+51>:	call   0xffffffff8141e3b0 <bpf_lsm_file_ioctl>
   0xffffffff818f0e68 <+56>:	test   %eax,%eax
   0xffffffff818f0e6a <+58>:	jne    0xffffffff818f0e4d <security_file_ioctl+29>
   0xffffffff818f0e6c <+60>:	jmp    0xffffffff818f0e47 <security_file_ioctl+23>
   0xffffffff818f0e6e <+62>:	endbr64
   0xffffffff818f0e72 <+66>:	mov    %r14,%rdi
   0xffffffff818f0e75 <+69>:	mov    %ebp,%esi
   0xffffffff818f0e77 <+71>:	mov    %rbx,%rdx
   0xffffffff818f0e7a <+74>:	nopl   0x0(%rax,%rax,1)
   0xffffffff818f0e7f <+79>:	test   %eax,%eax
   0xffffffff818f0e81 <+81>:	jne    0xffffffff818f0e4d <security_file_ioctl+29>
   0xffffffff818f0e83 <+83>:	jmp    0xffffffff818f0e49 <security_file_ioctl+25>
   0xffffffff818f0e85 <+85>:	endbr64
   0xffffffff818f0e89 <+89>:	mov    %r14,%rdi
   0xffffffff818f0e8c <+92>:	mov    %ebp,%esi
   0xffffffff818f0e8e <+94>:	mov    %rbx,%rdx
   0xffffffff818f0e91 <+97>:	pop    %rbx
   0xffffffff818f0e92 <+98>:	pop    %r14
   0xffffffff818f0e94 <+100>:	pop    %rbp
   0xffffffff818f0e95 <+101>:	ret

Acked-by: Song Liu <song@kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
 security/Kconfig | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Kees Cook Dec. 8, 2023, 5:36 p.m. UTC | #1 
On Fri, Nov 10, 2023 at 11:20:37PM +0100, KP Singh wrote:
> [...]
> ---
>  security/Kconfig | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Did something go missing from this patch? I don't see anything depending
on CONFIG_SECURITY_HOOK_LIKELY (I think this was working in v7, though?)

Regardless, Paul, please take patches 1-4, they bring us measurable
speed-ups across the board.

-Kees

> 
> diff --git a/security/Kconfig b/security/Kconfig
> index 52c9af08ad35..317018dcbc67 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -32,6 +32,17 @@ config SECURITY
>  
>  	  If you are unsure how to answer this question, answer N.
>  
> +config SECURITY_HOOK_LIKELY
> +	bool "LSM hooks are likely to be initialized"
> +	depends on SECURITY && EXPERT
> +	default SECURITY_SELINUX || SECURITY_SMACK || SECURITY_TOMOYO || SECURITY_APPARMOR
> +	help
> +	  This controls the behaviour of the static keys that guard LSM hooks.
> +	  If LSM hooks are likely to be initialized by LSMs, then one gets
> +	  better performance by enabling this option. However, if the system is
> +	  using an LSM where hooks are much likely to be disabled, one gets
> +	  better performance by disabling this config.
> +
>  config SECURITYFS
>  	bool "Enable the securityfs filesystem"
>  	help
> -- 
> 2.42.0.869.gea05f2083d-goog
>
Paul Moore Dec. 8, 2023, 5:46 p.m. UTC | #2 
On Fri, Dec 8, 2023 at 12:36 PM Kees Cook <keescook@chromium.org> wrote:
> On Fri, Nov 10, 2023 at 11:20:37PM +0100, KP Singh wrote:
> > [...]
> > ---
> >  security/Kconfig | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
>
> Did something go missing from this patch? I don't see anything depending
> on CONFIG_SECURITY_HOOK_LIKELY (I think this was working in v7, though?)
>
> Regardless, Paul, please take patches 1-4, they bring us measurable
> speed-ups across the board.

As I mentioned when you were poking me off-list, this is in my review
queue and I will get to it when it reaches the top.  I can promise you
that continued nudging doesn't move the patchset further up in the
queue, it actually has the opposite effect.
Paul Moore Dec. 8, 2023, 5:55 p.m. UTC | #3 
On Fri, Dec 8, 2023 at 12:46 PM Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Dec 8, 2023 at 12:36 PM Kees Cook <keescook@chromium.org> wrote:
> > On Fri, Nov 10, 2023 at 11:20:37PM +0100, KP Singh wrote:
> > > [...]
> > > ---
> > >  security/Kconfig | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> >
> > Did something go missing from this patch? I don't see anything depending
> > on CONFIG_SECURITY_HOOK_LIKELY (I think this was working in v7, though?)

I guess while I'm at it, and for the sake of the mailing list, it is
worth mentioning that I voiced my dislike of the
CONFIG_SECURITY_HOOK_LIKELY Kconfig option earlier this year yet it
continues to appear in the patchset.  It's hard to give something
priority when I do provide some feedback and it is apparently ignored.

> > Regardless, Paul, please take patches 1-4, they bring us measurable
> > speed-ups across the board.
>
> As I mentioned when you were poking me off-list, this is in my review
> queue and I will get to it when it reaches the top.  I can promise you
> that continued nudging doesn't move the patchset further up in the
> queue, it actually has the opposite effect.
Kees Cook Dec. 8, 2023, 6:22 p.m. UTC | #4 
On Fri, Dec 08, 2023 at 12:55:16PM -0500, Paul Moore wrote:
> On Fri, Dec 8, 2023 at 12:46 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Fri, Dec 8, 2023 at 12:36 PM Kees Cook <keescook@chromium.org> wrote:
> > > On Fri, Nov 10, 2023 at 11:20:37PM +0100, KP Singh wrote:
> > > > [...]
> > > > ---
> > > >  security/Kconfig | 11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > >
> > > Did something go missing from this patch? I don't see anything depending
> > > on CONFIG_SECURITY_HOOK_LIKELY (I think this was working in v7, though?)
> 
> I guess while I'm at it, and for the sake of the mailing list, it is
> worth mentioning that I voiced my dislike of the
> CONFIG_SECURITY_HOOK_LIKELY Kconfig option earlier this year yet it
> continues to appear in the patchset.  It's hard to give something
> priority when I do provide some feedback and it is apparently ignored.

The CONFIG was created specifically to address earlier concerns about
not being able to choose whether to use this performance improvement. :P
What's the right direction forward?
Paul Moore Dec. 8, 2023, 8:51 p.m. UTC | #5 
On Fri, Dec 8, 2023 at 1:22 PM Kees Cook <keescook@chromium.org> wrote:
> On Fri, Dec 08, 2023 at 12:55:16PM -0500, Paul Moore wrote:
> > On Fri, Dec 8, 2023 at 12:46 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Fri, Dec 8, 2023 at 12:36 PM Kees Cook <keescook@chromium.org> wrote:
> > > > On Fri, Nov 10, 2023 at 11:20:37PM +0100, KP Singh wrote:
> > > > > [...]
> > > > > ---
> > > > >  security/Kconfig | 11 +++++++++++
> > > > >  1 file changed, 11 insertions(+)
> > > >
> > > > Did something go missing from this patch? I don't see anything depending
> > > > on CONFIG_SECURITY_HOOK_LIKELY (I think this was working in v7, though?)
> >
> > I guess while I'm at it, and for the sake of the mailing list, it is
> > worth mentioning that I voiced my dislike of the
> > CONFIG_SECURITY_HOOK_LIKELY Kconfig option earlier this year yet it
> > continues to appear in the patchset.  It's hard to give something
> > priority when I do provide some feedback and it is apparently ignored.
>
> The CONFIG was created specifically to address earlier concerns about
> not being able to choose whether to use this performance improvement. :P
> What's the right direction forward?

Are you honestly uncertain after our discussions today?  I'll be
honest and say that I'm a little confused as I thought I made it very
clear when I told you to just be patient off-list, and reminded you in
this thread that the patchset was in my review queue and I will get to
it once it bubbles to the top.  I don't know what else to say here ...
?

As far as the CONFIG_SECURITY_HOOK_LIKELY patch, looking back at my
comments from September [1] there is a clear statement that I am not
in favor of this patch along with a brief explanation as to why:

 "I'm not in favor of adding a Kconfig option for something
  like this.  If you have an extremely well defined use case
  then you can probably do the work to figure out the
  "correct" value for the tunable, but for a general purpose
  kernel build that will have different LSMs active, a
  variety of different BPF LSM hook implementations at
  different times, etc. there is little hope to getting this
  right."

... and that was back when the knob actually did something, as you
pointed out in this thread, the v8 version of this patch doesn't
appear to do anything, which is really baffling and not a good sign.
As far as what to do about this patch, in our off-list discussion I
asked you and KP to refrain from respinning the patchset just to get
rid of this patch, but keep it in mind for future submissions.

Hopefully by repeating the important bits of the conversation you now
understand that there is nothing you can do at this moment to speed my
review of this patchset, but there are things you, and KP, can do in
the future if additional respins are needed.  However, if you are
still confused, it may be best to go do something else for a bit and
then revisit this email because there is nothing more that I can say
on this topic at this point in time.

[1] https://lore.kernel.org/linux-security-module/CAHC9VhSSX0KRuWRURUmt2tUis6fbbmozUbcoeAPkLRmfR2jqAg@mail.gmail.com/
Kees Cook Dec. 8, 2023, 9:13 p.m. UTC | #6 
On Fri, Dec 08, 2023 at 03:51:47PM -0500, Paul Moore wrote:
> Hopefully by repeating the important bits of the conversation you now
> understand that there is nothing you can do at this moment to speed my
> review of this patchset, but there are things you, and KP, can do in
> the future if additional respins are needed.  However, if you are
> still confused, it may be best to go do something else for a bit and
> then revisit this email because there is nothing more that I can say
> on this topic at this point in time.

I moved to the list because off-list discussions (that I got involuntarily
CCed into and never replied to at all) tend to be unhelpful as no one else
can share in any context they may provide. And I'm not trying to rush
you; I'm trying to make review easier. While looking at the v8 again I
saw an obvious problem with it, so I commented on it so that it's clear
to you that it'll need work when you do get around to the review.

As far as the CONFIG topic, I think we're talking past each other (it IS
figuring out the correct value, but it looks like you don't want it even
to be a choice at all), but we can discuss that more when this series
bubbles up your list.
Paul Moore Dec. 8, 2023, 9:43 p.m. UTC | #7 
On Fri, Dec 8, 2023 at 4:13 PM Kees Cook <keescook@chromium.org> wrote:
> On Fri, Dec 08, 2023 at 03:51:47PM -0500, Paul Moore wrote:
> > Hopefully by repeating the important bits of the conversation you now
> > understand that there is nothing you can do at this moment to speed my
> > review of this patchset, but there are things you, and KP, can do in
> > the future if additional respins are needed.  However, if you are
> > still confused, it may be best to go do something else for a bit and
> > then revisit this email because there is nothing more that I can say
> > on this topic at this point in time.
>
> I moved to the list because off-list discussions (that I got involuntarily
> CCed into and never replied to at all) tend to be unhelpful as no one else
> can share in any context they may provide. And I'm not trying to rush
> you; I'm trying to make review easier.

From my perspective whatever good intentions you had at the start were
completely lost when you asked "What's the right direction forward?"
after I had already explained things multiple times *today*.  That's
the sort of thing that drives really bothers me.

> While looking at the v8 again I
> saw an obvious problem with it, so I commented on it so that it's clear
> to you that it'll need work when you do get around to the review.

That's fair.  The Kconfig patch shouldn't have even been part of the
v8 patchset as far as I'm concerned, both because I explained I didn't
want to merge something like that (and was ignored) and because it
doesn't appear to do anything.  From where I sit this was, and
remains, equally parts comical and frustrating.
Kees Cook Dec. 8, 2023, 10:05 p.m. UTC | #8 
On Fri, Dec 08, 2023 at 04:43:57PM -0500, Paul Moore wrote:
> On Fri, Dec 8, 2023 at 4:13 PM Kees Cook <keescook@chromium.org> wrote:
> > On Fri, Dec 08, 2023 at 03:51:47PM -0500, Paul Moore wrote:
> > > Hopefully by repeating the important bits of the conversation you now
> > > understand that there is nothing you can do at this moment to speed my
> > > review of this patchset, but there are things you, and KP, can do in
> > > the future if additional respins are needed.  However, if you are
> > > still confused, it may be best to go do something else for a bit and
> > > then revisit this email because there is nothing more that I can say
> > > on this topic at this point in time.
> >
> > I moved to the list because off-list discussions (that I got involuntarily
> > CCed into and never replied to at all) tend to be unhelpful as no one else
> > can share in any context they may provide. And I'm not trying to rush
> > you; I'm trying to make review easier.
> 
> From my perspective whatever good intentions you had at the start were
> completely lost when you asked "What's the right direction forward?"
> after I had already explained things multiple times *today*.  That's
> the sort of thing that drives really bothers me.

Okay, I understand now. Sorry for frustrating you! By "way forward",
I meant I didn't understand how to address what looked like conflicting
feedback. I think my confusion was over separating the goal ("this
feature should be automatically enabled when it is known to be useful")
from an interpretation of earlier feedback as "I don't want a CONFIG [that
leaves this up to the user]", when what you really wanted understood was
"I don't want a CONFIG *ever*, regardless of whether it picks the correct
setting automatically".

> 
> > While looking at the v8 again I
> > saw an obvious problem with it, so I commented on it so that it's clear
> > to you that it'll need work when you do get around to the review.
> 
> That's fair.  The Kconfig patch shouldn't have even been part of the
> v8 patchset as far as I'm concerned, both because I explained I didn't
> want to merge something like that (and was ignored) and because it
> doesn't appear to do anything.  From where I sit this was, and
> remains, equally parts comical and frustrating.

Agreed. :) Anyway, when you do review it, I think you can just ignore
patch 5, and if a v9 isn't needed, a brand new patch for that logic can
be created later.
KP Singh Dec. 8, 2023, 10:40 p.m. UTC | #9 
On Fri, Dec 8, 2023 at 11:05 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Dec 08, 2023 at 04:43:57PM -0500, Paul Moore wrote:
> > On Fri, Dec 8, 2023 at 4:13 PM Kees Cook <keescook@chromium.org> wrote:
> > > On Fri, Dec 08, 2023 at 03:51:47PM -0500, Paul Moore wrote:
> > > > Hopefully by repeating the important bits of the conversation you now
> > > > understand that there is nothing you can do at this moment to speed my
> > > > review of this patchset, but there are things you, and KP, can do in
> > > > the future if additional respins are needed.  However, if you are
> > > > still confused, it may be best to go do something else for a bit and
> > > > then revisit this email because there is nothing more that I can say
> > > > on this topic at this point in time.
> > >
> > > I moved to the list because off-list discussions (that I got involuntarily
> > > CCed into and never replied to at all) tend to be unhelpful as no one else
> > > can share in any context they may provide. And I'm not trying to rush
> > > you; I'm trying to make review easier.
> >
> > From my perspective whatever good intentions you had at the start were
> > completely lost when you asked "What's the right direction forward?"
> > after I had already explained things multiple times *today*.  That's
> > the sort of thing that drives really bothers me.
>
> Okay, I understand now. Sorry for frustrating you! By "way forward",
> I meant I didn't understand how to address what looked like conflicting
> feedback. I think my confusion was over separating the goal ("this
> feature should be automatically enabled when it is known to be useful")
> from an interpretation of earlier feedback as "I don't want a CONFIG [that
> leaves this up to the user]", when what you really wanted understood was
> "I don't want a CONFIG *ever*, regardless of whether it picks the correct
> setting automatically".
>
> >
> > > While looking at the v8 again I
> > > saw an obvious problem with it, so I commented on it so that it's clear
> > > to you that it'll need work when you do get around to the review.
> >
> > That's fair.  The Kconfig patch shouldn't have even been part of the
> > v8 patchset as far as I'm concerned, both because I explained I didn't
> > want to merge something like that (and was ignored) and because it
> > doesn't appear to do anything.  From where I sit this was, and
> > remains, equally parts comical and frustrating.


Paul, as I said I will include it in v3 and we can drop it if that's
the consensus.

https://lore.kernel.org/bpf/CACYkzJ7KBBJV-CWPkMCqT6rK6yVEOJzhqUjvWzp9BAm-rx3Gsg@mail.gmail.com/

Following that, I received Acks on the patch, so I kept it. I wasn't
sure if this was going to be perceived as "ignoring your feedback".
Definitely not my intention. I was just giving an option for folks who
wanted to test the patch so that we get the defaults right. I am
totally okay with us dropping the config patch.


>
> Agreed. :) Anyway, when you do review it, I think you can just ignore
> patch 5, and if a v9 isn't needed, a brand new patch for that logic can
> be created later.
>
> --
> Kees Cook
Tetsuo Handa Dec. 8, 2023, 10:56 p.m. UTC | #10 
On 2023/12/09 7:05, Kees Cook wrote:
> Okay, I understand now. Sorry for frustrating you! By "way forward",
> I meant I didn't understand how to address what looked like conflicting
> feedback. I think my confusion was over separating the goal ("this
> feature should be automatically enabled when it is known to be useful")
> from an interpretation of earlier feedback as "I don't want a CONFIG [that
> leaves this up to the user]", when what you really wanted understood was
> "I don't want a CONFIG *ever*, regardless of whether it picks the correct
> setting automatically".

Is it possible to change the direction from "call all individual callbacks from security/security.c"
to "call next callback at end of current callback if current callback succeeded and next callback is
defined, and security/security.c calls only the first callback"
( https://lkml.kernel.org/r/38b318a5-0a16-4cc2-878e-4efa632236f3@I-love.SAKURA.ne.jp ),
something like

static int module_specific_some_ops(args) {
	if (logic_for_this_module(arg)) {
		return -EPERM;
	}
	return 0;
}

static int autogenerated_some_ops(args) {
	int ret = module_specific_some_ops(args);
	if (ret == 0) {
		ret = static_call(next_registered_hook)(args);
	}
	return ret;
}

and let LSM_HOOK_INIT() macro generate autogenerated_some_ops() part ?
Paul Moore Dec. 8, 2023, 10:59 p.m. UTC | #11 
On Fri, Dec 8, 2023 at 5:40 PM KP Singh <kpsingh@kernel.org> wrote:
> On Fri, Dec 8, 2023 at 11:05 PM Kees Cook <keescook@chromium.org> wrote:
> > On Fri, Dec 08, 2023 at 04:43:57PM -0500, Paul Moore wrote:
> > > On Fri, Dec 8, 2023 at 4:13 PM Kees Cook <keescook@chromium.org> wrote:
> > > > On Fri, Dec 08, 2023 at 03:51:47PM -0500, Paul Moore wrote:
> > > > > Hopefully by repeating the important bits of the conversation you now
> > > > > understand that there is nothing you can do at this moment to speed my
> > > > > review of this patchset, but there are things you, and KP, can do in
> > > > > the future if additional respins are needed.  However, if you are
> > > > > still confused, it may be best to go do something else for a bit and
> > > > > then revisit this email because there is nothing more that I can say
> > > > > on this topic at this point in time.
> > > >
> > > > I moved to the list because off-list discussions (that I got involuntarily
> > > > CCed into and never replied to at all) tend to be unhelpful as no one else
> > > > can share in any context they may provide. And I'm not trying to rush
> > > > you; I'm trying to make review easier.
> > >
> > > From my perspective whatever good intentions you had at the start were
> > > completely lost when you asked "What's the right direction forward?"
> > > after I had already explained things multiple times *today*.  That's
> > > the sort of thing that drives really bothers me.
> >
> > Okay, I understand now. Sorry for frustrating you! By "way forward",
> > I meant I didn't understand how to address what looked like conflicting
> > feedback. I think my confusion was over separating the goal ("this
> > feature should be automatically enabled when it is known to be useful")
> > from an interpretation of earlier feedback as "I don't want a CONFIG [that
> > leaves this up to the user]", when what you really wanted understood was
> > "I don't want a CONFIG *ever*, regardless of whether it picks the correct
> > setting automatically".
> >
> > >
> > > > While looking at the v8 again I
> > > > saw an obvious problem with it, so I commented on it so that it's clear
> > > > to you that it'll need work when you do get around to the review.
> > >
> > > That's fair.  The Kconfig patch shouldn't have even been part of the
> > > v8 patchset as far as I'm concerned, both because I explained I didn't
> > > want to merge something like that (and was ignored) and because it
> > > doesn't appear to do anything.  From where I sit this was, and
> > > remains, equally parts comical and frustrating.
>
>
> Paul, as I said I will include it in v3 and we can drop it if that's
> the consensus.
>
> https://lore.kernel.org/bpf/CACYkzJ7KBBJV-CWPkMCqT6rK6yVEOJzhqUjvWzp9BAm-rx3Gsg@mail.gmail.com/
>
> Following that, I received Acks on the patch, so I kept it. I wasn't
> sure if this was going to be perceived as "ignoring your feedback".
> Definitely not my intention. I was just giving an option for folks who
> wanted to test the patch so that we get the defaults right. I am
> totally okay with us dropping the config patch.

<heavy sarcasm>I'm glad you're okay with dropping a patch I said I
wasn't going to merge three months ago.  I'm also glad you're okay
with dropping a patch that does absolutely nothing.</heavy sarcasm>

Come on KP, you're better than this.  Continuing to carry a patch that
I've said I'm not going to merge only creates confusion about what
will be accepted/supported (see today's exchange as a perfect
example).  There is no need to keep the patch going "for reference",
to record ACKs, or anything similar to that; all the reviews, ACKs,
etc. happened on a public list so we have that covered from a
historical perspective.

I suppose there is a worthy offshoot discussion about consensus and
maintainer discretion, but I'm too tired and annoyed to give that
discussion the attention it deserves, so let's just say that when I
say stuff like I did back in the v2 patchset that should be taken as a
"regardless of what consensus there may be, I'm not going to merge
this patch."
KP Singh Dec. 8, 2023, 11:06 p.m. UTC | #12 
On Fri, Dec 8, 2023 at 11:59 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Dec 8, 2023 at 5:40 PM KP Singh <kpsingh@kernel.org> wrote:
> > On Fri, Dec 8, 2023 at 11:05 PM Kees Cook <keescook@chromium.org> wrote:
> > > On Fri, Dec 08, 2023 at 04:43:57PM -0500, Paul Moore wrote:
> > > > On Fri, Dec 8, 2023 at 4:13 PM Kees Cook <keescook@chromium.org> wrote:
> > > > > On Fri, Dec 08, 2023 at 03:51:47PM -0500, Paul Moore wrote:
> > > > > > Hopefully by repeating the important bits of the conversation you now
> > > > > > understand that there is nothing you can do at this moment to speed my
> > > > > > review of this patchset, but there are things you, and KP, can do in
> > > > > > the future if additional respins are needed.  However, if you are
> > > > > > still confused, it may be best to go do something else for a bit and
> > > > > > then revisit this email because there is nothing more that I can say
> > > > > > on this topic at this point in time.
> > > > >
> > > > > I moved to the list because off-list discussions (that I got involuntarily
> > > > > CCed into and never replied to at all) tend to be unhelpful as no one else
> > > > > can share in any context they may provide. And I'm not trying to rush
> > > > > you; I'm trying to make review easier.
> > > >
> > > > From my perspective whatever good intentions you had at the start were
> > > > completely lost when you asked "What's the right direction forward?"
> > > > after I had already explained things multiple times *today*.  That's
> > > > the sort of thing that drives really bothers me.
> > >
> > > Okay, I understand now. Sorry for frustrating you! By "way forward",
> > > I meant I didn't understand how to address what looked like conflicting
> > > feedback. I think my confusion was over separating the goal ("this
> > > feature should be automatically enabled when it is known to be useful")
> > > from an interpretation of earlier feedback as "I don't want a CONFIG [that
> > > leaves this up to the user]", when what you really wanted understood was
> > > "I don't want a CONFIG *ever*, regardless of whether it picks the correct
> > > setting automatically".
> > >
> > > >
> > > > > While looking at the v8 again I
> > > > > saw an obvious problem with it, so I commented on it so that it's clear
> > > > > to you that it'll need work when you do get around to the review.
> > > >
> > > > That's fair.  The Kconfig patch shouldn't have even been part of the
> > > > v8 patchset as far as I'm concerned, both because I explained I didn't
> > > > want to merge something like that (and was ignored) and because it
> > > > doesn't appear to do anything.  From where I sit this was, and
> > > > remains, equally parts comical and frustrating.
> >
> >
> > Paul, as I said I will include it in v3 and we can drop it if that's
> > the consensus.
> >
> > https://lore.kernel.org/bpf/CACYkzJ7KBBJV-CWPkMCqT6rK6yVEOJzhqUjvWzp9BAm-rx3Gsg@mail.gmail.com/
> >
> > Following that, I received Acks on the patch, so I kept it. I wasn't
> > sure if this was going to be perceived as "ignoring your feedback".
> > Definitely not my intention. I was just giving an option for folks who
> > wanted to test the patch so that we get the defaults right. I am
> > totally okay with us dropping the config patch.
>
> <heavy sarcasm>I'm glad you're okay with dropping a patch I said I
> wasn't going to merge three months ago.  I'm also glad you're okay
> with dropping a patch that does absolutely nothing.</heavy sarcasm>

The patch does something (it's in the patch description). But it's
something that you don't think is worth tweaking and that's fine.

>
> Come on KP, you're better than this.  Continuing to carry a patch that
> I've said I'm not going to merge only creates confusion about what
> will be accepted/supported (see today's exchange as a perfect
> example).  There is no need to keep the patch going "for reference",

Okay, I won't. I honestly did not think this was that big a deal and
would bother you so much and, please do assume good intent, I did not
want to create confusion here.

- KP

> to record ACKs, or anything similar to that; all the reviews, ACKs,
> etc. happened on a public list so we have that covered from a
> historical perspective.
>
> I suppose there is a worthy offshoot discussion about consensus and
> maintainer discretion, but I'm too tired and annoyed to give that
> discussion the attention it deserves, so let's just say that when I
> say stuff like I did back in the v2 patchset that should be taken as a
> "regardless of what consensus there may be, I'm not going to merge
> this patch."
>
> --
> paul-moore.com
Paul Moore Dec. 8, 2023, 11:27 p.m. UTC | #13 
On Fri, Dec 8, 2023 at 6:06 PM KP Singh <kpsingh@kernel.org> wrote:
> On Fri, Dec 8, 2023 at 11:59 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Fri, Dec 8, 2023 at 5:40 PM KP Singh <kpsingh@kernel.org> wrote:
> > > On Fri, Dec 8, 2023 at 11:05 PM Kees Cook <keescook@chromium.org> wrote:
> > > > On Fri, Dec 08, 2023 at 04:43:57PM -0500, Paul Moore wrote:
> > > > > On Fri, Dec 8, 2023 at 4:13 PM Kees Cook <keescook@chromium.org> wrote:
> > > > > > On Fri, Dec 08, 2023 at 03:51:47PM -0500, Paul Moore wrote:
> > > > > > > Hopefully by repeating the important bits of the conversation you now
> > > > > > > understand that there is nothing you can do at this moment to speed my
> > > > > > > review of this patchset, but there are things you, and KP, can do in
> > > > > > > the future if additional respins are needed.  However, if you are
> > > > > > > still confused, it may be best to go do something else for a bit and
> > > > > > > then revisit this email because there is nothing more that I can say
> > > > > > > on this topic at this point in time.
> > > > > >
> > > > > > I moved to the list because off-list discussions (that I got involuntarily
> > > > > > CCed into and never replied to at all) tend to be unhelpful as no one else
> > > > > > can share in any context they may provide. And I'm not trying to rush
> > > > > > you; I'm trying to make review easier.
> > > > >
> > > > > From my perspective whatever good intentions you had at the start were
> > > > > completely lost when you asked "What's the right direction forward?"
> > > > > after I had already explained things multiple times *today*.  That's
> > > > > the sort of thing that drives really bothers me.
> > > >
> > > > Okay, I understand now. Sorry for frustrating you! By "way forward",
> > > > I meant I didn't understand how to address what looked like conflicting
> > > > feedback. I think my confusion was over separating the goal ("this
> > > > feature should be automatically enabled when it is known to be useful")
> > > > from an interpretation of earlier feedback as "I don't want a CONFIG [that
> > > > leaves this up to the user]", when what you really wanted understood was
> > > > "I don't want a CONFIG *ever*, regardless of whether it picks the correct
> > > > setting automatically".
> > > >
> > > > >
> > > > > > While looking at the v8 again I
> > > > > > saw an obvious problem with it, so I commented on it so that it's clear
> > > > > > to you that it'll need work when you do get around to the review.
> > > > >
> > > > > That's fair.  The Kconfig patch shouldn't have even been part of the
> > > > > v8 patchset as far as I'm concerned, both because I explained I didn't
> > > > > want to merge something like that (and was ignored) and because it
> > > > > doesn't appear to do anything.  From where I sit this was, and
> > > > > remains, equally parts comical and frustrating.
> > >
> > >
> > > Paul, as I said I will include it in v3 and we can drop it if that's
> > > the consensus.
> > >
> > > https://lore.kernel.org/bpf/CACYkzJ7KBBJV-CWPkMCqT6rK6yVEOJzhqUjvWzp9BAm-rx3Gsg@mail.gmail.com/
> > >
> > > Following that, I received Acks on the patch, so I kept it. I wasn't
> > > sure if this was going to be perceived as "ignoring your feedback".
> > > Definitely not my intention. I was just giving an option for folks who
> > > wanted to test the patch so that we get the defaults right. I am
> > > totally okay with us dropping the config patch.
> >
> > <heavy sarcasm>I'm glad you're okay with dropping a patch I said I
> > wasn't going to merge three months ago.  I'm also glad you're okay
> > with dropping a patch that does absolutely nothing.</heavy sarcasm>
>
> The patch does something (it's in the patch description). But it's
> something that you don't think is worth tweaking and that's fine.
>
> > Come on KP, you're better than this.  Continuing to carry a patch that
> > I've said I'm not going to merge only creates confusion about what
> > will be accepted/supported (see today's exchange as a perfect
> > example).  There is no need to keep the patch going "for reference",
>
> Okay, I won't. I honestly did not think this was that big a deal and
> would bother you so much and, please do assume good intent, I did not
> want to create confusion here.

The patch itself isn't the problem, it's at the end of the patchset
and easily dropped.  The problem is that you pinged me off-list to try
and get me to move your patchset up the review queue, which isn't
something I appreciate, especially when the feedback I provided
previously was not acted upon ... and yes, I've heard your arguments
about why you continued to carry the patch, but please understand when
I explicitly say "no thank you" to a patch, I want you to drop that
patch; "no thank you" shouldn't be ambiguous.

To summarize:
1. Don't ping me off-list to review patchsets.  I personally find that
incredibly annoying and it guarantees that the patchset gets pushed to
the end of the list (spiteful? sure, but it helps soothe the jagged
nerves of this haggard maintainer).
2. Take maintainer feedback seriously.  If you ignore feedback,
providing a proper review tends to be a waste of time; there are
plenty of other patchsets with authors who are receptive to comments.

Anyway, go enjoy your weekend and just do better next time.  What's
done is done.
diff mbox series

Patch

diff --git a/security/Kconfig b/security/Kconfig
index 52c9af08ad35..317018dcbc67 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -32,6 +32,17 @@  config SECURITY
 
 	  If you are unsure how to answer this question, answer N.
 
+config SECURITY_HOOK_LIKELY
+	bool "LSM hooks are likely to be initialized"
+	depends on SECURITY && EXPERT
+	default SECURITY_SELINUX || SECURITY_SMACK || SECURITY_TOMOYO || SECURITY_APPARMOR
+	help
+	  This controls the behaviour of the static keys that guard LSM hooks.
+	  If LSM hooks are likely to be initialized by LSMs, then one gets
+	  better performance by enabling this option. However, if the system is
+	  using an LSM where hooks are much likely to be disabled, one gets
+	  better performance by disabling this config.
+
 config SECURITYFS
 	bool "Enable the securityfs filesystem"
 	help