Re: [PATCH] perf bench: add --write-size option to sched pipe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Integer overflow in write_size cast bypasses pipe capacity validation and causes a deadlock.
- [High] Missing validation for write_size == 0 causes benchmark hangs or bogus results.
- [Medium] File descriptor leak on the fcntl error path.
--

commit 2c945dc323a6b416d95bca3f92808b828bcc9c74
Author: Breno Leitao <leitao@debian.org>

perf bench: add --write-size option to sched pipe

This commit adds a -s/--write-size option to the sched pipe benchmark to set
the bytes written and read per ping-pong iteration. It also resizes the pipe
using F_SETPIPE_SZ to match the requested size.

> diff --git a/tools/perf/bench/sched-pipe.c b/tools/perf/bench/sched-pipe.c
> --- a/tools/perf/bench/sched-pipe.c
> +++ b/tools/perf/bench/sched-pipe.c
[ ... ]
> @@ -48,6 +49,7 @@ static	int			loops = LOOPS_DEFAULT;
>  static bool			threaded;
>  
>  static bool			nonblocking;
> +static unsigned int		write_size = sizeof(int);

Is there a missing lower-bound check for write_size? If a user passes 0,
the vfs_write system call will return 0 immediately without invoking
pipe_write(), meaning no data is transferred and no readers are woken up.

In blocking mode, both threads could instantly spin through their loops
transferring 0 bytes, generating a falsely fast benchmark result without
measuring context switch latency. In non-blocking mode, the threads rely
on epoll_wait(), which would hang forever since the 0-byte write wouldn't
trigger EPOLLIN.

[ ... ]
> @@ -204,10 +209,10 @@ static void *worker_thread(void *__tdata)
>  	}
>  
>  	for (i = 0; i < loops; i++) {
> -		ret = write(td->pipe_write, &m, sizeof(int));
> -		BUG_ON(ret != sizeof(int));
> +		ret = write(td->pipe_write, td->buf, write_size);
> +		BUG_ON(ret != (int)write_size);

Could this cast to (int) cause an unexpected crash? If write_size is larger
than INT_MAX, the cast results in a negative number, and positive return
values from write() would be compared against this negative number,
triggering the BUG_ON.

>  		ret = read_pipe(td);
> -		BUG_ON(ret != sizeof(int));
> +		BUG_ON(ret != (int)write_size);
>  	}
>  
>  	return NULL;
> @@ -239,6 +244,22 @@ int bench_sched_pipe(int argc, const char **argv)
>  	BUG_ON(pipe2(pipe_1, flags));
>  	BUG_ON(pipe2(pipe_2, flags));
>  
> +	/*
> +	 * On custom write_size, set the pipe size to acommodate write_size
> +	 */
> +	if (write_size > sizeof(int) &&
> +	    (fcntl(pipe_1[1], F_SETPIPE_SZ, write_size) < (int)write_size ||
> +	     fcntl(pipe_2[1], F_SETPIPE_SZ, write_size) < (int)write_size)) {

Does this signed cast bypass the pipe capacity validation when write_size
exceeds INT_MAX?

If fcntl() fails because the size exceeds system limits and returns -1, the
check evaluates as -1 < (negative number). Since -1 is not less than any
negative number, this evaluates to false, silently bypassing the error
handler.

The benchmark would then proceed with default 64KB pipes but attempt to
write >2GB payloads into them. Because the ping-pong design requires both
threads to fully execute write() before read(), both threads would fill
their respective pipes and block indefinitely, resulting in a deadlock.

> +		fprintf(stderr, "--write-size %u exceeds /proc/sys/fs/pipe-max-size\n",
> +			write_size);
> +		return -1;

Does this return path leak the pipe file descriptors? The benchmark allocates
four file descriptors via pipe2() just above this block. If the early return
is taken, it appears to exit without passing through a cleanup path to close
the acquired file descriptors.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-perf_bench_pipe-v1-1-3c5b805ba178@debian.org?part=1