| Message ID | 20230508075812.76077-2-eiichi.tsukata@nutanix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | audit: refactor and fix for potential deadlock | expand |
Hi Eiichi! Just one one for your patch. On 08.05.2023 10:58, Eiichi Tsukata wrote: > Currently audit queue full checks are done in multiple places. > Consolidate them into one audit_queue_full(). > > Signed-off-by: Eiichi Tsukata <eiichi.tsukata@nutanix.com> > --- > kernel/audit.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/kernel/audit.c b/kernel/audit.c > index 9bc0b0301198..c15694e1a76b 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -341,6 +341,12 @@ static inline int audit_rate_check(void) > return retval; > } > > +static inline int audit_queue_full(const struct sk_buff_head *queue) > +{ > + return audit_backlog_limit && > + (skb_queue_len(queue) > audit_backlog_limit); It seems that we should use `>=` here. > +} > + > /** > * audit_log_lost - conditionally log lost audit message event > * @message: the message stating reason for lost audit message > @@ -579,8 +585,7 @@ static void kauditd_hold_skb(struct sk_buff *skb, int error) > * record on the retry queue unless it's full, in which case drop it > */ > if (error == -EAGAIN) { > - if (!audit_backlog_limit || > - skb_queue_len(&audit_retry_queue) < audit_backlog_limit) { > + if (!audit_queue_full(&audit_retry_queue)) { > skb_queue_tail(&audit_retry_queue, skb); > return; > } > @@ -589,8 +594,7 @@ static void kauditd_hold_skb(struct sk_buff *skb, int error) > } > > /* if we have room in the hold queue, queue the message */ > - if (!audit_backlog_limit || > - skb_queue_len(&audit_hold_queue) < audit_backlog_limit) { > + if (!audit_queue_full(&audit_hold_queue)) { > skb_queue_tail(&audit_hold_queue, skb); > return; > } > @@ -613,8 +617,7 @@ static void kauditd_hold_skb(struct sk_buff *skb, int error) > */ > static void kauditd_retry_skb(struct sk_buff *skb, __always_unused int error) > { > - if (!audit_backlog_limit || > - skb_queue_len(&audit_retry_queue) < audit_backlog_limit) { > + if (!audit_queue_full(&audit_retry_queue)) { > skb_queue_tail(&audit_retry_queue, skb); > return; > } > @@ -1564,8 +1567,7 @@ static void audit_receive(struct sk_buff *skb) > audit_ctl_unlock(); > > /* can't block with the ctrl lock, so penalize the sender now */ > - if (audit_backlog_limit && > - (skb_queue_len(&audit_queue) > audit_backlog_limit)) { > + if (audit_queue_full(&audit_queue)) { > DECLARE_WAITQUEUE(wait, current); > > /* wake kauditd to try and flush the queue */ > @@ -1866,8 +1868,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, > if (!(auditd_test_task(current) || audit_ctl_owner_current())) { > long stime = audit_backlog_wait_time; > > - while (audit_backlog_limit && > - (skb_queue_len(&audit_queue) > audit_backlog_limit)) { > + while (audit_queue_full(&audit_queue)) { > /* wake kauditd to try and flush the queue */ > wake_up_interruptible(&kauditd_wait); >
> On May 10, 2023, at 15:54, Rinat Gadelshin <rgadelsh@gmail.com> wrote: > > Hi Eiichi! > > Just one one for your patch. > > On 08.05.2023 10:58, Eiichi Tsukata wrote: >> Currently audit queue full checks are done in multiple places. >> Consolidate them into one audit_queue_full(). >> >> Signed-off-by: Eiichi Tsukata <eiichi.tsukata@nutanix.com> >> --- >> kernel/audit.c | 21 +++++++++++---------- >> 1 file changed, 11 insertions(+), 10 deletions(-) >> >> diff --git a/kernel/audit.c b/kernel/audit.c >> index 9bc0b0301198..c15694e1a76b 100644 >> --- a/kernel/audit.c >> +++ b/kernel/audit.c >> @@ -341,6 +341,12 @@ static inline int audit_rate_check(void) >> return retval; >> } >> +static inline int audit_queue_full(const struct sk_buff_head *queue) >> +{ >> + return audit_backlog_limit && >> + (skb_queue_len(queue) > audit_backlog_limit); > It seems that we should use `>=` here. Hi Rinat Could you provide the detailed reason? Currently queue full checks are done with ‘>’, on the other hand queue NOT full checks are done with ‘<‘. Looking into other similar checks in the kernel, unix_recvq_full() is using ‘>’. Paul, how do you think about it? Eiichi
On 10.05.2023 10:17, Eiichi Tsukata wrote: > >> On May 10, 2023, at 15:54, Rinat Gadelshin <rgadelsh@gmail.com> wrote: >> >> Hi Eiichi! >> >> Just one one for your patch. >> >> On 08.05.2023 10:58, Eiichi Tsukata wrote: >>> Currently audit queue full checks are done in multiple places. >>> Consolidate them into one audit_queue_full(). >>> >>> Signed-off-by: Eiichi Tsukata <eiichi.tsukata@nutanix.com> >>> --- >>> kernel/audit.c | 21 +++++++++++---------- >>> 1 file changed, 11 insertions(+), 10 deletions(-) >>> >>> diff --git a/kernel/audit.c b/kernel/audit.c >>> index 9bc0b0301198..c15694e1a76b 100644 >>> --- a/kernel/audit.c >>> +++ b/kernel/audit.c >>> @@ -341,6 +341,12 @@ static inline int audit_rate_check(void) >>> return retval; >>> } >>> +static inline int audit_queue_full(const struct sk_buff_head *queue) >>> +{ >>> + return audit_backlog_limit && >>> + (skb_queue_len(queue) > audit_backlog_limit); >> It seems that we should use `>=` here. > Hi Rinat > > Could you provide the detailed reason? > > Currently queue full checks are done with ‘>’, > on the other hand queue NOT full checks are done with ‘<‘. > > Looking into other similar checks in the kernel, unix_recvq_full() is using ‘>’. Was (OR statement): `if (!audit_backlog_limit || skb_queue_len(&audit_retry_queue) < audit_backlog_limit) For AND-statement it should be `if (audit_backlog_limit && (skb_queue_len(&audit_retry_queue) >= audit_backlog_limit)) Otherwise we get false for case `(skb_queue_len(&audit_retry_queue) == audit_backlog_limit)` which was true for the old implementation. > > Paul, how do you think about it? > > Eiichi > >
diff --git a/kernel/audit.c b/kernel/audit.c index 9bc0b0301198..c15694e1a76b 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -341,6 +341,12 @@ static inline int audit_rate_check(void) return retval; } +static inline int audit_queue_full(const struct sk_buff_head *queue) +{ + return audit_backlog_limit && + (skb_queue_len(queue) > audit_backlog_limit); +} + /** * audit_log_lost - conditionally log lost audit message event * @message: the message stating reason for lost audit message @@ -579,8 +585,7 @@ static void kauditd_hold_skb(struct sk_buff *skb, int error) * record on the retry queue unless it's full, in which case drop it */ if (error == -EAGAIN) { - if (!audit_backlog_limit || - skb_queue_len(&audit_retry_queue) < audit_backlog_limit) { + if (!audit_queue_full(&audit_retry_queue)) { skb_queue_tail(&audit_retry_queue, skb); return; } @@ -589,8 +594,7 @@ static void kauditd_hold_skb(struct sk_buff *skb, int error) } /* if we have room in the hold queue, queue the message */ - if (!audit_backlog_limit || - skb_queue_len(&audit_hold_queue) < audit_backlog_limit) { + if (!audit_queue_full(&audit_hold_queue)) { skb_queue_tail(&audit_hold_queue, skb); return; } @@ -613,8 +617,7 @@ static void kauditd_hold_skb(struct sk_buff *skb, int error) */ static void kauditd_retry_skb(struct sk_buff *skb, __always_unused int error) { - if (!audit_backlog_limit || - skb_queue_len(&audit_retry_queue) < audit_backlog_limit) { + if (!audit_queue_full(&audit_retry_queue)) { skb_queue_tail(&audit_retry_queue, skb); return; } @@ -1564,8 +1567,7 @@ static void audit_receive(struct sk_buff *skb) audit_ctl_unlock(); /* can't block with the ctrl lock, so penalize the sender now */ - if (audit_backlog_limit && - (skb_queue_len(&audit_queue) > audit_backlog_limit)) { + if (audit_queue_full(&audit_queue)) { DECLARE_WAITQUEUE(wait, current); /* wake kauditd to try and flush the queue */ @@ -1866,8 +1868,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, if (!(auditd_test_task(current) || audit_ctl_owner_current())) { long stime = audit_backlog_wait_time; - while (audit_backlog_limit && - (skb_queue_len(&audit_queue) > audit_backlog_limit)) { + while (audit_queue_full(&audit_queue)) { /* wake kauditd to try and flush the queue */ wake_up_interruptible(&kauditd_wait);
Currently audit queue full checks are done in multiple places. Consolidate them into one audit_queue_full(). Signed-off-by: Eiichi Tsukata <eiichi.tsukata@nutanix.com> --- kernel/audit.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)