Re: [PATCH v2 2/2] libtracecmd: Support changing /proc/kallsyms

Sorry for the late reply, been way too busy on the kernel side.

On Mon,  2 Jun 2025 22:38:33 +0100
Ilya Leoshkevich <[email protected]> wrote:

> Running BPF selftests under trace-cmd intermittently fails with:
> 
>     error in size of file '/proc/kallsyms'
> 
> This is because these selftests load and unload BPF programs.
> bpf_prog_put() uses workqueues and RCU, so these programs disappear
> from /proc/kallsyms after a delay.
> 
> trace-cmd reads /proc/kallsyms twice: the first time to compute its
> size, and the second time to copy it into the trace file. If the
> resulting sizes don't match, which is what happens in this case,
> recording fails.
> 
> Fix by updating the size. This will work even for sending trace data
> over the network thanks to trace_msg_cache.

I'm perfectly fine with the logic, just some nits about the patch.
Could you send a v3? Comments below.

> 
> Signed-off-by: Ilya Leoshkevich <[email protected]>
> ---
>  lib/trace-cmd/trace-output.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/trace-cmd/trace-output.c b/lib/trace-cmd/trace-output.c
> index 00b87a19..e9ec8f51 100644
> --- a/lib/trace-cmd/trace-output.c
> +++ b/lib/trace-cmd/trace-output.c
> @@ -577,6 +577,27 @@ __hidden int tcmd_out_update_section_header(struct tracecmd_output *handle, tsiz
>  	return 0;
>  }
>  

Needs a comment here:

/*
 * For files that can change while reading it (like /proc/kallsyms) the
 * normal "size != check_size" can fail. That's because from the time
 * the file size is read to write in the size to the time it is read to copy
 * it into the file, it can change. In this case, the size that was stored
 * in the file needs to be updated.
 */
> +static int tcmd_out_update_endian_4(struct tracecmd_output *handle,
> +				    off_t offset, int val)

This is a static function, it doesn't need the "tcmd_" prefix. That should
be reserved for global functions that are "__hidden".

> +{
> +	tsize_t current;
> +	int endian4;
> +
> +	if (offset == (off_t)-1)
> +		return -1;

Add newline here.

> +	current = do_lseek(handle, 0, SEEK_CUR);
> +	if (current == (off_t)-1)
> +		return -1;

And here.

> +	if (do_lseek(handle, offset, SEEK_SET) == (off_t)-1)
> +		return -1;

And here.

> +	endian4 = convert_endian_4(handle, val);
> +	if (tcmd_do_write_check(handle, &endian4, 4))
> +		return -1;

And here.

> +	if (do_lseek(handle, current, SEEK_SET) == (off_t)-1)
> +		return -1;

And here.

Other than that, looks good.

-- Steve

> +	return 0;
> +}
> +
>  static int save_string_section(struct tracecmd_output *handle, bool compress)
>  {
>  	enum tracecmd_section_flags flags = 0;
> @@ -1134,6 +1155,7 @@ static int read_proc_kallsyms(struct tracecmd_output *handle, bool compress)
>  	enum tracecmd_section_flags flags = 0;
>  	unsigned int size, check_size, endian4;
>  	const char *path = "/proc/kallsyms";
> +	off_t size_offset;
>  	tsize_t offset;
>  	struct stat st;
>  	int ret;
> @@ -1165,13 +1187,15 @@ static int read_proc_kallsyms(struct tracecmd_output *handle, bool compress)
>  	}
>  	size = get_size(path);
>  	endian4 = convert_endian_4(handle, size);
> +	size_offset = do_lseek(handle, 0, SEEK_CUR);
>  	ret = tcmd_do_write_check(handle, &endian4, 4);
>  	if (ret)
>  		goto out;
>  
>  	set_proc_kptr_restrict(0);
>  	check_size = copy_file(handle, path);
> -	if (size != check_size) {
> +	if (size != check_size &&
> +	    tcmd_out_update_endian_4(handle, size_offset, check_size)) {
>  		errno = EINVAL;
>  		tracecmd_warning("error in size of file '%s'", path);
>  		set_proc_kptr_restrict(1);