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: <04372a53-b101-dd98-1daa-5391d2d9b09f@bytedance.com>
Date:   Tue, 8 Nov 2022 10:50:01 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.4.1
Subject: Re: [PATCH] kretprobe events missing on 2-core KVM guest
Content-Language: en-US
To:     Solar Designer <solar@openwall.com>,
        Masami Hiramatsu <mhiramat@kernel.org>
Cc:     davem@davemloft.net, anil.s.keshavamurthy@intel.com,
        naveen.n.rao@linux.ibm.com, linux-kernel@vger.kernel.org,
        mattwu@163.com, Adam Zabrocki <pi3@pi3.com.pl>
References: <20221025100117.18667-1-wuqiang.matt@bytedance.com>
 <20221026003315.266d59d5c0780c2817be3a0d@kernel.org>
 <20221107133628.GA31649@openwall.com>
From:   wuqiang <wuqiang.matt@bytedance.com>
In-Reply-To: <20221107133628.GA31649@openwall.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2022/11/7 21:36, Solar Designer wrote:
> On Wed, Oct 26, 2022 at 12:33:15AM +0900, Masami Hiramatsu wrote:
>> On Tue, 25 Oct 2022 18:01:17 +0800
>> wuqiang <wuqiang.matt@bytedance.com> wrote:
>>
>>> Default value of maxactive is set as num_possible_cpus() for nonpreemptable
>>> systems. For a 2-core system, only 2 kretprobe instances would be allocated
>>> in default, then these 2 instances for execve kretprobe are very likely to
>>> be used up with a pipelined command.
>>>
>>> This patch increases the minimum of maxactive to 10.
>>
>> This looks reasonable to me.
>>
>> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Reasonable yes, but:
> 
> Is 10 enough?  How exactly do those instances get used up without
> preemption?  Perhaps because execve can sleep?  If so, perhaps we should
> use the same logic without preemption that we do with preemption?  So
> maybe just make this line unconditional? -
> 
> 		rp->maxactive = max_t(unsigned int, 10, 2*num_possible_cpus());

I agree to make it unconditional, though it could cost a bit more memory.

Here's my testcase: a shell script was added to crontab, and the content of
the script is:

   #!/bin/sh
   do_something_with_magic `tr -dc a-z < /dev/urandom | head -c 10`

cron will trigger a series of program executions (4 times every hour). Then
we noticed events loss after 3-4 hours of testings.

The issue is caused by a burst of series of execve requests. The best number
of instances could be different case by case, and should be the user's duty
to decide, but num_possible_cpus() as a default value is inadequate. For my
testcase, 8 is enough as verified, and 10 is chosen to keep it identical.

The handling of execve syscall can be suspended or voluntarily yield up cpu
due to i/o or memory resources and then a new execve gets its time slice to
start. It could be worse for scenarios of resource throttling or routines
that are heavier than execve (though rare as I think).

> Also, the behavior was documented in Documentation/trace/kprobes.rst, so
> perhaps that file should be updated at the same time with the code.

Right, will update in next version.

>>> Signed-off-by: wuqiang <wuqiang.matt@bytedance.com>
>>> ---
>>>   kernel/kprobes.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>>> index 3220b0a2fb4a..b781dee3f552 100644
>>> --- a/kernel/kprobes.c
>>> +++ b/kernel/kprobes.c
>>> @@ -2211,7 +2211,7 @@ int register_kretprobe(struct kretprobe *rp)
>>>   #ifdef CONFIG_PREEMPTION
>>>   		rp->maxactive = max_t(unsigned int, 10, 2*num_possible_cpus());
>>>   #else
>>> -		rp->maxactive = num_possible_cpus();
>>> +		rp->maxactive = max_t(unsigned int, 10, num_possible_cpus());
>>>   #endif
>>>   	}
>>>   #ifdef CONFIG_KRETPROBE_ON_RETHOOK
>>> -- 
>>> 2.34.1
>>>
>>
>>
>> -- 
>> Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Thanks,
> 
> Alexander

Best regards,
wuqiang