linux-kernel - Re: [PATCH v2 2/2] trace: allocate space from temparary trace sequence buffer

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, 15 Dec 2022 08:23:45 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Linyu Yuan <quic_linyyuan@...cinc.com>
Cc:     Masami Hiramatsu <mhiramat@...nel.org>,
        <linux-kernel@...r.kernel.org>, <bpf@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] trace: allocate space from temparary trace
 sequence buffer

On Thu, 15 Dec 2022 12:33:27 +0800
Linyu Yuan <quic_linyyuan@...cinc.com> wrote:

> --- a/include/linux/trace_seq.h
> +++ b/include/linux/trace_seq.h
> @@ -95,6 +95,7 @@ extern void trace_seq_bitmask(struct trace_seq *s, const unsigned long *maskp,
>  extern int trace_seq_hex_dump(struct trace_seq *s, const char *prefix_str,
>  			      int prefix_type, int rowsize, int groupsize,
>  			      const void *buf, size_t len, bool ascii);
> +void *trace_seq_alloc_buffer(struct trace_seq *s, int len);

So, I really don't like the name with "alloc" in it. That makes the
assumption that it also needs to be freed. Which it does not, and why it
confused me last night.

A better name would be trace_seq_acquire(s, len);

And it should return a char *, as it it process stings and not arbitrary
binary data.

> +/**
> + * trace_seq_alloc_buffer - allocate seq buffer with size len
> + * @s: trace sequence descriptor
> + * @len: size of buffer to be allocated
> + *
> + * allocate space with size of @len from seq buffer for output usage,
> + * On success, it returns start address of the allocated buffer,
> + * user can fill data start from the address, user should make sure the
> + * data length not exceed the @len, if it exceed, behavior is undefined.
> + *
> + * Returns NULL if no buffer can be allocated, it also means system will
> + * crash, it is user responsiblity to make sure total buffer used will
> + * not exceed PAGE_SIZE.
> + *
> + * it allow multiple usage in one trace output function call.
> + */
> +void *trace_seq_alloc_buffer(struct trace_seq *s, int len)

char *trace_seq_acquire(struct trace_seq *s, int len)

> +{
> +	char *buf = trace_seq_buffer_ptr(s);
> +

You need to check the length first before committing:

	if (seq_buf_buffer_left(&s->seq) < len)
		return NULL;

	
> +	seq_buf_commit(&s->seq, len);

Because the above is a bug if len is too big.

> +
> +	if (unlikely(seq_buf_has_overflowed(&s->seq))) {

And then we don't need this either.

I must apologize for the response last night. It was past my normal bed
time, and I must really avoid reviewing patches when I should be going to
bed ;-)

-- Steve


> +		s->full = 1;
> +		return NULL;
> +	}
> +
> +	return (void *)buf;
> +}
> +EXPORT_SYMBOL(trace_seq_alloc_buffer);