cxl: trace: Fix macro safety in CXL_EVT_TP_fast_assign

[PATCH] [PATCH] cxl: trace: Fix macro safety in CXL_EVT_TP_fast_assign

Posted by shaikh.kamal 7 months, 3 weeks ago

Fix checkpatch.pl detected error
The CXL_EVT_TP_fast_assign macro assigns multiple fields, but does not
wrap the body in a `do { ... } while (0)` block. This can lead to
unexpected behavior when used in conditional branches.

Add checks to ensure cxlmd is valid before accessing its fields.

Signed-off-by: shaikh.kamal <shaikhkamal2012@gmail.com>
---
 drivers/cxl/core/trace.h | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
index 25ebfbc1616c..a1a1014ee1fe 100644
--- a/drivers/cxl/core/trace.h
+++ b/drivers/cxl/core/trace.h
@@ -249,18 +249,24 @@ TRACE_EVENT(cxl_overflow,
 	__field(u8, hdr_maint_op_class)				\
 	__field(u8, hdr_maint_op_sub_class)
 
-#define CXL_EVT_TP_fast_assign(cxlmd, l, hdr)					\
-	__assign_str(memdev);				\
-	__assign_str(host);			\
-	__entry->log = (l);							\
-	__entry->serial = (cxlmd)->cxlds->serial;				\
-	__entry->hdr_length = (hdr).length;					\
-	__entry->hdr_flags = get_unaligned_le24((hdr).flags);			\
-	__entry->hdr_handle = le16_to_cpu((hdr).handle);			\
-	__entry->hdr_related_handle = le16_to_cpu((hdr).related_handle);	\
-	__entry->hdr_timestamp = le64_to_cpu((hdr).timestamp);			\
-	__entry->hdr_maint_op_class = (hdr).maint_op_class;			\
-	__entry->hdr_maint_op_sub_class = (hdr).maint_op_sub_class
+#define CXL_EVT_TP_fast_assign(cxlmd, l, hdr) \
+	do { \
+		if (!(cxlmd)) { \
+			pr_err("Invalid arguments to CXL_EVT_TP_fast_assign\n"); \
+			break; \
+		} \
+		__assign_str(memdev); \
+		__assign_str(host); \
+		__entry->log = (l); \
+		__entry->serial = (cxlmd)->cxlds->serial; \
+		__entry->hdr_length = (hdr).length; \
+		__entry->hdr_flags = get_unaligned_le24((hdr).flags); \
+		__entry->hdr_handle = le16_to_cpu((hdr).handle); \
+		__entry->hdr_related_handle = le16_to_cpu((hdr).related_handle); \
+		__entry->hdr_timestamp = le64_to_cpu((hdr).timestamp); \
+		__entry->hdr_maint_op_class = (hdr).maint_op_class; \
+		__entry->hdr_maint_op_sub_class = (hdr).maint_op_sub_class; \
+	} while (0)
 
 #define CXL_EVT_TP_printk(fmt, ...) \
 	TP_printk("memdev=%s host=%s serial=%lld log=%s : time=%llu uuid=%pUb "	\
-- 
2.43.0

Re: [PATCH] [PATCH] cxl: trace: Fix macro safety in CXL_EVT_TP_fast_assign

Posted by Ira Weiny 7 months, 3 weeks ago

shaikh.kamal wrote:
> Fix checkpatch.pl detected error
> The CXL_EVT_TP_fast_assign macro assigns multiple fields, but does not
> wrap the body in a `do { ... } while (0)` block. This can lead to
> unexpected behavior when used in conditional branches.
> 
> Add checks to ensure cxlmd is valid before accessing its fields.
> 
> Signed-off-by: shaikh.kamal <shaikhkamal2012@gmail.com>

Nak

This is not a normal macro.  checkpatch.pl does not apply in this case.
Also checking cxlmd here is not necessary.  The callers of the tracepoint
know very well that it needs to be valid.

Ira

> ---
>  drivers/cxl/core/trace.h | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index 25ebfbc1616c..a1a1014ee1fe 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h
> @@ -249,18 +249,24 @@ TRACE_EVENT(cxl_overflow,
>  	__field(u8, hdr_maint_op_class)				\
>  	__field(u8, hdr_maint_op_sub_class)
>  
> -#define CXL_EVT_TP_fast_assign(cxlmd, l, hdr)					\
> -	__assign_str(memdev);				\
> -	__assign_str(host);			\
> -	__entry->log = (l);							\
> -	__entry->serial = (cxlmd)->cxlds->serial;				\
> -	__entry->hdr_length = (hdr).length;					\
> -	__entry->hdr_flags = get_unaligned_le24((hdr).flags);			\
> -	__entry->hdr_handle = le16_to_cpu((hdr).handle);			\
> -	__entry->hdr_related_handle = le16_to_cpu((hdr).related_handle);	\
> -	__entry->hdr_timestamp = le64_to_cpu((hdr).timestamp);			\
> -	__entry->hdr_maint_op_class = (hdr).maint_op_class;			\
> -	__entry->hdr_maint_op_sub_class = (hdr).maint_op_sub_class
> +#define CXL_EVT_TP_fast_assign(cxlmd, l, hdr) \
> +	do { \
> +		if (!(cxlmd)) { \
> +			pr_err("Invalid arguments to CXL_EVT_TP_fast_assign\n"); \
> +			break; \
> +		} \
> +		__assign_str(memdev); \
> +		__assign_str(host); \
> +		__entry->log = (l); \
> +		__entry->serial = (cxlmd)->cxlds->serial; \
> +		__entry->hdr_length = (hdr).length; \
> +		__entry->hdr_flags = get_unaligned_le24((hdr).flags); \
> +		__entry->hdr_handle = le16_to_cpu((hdr).handle); \
> +		__entry->hdr_related_handle = le16_to_cpu((hdr).related_handle); \
> +		__entry->hdr_timestamp = le64_to_cpu((hdr).timestamp); \
> +		__entry->hdr_maint_op_class = (hdr).maint_op_class; \
> +		__entry->hdr_maint_op_sub_class = (hdr).maint_op_sub_class; \
> +	} while (0)
>  
>  #define CXL_EVT_TP_printk(fmt, ...) \
>  	TP_printk("memdev=%s host=%s serial=%lld log=%s : time=%llu uuid=%pUb "	\
> -- 
> 2.43.0
>

Re: [PATCH] [PATCH] cxl: trace: Fix macro safety in CXL_EVT_TP_fast_assign

Posted by shaikh kamaluddin 7 months, 3 weeks ago

On 4/24/25 03:22, Ira Weiny wrote:
> shaikh.kamal wrote:
>> Fix checkpatch.pl detected error
>> The CXL_EVT_TP_fast_assign macro assigns multiple fields, but does not
>> wrap the body in a `do { ... } while (0)` block. This can lead to
>> unexpected behavior when used in conditional branches.
>>
>> Add checks to ensure cxlmd is valid before accessing its fields.
>>
>> Signed-off-by: shaikh.kamal <shaikhkamal2012@gmail.com>
> Nak
>
> This is not a normal macro.  checkpatch.pl does not apply in this case.
> Also checking cxlmd here is not necessary.  The callers of the tracepoint
> know very well that it needs to be valid.
>
> Ira
> Thanks for your comments.
>> ---
>>   drivers/cxl/core/trace.h | 30 ++++++++++++++++++------------
>>   1 file changed, 18 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
>> index 25ebfbc1616c..a1a1014ee1fe 100644
>> --- a/drivers/cxl/core/trace.h
>> +++ b/drivers/cxl/core/trace.h
>> @@ -249,18 +249,24 @@ TRACE_EVENT(cxl_overflow,
>>   	__field(u8, hdr_maint_op_class)				\
>>   	__field(u8, hdr_maint_op_sub_class)
>>   
>> -#define CXL_EVT_TP_fast_assign(cxlmd, l, hdr)					\
>> -	__assign_str(memdev);				\
>> -	__assign_str(host);			\
>> -	__entry->log = (l);							\
>> -	__entry->serial = (cxlmd)->cxlds->serial;				\
>> -	__entry->hdr_length = (hdr).length;					\
>> -	__entry->hdr_flags = get_unaligned_le24((hdr).flags);			\
>> -	__entry->hdr_handle = le16_to_cpu((hdr).handle);			\
>> -	__entry->hdr_related_handle = le16_to_cpu((hdr).related_handle);	\
>> -	__entry->hdr_timestamp = le64_to_cpu((hdr).timestamp);			\
>> -	__entry->hdr_maint_op_class = (hdr).maint_op_class;			\
>> -	__entry->hdr_maint_op_sub_class = (hdr).maint_op_sub_class
>> +#define CXL_EVT_TP_fast_assign(cxlmd, l, hdr) \
>> +	do { \
>> +		if (!(cxlmd)) { \
>> +			pr_err("Invalid arguments to CXL_EVT_TP_fast_assign\n"); \
>> +			break; \
>> +		} \
>> +		__assign_str(memdev); \
>> +		__assign_str(host); \
>> +		__entry->log = (l); \
>> +		__entry->serial = (cxlmd)->cxlds->serial; \
>> +		__entry->hdr_length = (hdr).length; \
>> +		__entry->hdr_flags = get_unaligned_le24((hdr).flags); \
>> +		__entry->hdr_handle = le16_to_cpu((hdr).handle); \
>> +		__entry->hdr_related_handle = le16_to_cpu((hdr).related_handle); \
>> +		__entry->hdr_timestamp = le64_to_cpu((hdr).timestamp); \
>> +		__entry->hdr_maint_op_class = (hdr).maint_op_class; \
>> +		__entry->hdr_maint_op_sub_class = (hdr).maint_op_sub_class; \
>> +	} while (0)
>>   
>>   #define CXL_EVT_TP_printk(fmt, ...) \
>>   	TP_printk("memdev=%s host=%s serial=%lld log=%s : time=%llu uuid=%pUb "	\
>> -- 
>> 2.43.0
>>
>

Re: [PATCH] [PATCH] cxl: trace: Fix macro safety in CXL_EVT_TP_fast_assign

Posted by Steven Rostedt 7 months, 3 weeks ago

On Tue, 22 Apr 2025 00:15:17 +0530
"shaikh.kamal" <shaikhkamal2012@gmail.com> wrote:

> Fix checkpatch.pl detected error

First, checkpatch.pl should never be used on existing code unless it's
yours. As the name suggests, it's for checking patches. It's not to check
what's already been accepted. Please do not submit patches against accepted
code because of what checkpatch.pl reports.

If you run it on code and it reports something that you find is a real bug,
then sure, fix it. But don't submit patches on code you do not understand
just because checkpatch.pl says there's an issue with it.

> The CXL_EVT_TP_fast_assign macro assigns multiple fields, but does not
> wrap the body in a `do { ... } while (0)` block. This can lead to
> unexpected behavior when used in conditional branches.

Next, this is not a normal macro. It's a trace event macro and
checkpatch.pl fails miserably on pretty much all tracing macros.

> 
> Add checks to ensure cxlmd is valid before accessing its fields.

If it is invalid, and we do what your patch suggests and just not write
anything, the event it was writing into has already been created. If we
exit out of this macro, then the event will simply contain garbage, but is
already on its way to user space. That means the output to user space will
be garbage. That's a bug. In other words, if cxlmd is NULL, it had better
not be calling this macro in the fist place!

-- Steve

Re: [PATCH] [PATCH] cxl: trace: Fix macro safety in CXL_EVT_TP_fast_assign

Posted by shaikh kamaluddin 7 months, 3 weeks ago

On 4/22/25 00:42, Steven Rostedt wrote:
> On Tue, 22 Apr 2025 00:15:17 +0530
> "shaikh.kamal" <shaikhkamal2012@gmail.com> wrote:
>
>> Fix checkpatch.pl detected error
> First, checkpatch.pl should never be used on existing code unless it's
> yours. As the name suggests, it's for checking patches. It's not to check
> what's already been accepted. Please do not submit patches against accepted
> code because of what checkpatch.pl reports.
Thanks for your suggestion.
>
> If you run it on code and it reports something that you find is a real bug,
> then sure, fix it. But don't submit patches on code you do not understand
> just because checkpatch.pl says there's an issue with it.
Okay got it.
>
>> The CXL_EVT_TP_fast_assign macro assigns multiple fields, but does not
>> wrap the body in a `do { ... } while (0)` block. This can lead to
>> unexpected behavior when used in conditional branches.
> Next, this is not a normal macro. It's a trace event macro and
> checkpatch.pl fails miserably on pretty much all tracing macros.
>
> Thanks for your comments.
>
>> Add checks to ensure cxlmd is valid before accessing its fields.
> If it is invalid, and we do what your patch suggests and just not write
> anything, the event it was writing into has already been created. If we
> exit out of this macro, then the event will simply contain garbage, but is
> already on its way to user space. That means the output to user space will
> be garbage. That's a bug. In other words, if cxlmd is NULL, it had better
> not be calling this macro in the fist place!
>
> -- Steve
> Thanks for your suggestions and comments.