Re: [PATCH v2 2/2] selftests: livepatch: Test livepatching a heavily called syscall

[Marcos Paulo de Souza <mpdesouza@suse.de>]

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?
> 
...
> > +	 */
> > +	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:

  --- 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?