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;
Message-ID: <527bf278-48a3-1b43-a23f-dd4cbb92ef19@gmail.com>
Date:   Wed, 10 May 2023 09:54:39 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH 1/4] audit: refactor queue full checks
To:     Eiichi Tsukata <eiichi.tsukata@nutanix.com>, paul@paul-moore.com,
        eparis@redhat.com, linux-kernel@vger.kernel.org,
        audit@vger.kernel.org
References: <20230508075812.76077-1-eiichi.tsukata@nutanix.com>
 <20230508075812.76077-2-eiichi.tsukata@nutanix.com>
Content-Language: en-US, ru-RU
From:   Rinat Gadelshin <rgadelsh@gmail.com>
In-Reply-To: <20230508075812.76077-2-eiichi.tsukata@nutanix.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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);
>