On Tue, May 13, 2025 at 10:04 AM Andrew Morton <[email protected]> wrote: > > On Tue, 13 May 2025 09:48:15 +0800 Jason Xing <[email protected]> > wrote: > > > > > +{ > > > > + 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/[email protected]/ Thanks, Jason
