Messages in this thread | | | Date | Tue, 15 Mar 2022 17:06:55 -0700 | Subject | Re: [PATCH v33 25/29] Audit: Allow multiple records in an audit_buffer | From | Casey Schaufler <> |
| |
On 3/15/2022 4:47 PM, Paul Moore wrote: > On Thu, Mar 10, 2022 at 6:59 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> Replace the single skb pointer in an audit_buffer with >> a list of skb pointers. Add the audit_stamp information >> to the audit_buffer as there's no guarantee that there >> will be an audit_context containing the stamp associated >> with the event. At audit_log_end() time create auxiliary >> records (none are currently defined) as have been added >> to the list. >> >> Suggested-by: Paul Moore <paul@paul-moore.com> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> kernel/audit.c | 53 +++++++++++++++++++++++++++++++++----------------- >> 1 file changed, 35 insertions(+), 18 deletions(-) >> >> diff --git a/kernel/audit.c b/kernel/audit.c >> index f012c3786264..4713e66a12af 100644 >> --- a/kernel/audit.c >> +++ b/kernel/audit.c >> @@ -197,8 +197,10 @@ static struct audit_ctl_mutex { >> * to place it on a transmit queue. Multiple audit_buffers can be in >> * use simultaneously. */ >> struct audit_buffer { >> - struct sk_buff *skb; /* formatted skb ready to send */ >> + struct sk_buff *skb; /* the skb for audit_log functions */ >> + struct sk_buff_head skb_list; /* formatted skbs, ready to send */ >> struct audit_context *ctx; /* NULL or associated context */ >> + struct audit_stamp stamp; /* audit stamp for these records */ >> gfp_t gfp_mask; >> }; >> >> @@ -1744,7 +1746,6 @@ static void audit_buffer_free(struct audit_buffer *ab) >> if (!ab) >> return; >> >> - kfree_skb(ab->skb); > I like the safety in knowing that audit_buffer_free() would free the > ab->skb memory, I'm not sure I want to get rid of that. With the > understanding that ab->skb is always going to be present somewhere in > ab->skb_list, any reason not to do something like this? > > while ((skb = skb_dequeue(&ab->skb_list))) > kfree_skb(skb);
Sure, I'll give this a try. Thanks for the review and suggestions.
> >> kmem_cache_free(audit_buffer_cache, ab); >> } >> >> @@ -1760,11 +1761,15 @@ static struct audit_buffer *audit_buffer_alloc(struct audit_context *ctx, >> ab->skb = nlmsg_new(AUDIT_BUFSIZ, gfp_mask); >> if (!ab->skb) >> goto err; >> - if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0)) >> + if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0)) { >> + kfree_skb(ab->skb); >> goto err; >> + } > Assuming we restore the audit_buffer_free() functionality as mentioned > above, if we move the ab->skb_list init and enqueue calls before we > attempt the nlmsg_put() we can drop the kfree_skb() call and just use > the existing audit_buffer_free() call at the err target. > > >> ab->ctx = ctx; >> ab->gfp_mask = gfp_mask; >> + skb_queue_head_init(&ab->skb_list); >> + skb_queue_tail(&ab->skb_list, ab->skb); >> >> return ab; >> > -- > paul-moore.com
| |