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] [day] [month] [year] [list]

Date:   Fri, 16 Dec 2022 10:05:14 +0800
From:   Linyu Yuan <quic_linyyuan@...cinc.com>
To:     Steven Rostedt <rostedt@...dmis.org>
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 12/15/2022 9:23 PM, Steven Rostedt wrote:
> 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.
thanks, will update next version.
>
>> +/**
>> + * 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;
will change next version.
> 	
>> +	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 ;-)

sorry to bring your attention near your bed time.

I accept late response next time 😁.

thanks again for all your comment today, it is very good.

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