* Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated [not found] ` <CAHC9VhRnM0FKrHvSgf=Pm2BpQSVT2pLWWH8Z7YvG5ExBYb_Ekg@mail.gmail.com> @ 2018-03-07 23:43 ` Paul Moore 2018-03-08 9:12 ` Richard Guy Briggs 2018-03-10 10:15 ` Steve Grubb 2018-03-08 1:06 ` Andy Lutomirski 1 sibling, 2 replies; 11+ messages in thread From: Paul Moore @ 2018-03-07 23:43 UTC (permalink / raw) To: Jiri Kosina, Andy Lutomirski Cc: linux-audit, Andrew Morton, Michal Hocko, Oleg Nesterov, LKML On Wed, Mar 7, 2018 at 6:41 PM, Paul Moore <paul@paul-moore.com> wrote: > On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina <jikos@kernel.org> 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 www.paul-moore.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated 2018-03-07 23:43 ` [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated Paul Moore @ 2018-03-08 9:12 ` Richard Guy Briggs 2018-03-08 14:30 ` Andy Lutomirski 2018-03-10 10:15 ` Steve Grubb 1 sibling, 1 reply; 11+ messages in thread From: Richard Guy Briggs @ 2018-03-08 9:12 UTC (permalink / raw) To: Paul Moore Cc: Jiri Kosina, Andy Lutomirski, linux-audit, Andrew Morton, Michal Hocko, Oleg Nesterov, LKML On 2018-03-07 18:43, Paul Moore wrote: > On Wed, Mar 7, 2018 at 6:41 PM, Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina <jikos@kernel.org> 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. > Link to the patch is below. > > * https://marc.info/?t=152041887600003&r=1&w=2 > > paul moore - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated 2018-03-08 9:12 ` Richard Guy Briggs @ 2018-03-08 14:30 ` Andy Lutomirski 2018-03-08 16:03 ` Richard Guy Briggs 0 siblings, 1 reply; 11+ messages in thread From: Andy Lutomirski @ 2018-03-08 14:30 UTC (permalink / raw) To: Richard Guy Briggs Cc: Paul Moore, Jiri Kosina, Andy Lutomirski, linux-audit, Andrew Morton, Michal Hocko, Oleg Nesterov, LKML > On Mar 8, 2018, at 1:12 AM, Richard Guy Briggs <rgb@redhat.com> wrote: > >> On 2018-03-07 18:43, Paul Moore wrote: >>> On Wed, Mar 7, 2018 at 6:41 PM, Paul Moore <paul@paul-moore.com> wrote: >>>> On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina <jikos@kernel.org> 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 > > -- > Richard Guy Briggs <rgb@redhat.com> > Sr. S/W Engineer, Kernel Security, Base Operating Systems > Remote, Ottawa, Red Hat Canada > IRC: rgb, SunRaycer > Voice: +1.647.777.2635, Internal: (81) 32635 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated 2018-03-08 14:30 ` Andy Lutomirski @ 2018-03-08 16:03 ` Richard Guy Briggs 0 siblings, 0 replies; 11+ messages in thread From: Richard Guy Briggs @ 2018-03-08 16:03 UTC (permalink / raw) To: Andy Lutomirski Cc: Paul Moore, Jiri Kosina, Andy Lutomirski, linux-audit, Andrew Morton, Michal Hocko, Oleg Nesterov, LKML On 2018-03-08 06:30, Andy Lutomirski wrote: > > > > On Mar 8, 2018, at 1:12 AM, Richard Guy Briggs <rgb@redhat.com> wrote: > > > >> On 2018-03-07 18:43, Paul Moore wrote: > >>> On Wed, Mar 7, 2018 at 6:41 PM, Paul Moore <paul@paul-moore.com> wrote: > >>>> On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina <jikos@kernel.org> 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. They'll get approved. > >> Link to the patch is below. > >> > >> * https://marc.info/?t=152041887600003&r=1&w=2 > >> > >> paul moore > > > > - RGB - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated 2018-03-07 23:43 ` [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated Paul Moore 2018-03-08 9:12 ` Richard Guy Briggs @ 2018-03-10 10:15 ` Steve Grubb 2018-03-14 0:22 ` Andy Lutomirski 1 sibling, 1 reply; 11+ messages in thread From: Steve Grubb @ 2018-03-10 10:15 UTC (permalink / raw) To: Paul Moore Cc: Jiri Kosina, Andy Lutomirski, linux-audit, Andrew Morton, Michal Hocko, Oleg Nesterov, LKML On Wed, 7 Mar 2018 18:43:42 -0500 Paul Moore <paul@paul-moore.com> 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. So, we need them all to be auditable if there is any intent to audit. It doesn't matter if there are rules loaded or not. All processes have to stay within reach. What might be acceptable is to add one more state to audit boot variable to indicate that auditing is never expected. We currently have: disabled - which means we'll decide later, enabled, and immutable (no changes allowed). Then have calls to audit_enable or loading rules fail on that flag state so that user space can log that there is a conflict (boot vs daemon) that has to be resolved. As long as we can fail in a discoverable way, I think it would be OK to do something like this. Also, I don't think we want that to be the default state at the moment because the current default is keep all processes auditable. -Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated 2018-03-10 10:15 ` Steve Grubb @ 2018-03-14 0:22 ` Andy Lutomirski 2018-03-14 0:28 ` Jiri Kosina 0 siblings, 1 reply; 11+ messages in thread From: Andy Lutomirski @ 2018-03-14 0:22 UTC (permalink / raw) To: Steve Grubb Cc: Paul Moore, Jiri Kosina, Andy Lutomirski, linux-audit, Andrew Morton, Michal Hocko, Oleg Nesterov, LKML On Sat, Mar 10, 2018 at 10:15 AM, Steve Grubb <sgrubb@redhat.com> wrote: > On Wed, 7 Mar 2018 18:43:42 -0500 > Paul Moore <paul@paul-moore.com> 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. When was that decided? That's what this patch does. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated 2018-03-14 0:22 ` Andy Lutomirski @ 2018-03-14 0:28 ` Jiri Kosina 2018-03-14 0:35 ` Andy Lutomirski 2018-03-19 17:04 ` Steve Grubb 0 siblings, 2 replies; 11+ messages in thread From: Jiri Kosina @ 2018-03-14 0:28 UTC (permalink / raw) To: Andy Lutomirski Cc: Steve Grubb, Paul Moore, linux-audit, Andrew Morton, Michal Hocko, Oleg Nesterov, LKML 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. Thanks, -- Jiri Kosina SUSE Labs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated 2018-03-14 0:28 ` Jiri Kosina @ 2018-03-14 0:35 ` Andy Lutomirski 2018-03-19 17:15 ` Steve Grubb 2018-03-19 17:04 ` Steve Grubb 1 sibling, 1 reply; 11+ messages in thread From: Andy Lutomirski @ 2018-03-14 0:35 UTC (permalink / raw) To: Jiri Kosina Cc: Andy Lutomirski, Steve Grubb, Paul Moore, linux-audit, Andrew Morton, Michal Hocko, Oleg Nesterov, LKML On Wed, Mar 14, 2018 at 12:28 AM, Jiri Kosina <jikos@kernel.org> wrote: > 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. Me too. That being said, on re-review of my old code, I think that audit_dec_n_rules() may be the wrong approach. It may be better to leave TIF_AUDIT_SYSCALL set but to make the audit code itself clear the flag the next time through. That way we don't end up with a partially filled in syscall audit record that never gets consumed if we clear TIF_AUDIT_SYSCALL in the middle of a syscall. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated 2018-03-14 0:35 ` Andy Lutomirski @ 2018-03-19 17:15 ` Steve Grubb 0 siblings, 0 replies; 11+ messages in thread From: Steve Grubb @ 2018-03-19 17:15 UTC (permalink / raw) To: Andy Lutomirski Cc: Michal Hocko, Jiri Kosina, Oleg Nesterov, LKML, linux-audit, Andrew Morton On Tuesday, March 13, 2018 8:35:44 PM EDT Andy Lutomirski wrote: > On Wed, Mar 14, 2018 at 12:28 AM, Jiri Kosina <jikos@kernel.org> wrote: > > 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. > > Me too. > > That being said, on re-review of my old code, I think that > audit_dec_n_rules() may be the wrong approach. It may be better to > leave TIF_AUDIT_SYSCALL set but to make the audit code itself clear > the flag the next time through. That way we don't end up with a > partially filled in syscall audit record that never gets consumed if > we clear TIF_AUDIT_SYSCALL in the middle of a syscall. So what happened when auditd is being restarted due to system update? Its possible to have no auditd, no rules, and audit_enabled == 0. But its getting ready to start a new auditd, enable audit, and load rules. In this case, we expect tasks already dismissed by task filter to stay dismissed because the task filter only runs at fork time. For example, suppose httpd is not desired to be audited. The admin sets up a task rule that sets it to never for httpd. The rule triggers at fork and marks it inauditable. Would this patch cause httpd to suddenly become auditable again during an auditd restart? Rather than play run time games which is ultimately racey and prone to missing something, what about the approach of controlling this from a boot variable and letting user space see an error should auditing try to do its normal thing when its not wanted? This way auditd can exit and there aren't any races. -Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated 2018-03-14 0:28 ` Jiri Kosina 2018-03-14 0:35 ` Andy Lutomirski @ 2018-03-19 17:04 ` Steve Grubb 1 sibling, 0 replies; 11+ messages in thread From: Steve Grubb @ 2018-03-19 17:04 UTC (permalink / raw) To: Jiri Kosina Cc: Andy Lutomirski, Paul Moore, linux-audit, Andrew Morton, Michal Hocko, Oleg Nesterov, LKML On Tuesday, March 13, 2018 8:28:57 PM EDT Jiri Kosina wrote: > 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. There was some discussion about removing the flag here: https://www.redhat.com/archives/linux-audit/2007-October/msg00053.html -Steve > 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated [not found] ` <CAHC9VhRnM0FKrHvSgf=Pm2BpQSVT2pLWWH8Z7YvG5ExBYb_Ekg@mail.gmail.com> 2018-03-07 23:43 ` [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated Paul Moore @ 2018-03-08 1:06 ` Andy Lutomirski 1 sibling, 0 replies; 11+ messages in thread From: Andy Lutomirski @ 2018-03-08 1:06 UTC (permalink / raw) To: Paul Moore, linux-audit Cc: Jiri Kosina, Andy Lutomirski, Oleg Nesterov, Andrew Morton, Michal Hocko, LKML On Wed, Mar 7, 2018 at 11:41 PM, Paul Moore <paul@paul-moore.com> wrote: > On Wed, Mar 7, 2018 at 11:48 AM, Jiri Kosina <jikos@kernel.org> 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. > It's sort of my patch, and I was reasonably happy with it, so definitely no NACK from me. The only caveat I have is that I wrote the original version so long ago that we need to re-audit the code. In particular, I want to make sure that the following two cases don't result in warnings, oopses, or other misbehavior: 1. Do a syscall with TIF_AUDIT_SYSCALL clear. Return with TIF_AUDIT_SYSCALL set and that syscall enabled for auditing. 2.. Do a VFS syscall with TIF_AUDIT_CLEAR and have TIF_AUDIT_SYSCALL set before we execute any VFS code. The VFS code will call into the audit code to log the paths it touches (IIRC). Again, this shouldn't warn or otherwise misbehave. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-03-19 17:15 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <nycvar.YFH.7.76.1803071120410.15778@cbobk.fhfr.pm>
[not found] ` <CALCETrWhZ5ermtpfMt6UdLAC0M9mv7L_TsTfErGStPxznz-AWg@mail.gmail.com>
[not found] ` <nycvar.YFH.7.76.1803071745180.15778@cbobk.fhfr.pm>
[not found] ` <CAHC9VhRnM0FKrHvSgf=Pm2BpQSVT2pLWWH8Z7YvG5ExBYb_Ekg@mail.gmail.com>
2018-03-07 23:43 ` [PATCH] audit: set TIF_AUDIT_SYSCALL only if audit filter has been populated Paul Moore
2018-03-08 9:12 ` Richard Guy Briggs
2018-03-08 14:30 ` Andy Lutomirski
2018-03-08 16:03 ` Richard Guy Briggs
2018-03-10 10:15 ` Steve Grubb
2018-03-14 0:22 ` Andy Lutomirski
2018-03-14 0:28 ` Jiri Kosina
2018-03-14 0:35 ` Andy Lutomirski
2018-03-19 17:15 ` Steve Grubb
2018-03-19 17:04 ` Steve Grubb
2018-03-08 1:06 ` Andy Lutomirski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox