Fix syscall detection running for worker and not job

polkadot/node/core/pvf/common/src/error.rs

@@ -77,7 +77,7 @@ pub enum PrepareError {
 	#[codec(index = 9)]
 	ClearWorkerDir(String),
 	/// The preparation job process died, due to OOM, a seccomp violation, or some other factor.
-	JobDied(String),
+	JobDied { err: String, job_pid: i32 },
 	#[codec(index = 10)]
 	/// Some error occurred when interfacing with the kernel.
 	#[codec(index = 11)]
@@ -96,7 +96,7 @@ impl PrepareError {
 		match self {
 			Prevalidation(_) | Preparation(_) | JobError(_) | OutOfMemory => true,
 			IoErr(_) |
-			JobDied(_) |
+			JobDied { .. } |
 			CreateTmpFile(_) |
 			RenameTmpFile { .. } |
 			ClearWorkerDir(_) |
@@ -119,7 +119,8 @@ impl fmt::Display for PrepareError {
 			JobError(err) => write!(f, "panic: {}", err),
 			TimedOut => write!(f, "prepare: timeout"),
 			IoErr(err) => write!(f, "prepare: io error while receiving response: {}", err),
-			JobDied(err) => write!(f, "prepare: prepare job died: {}", err),
+			JobDied { err, job_pid } =>
+				write!(f, "prepare: prepare job with pid {job_pid} died: {err}"),
 			CreateTmpFile(err) => write!(f, "prepare: error creating tmp file: {}", err),
 			RenameTmpFile { err, src, dest } =>
 				write!(f, "prepare: error renaming tmp file ({:?} -> {:?}): {}", src, dest, err),

polkadot/node/core/pvf/common/src/execute.rs

@@ -46,7 +46,7 @@ pub enum WorkerResponse {
 	///
 	/// We cannot treat this as an internal error because malicious code may have killed the job.
 	/// We still retry it, because in the non-malicious case it is likely spurious.
-	JobDied(String),
+	JobDied { err: String, job_pid: i32 },
 	/// An unexpected error occurred in the job process, e.g. failing to spawn a thread, panic,
 	/// etc.
 	///

polkadot/node/core/pvf/execute-worker/src/lib.rs

@@ -354,7 +354,7 @@ fn handle_child_process(
 /// - The response, either `Ok` or some error state.
 fn handle_parent_process(
 	mut pipe_read: PipeReader,
-	child: Pid,
+	job_pid: Pid,
 	worker_pid: u32,
 	usage_before: Usage,
 	timeout: Duration,
@@ -367,10 +367,11 @@ fn handle_parent_process(
 		// Should retry at any rate.
 		.map_err(|err| io::Error::new(io::ErrorKind::Other, err.to_string()))?;
 
-	let status = nix::sys::wait::waitpid(child, None);
+	let status = nix::sys::wait::waitpid(job_pid, None);
 	gum::trace!(
 		target: LOG_TARGET,
 		%worker_pid,
+		%job_pid,
 		"execute worker received wait status from job: {:?}",
 		status,
 	);
@@ -390,6 +391,7 @@ fn handle_parent_process(
 		gum::warn!(
 			target: LOG_TARGET,
 			%worker_pid,
+			%job_pid,
 			"execute job took {}ms cpu time, exceeded execute timeout {}ms",
 			cpu_tv.as_millis(),
 			timeout.as_millis(),
@@ -422,6 +424,7 @@ fn handle_parent_process(
 					gum::warn!(
 						target: LOG_TARGET,
 						%worker_pid,
+						%job_pid,
 						"execute job error: {}",
 						job_error,
 					);
@@ -437,15 +440,18 @@ fn handle_parent_process(
 		//
 		// The job gets SIGSYS on seccomp violations, but this signal may have been sent for some
 		// other reason, so we still need to check for seccomp violations elsewhere.
-		Ok(WaitStatus::Signaled(_pid, signal, _core_dump)) =>
-			Ok(WorkerResponse::JobDied(format!("received signal: {signal:?}"))),
+		Ok(WaitStatus::Signaled(_pid, signal, _core_dump)) => Ok(WorkerResponse::JobDied {
+			err: format!("received signal: {signal:?}"),
+			job_pid: job_pid.as_raw(),
+		}),
 		Err(errno) => Ok(internal_error_from_errno("waitpid", errno)),
 
 		// It is within an attacker's power to send an unexpected exit status. So we cannot treat
 		// this as an internal error (which would make us abstain), but must vote against.
-		Ok(unexpected_wait_status) => Ok(WorkerResponse::JobDied(format!(
-			"unexpected status from wait: {unexpected_wait_status:?}"
-		))),
+		Ok(unexpected_wait_status) => Ok(WorkerResponse::JobDied {
+			err: format!("unexpected status from wait: {unexpected_wait_status:?}"),
+			job_pid: job_pid.as_raw(),
+		}),
 	}
 }
 

polkadot/node/core/pvf/prepare-worker/src/lib.rs

@@ -257,9 +257,9 @@ pub fn worker_entrypoint(
 
 						handle_parent_process(
 							pipe_reader,
+							worker_pid,
 							child,
 							temp_artifact_dest.clone(),
-							worker_pid,
 							usage_before,
 							preparation_timeout,
 						)
@@ -506,9 +506,9 @@ fn handle_child_process(
 /// - If the child process timeout, it returns `PrepareError::TimedOut`.
 fn handle_parent_process(
 	mut pipe_read: PipeReader,
-	child: Pid,
-	temp_artifact_dest: PathBuf,
 	worker_pid: u32,
+	job_pid: Pid,
+	temp_artifact_dest: PathBuf,
 	usage_before: Usage,
 	timeout: Duration,
 ) -> Result<PrepareWorkerSuccess, PrepareError> {
@@ -518,10 +518,11 @@ fn handle_parent_process(
 		.read_to_end(&mut received_data)
 		.map_err(|err| PrepareError::IoErr(err.to_string()))?;
 
-	let status = nix::sys::wait::waitpid(child, None);
+	let status = nix::sys::wait::waitpid(job_pid, None);
 	gum::trace!(
 		target: LOG_TARGET,
 		%worker_pid,
+		%job_pid,
 		"prepare worker received wait status from job: {:?}",
 		status,
 	);
@@ -539,6 +540,7 @@ fn handle_parent_process(
 		gum::warn!(
 			target: LOG_TARGET,
 			%worker_pid,
+			%job_pid,
 			"prepare job took {}ms cpu time, exceeded prepare timeout {}ms",
 			cpu_tv.as_millis(),
 			timeout.as_millis(),
@@ -573,6 +575,7 @@ fn handle_parent_process(
 					gum::debug!(
 						target: LOG_TARGET,
 						%worker_pid,
+						%job_pid,
 						"worker: writing artifact to {}",
 						temp_artifact_dest.display(),
 					);
@@ -593,15 +596,18 @@ fn handle_parent_process(
 		//
 		// The job gets SIGSYS on seccomp violations, but this signal may have been sent for some
 		// other reason, so we still need to check for seccomp violations elsewhere.
-		Ok(WaitStatus::Signaled(_pid, signal, _core_dump)) =>
-			Err(PrepareError::JobDied(format!("received signal: {signal:?}"))),
+		Ok(WaitStatus::Signaled(_pid, signal, _core_dump)) => Err(PrepareError::JobDied {
+			err: format!("received signal: {signal:?}"),
+			job_pid: job_pid.as_raw(),
+		}),
 		Err(errno) => Err(error_from_errno("waitpid", errno)),
 
 		// An attacker can make the child process return any exit status it wants. So we can treat
 		// all unexpected cases the same way.
-		Ok(unexpected_wait_status) => Err(PrepareError::JobDied(format!(
-			"unexpected status from wait: {unexpected_wait_status:?}"
-		))),
+		Ok(unexpected_wait_status) => Err(PrepareError::JobDied {
+			err: format!("unexpected status from wait: {unexpected_wait_status:?}"),
+			job_pid: job_pid.as_raw(),
+		}),
 	}
 }
 

polkadot/node/core/pvf/src/execute/worker_intf.rs

@@ -201,7 +201,7 @@ pub async fn start_work(
 				idle_worker: IdleWorker { stream, pid, worker_dir },
 			},
 			WorkerResponse::JobTimedOut => Outcome::HardTimeout,
-			WorkerResponse::JobDied(err) => Outcome::JobDied { err },
+			WorkerResponse::JobDied { err, job_pid: _ } => Outcome::JobDied { err },
 			WorkerResponse::JobError(err) => Outcome::JobError { err },
 
 			WorkerResponse::InternalError(err) => Outcome::InternalError { err },
@@ -238,18 +238,17 @@ async fn handle_response(
 		}
 	}
 
-	if let WorkerResponse::JobDied(_) = response {
+	if let WorkerResponse::JobDied { err: _, job_pid } = response {
 		// The job died. Check if it was due to a seccomp violation.
 		//
 		// NOTE: Log, but don't change the outcome. Not all validators may have
 		// auditing enabled, so we don't want attackers to abuse a non-deterministic
 		// outcome.
-		for syscall in
-			security::check_seccomp_violations_for_worker(audit_log_file, worker_pid).await
-		{
+		for syscall in security::check_seccomp_violations_for_job(audit_log_file, job_pid).await {
 			gum::error!(
 				target: LOG_TARGET,
-				worker_pid,
+				%worker_pid,
+				%job_pid,
 				%syscall,
 				?validation_code_hash,
 				?artifact_path,

polkadot/node/core/pvf/src/prepare/pool.rs

@@ -388,14 +388,14 @@ fn handle_mux(
 					Ok(())
 				},
 				// The worker might still be usable, but we kill it just in case.
-				Outcome::JobDied(err) => {
+				Outcome::JobDied { err, job_pid } => {
 					if attempt_retire(metrics, spawned, worker) {
 						reply(
 							from_pool,
 							FromPool::Concluded {
 								worker,
 								rip: true,
-								result: Err(PrepareError::JobDied(err)),
+								result: Err(PrepareError::JobDied { err, job_pid }),
 							},
 						)?;
 					}

polkadot/node/core/pvf/src/prepare/worker_intf.rs

@@ -104,7 +104,7 @@ pub enum Outcome {
 	/// The preparation job process died, due to OOM, a seccomp violation, or some other factor.
 	///
 	/// The worker might still be usable, but we kill it just in case.
-	JobDied(String),
+	JobDied { err: String, job_pid: i32 },
 }
 
 /// Given the idle token of a worker and parameters of work, communicates with the worker and
@@ -218,25 +218,26 @@ async fn handle_response(
 			Ok(result) => result,
 			// Timed out on the child. This should already be logged by the child.
 			Err(PrepareError::TimedOut) => return Outcome::TimedOut,
-			Err(PrepareError::JobDied(err)) => {
+			Err(PrepareError::JobDied { err, job_pid }) => {
 				// The job died. Check if it was due to a seccomp violation.
 				//
 				// NOTE: Log, but don't change the outcome. Not all validators may have
 				// auditing enabled, so we don't want attackers to abuse a non-deterministic
 				// outcome.
 				for syscall in
-					security::check_seccomp_violations_for_worker(audit_log_file, worker_pid).await
+					security::check_seccomp_violations_for_job(audit_log_file, job_pid).await
 				{
 					gum::error!(
 						target: LOG_TARGET,
 						%worker_pid,
+						%job_pid,
 						%syscall,
 						?pvf,
 						"A forbidden syscall was attempted! This is a violation of our seccomp security policy. Report an issue ASAP!"
 					);
 				}
 
-				return Outcome::JobDied(err)
+				return Outcome::JobDied { err, job_pid }
 			},
 			Err(PrepareError::OutOfMemory) => return Outcome::OutOfMemory,
 			Err(err) => return Outcome::Concluded { worker, result: Err(err) },

polkadot/node/core/pvf/src/security.rs

@@ -345,25 +345,25 @@ impl AuditLogFile {
 	}
 }
 
-/// Check if a seccomp violation occurred for the given worker. As the syslog may be in a different
-/// location, or seccomp auditing may be disabled, this function provides a best-effort attempt
-/// only.
+/// Check if a seccomp violation occurred for the given job process. As the syslog may be in a
+/// different location, or seccomp auditing may be disabled, this function provides a best-effort
+/// attempt only.
 ///
 /// The `audit_log_file` must have been obtained before the job started. It only allows reading
 /// entries that were written since it was obtained, so that we do not consider events from previous
 /// processes with the same pid. This can still be racy, but it's unlikely and fine for a
 /// best-effort attempt.
-pub async fn check_seccomp_violations_for_worker(
+pub async fn check_seccomp_violations_for_job(
 	audit_log_file: Option<AuditLogFile>,
-	worker_pid: u32,
+	job_pid: i32,
 ) -> Vec<u32> {
-	let audit_event_pid_field = format!("pid={worker_pid}");
+	let audit_event_pid_field = format!("pid={job_pid}");
 
 	let audit_log_file = match audit_log_file {
 		Some(file) => {
 			gum::trace!(
 				target: LOG_TARGET,
-				%worker_pid,
+				%job_pid,
 				audit_log_path = ?file.path,
 				"checking audit log for seccomp violations",
 			);
@@ -372,7 +372,7 @@ pub async fn check_seccomp_violations_for_worker(
 		None => {
 			gum::warn!(
 				target: LOG_TARGET,
-				%worker_pid,
+				%job_pid,
 				"could not open either {AUDIT_LOG_PATH} or {SYSLOG_PATH} for reading audit logs"
 			);
 			return vec![]

polkadot/node/core/pvf/tests/it/process.rs

@@ -246,7 +246,7 @@ rusty_fork_test! {
 			// Note that we get a more specific error if the job died than if the whole worker died.
 			assert_matches!(
 				result,
-				Err(PrepareError::JobDied(err)) if err == "received signal: SIGKILL"
+				Err(PrepareError::JobDied{ err, job_pid: _ }) if err == "received signal: SIGKILL"
 			);
 		})
 	}