Re: [PATCH 2/6] tracing: Replace memcpy() with __get_task_comm()

Steven Rostedt
2024-06-03T22:18:34Z

On Mon, 3 Jun 2024 14:42:10 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, 3 Jun 2024 at 14:19, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > - __array( char, comm, TASK_COMM_LEN )
> > + __string( comm, strlen(comm) )
>
> Is this actually safe is 'comm[]' is being modified at the same time?
> The 'strlen()' will not be consistent with the string copy.

First, I realized that it should actually be:

__string( comm, task->comm )

But your question is still a valid question, as the internal logic will
call strlen() on the second parameter.

>
> Because that is very much the case. It's not a stable source.
>
> For example, strlen() may return 5. But by the time you then actually
> copy the data, the string may have changed, and there would not
> necessarily be a NUL character at comm[5] any more. It might be
> further in the string, or it might be earlier.

The logic behind __string() and __assign_str() will always add a NUL
character.

__string() is defined as:

static inline const char *__string_src(const char *str)
{
if (!str)
return EVENT_NULL_STR;
return str;
}

#undef __dynamic_array
#define __dynamic_array(type, item, len) \
__item_length = (len) * sizeof(type); \
__data_offsets->item = __data_size + \
offsetof(typeof(*entry), __data); \
__data_offsets->item |= __item_length << 16; \
__data_size += __item_length;

#undef __string
#define __string(item, src) __dynamic_array(char, item, \
strlen(__string_src(src)) + 1) \
__data_offsets->item##_ptr_ = src;

The above will use the strlen(src) to specify the amount of memory to
allocate on the ring buffer: "strlen(__string_src(src)) + 1)"

This is stored on a special structure for the entry and used in the
__assign_str() (the reason I removed the source to that macro during this
merge window).

#undef __assign_str
#define __assign_str(dst) \
do { \
char *__str__ = __get_str(dst); \
int __len__ = __get_dynamic_array_len(dst) - 1; \
memcpy(__str__, __data_offsets.dst##_ptr_ ? : \
EVENT_NULL_STR, __len__); \
__str__[__len__] = '\0'; \
} while (0)

The source of the string is copied via memcpy() using the length stored
from the __string() macro (minus 1), and then '\0' is added to it to force
the NUL character to be in the memory for the string.

-- Steve