On Wed, Mar 7, 2018 at 6:41 PM, Paul Moore <paul(a)paul-moore.com&gt; wrote: > On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina <jikos(a)kernel.org&gt; wrote: >> On Wed, 7 Mar 2018, Andy Lutomirski wrote: >>> Wow, this was a long time ago. >> >> Oh yeah; but it now resurfaced on our side, as we are of course receiving >> a lot of requests with respect to making syscall performance great again >> :) > > Ooof. I'm not sure I can handle making more things "great again" ;) > >>> From memory and a bit of email diving, there are two reasons. >>> >>> 1. The probably was partially solved (by Oleg, IIRC) by making auditctl >>> -a task,never cause newly spawned tasks to not suck. Yes, it's a >>> very partial solution. After considerable nagging, I got Fedora to >>> default to -a task,never. >> >> Hm, right; that's a bit inconvenient, because it takes each and every >> vendor having to realize this option, and put it in. Making kernel do the >> right thing automatically sounds like a better option to me. > > This predates audit falling into my lap, but speaking generally I > think it would be good if the kernel did The Right Thing, so long as > it isn't too painful. > >>> 2. This patch, as is, may be a bit problematic. In particular, if one >>> task changes the audit rules while another task is in the middle of >>> the syscall, then it's too late to audit that syscall correctly. >>> This could be seen as a bug or it could be seen as being just fine. >> >> I don't think this should be a problem, given the fact that the whole >> timing/ordering is not predictable anyway due to scheduling. >> >> Paul, what do you think? > > I'm not overly concerned about the race condition between configuring > the audit filters and syscalls that are currently in-flight; after all > we have that now and "fixing" it would be pretty much impractical > (impossible maybe?). Most serious audit users configure it during > boot and let it run, frequent runtime changes are not common as far as > I can tell.

> I just looked quickly at the patch and decided it isn't something I'm > going to be able to carefully review in the time I've got left today, > so it's going to have to wait until tomorrow and Friday ... however, > speaking on general principle I don't have an objection to the ideas > put forth here.

> Andy, if you've got any Reviewed-by/Tested-by/NACK/etc. you want to > add, that would be good to have. ... and I just realized that linux-audit isn't on the To/CC line, adding them now.

Link to the patch is below. * https://marc.info/?t=152041887600003&r=1&w=2 paul moore

> On Mar 8, 2018, at 1:12 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > >> On 2018-03-07 18:43, Paul Moore wrote: >>> On Wed, Mar 7, 2018 at 6:41 PM, Paul Moore <paul(a)paul-moore.com&gt; wrote: >>>> On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina <jikos(a)kernel.org&gt; wrote: >>>>> On Wed, 7 Mar 2018, Andy Lutomirski wrote: >>>>> Wow, this was a long time ago. >>>> >>>> Oh yeah; but it now resurfaced on our side, as we are of course receiving >>>> a lot of requests with respect to making syscall performance great again >>>> :) >>> >>> Ooof. I'm not sure I can handle making more things "great again" ;) >>> >>>>> From memory and a bit of email diving, there are two reasons. >>>>> >>>>> 1. The probably was partially solved (by Oleg, IIRC) by making auditctl >>>>> -a task,never cause newly spawned tasks to not suck. Yes, it's a >>>>> very partial solution. After considerable nagging, I got Fedora to >>>>> default to -a task,never. >>>> >>>> Hm, right; that's a bit inconvenient, because it takes each and every >>>> vendor having to realize this option, and put it in. Making kernel do the >>>> right thing automatically sounds like a better option to me. >>> >>> This predates audit falling into my lap, but speaking generally I >>> think it would be good if the kernel did The Right Thing, so long as >>> it isn't too painful. >>> >>>>> 2. This patch, as is, may be a bit problematic. In particular, if one >>>>> task changes the audit rules while another task is in the middle of >>>>> the syscall, then it's too late to audit that syscall correctly. >>>>> This could be seen as a bug or it could be seen as being just fine. >>>> >>>> I don't think this should be a problem, given the fact that the whole >>>> timing/ordering is not predictable anyway due to scheduling. >>>> >>>> Paul, what do you think? >>> >>> I'm not overly concerned about the race condition between configuring >>> the audit filters and syscalls that are currently in-flight; after all >>> we have that now and "fixing" it would be pretty much impractical >>> (impossible maybe?). Most serious audit users configure it during >>> boot and let it run, frequent runtime changes are not common as far as >>> I can tell. > > I'd agree the race condition here can't easily be fixed and isn't worth > fixing for the reasons already stated (rules don't change often and may > even be locked once in place relatively early, scheduling uncertainties). > >>> I just looked quickly at the patch and decided it isn't something I'm >>> going to be able to carefully review in the time I've got left today, >>> so it's going to have to wait until tomorrow and Friday ... however, >>> speaking on general principle I don't have an objection to the ideas >>> put forth here. > > The approach seems a bit draconian since it touches all tasks but only > when adding the first rule or deleting the last. > > What we lose is the ability to set or clear individual task auditing and > have it stick to speed up non-audited tasks when there are rules > present, though this isn't currently used, in favour of audit_context > presence. > >>> Andy, if you've got any Reviewed-by/Tested-by/NACK/etc. you want to >>> add, that would be good to have. >> >> ... and I just realized that linux-audit isn't on the To/CC line, >> adding them now. > > (and Andy's non-NACK missed too...) The mailing list *is* in MAINTAINERS. > The mailing list bounces my emails.

