Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20220927225944.2254360-1-ankur.a.arora@oracle.com>
 <20220927225944.2254360-4-ankur.a.arora@oracle.com>
 <CAHC9VhSY+ELJ_yX+U+ZzAPOtjJ=eGxtmLTtgpm6NhkYE3Yffuw@mail.gmail.com>
User-agent: mu4e 1.4.10; emacs 27.2
From:   Ankur Arora <ankur.a.arora@oracle.com>
To:     Paul Moore <paul@paul-moore.com>
Cc:     Ankur Arora <ankur.a.arora@oracle.com>, linux-audit@redhat.com,
        eparis@redhat.com, linux-kernel@vger.kernel.org,
        boris.ostrovsky@oracle.com, konrad.wilk@oracle.com
Subject: Re: [PATCH 3/3] audit: unify
 audit_filter_{uring(),inode_name(),syscall()}
In-reply-to: <CAHC9VhSY+ELJ_yX+U+ZzAPOtjJ=eGxtmLTtgpm6NhkYE3Yffuw@mail.gmail.com>
Date:   Thu, 29 Sep 2022 13:23:55 -0700
Message-ID: <87mtaiexpg.fsf@oracle.com>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Y1RbZP0rDyxkOArEdXWvHsWB8Jjdkt25+xarFpuN1gpAJHaImd1IMkFk1VRq?=
 =?us-ascii?Q?Jn10tzB5jczsiDtN69eyPbteQV+8k5WlfPq7TeDdAQkFDvqYCypzAIVMynyJ?=
 =?us-ascii?Q?JKsJADeLbA8VhoLvkhmnNMcchDuB1AyZzUE6RK6uORl13WblTObctGMPtA/f?=
 =?us-ascii?Q?d3JquVAvQCWZSYaTWC7xuUkhYvmRrBMqEDXOgMRM9h0mcPR453yZV3FprNOq?=
 =?us-ascii?Q?UMDeWJ1k/MmWyqMA9FjrYTiR/9OJsyTW2H0oOADq5esI6N6PhwSA1K8UGBS3?=
 =?us-ascii?Q?YXH6ow78qEhpq9TdJYj2dBpPuXKgOKaHosAo/Rc6TnnNoD9hRzVZ35NjF6OH?=
 =?us-ascii?Q?CvpdKMEgnit38pMxkMSXYo40C4V/HD2YLCaJU71g/n1f7jWInav103STE9xu?=
 =?us-ascii?Q?UFdEqolkJyxNL1su4pPJBrtwUo6Sg9unt0qto8jrVoCWMWrRt/MjhgeEFqDo?=
 =?us-ascii?Q?V1shdGW139A9wgdjxHYNc5+38BcGoWMNm2IbzooupfiTH7oKouM3GvZL9A79?=
 =?us-ascii?Q?o42tQhb4I/JBV4kSN2pFN9Y4VswVIDdv+Z6fWEHUaCct7YlZiX0dRYO84o44?=
 =?us-ascii?Q?Tss1otiq1ntWnIPcQvxU+eAbG1UQEkpRRIdrdnwbpA6MK26UXAeTeyHSzIHX?=
 =?us-ascii?Q?CT1QwDRZz3jHFnYqFSQhYM/ve7T+EN6uWMiFnIaUdXBSR50fh1EefZAcHdTP?=
 =?us-ascii?Q?ZvDX5Ct3lMSUVXq8n+PP1WtGqTm1t9c1BN2XMNRs7ri8cHsDGvRH1yCHjwQC?=
 =?us-ascii?Q?qUJ0M9WvG2vmYjlmFShr9YQrug92vLHAdORdZukHdLHfBNYV8TIvsimGnqOB?=
 =?us-ascii?Q?58JZQFI/4kGKDox65sapWSE1EIR+u0tVY9irMVDeFf7dcXBR++n5FgsPGzmK?=
 =?us-ascii?Q?FJ2aB1/X4iFx3lHbvSIXoAajotUUYHnXqW56K6vbNTnXY7oMiWwOWiUUQyuD?=
 =?us-ascii?Q?ZYduabZ2EeCoRKlyAUH/dTeIZWXwCroOYOhY9Sp8AYyUn+Avy+EbwJgdAkBC?=
 =?us-ascii?Q?17zc0gCmwTZgYCW6hHlRNoz1W1ZfPJiOQTDqTeJOne0YLB81d/0mbtOzmU9A?=
 =?us-ascii?Q?ibZQvj0A8OnVqaswMlzkho23c+howHY84qsvQC3LCqGTwaDx45zE7up7mU+N?=
 =?us-ascii?Q?NjummUqqvXl/uM/Yo8333k0/d54MuOV09pylTlB5Oagd6ZZyyJ7nd1YckVbf?=
 =?us-ascii?Q?Hr3D5kqyJ1+SQjI8PZMxxZehCb+VbE0YiV65jsdKVn4PQCEslF555zHOi/wm?=
 =?us-ascii?Q?7Sh2/ka8OvkfhnDpLYGTN3VFX53NvNkQbkQ+O+LbKXG8JKsYD2rgxNMADLYO?=
 =?us-ascii?Q?bVNJOAi2XweQF/v1tU28jRczURE8pJSTPHAM74I1zNPKiMlH7s6ZOrIys+jy?=
 =?us-ascii?Q?E9vWtebceamy3jFunHutRdnKQ41hz2FlqPA2c3Klh5pB+M1RURMMtD8Hq5Vn?=
 =?us-ascii?Q?vkc9Es7knJ+NJVQzRAIi2FFYwLgdndLD/EfzzVy+bQSdlI+uSZWYsot5LO9l?=
 =?us-ascii?Q?mPHqR+befqhLGqGE72yup1rJi/RebftLQtVZNQfwCTWGsC9FthdyZcNYQICO?=
 =?us-ascii?Q?eP8rhy2UUrIG7pz9AEPh1kgopyJuwVErBsoiyFlstLaZfamSfFqz5L1qRSu9?=
 =?us-ascii?Q?zw=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 32616505-efc7-47ce-73c9-08daa258e4d9
X-MS-Exchange-CrossTenant-AuthSource: CO6PR10MB5409.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Sep 2022 20:26:31.0217
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: NdFywi+T6Ac9MZCLirz77CCTXmf6AmK6PWkqCCNNEoR3E0+3I7iZTi7KbhvOHrdAhjE5hZUWFU0Y/IyDTHFgxSIM4wweUub8/kogqcSk5ng=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ1PR10MB5977
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-29_11,2022-09-29_03,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 malwarescore=0
 suspectscore=0 adultscore=0 mlxscore=0 spamscore=0 bulkscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2209130000 definitions=main-2209290128
X-Proofpoint-ORIG-GUID: WuZx8OroHtILJCoU_IA0Hjsq865CuQ0K
X-Proofpoint-GUID: WuZx8OroHtILJCoU_IA0Hjsq865CuQ0K
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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;
>> +}
>> +