* [PATCH] audit: process errors from filter user rules @ 2013-12-05 3:45 Richard Guy Briggs 2013-12-05 20:52 ` Eric Paris 0 siblings, 1 reply; 3+ messages in thread From: Richard Guy Briggs @ 2013-12-05 3:45 UTC (permalink / raw) To: linux-audit, linux-kernel; +Cc: Richard Guy Briggs, Eric Paris, Steve Grubb Errors from filter user rules were previously ignored, and worse, an error on a AUDIT_NEVER rule disabled logging on that rule. On -ESTALE, retry up to 5 times. On error on AUDIT_NEVER rules, log. Signed-off-by: Richard Guy Briggs <rgb@redhat.com> --- kernel/audit.c | 2 +- kernel/auditfilter.c | 44 +++++++++++++++++++++++++++++++------------- 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/kernel/audit.c b/kernel/audit.c index 4cbc945..c93cf06 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -706,7 +706,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) return 0; err = audit_filter_user(msg_type); - if (err == 1) { + if (err) { /* match or error */ err = 0; if (msg_type == AUDIT_USER_TTY) { err = tty_audit_push_current(); diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c index b4c6e03..1a7dfa5 100644 --- a/kernel/auditfilter.c +++ b/kernel/auditfilter.c @@ -1272,8 +1272,8 @@ static int audit_filter_user_rules(struct audit_krule *rule, int type, break; } - if (!result) - return 0; + if (result <= 0) + return result; } switch (rule->action) { case AUDIT_NEVER: *state = AUDIT_DISABLED; break; @@ -1286,19 +1286,37 @@ int audit_filter_user(int type) { enum audit_state state = AUDIT_DISABLED; struct audit_entry *e; - int ret = 1; - - rcu_read_lock(); - list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_USER], list) { - if (audit_filter_user_rules(&e->rule, type, &state)) { - if (state == AUDIT_DISABLED) - ret = 0; - break; + int rc, count = 0, retry = 0, ret = 1; /* Audit by default */ +#define FILTER_RETRY_LIMIT 5 + + do { + rcu_read_lock(); + list_for_each_entry_rcu(e, + &audit_filter_list[AUDIT_FILTER_USER], + list) { + retry = 0; + rc = audit_filter_user_rules(&e->rule, type, &state); + if (rc > 0) { + if (state == AUDIT_DISABLED) + ret = 0; + break; + } else if (rc < 0) { + if (rc == -ESTALE && count < FILTER_RETRY_LIMIT) { + rcu_read_unlock(); + count++; + retry = 1; + cond_resched(); + } else { + ret = rc; + } + break; + } } - } - rcu_read_unlock(); + if (!retry) + rcu_read_unlock(); + } while (retry); - return ret; /* Audit by default */ + return ret; } int audit_filter_type(int type) -- 1.7.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] audit: process errors from filter user rules 2013-12-05 3:45 [PATCH] audit: process errors from filter user rules Richard Guy Briggs @ 2013-12-05 20:52 ` Eric Paris 2013-12-09 20:21 ` Richard Guy Briggs 0 siblings, 1 reply; 3+ messages in thread From: Eric Paris @ 2013-12-05 20:52 UTC (permalink / raw) To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel, Steve Grubb I know we talked about this patch, and it seemed like a good idea at the time, but honestly, these races are so rare, it isn't worth the code complexity. I tried to simplify the readability of your code and got something better, but still the loop is needless... Just log the messages on any error, even with a dontaudit rule... How about a function like: int audit_filter_user(int type) { enum audit_state state = AUDIT_DISABLED; struct audit_entry *e; int rc, ret; ret = 1; /* Audit by default */ rcu_read_lock(); list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_USER], list) { rc = audit_filter_user_rules(&e->rule, type, &state); if (rc > 0 && state == AUDIT_DISABLED) ret = 0; break; } rcu_read_unlock(); return ret; } and use some sense with the 80 character line length rule. It's 81 long. Just let it be long even if checkpatch whines.... -Eric On Wed, 2013-12-04 at 22:45 -0500, Richard Guy Briggs wrote: > Errors from filter user rules were previously ignored, and worse, an error on > a AUDIT_NEVER rule disabled logging on that rule. On -ESTALE, retry up to 5 > times. On error on AUDIT_NEVER rules, log. > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > kernel/audit.c | 2 +- > kernel/auditfilter.c | 44 +++++++++++++++++++++++++++++++------------- > 2 files changed, 32 insertions(+), 14 deletions(-) > > diff --git a/kernel/audit.c b/kernel/audit.c > index 4cbc945..c93cf06 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -706,7 +706,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) > return 0; > > err = audit_filter_user(msg_type); > - if (err == 1) { > + if (err) { /* match or error */ > err = 0; > if (msg_type == AUDIT_USER_TTY) { > err = tty_audit_push_current(); > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > index b4c6e03..1a7dfa5 100644 > --- a/kernel/auditfilter.c > +++ b/kernel/auditfilter.c > @@ -1272,8 +1272,8 @@ static int audit_filter_user_rules(struct audit_krule *rule, int type, > break; > } > > - if (!result) > - return 0; > + if (result <= 0) > + return result; > } > switch (rule->action) { > case AUDIT_NEVER: *state = AUDIT_DISABLED; break; > @@ -1286,19 +1286,37 @@ int audit_filter_user(int type) > { > enum audit_state state = AUDIT_DISABLED; > struct audit_entry *e; > - int ret = 1; > - > - rcu_read_lock(); > - list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_USER], list) { > - if (audit_filter_user_rules(&e->rule, type, &state)) { > - if (state == AUDIT_DISABLED) > - ret = 0; > - break; > + int rc, count = 0, retry = 0, ret = 1; /* Audit by default */ > +#define FILTER_RETRY_LIMIT 5 > + > + do { > + rcu_read_lock(); > + list_for_each_entry_rcu(e, > + &audit_filter_list[AUDIT_FILTER_USER], > + list) { > + retry = 0; > + rc = audit_filter_user_rules(&e->rule, type, &state); > + if (rc > 0) { > + if (state == AUDIT_DISABLED) > + ret = 0; > + break; > + } else if (rc < 0) { > + if (rc == -ESTALE && count < FILTER_RETRY_LIMIT) { > + rcu_read_unlock(); > + count++; > + retry = 1; > + cond_resched(); > + } else { > + ret = rc; > + } > + break; > + } > } > - } > - rcu_read_unlock(); > + if (!retry) > + rcu_read_unlock(); > + } while (retry); > > - return ret; /* Audit by default */ > + return ret; > } > > int audit_filter_type(int type) ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] audit: process errors from filter user rules 2013-12-05 20:52 ` Eric Paris @ 2013-12-09 20:21 ` Richard Guy Briggs 0 siblings, 0 replies; 3+ messages in thread From: Richard Guy Briggs @ 2013-12-09 20:21 UTC (permalink / raw) To: Eric Paris; +Cc: linux-audit, linux-kernel, Steve Grubb On 13/12/05, Eric Paris wrote: > I know we talked about this patch, and it seemed like a good idea at the > time, but honestly, these races are so rare, it isn't worth the code > complexity. I tried to simplify the readability of your code and got > something better, but still the loop is needless... Your original code suggestion was cleaner and simpler than mine, but I tried to code it in a structured manner without gotos, perhaps naively believing structured code to be superior. I think that effort backfired and made it more complex. I'm ok going back to your original ideas. I do still think that retry and passing back of error codes is the better approach. > Just log the messages on any error, even with a dontaudit rule... How > about a function like: I suspect this isn't what you intended. The way this is written, it always breaks after the first rule, not checking any of the others. > int audit_filter_user(int type) > { > enum audit_state state = AUDIT_DISABLED; > struct audit_entry *e; > int rc, ret; > > ret = 1; /* Audit by default */ > > rcu_read_lock(); > list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_USER], list) { > rc = audit_filter_user_rules(&e->rule, type, &state); > if (rc > 0 && state == AUDIT_DISABLED) > ret = 0; > break; > } > rcu_read_unlock(); > return ret; > } Did you mean? list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_USER], list) { rc = audit_filter_user_rules(&e->rule, type, &state); if (rc) { if (rc > 0 && state == AUDIT_DISABLED) ret = 0; break; } } > and use some sense with the 80 character line length rule. It's 81 > long. Just let it be long even if checkpatch whines.... Yeah, I haven't been too pedantic about line length warnings barely over 80 characters when it affects readability... > -Eric > > On Wed, 2013-12-04 at 22:45 -0500, Richard Guy Briggs wrote: > > Errors from filter user rules were previously ignored, and worse, an error on > > a AUDIT_NEVER rule disabled logging on that rule. On -ESTALE, retry up to 5 > > times. On error on AUDIT_NEVER rules, log. > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > kernel/audit.c | 2 +- > > kernel/auditfilter.c | 44 +++++++++++++++++++++++++++++++------------- > > 2 files changed, 32 insertions(+), 14 deletions(-) > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > index 4cbc945..c93cf06 100644 > > --- a/kernel/audit.c > > +++ b/kernel/audit.c > > @@ -706,7 +706,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) > > return 0; > > > > err = audit_filter_user(msg_type); > > - if (err == 1) { > > + if (err) { /* match or error */ > > err = 0; > > if (msg_type == AUDIT_USER_TTY) { > > err = tty_audit_push_current(); > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > > index b4c6e03..1a7dfa5 100644 > > --- a/kernel/auditfilter.c > > +++ b/kernel/auditfilter.c > > @@ -1272,8 +1272,8 @@ static int audit_filter_user_rules(struct audit_krule *rule, int type, > > break; > > } > > > > - if (!result) > > - return 0; > > + if (result <= 0) > > + return result; > > } > > switch (rule->action) { > > case AUDIT_NEVER: *state = AUDIT_DISABLED; break; > > @@ -1286,19 +1286,37 @@ int audit_filter_user(int type) > > { > > enum audit_state state = AUDIT_DISABLED; > > struct audit_entry *e; > > - int ret = 1; > > - > > - rcu_read_lock(); > > - list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_USER], list) { > > - if (audit_filter_user_rules(&e->rule, type, &state)) { > > - if (state == AUDIT_DISABLED) > > - ret = 0; > > - break; > > + int rc, count = 0, retry = 0, ret = 1; /* Audit by default */ > > +#define FILTER_RETRY_LIMIT 5 > > + > > + do { > > + rcu_read_lock(); > > + list_for_each_entry_rcu(e, > > + &audit_filter_list[AUDIT_FILTER_USER], > > + list) { > > + retry = 0; > > + rc = audit_filter_user_rules(&e->rule, type, &state); > > + if (rc > 0) { > > + if (state == AUDIT_DISABLED) > > + ret = 0; > > + break; > > + } else if (rc < 0) { > > + if (rc == -ESTALE && count < FILTER_RETRY_LIMIT) { > > + rcu_read_unlock(); > > + count++; > > + retry = 1; > > + cond_resched(); > > + } else { > > + ret = rc; > > + } > > + break; > > + } > > } > > - } > > - rcu_read_unlock(); > > + if (!retry) > > + rcu_read_unlock(); > > + } while (retry); > > > > - return ret; /* Audit by default */ > > + return ret; > > } > > > > int audit_filter_type(int type) > > - RGB -- Richard Guy Briggs <rbriggs@redhat.com> Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-12-09 20:22 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-12-05 3:45 [PATCH] audit: process errors from filter user rules Richard Guy Briggs 2013-12-05 20:52 ` Eric Paris 2013-12-09 20:21 ` Richard Guy Briggs
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox