Hello Matthew Sakai,
The patch 25479d97b12c: "dm vdo: add statistics reporting" from Nov
16, 2023 (linux-next), leads to the following (unpublished) Smatch
static checker warning:
drivers/md/dm-vdo/funnel-workqueue.c:545 vdo_dump_completion_to_buffer() warn: why check scnprintf return value?
drivers/md/dm-vdo/message-stats.c:26 write_u64() warn: why check scnprintf return value?
drivers/md/dm-vdo/message-stats.c:43 write_u32() warn: why check scnprintf return value?
drivers/md/dm-vdo/message-stats.c:60 write_block_count_t() warn: why check scnprintf return value?
drivers/md/dm-vdo/message-stats.c:77 write_string() warn: why check scnprintf return value?
drivers/md/dm-vdo/message-stats.c:94 write_bool() warn: why check scnprintf return value?
drivers/md/dm-vdo/message-stats.c:111 write_u8() warn: why check scnprintf return value?
drivers/md/dm-vdo/message-stats.c
99 static int write_u8(char *prefix,
100 u8 value,
101 char *suffix,
102 char **buf,
103 unsigned int *maxlen)
104 {
105 int count = scnprintf(*buf, *maxlen, "%s%u%s",
106 prefix == NULL ? "" : prefix,
107 value,
108 suffix == NULL ? "" : suffix);
109 *buf += count;
110 *maxlen -= count;
--> 111 if (count >= *maxlen)
^^^^^^^^^^^^^^^^
snprintf() returns the number of bytes that would have been printed if
we had enough space and scnprintf() returns the number of bytes that
were printed. (Both functions exclude the NUL terminator from the
returned count).
It seldom makes sense to check the return from scnprintf(). In this
case count can never be >= *maxlen. The most it can be is *maxlen - 1.
112 return VDO_UNEXPECTED_EOF;
113 return VDO_SUCCESS;
114 }
The code in vdo_dump_completion_to_buffer() is almost basically correct.
It's just off by one in a totally harmless way and I wouldn't have
reported it if not for these other impossible conditions.
drivers/md/dm-vdo/funnel-workqueue.c
538 void vdo_dump_completion_to_buffer(struct vdo_completion *completion, char *buffer,
539 size_t length)
540 {
541 size_t current_length =
542 scnprintf(buffer, length, "%.*s/", TASK_COMM_LEN,
543 (completion->my_queue == NULL ? "-" : completion->my_queue->name));
544
545 if (current_length < length - 1) {
The highest valid return is current_length == length - 1 but that's
treated as invalid. If we changed to snprintf() then we could fix the
condition to allow the final character:
current_length = snprintf(...);
if (current_length <= length - 1) {
^^^^
546 get_function_name((void *) completion->callback, buffer + current_length,
547 length - current_length);
548 }
549 }
regards,
dan carpenter
