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

[Steven Rostedt <rostedt@goodmis.org>]

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