Fw:Re:[RFC,v3,1/1] audit: speed up syscall rule filtering

Fw:Re:[RFC,v3,1/1] audit: speed up syscall rule filtering

[yang.yang29]

[-- Attachment #1.1: Type: text/plain, Size: 5555 bytes --]

Let me know if you are going to pursue the suggestion above about
reordering the audit_filter_*() functions as I'll hold off on the
other changes.
----------------------------------------------------
I will follow all your suggestions, do the necessary tests.

And as you said, for reordering the audit_filter_*(), the performance 
would be roughly the same when there is no matching rule.
So I thinks it's another way to  promote performance, different from 
patch "[RFC,v3,1/1] audit: speed up syscall rule filtering".I will try to fix
"wasteful and inelegant " problems.

Great thanks!



------------------Original Mail------------------
Sender: PaulMoore
To: yang yang10192021;
CC: linux-audit@redhat.com;linux-kernel@vger.kernel.org;
Date: 2021/01/28 10:52
Subject: Re:[RFC,v3,1/1] audit: speed up syscall rule filtering
On Sun, Jan 24, 2021 at 8:04 AM <yang.yang29@zte.com.cn> wrote:
>
> From 85b3eccf7f12b091b78cc5ba8abfaf759cf0334e Mon Sep 17 00:00:00 2001
> From: Yang Yang <yang.yang29@zte.com.cn>
> Date: Sun, 24 Jan 2021 20:40:50 +0800
> Subject: [PATCH] audit: speed up syscall rule filtering
> audit_filter_syscall() traverses struct list_head audit_filter_list to find
> out whether current syscall match one rule. This takes o(n), which is not
> necessary, specially for user who add a very few syscall rules. On the other
> hand, user may not much care about rule add/delete speed. So do o(n)
> calculates when rule changes, and ease the burden of audit_filter_syscall().
>
> Define audit_rule_syscall_mask[NR_syscalls], every element stands for
> one syscall.audit_rule_syscall_mask[n] == 0 indicates no rule cares about
> syscall n, so we can avoid unnecessary calling audit_filter_syscall().
> audit_rule_syscall_mask[n] > 0 indicates at least one rule cares about
> syscall n, then calls audit_filter_syscall(). Update
> audit_rule_syscall_mask[n] when syscall rule changes.
>
> Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
> ---
>  include/linux/audit.h |  3 +++
>  kernel/auditfilter.c  |  4 ++++
>  kernel/auditsc.c      | 36 ++++++++++++++++++++++++++++++++----
>  3 files changed, 39 insertions(+), 4 deletions(-)
....
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index ce8c9e2..1b8ff4e 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1627,8 +1653,9 @@ void __audit_free(struct task_struct *tsk)
>                 context->return_valid = AUDITSC_INVALID;
>                 context->return_code = 0;
>
> -               audit_filter_syscall(tsk, context,
> -                                    &audit_filter_list[AUDIT_FILTER_EXIT]);
> +               if (unlikely(audit_rule_syscall_mask[context->major]))
> +                       audit_filter_syscall(tsk, context,
> +                                            &audit_filter_list[AUDIT_FILTER_EXIT]);
>                 audit_filter_inodes(tsk, context);
>                 if (context->current_state == AUDIT_RECORD_CONTEXT)
>                         audit_log_exit();
> @@ -1735,8 +1762,9 @@ void __audit_syscall_exit(int success, long return_code)
>                 else
>                         context->return_code  = return_code;
>
> -               audit_filter_syscall(current, context,
> -                                    &audit_filter_list[AUDIT_FILTER_EXIT]);
> +               if (unlikely(audit_rule_syscall_mask[context->major]))
> +                       audit_filter_syscall(current, context,
> +                                            &audit_filter_list[AUDIT_FILTER_EXIT]);
>                 audit_filter_inodes(current, context);
>                 if (context->current_state == AUDIT_RECORD_CONTEXT)
>                         audit_log_exit();
Looking at this revision I believe I may not have been as clear as I
should have been with my last suggestion.  Let me try to do better
here.
Thus far I'm not very happy with the audit_rule_syscall_mask[]
additions; it looks both wasteful and inelegant to me at the moment.
I would much rather see if we can improve the existing code by fixing
inefficiencies in how we handle the rule filtering.  This is why my
previous comments suggested looking at the audit_filter_syscall() and
audit_filter_inodes() calls in __audit_free() and
__audit_syscall_exit(), the latter of course being more important due
to its frequency.
In both cases an audit_filter_inode() AUDIT_RECORD_CONTEXT decision
takes precedence over any audit_filter_syscall() decision due to the
code being structured as so:
audit_filter_syscall(...);
audit_filter_inodes(...);
if (state == AUDIT_RECORD_CONTEXT)
audit_log_exit();
.... my suggestion is to investigate what performance benefits might be
had by leveraging this precedence, for example:
audit_filter_inodes(...);
if (state != AUDIT_RECORD_CONTEXT)
audit_filter_syscall(...);
if (state == AUDIT_RECORD_CONTEXT)
audit_log_exit();
.... of course I would expect the performance to be roughly the same
when there is no matching rule, but I think there would be a
performance when in those cases where a watched inode triggers an
audit rule.
Beyond that, there is probably work we could do to combine some
aspects of audit_filter_syscall() and audit_filter_inodes() to
eliminate some redundancy, e.g. reduce the number of audit_in_mask()
calls.  Actually looking a bit closer there are a number of
improvements that could likely be made, some might have some
performance impacts.
Let me know if you are going to pursue the suggestion above about
reordering the audit_filter_*() functions as I'll hold off on the
other changes.
--
paul moore
www.paul-moore.com

[-- Attachment #2: Type: text/plain, Size: 102 bytes --]

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit