linux-kernel - Re: [PATCH 09/24] perf daemon: Add signalfd 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, 4 Feb 2021 16:06:27 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Jiri Olsa <jolsa@...nel.org>,
        Alexei Budankov <abudankov@...wei.com>,
        lkml <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Ingo Molnar <mingo@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Namhyung Kim <namhyung@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Michael Petlan <mpetlan@...hat.com>,
        Ian Rogers <irogers@...gle.com>,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 09/24] perf daemon: Add signalfd support

On Wed, Feb 03, 2021 at 06:24:09PM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> > @@ -351,6 +355,103 @@ static int session__run(struct session *session, struct daemon *daemon)
> >  	return -1;
> >  }
> >  
> > +static pid_t handle_signalfd(struct daemon *daemon)
> > +{
> > +	struct signalfd_siginfo si;
> > +	struct session *session;
> > +	ssize_t err;
> > +	int status;
> > +	pid_t pid;
> > +
> > +	err = read(daemon->signal_fd, &si, sizeof(struct signalfd_siginfo));
> 
> 
> I saw these and recalled we have a readn() function, shouldn't we be
> usint it in this series?

right, but the read call in here needs to succeed with the data
size otherwise it's an error

but perhaps we could use it later for the control descriptor
read/write, I'll check

> 
> Its even in tools/lib/perf/lib.c
> 
> > +	if (err != sizeof(struct signalfd_siginfo))
> > +		return -1;
> > +
> > +	list_for_each_entry(session, &daemon->sessions, list) {
> > +
> > +		if (session->pid != (int) si.ssi_pid)
> > +			continue;
> > +
> > +		pid = waitpid(session->pid, &status, 0);
> > +		if (pid == session->pid) {
> > +			if (WIFEXITED(status)) {
> > +				pr_info("session '%s' exited, status=%d\n",
> > +					session->name, WEXITSTATUS(status));
> > +			} else if (WIFSIGNALED(status)) {
> > +				pr_info("session '%s' killed (signal %d)\n",
> > +					session->name, WTERMSIG(status));
> > +			} else if (WIFSTOPPED(status)) {
> > +				pr_info("session '%s' stopped (signal %d)\n",
> > +					session->name, WSTOPSIG(status));
> > +			} else {
> > +				pr_info("session '%s' Unexpected status (0x%x)\n",
> > +					session->name, status);
> > +			}
> > +		}
> > +
> > +		session->state = SESSION_STATE__KILL;
> > +		session->pid = -1;
> > +		return pid;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int session__wait(struct session *session, struct daemon *daemon, int secs)
> > +{
> > +	struct pollfd pollfd = {
> > +		.fd	= daemon->signal_fd,
> > +		.events	= POLLIN,
> > +	};
> > +	pid_t wpid = 0, pid = session->pid;
> > +	time_t start;
> > +
> > +	start = time(NULL);
> 
> 
> 	time_t start = time(NULL);
> > +
> > +	do {
> > +		if (poll(&pollfd, 1, 1000))
> > +			wpid = handle_signalfd(daemon);
> 
> Shouldn't we check for -1 and handle that differently?

ah right, check for error, will add

> 
> > +
> > +		if (start + secs < time(NULL))
> > +			return -1;
> > +	} while (wpid != pid);
> > +
> > +	return 0;
> > +}
> > +
> > +static bool daemon__has_alive_session(struct daemon *daemon)
> > +{
> > +	struct session *session;
> > +
> > +	list_for_each_entry(session, &daemon->sessions, list) {
> > +		if (session->pid != -1)
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +static int daemon__wait(struct daemon *daemon, int secs)
> > +{
> > +	struct pollfd pollfd = {
> > +		.fd	= daemon->signal_fd,
> > +		.events	= POLLIN,
> > +	};
> > +	time_t start;
> > +
> > +	start = time(NULL);
> > +
> > +	do {
> > +		if (poll(&pollfd, 1, 1000))
> > +			handle_signalfd(daemon);
> 
> ditto

ok

thanks,
jirka