Re: linux-next: build failure after merge of the kspp tree

[Kees Cook <keescook@chromium.org>]

On Tue, Jan 25, 2022 at 04:28:59PM -0500, Steven Rostedt wrote:
> On Tue, 25 Jan 2022 16:23:26 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > > >  #undef __get_rel_dynamic_array
> > > > -#define __get_rel_dynamic_array(field)	\
> > > > -		((void *)(&__entry->__rel_loc_##field) +	\
> > > > -		 sizeof(__entry->__rel_loc_##field) +		\
> > > > +#define __get_rel_dynamic_array(field)					\
> > > > +		((void *)__entry + 					\
> > > > +		 offsetof(typeof(*__entry), __rel_loc_##field) +	\
> > > > +		 sizeof(__entry->__rel_loc_##field) +			\
> > > >  		 (__entry->__rel_loc_##field & 0xffff))
> > > >  
> > > >  #undef __get_rel_dynamic_array_len    
> > > 
> 
> I also do not like the the inconsistency between
> sizeof(__entry->__rel_loc_##field) and sizeof(u32) that is used in the
> calculation part:
> 
> 
> #define __rel_dynamic_array(type, item, len)				\
> 	__item_length = (len) * sizeof(type);				\
> 	__data_offsets->item = __data_size +				\
> 			       offsetof(typeof(*entry), __data) -	\
> 			       offsetof(typeof(*entry), __rel_loc_##item) -	\
> 			       sizeof(u32);				\
> 	__data_offsets->item |= __item_length << 16;			\
> 	__data_size += __item_length;
> 
> Why is one using sizeof(u32) and the other using the size of the field?

It might make more sense to calculate everything as an offset within
__data[] instead of from the start of __entry. The patch I sent just did
in perf.h exactly what Masami did in trace_event.h. That worked. I had
an earlier version that did this horrible thing which could probably be
significantly improved, since I just subtract the offset of __data:

 #undef __get_rel_dynamic_array
 #define __get_rel_dynamic_array(field) \
-		((void *)(&__entry->__rel_loc_##field) +        \
-		 sizeof(__entry->__rel_loc_##field) +           \
-		 (__entry->__rel_loc_##field & 0xffff))
+		((void *)&__entry->__data[			\
+		 offsetof(typeof(*__entry), __rel_loc_##field)	\
+		 + sizeof(__entry->__rel_loc_##field)		\
+		 + (__entry->__rel_loc_##field & 0xffff)	\
+		 - offsetof(typeof(*__entry), __data)		\
+		])

> Just to let you know what is happening. As dynamic elements of the trace
> event needs to be appended at the end of the event, the above macros are
> defined and then run through the TRACE_EVENT() macro, where the
> TP_STRUCT__entry() is parsed to calculate where each item will be for that
> event.
> 
> static inline notrace int trace_event_get_offsets_##call(		\
> 	struct trace_event_data_offsets_##call *__data_offsets, proto)	\
> {									\
> 	int __data_size = 0;						\
> 	int __maybe_unused __item_length;				\
> 	struct trace_event_raw_##call __maybe_unused *entry;		\
> 									\
> 	tstruct;							\
> 									\
> 	return __data_size;						\
> }
> 
> 
> The tstruct is the TP_STRUCT__entry() and for each __rel_dynamic_array() or
> __dynamic_array(), the __data_size gets updated and saved into the
> __data_offsets that holds where each item is.
> 
> The rel versions sets the offset from its location to the data, where as
> the non rel versions sets the offset from the beginning of the event to the
> data.

Could this just be

#define __get_rel_dynamic_array(field) \
	((void *)(&__entry->data[__entry->__rel_loc_##field & 0xffff])

?

-- 
Kees Cook