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

On Mon, 12 May 2025 10:49:32 +0800 Jason Xing [off-list ref] wrote:

quoted

From: Jason Xing <kernelxing@tencent.com>

In this version, only support dumping the counter for buffer full and
implement the framework of how it works. Users MUST pass a valid @buf
with a valid @len that is required to be larger than RELAY_DUMP_BUF_MAX_LEN
to acquire which information indicated by @flags to dump.

RELAY_DUMP_BUF_MAX_LEN shows the maximum len of the buffer if users
choose to dump all the values.

Users can use this buffer to do whatever they expect in their own kernel
module, say, print to console/dmesg or write them into the relay buffer.

...

+/**
+ *   relay_dump - dump statistics of the specified channel buffer
+ *   @chan: the channel
+ *   @buf: buf to store statistics
+ *   @len: len of buf to check
+ *   @flags: select particular information to dump
+ */
+void relay_dump(struct rchan *chan, char *buf, int len, int flags)

`size_t' is probably a more appropriate type for `len'.

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);
?

quoted

+     if (chan->is_global) {
+             rbuf = *per_cpu_ptr(chan->buf, 0);
+             full_counter = rbuf->stats.full;
+     } else {
+             for_each_possible_cpu(i) {

I'm thinking that at this stage in the patch series, this should be
for_each_online_cpu(), then adjust that in patch [5/5].

Point taken. Will change it.

quoted

+                     if ((rbuf = *per_cpu_ptr(chan->buf, i)))
+                             full_counter += rbuf->stats.full;
+     }
+
+     if (flags & RELAY_DUMP_BUF_FULL)
+             offset += snprintf(buf, sizeof(unsigned int), "%u", full_counter);

This seems strange.  sizeof(unsigned int) has nothing to do with the
number of characters which are consumed by expansion of "%u"?

Sorry, my bad. It should be:
offset += snprintf(buf, len, "%u", full_counter);

Passing 'len' as the second parameter.

Thanks,
Jason

quoted

+
+     snprintf(buf + offset, 1, "\n");
+}
+EXPORT_SYMBOL_GPL(relay_dump);
+
 /**
  *   relay_file_open - open file op for relay files
  *   @inode: the inode