On Tue, May 13, 2025 at 10:04 AM Andrew Morton
[off-list ref] wrote:

On Tue, 13 May 2025 09:48:15 +0800 Jason Xing [off-list ref] wrote:

quoted

quoted

quoted

+{
+     unsigned int i, full_counter = 0;
+     struct rchan_buf *rbuf;
+     int offset = 0;
+
+     if (!chan || !buf || flags & ~RELAY_DUMP_MASK)
+             return;
+
+     if (len < RELAY_DUMP_BUF_MAX_LEN)
+             return;

So we left the memory at *buf uninitialized but failed to tell the
caller this.  The caller will then proceed to use uninitialized memory.

It's a programming error, so simply going BUG seems OK.

Are you suggesting that I should remove the above check because
developers should take care of the length of the buffer to write
outside of the relay_dump function? or use this instead:
WARN_ON_ONCE(len < RELAY_DUMP_BUF_MAX_LEN);
?

It's a poor interface - it returns uninitialized data while not
alerting the caller to this.  You'll figure something out ;)

Perhaps

        BUG_ON(len < RELAY_DUMP_BUF_MAX_LEN);

I'm unsure if BUG_ON is appropriate here since technically speaking
it's not a bug. For now, only sizeof(u32) is used in the buffer.

        *buf = '\0';
        if (!chan || (flags & ~RELAY_DUMP_MASK))
                return;

We don't need to check for !buf - the oops message contains the same info.

Got it. Thanks.

Maybe we don't need to check !chan either.  Can it be NULL here?

It depends on how users call this. If users call this without
initialization of chan, relay_dump() can avoid the crash. It works
like kfree() which prevents the NULL object from being freed.

BTW, should I merge this commit [1] into the series in V2 so that you
can easily review?

[1]: https://lore.kernel.org/all/20250507134225.63248-1-kerneljasonxing@gmail.com/ (local)

Thanks,
Jason