netdev - Re: [PATCH net-next] selftests: net: exit cleanly on SIGTERM / timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <68102b0477fcc_2609d429482@willemb.c.googlers.com.notmuch>
Date: Mon, 28 Apr 2025 21:27:32 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jakub Kicinski <kuba@...nel.org>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: davem@...emloft.net, 
 netdev@...r.kernel.org, 
 edumazet@...gle.com, 
 pabeni@...hat.com, 
 andrew+netdev@...n.ch, 
 horms@...nel.org, 
 petrm@...dia.com, 
 willemb@...gle.com, 
 sdf@...ichev.me, 
 linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next] selftests: net: exit cleanly on SIGTERM /
 timeout

Reviewed-by: Willem de Bruijn <willemb@...gle.com>

Jakub Kicinski wrote:
> On Sat, 26 Apr 2025 11:15:34 -0400 Willem de Bruijn wrote:
> > > @@ -193,6 +198,19 @@ KSFT_DISRUPTIVE = True
> > >      return env
> > >  
> > >  
> > > +term_cnt = 0
> > > +  
> > 
> > A bit ugly to initialize this here. Also, it already is initialized
> > below.
> 
> We need a global so that the signal handler can access it.
> Python doesn't have syntax to define a variable without a value.
> Or do you suggest term_cnt = None ?

I meant that the "global term_cnt" in ksft_run below already creates
the global var, and is guaranteed to do so before _ksft_intr, so no
need to also define it outside a function.

Obviously not very important, don't mean to ask for a respin. LGTM.

> The whole term_cnt dance is super ugly, couldn't think of a cleaner way.
> It's really annoying that ksft infra sends 2 terminating signals one
> immediately after the other :|
> 
> > > +def _ksft_intr(signum, frame):
> > > +    # ksft runner.sh sends 2 SIGTERMs in a row on a timeout
> > > +    # if we don't ignore the second one it will stop us from handling cleanup
> > > +    global term_cnt
> > > +    term_cnt += 1
> > > +    if term_cnt == 1:
> > > +        raise KsftTerminate()
> > > +    else:
> > > +        ksft_pr(f"Ignoring SIGTERM (cnt: {term_cnt}), already exiting...")
> > > +
> > > +
> > >  def ksft_run(cases=None, globs=None, case_pfx=None, args=()):
> > >      cases = cases or []
> > >  
> > > @@ -205,6 +223,10 @@ KSFT_DISRUPTIVE = True
> > >                      cases.append(value)
> > >                      break
> > >  
> > > +    global term_cnt
> > > +    term_cnt = 0
> > > +    prev_sigterm = signal.signal(signal.SIGTERM, _ksft_intr)
> > > +
> > >      totals = {"pass": 0, "fail": 0, "skip": 0, "xfail": 0}
> > >  
> > >      print("TAP version 13")
> > > @@ -229,11 +251,12 @@ KSFT_DISRUPTIVE = True
> > >              cnt_key = 'xfail'
> > >          except BaseException as e:
> > >              stop |= isinstance(e, KeyboardInterrupt)
> > > +            stop |= isinstance(e, KsftTerminate)
> > >              tb = traceback.format_exc()
> > >              for line in tb.strip().split('\n'):
> > >                  ksft_pr("Exception|", line)
> > >              if stop:
> > > -                ksft_pr("Stopping tests due to KeyboardInterrupt.")
> > > +                ksft_pr(f"Stopping tests due to {type(e).__name__}.")
> > >              KSFT_RESULT = False
> > >              cnt_key = 'fail'
> > >  
> > > @@ -248,6 +271,8 @@ KSFT_DISRUPTIVE = True
> > >          if stop:
> > >              break
> > >  
> > > +    signal.signal(signal.SIGTERM, prev_sigterm)
> > > +  
> > 
> > Why is prev_sigterm saved and reassigned as handler here?
> 
> Because we ignore all signals when cnt > 2 I didn't want to keep our
> handler installed. Just in case something after ksft_run() hangs.
> It should be equivalent to
> 
> 	signal.signal(signal.SIGTERM, signal.SIG_DLF)
> 
> if the prev is of concern. Then again keeping prev doesn't change #LOC

Oh I see. Ok.