Messages in this thread | | | From | Ankur Arora <> | Subject | Re: [PATCH 3/3] audit: unify audit_filter_{uring(),inode_name(),syscall()} | Date | Thu, 29 Sep 2022 13:23:55 -0700 |
| |
Paul Moore <paul@paul-moore.com> writes:
> On Tue, Sep 27, 2022 at 7:00 PM Ankur Arora <ankur.a.arora@oracle.com> wrote: >> >> audit_filter_uring(), audit_filter_inode_name() are substantially similar >> to audit_filter_syscall(). Move the core logic to __audit_filter() which >> can be parametrized for all three. >> >> On a Skylakex system, getpid() latency (all results aggregated >> across 12 boot cycles): >> >> Min Mean Median Max pstdev >> (ns) (ns) (ns) (ns) >> >> - 173.11 182.51 179.65 202.09 (+- 4.34%) >> + 162.11 175.26 173.71 190.56 (+- 4.33%) >> >> Performance counter stats for 'bin/getpid' (3 runs) go from: >> cycles 706.13 ( +- 4.13% ) >> instructions 1654.70 ( +- .06% ) >> IPC 2.35 ( +- 4.25% ) >> branches 430.99 ( +- .06% ) >> branch-misses 0.50 ( +- 2.00% ) >> L1-dcache-loads 440.02 ( +- .07% ) >> L1-dcache-load-misses 5.22 ( +- 82.75% ) >> >> to: >> cycles 678.79 ( +- 4.22% ) >> instructions 1657.79 ( +- .05% ) >> IPC 2.45 ( +- 4.08% ) >> branches 432.00 ( +- .05% ) >> branch-misses 0.38 ( +- 23.68% ) >> L1-dcache-loads 444.96 ( +- .03% ) >> L1-dcache-load-misses 5.13 ( +- 73.09% ) >> >> (Both aggregated over 12 boot cycles.) >> >> Unclear if the improvement is just run-to-run variation or because of >> a slightly denser loop (the list parameter in the list_for_each_entry_rcu() >> exit check now comes from a register rather than a constant as before.) >> >> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com> >> --- >> kernel/auditsc.c | 86 +++++++++++++++++++++++++----------------------- >> 1 file changed, 44 insertions(+), 42 deletions(-) >> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c >> index bf26f47b5226..dd89a52988b0 100644 >> --- a/kernel/auditsc.c >> +++ b/kernel/auditsc.c >> @@ -805,6 +805,41 @@ static bool audit_in_mask(const struct audit_krule *rule, unsigned long val) >> return rule->mask[word] & bit; >> } >> >> +/** >> + * __audit_filter - common filter >> + * > > Please remove the vertical whitespace between the function description > line and the parameter descriptions. > > I'd also suggest renaming this function to "__audit_filter_op(...)" > since we have a few audit filtering functions and a generic > "__audit_filter()" function with no qualification in the name seems > just a bit too generic to not be misleading ... at least I think so. > > I also might try to be just a bit more descriptive in the text above, > for example: > > "__audit_filter_op - filter helper function for operations (syscall/uring/etc.)" > >> + * @tsk: associated task >> + * @ctx: audit context >> + * @list: audit filter list >> + * @op: current syscall/uring_op >> + * @name: name to be filtered (used by audit_filter_inode_name) > > I would change this to "@name: audit_name to use in filtering (can be > NULL)" and leave it at that. > >> + * >> + * return: 1 if we hit a filter, 0 if we don't > > The function header block comment description should be in regular > English sentences. Perhaps something like this: > > "Run the audit filters specified in @list against @tsk using @ctx, > @op, and @name as necessary; the caller is responsible for ensuring > that the call is made while the RCU read lock is held. The @name > parameter can be NULL, but all others must be specified. Returns > 1/true if the filter finds a match, 0/false if none are found." > >> + */ >> +static int __audit_filter(struct task_struct *tsk, >> + struct audit_context *ctx, >> + struct list_head *list, >> + unsigned long op, >> + struct audit_names *name) >> +{ >> + struct audit_entry *e; >> + enum audit_state state; >> + >> + rcu_read_lock(); > > I would move the RCU locking to the callers since not every caller requires it. > >> + list_for_each_entry_rcu(e, list, list) { >> + if (unlikely(audit_in_mask(&e->rule, op))) { > > As discussed in patch 2/3, please remove the unlikely() call.
I'll pull it out of this patch.
And thanks for the comments. Will address and send out a v2.
Ankur
> >> + if (audit_filter_rules(tsk, &e->rule, ctx, name, >> + &state, false)) { >> + rcu_read_unlock(); >> + ctx->current_state = state; >> + return 1; >> + } >> + } >> + } >> + rcu_read_unlock(); >> + return 0; >> +} >> +
| |