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: <20250428132425.318f2a51@kernel.org>
Date: Mon, 28 Apr 2025 13:24:25 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: 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

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 ?

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