linux-kernel - Re: [PATCH 06/24] perf daemon: Add config file support

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]

Date:   Thu, 11 Feb 2021 17:45:38 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     Jiri Olsa <jolsa@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Ingo Molnar <mingo@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Michael Petlan <mpetlan@...hat.com>,
        Ian Rogers <irogers@...gle.com>,
        Alexei Budankov <abudankov@...wei.com>
Subject: Re: [PATCH 06/24] perf daemon: Add config file support

On Thu, Feb 11, 2021 at 03:01:12PM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Tue, Feb 9, 2021 at 5:09 AM Jiri Olsa <jolsa@...nel.org> wrote:
> > +static int daemon__reconfig(struct daemon *daemon)
> > +{
> > +       struct daemon_session *session, *n;
> > +
> > +       list_for_each_entry_safe(session, n, &daemon->sessions, list) {
> > +               /* No change. */
> > +               if (session->state == OK)
> > +                       continue;
> > +
> > +               /* Remove session. */
> > +               if (session->state == KILL) {
> > +                       if (session->pid > 0) {
> > +                               daemon_session__kill(session);
> > +                               pr_info("reconfig: session '%s' killed\n", session->name);
> > +                       }
> > +                       daemon_session__remove(session);
> > +                       continue;
> > +               }
> > +
> > +               /* Reconfig session. */
> > +               if (session->pid > 0) {
> > +                       daemon_session__kill(session);
> > +                       pr_info("reconfig: session '%s' killed\n", session->name);
> > +               }
> > +               if (daemon_session__run(session, daemon))
> > +                       return -1;
> 
> Shouldn't it be 'continue'?  If there's a problematic session
> it'll prevent others from being processed.  And it seems this
> code will try to run it again and again.  Maybe we can put it
> in the KILL state (or a new FAILED state) IMHO.

so if there is misconfigured session, it will get executed,
and then we catch that it exited, like:

	# ./perf daemon start -f
	[2021-02-11 17:38:19.718166] daemon started (pid 1167439)
	[2021-02-11 17:38:19.719757] reconfig: ruining session [cycles:1167440]: abc -m 10M -e cycles --overwrite --switch-output -a
	[2021-02-11 17:38:19.720861] reconfig: ruining session [sched:1167441]: -m 20M -e sched:* --overwrite --switch-output -a
	[2021-02-11 17:38:21.132511] session 'cycles' exited, status=255

session will be removed

when the config is fixed and updated, daemon will pick it up:

	[2021-02-11 17:42:59.241140] reconfig: ruining session [cycles:1167642]: -m 10M -e cycles --overwrite --switch-output -a


daemon_session__run fails only there's no memory for allocation,
if mkdir fails (for other error than EEXIST) and if fork fails,
so pretty serious errors, where we want to bail out anyway

jirka