>> Link to the patch is below. >> >> * https://marc.info/?t=152041887600003&r=1&w=2 >> >> paul moore > > - RGB

On Wed, 7 Mar 2018 18:43:42 -0500 Paul Moore <paul(a)paul-moore.com&gt; wrote: > ... and I just realized that linux-audit isn't on the To/CC line, > adding them now. > > Link to the patch is below. > > * https://marc.info/?t=152041887600003&r=1&w=2 Yes...I wished I was in on the beginning of this discussion. Here's the problem. We need all tasks auditable unless specifically dismissed as uninteresting. This would be a task,never rule. The way we look at it, is if it boots with audit=1, then we know auditd is expected to run at some point. So, we need all tasks to stay auditable. If they weren't and auditd enabled auditing, then we'd need to walk the whole proctable and stab TIF_AUDIT_SYSCALL into every process in the system. It was decided that this is too ugly.

On Wed, 14 Mar 2018, Andy Lutomirski wrote: > > Yes...I wished I was in on the beginning of this discussion. Here's the > > problem. We need all tasks auditable unless specifically dismissed as > > uninteresting. This would be a task,never rule. > > > > The way we look at it, is if it boots with audit=1, then we know auditd > > is expected to run at some point. So, we need all tasks to stay > > auditable. If they weren't and auditd enabled auditing, then we'd need > > to walk the whole proctable and stab TIF_AUDIT_SYSCALL into every > > process in the system. It was decided that this is too ugly. > > When was that decided? That's what this patch does. I'd like to see some more justification as well. Namely, if I compare "setting TIF_AUDIT_SYSCALL for every process on a need-to-be-so basis" to "we always go through the slow path and pessimistically assume that audit is enabled and has reasonable ruleset loaded", I have my own (different) opinion of what is too ugly.

On Wed, 14 Mar 2018, Andy Lutomirski wrote: > > Yes...I wished I was in on the beginning of this discussion. Here's the > > problem. We need all tasks auditable unless specifically dismissed as > > uninteresting. This would be a task,never rule. > > > > The way we look at it, is if it boots with audit=1, then we know auditd > > is expected to run at some point. So, we need all tasks to stay > > auditable. If they weren't and auditd enabled auditing, then we'd need > > to walk the whole proctable and stab TIF_AUDIT_SYSCALL into every > > process in the system. It was decided that this is too ugly. > > When was that decided? That's what this patch does. I'd like to see some more justification as well.

Namely, if I compare "setting TIF_AUDIT_SYSCALL for every process on a need-to-be-so basis" to "we always go through the slow path and pessimistically assume that audit is enabled and has reasonable ruleset loaded", I have my own (different) opinion of what is too ugly.