On 11/24/22 8:05 AM, Marcos Paulo de Souza wrote:
> On Tue, Jul 12, 2022 at 10:56:11AM -0400, Joe Lawrence wrote:
>> On Thu, Jun 30, 2022 at 11:12:26AM -0300, Marcos Paulo de Souza wrote:
> ...
>> nit: no need to init global to 0
>>
>>> +static int sig_int;
>>> +
>>> +void hup_handler(int signum)
>>> +{
>>> + stop = 1;
>>> +}
>>> +
>>> +void int_handler(int signum)
>>> +{
>>> + stop = 1;
>>> + sig_int = 1;
>>> +}
>>> +
>>> +int main(int argc, char *argv[])
>>> +{
>>> + pid_t orig_pid, pid;
>>> + long count = 0;
>>> +
>>> + signal(SIGHUP, &hup_handler);
>>> + signal(SIGINT, &int_handler);
>>> +
>>> + orig_pid = syscall(SYS_getpid);
>>> +
>>> + while (!stop) {
>>> + pid = syscall(SYS_getpid);
>>> + if (pid != orig_pid)
>>> + return 1;
>>
>> This test doesn't care about the user program return code, but I wonder
>> if the status should be flipped -- this is the desired code path, not
>> the one at the end of main(), right?
>>
> ...
Ah, nevermind ...
>>> + */
>>> + mutex_lock(&kpid_mutex);
>>> + list_for_each_entry_safe(kpid, temp, &klp_pid_list, list) {
>>> + if (current->pid == kpid->pid) {
>>> + list_del(&kpid->list);
>>> + kfree(kpid);
>>> + npids--;
>>> + break;
>>
>> I think it would be safer to return task_tgid_vnr() here, but ...
>>
>>> + }
>>> + }
>>> + mutex_unlock(&kpid_mutex);
>>> +
>>> + return task_tgid_vnr(current);
>>
>> task_pid_vnr() here. That way we're only changing behavior for the
>> processes in the list and not all programs across the system.
>
> I believe that these two suggestions can be linked per your answer. First of
> all, I didn't write the original test program, but I agree that we can make it
> better.
>
> My intent by upstreaming the test was to ensure that test programs
> would keep working even when livepatching getpid while having processes calling
> getpid nonstop. For the purpose of the test, the test module livepatches getpid,
> but keeping the same behavior as before. The only change is to keep track of the
> test programs that need to transition to livepatched state.
>
> Per your comment on the test program it seems that we expected to receive a
> different value from getpid, but it's not the case here. I believe the chec on
> test program is confusing and doesn't bring any benefit, so maybe it's better to
> remove it and keep the test even simpler:
>
Now that I re-read it months later, I see that I mistakenly thought that
the livepatch was supposed to modify the return value. So disregard my
earlier suggestions / questions about that.
> --- a/tools/testing/selftests/livepatch/test_programs/test_klp-call_getpid.c
> +++ b/tools/testing/selftests/livepatch/test_programs/test_klp-call_getpid.c
> @@ -26,18 +26,13 @@ void int_handler(int signum)
>
> int main(int argc, char *argv[])
> {
> - pid_t orig_pid, pid;
> long count = 0;
>
> signal(SIGHUP, &hup_handler);
> signal(SIGINT, &int_handler);
>
> - orig_pid = syscall(SYS_getpid);
> -
> while (!stop) {
> - pid = syscall(SYS_getpid);
> - if (pid != orig_pid)
> - return 1;
> + (void)syscall(SYS_getpid);
> count++;
> }
>
> As as only care about the processes transitioning to the livepatched state, we
> really don't care about the getpid return value (as it should be the same from patches
> and unpatched state).
>
> I believe this resolves both issues. What do you think?
>
Verifying orig_pid against the current pid return value would verify
that it was cleanly patched... though that seems incidental to the
purpose of the test. The simplification above is ok with me either way.
--
Joe
