[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 25 Jun 2021 15:28:25 +0800
From: Yun Zhou <yun.zhou@...driver.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: linux-kernel@...r.kernel.org, kernel-hardening@...ts.openwall.com,
ying.xue@...driver.com, "Li, Zhiquan" <Zhiquan.Li@...driver.com>
Subject: Re: [PATCH] seq_buf: let seq_buf_putmem_hex support len larger than 8
Hi Mr Steven,
I found that you had ever wanted to enhance trace_seq_putmem_hex() to
allow any size input(6d2289f3faa71dcc). Great minds think alike. Your
enhancement will let the function more robust, I think it is very advisable.
Now we only need modify two lines to solve a little flaw, and to let it
more more robust.
Regards,
Yun
On 6/25/21 12:08 PM, Steven Rostedt wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
>
> On Fri, 25 Jun 2021 11:41:35 +0800
> Yun Zhou <yun.zhou@...driver.com> wrote:
>
>> Hi Steve,
>>
>> Thanks very much for your friendly and clear feedback.
>>
>> Although in current kernel trace_seq_putmem_hex() is only used for
>> single word,
>>
>> I think it should/need support longer data. These are my arguments:
>>
>> 1. The design of double loop is used to process more data. If only
>> supports single word,
>>
>> the inner loop is enough, and the outer loop and the following
>> lines are no longer needed.
>>
>> len -= j / 2;
>>
>> hex[j++] = ' ';
>>
>> 2. The last line above try to split two words/dwords with space. If only
>> supports single word,
>>
>> this strange behavior is hard to understand.
>>
>> 3. If it only supports single word, I think parameter 'len' is redundant.
> Not really, we have to differentiate char, short, int and long long.
>
>> 4. The comments of both seq_buf_putmem_hex() and trace_seq_putmem_hex()
>> have not
>>
>> indicated the scope of 'len'.
>>
>> 5. If it only supports single word, we need to design a new function to
>> support bigger block of data.
>>
>> I think it is redundant since the current function can perfectly
>> deal with.
>>
>> 6. If follow my patch, it can support any length of data, including the
>> single word.
>>
>> How do you think?
> First, since you found a real bug, we need to just fix that first (single
> word as is done currently). Because this needs to go to stable, and what
> you are explaining above is an enhancement, and not something that needs to
> be backported.
>
> Second, is there a use case? Honestly, I never use the "hex" version of the
> output. That was only pulled in because it was implemented in the original
> code that was in the rt patch. I wish we could just get rid of it.
>
> Thus, if there's a use case for handling more than one word, then I'm fine
> with adding that enhancement. But if it is being done just because it can
> be, then I don't think we should bother.
>
> What use case do you have in mind?
>
> Anyway, please send just a fix patch, and then we can discuss the merits of
> this update later. I'd like the fix to be in ASAP.
>
> Thanks!
>
> -- Steve
Content of type "text/html" skipped
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
