[PATCH] SUNRPC: Fixup gss_status tracepoint error output

[PATCH] SUNRPC: Fixup gss_status tracepoint error output

[Benjamin Coddington]

The GSS routine errors are values, not flags.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 include/trace/events/rpcgss.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/trace/events/rpcgss.h b/include/trace/events/rpcgss.h
index 7f0c1ceae726..b0b6300a0cab 100644
--- a/include/trace/events/rpcgss.h
+++ b/include/trace/events/rpcgss.h
@@ -54,7 +54,7 @@ TRACE_DEFINE_ENUM(GSS_S_UNSEQ_TOKEN);
 TRACE_DEFINE_ENUM(GSS_S_GAP_TOKEN);
 
 #define show_gss_status(x)						\
-	__print_flags(x, "|",						\
+	__print_symbolic(x, 						\
 		{ GSS_S_BAD_MECH, "GSS_S_BAD_MECH" },			\
 		{ GSS_S_BAD_NAME, "GSS_S_BAD_NAME" },			\
 		{ GSS_S_BAD_NAMETYPE, "GSS_S_BAD_NAMETYPE" },		\
-- 
2.44.0

Re: [PATCH] SUNRPC: Fixup gss_status tracepoint error output

[Chuck Lever]

On Thu, Jul 11, 2024 at 11:24:01AM -0400, Benjamin Coddington wrote:
> The GSS routine errors are values, not flags.

My reading of kernel and user space GSS code is that these are
indeed flags and can be combined. The definitions are found in
include/linux/sunrpc/gss_err.h:

To wit:

116 /*                                                                              
117  * Routine errors:                                                              
118  */                                                                             
119 #define GSS_S_BAD_MECH (((OM_uint32) 1ul) << GSS_C_ROUTINE_ERROR_OFFSET)        
120 #define GSS_S_BAD_NAME (((OM_uint32) 2ul) << GSS_C_ROUTINE_ERROR_OFFSET)  

 ....


> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  include/trace/events/rpcgss.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/trace/events/rpcgss.h b/include/trace/events/rpcgss.h
> index 7f0c1ceae726..b0b6300a0cab 100644
> --- a/include/trace/events/rpcgss.h
> +++ b/include/trace/events/rpcgss.h
> @@ -54,7 +54,7 @@ TRACE_DEFINE_ENUM(GSS_S_UNSEQ_TOKEN);
>  TRACE_DEFINE_ENUM(GSS_S_GAP_TOKEN);
>  
>  #define show_gss_status(x)						\
> -	__print_flags(x, "|",						\
> +	__print_symbolic(x, 						\
>  		{ GSS_S_BAD_MECH, "GSS_S_BAD_MECH" },			\
>  		{ GSS_S_BAD_NAME, "GSS_S_BAD_NAME" },			\
>  		{ GSS_S_BAD_NAMETYPE, "GSS_S_BAD_NAMETYPE" },		\
> -- 
> 2.44.0
> 
> 

-- 
Chuck Lever

Re: [PATCH] SUNRPC: Fixup gss_status tracepoint error output

[Chuck Lever III]

> On Jul 11, 2024, at 11:28 AM, Chuck Lever <chuck.lever@oracle.com> wrote:
> 
> On Thu, Jul 11, 2024 at 11:24:01AM -0400, Benjamin Coddington wrote:
>> The GSS routine errors are values, not flags.

As always, I meant to precede this remark with "I could be wrong, but ..."


> My reading of kernel and user space GSS code is that these are
> indeed flags and can be combined. The definitions are found in
> include/linux/sunrpc/gss_err.h:
> 
> To wit:
> 
> 116 /*                                                                              
> 117  * Routine errors:                                                              
> 118  */                                                                             
> 119 #define GSS_S_BAD_MECH (((OM_uint32) 1ul) << GSS_C_ROUTINE_ERROR_OFFSET)        
> 120 #define GSS_S_BAD_NAME (((OM_uint32) 2ul) << GSS_C_ROUTINE_ERROR_OFFSET)  
> 
> ....


--
Chuck Lever

Re: [PATCH] SUNRPC: Fixup gss_status tracepoint error output

[Benjamin Coddington]

On 11 Jul 2024, at 11:28, Chuck Lever wrote:

> On Thu, Jul 11, 2024 at 11:24:01AM -0400, Benjamin Coddington wrote:
>> The GSS routine errors are values, not flags.
>
> My reading of kernel and user space GSS code is that these are
> indeed flags and can be combined. The definitions are found in
> include/linux/sunrpc/gss_err.h:
>
> To wit:
>
> 116 /*
> 117  * Routine errors:
> 118  */
> 119 #define GSS_S_BAD_MECH (((OM_uint32) 1ul) << GSS_C_ROUTINE_ERROR_OFFSET)
> 120 #define GSS_S_BAD_NAME (((OM_uint32) 2ul) << GSS_C_ROUTINE_ERROR_OFFSET)

I read this as just values shifted left by a constant.

No where in-kernel are they bitwise combined.  I noticed this problem in practice
while reading the tracepoint output from corrupted GSS hash routines.

Ben

Re: [PATCH] SUNRPC: Fixup gss_status tracepoint error output

[Chuck Lever III]

> On Jul 11, 2024, at 11:48 AM, Benjamin Coddington <bcodding@redhat.com> wrote:
> 
> On 11 Jul 2024, at 11:28, Chuck Lever wrote:
> 
>> On Thu, Jul 11, 2024 at 11:24:01AM -0400, Benjamin Coddington wrote:
>>> The GSS routine errors are values, not flags.
>> 
>> My reading of kernel and user space GSS code is that these are
>> indeed flags and can be combined. The definitions are found in
>> include/linux/sunrpc/gss_err.h:
>> 
>> To wit:
>> 
>> 116 /*
>> 117  * Routine errors:
>> 118  */
>> 119 #define GSS_S_BAD_MECH (((OM_uint32) 1ul) << GSS_C_ROUTINE_ERROR_OFFSET)
>> 120 #define GSS_S_BAD_NAME (((OM_uint32) 2ul) << GSS_C_ROUTINE_ERROR_OFFSET)
> 
> I read this as just values shifted left by a constant.
> 
> No where in-kernel are they bitwise combined.

The kernel gets GSS status values from user space code too.


> I noticed this problem in practice
> while reading the tracepoint output from corrupted GSS hash routines.

Can you describe the problem?


--
Chuck Lever

Re: [PATCH] SUNRPC: Fixup gss_status tracepoint error output

[Benjamin Coddington]

On 11 Jul 2024, at 11:52, Chuck Lever III wrote:

>> On Jul 11, 2024, at 11:48 AM, Benjamin Coddington <bcodding@redhat.com> wrote:
>>
>> On 11 Jul 2024, at 11:28, Chuck Lever wrote:
>>
>>> On Thu, Jul 11, 2024 at 11:24:01AM -0400, Benjamin Coddington wrote:
>>>> The GSS routine errors are values, not flags.
>>>
>>> My reading of kernel and user space GSS code is that these are
>>> indeed flags and can be combined. The definitions are found in
>>> include/linux/sunrpc/gss_err.h:
>>>
>>> To wit:
>>>
>>> 116 /*
>>> 117  * Routine errors:
>>> 118  */
>>> 119 #define GSS_S_BAD_MECH (((OM_uint32) 1ul) << GSS_C_ROUTINE_ERROR_OFFSET)
>>> 120 #define GSS_S_BAD_NAME (((OM_uint32) 2ul) << GSS_C_ROUTINE_ERROR_OFFSET)
>>
>> I read this as just values shifted left by a constant.
>>
>> No where in-kernel are they bitwise combined.
>
> The kernel gets GSS status values from user space code too.
>
>
>> I noticed this problem in practice
>> while reading the tracepoint output from corrupted GSS hash routines.
>
> Can you describe the problem?

It was a week ago or so, and I don't have the test setup any longer, but the
tracepoint would not print the actual error returned, rather the bitwise
combination of that error.

Look closer at the values - it makes no sense that these are bits, else
GSS_S_BAD_NAMETYPE is the same as GSS_S_BAD_MECH|GSS_S_BAD_NAME.

Ben

Re: [PATCH] SUNRPC: Fixup gss_status tracepoint error output

[Chuck Lever III]

> On Jul 11, 2024, at 12:00 PM, Benjamin Coddington <bcodding@redhat.com> wrote:
> 
> On 11 Jul 2024, at 11:52, Chuck Lever III wrote:
> 
>>> On Jul 11, 2024, at 11:48 AM, Benjamin Coddington <bcodding@redhat.com> wrote:
>>> 
>>> On 11 Jul 2024, at 11:28, Chuck Lever wrote:
>>> 
>>>> On Thu, Jul 11, 2024 at 11:24:01AM -0400, Benjamin Coddington wrote:
>>>>> The GSS routine errors are values, not flags.
>>>> 
>>>> My reading of kernel and user space GSS code is that these are
>>>> indeed flags and can be combined. The definitions are found in
>>>> include/linux/sunrpc/gss_err.h:
>>>> 
>>>> To wit:
>>>> 
>>>> 116 /*
>>>> 117  * Routine errors:
>>>> 118  */
>>>> 119 #define GSS_S_BAD_MECH (((OM_uint32) 1ul) << GSS_C_ROUTINE_ERROR_OFFSET)
>>>> 120 #define GSS_S_BAD_NAME (((OM_uint32) 2ul) << GSS_C_ROUTINE_ERROR_OFFSET)
>>> 
>>> I read this as just values shifted left by a constant.
>>> 
>>> No where in-kernel are they bitwise combined.
>> 
>> The kernel gets GSS status values from user space code too.
>> 
>> 
>>> I noticed this problem in practice
>>> while reading the tracepoint output from corrupted GSS hash routines.
>> 
>> Can you describe the problem?
> 
> It was a week ago or so, and I don't have the test setup any longer, but the
> tracepoint would not print the actual error returned, rather the bitwise
> combination of that error.
> 
> Look closer at the values - it makes no sense that these are bits, else
> GSS_S_BAD_NAMETYPE is the same as GSS_S_BAD_MECH|GSS_S_BAD_NAME.

Understood. Please add:

Fixes: 0c77668ddb4e ("SUNRPC: Introduce trace points in rpc_auth_gss.ko")
Reviewed-by: Chuck Lever <chuck.lever@oracle.com>


--
Chuck Lever

Re: [PATCH] SUNRPC: Fixup gss_status tracepoint error output

[Jeff Layton]

On Thu, 2024-07-11 at 11:24 -0400, Benjamin Coddington wrote:
> The GSS routine errors are values, not flags.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  include/trace/events/rpcgss.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/trace/events/rpcgss.h b/include/trace/events/rpcgss.h
> index 7f0c1ceae726..b0b6300a0cab 100644
> --- a/include/trace/events/rpcgss.h
> +++ b/include/trace/events/rpcgss.h
> @@ -54,7 +54,7 @@ TRACE_DEFINE_ENUM(GSS_S_UNSEQ_TOKEN);
>  TRACE_DEFINE_ENUM(GSS_S_GAP_TOKEN);
>  
>  #define show_gss_status(x)						\
> -	__print_flags(x, "|",						\
> +	__print_symbolic(x, 						\
>  		{ GSS_S_BAD_MECH, "GSS_S_BAD_MECH" },			\
>  		{ GSS_S_BAD_NAME, "GSS_S_BAD_NAME" },			\
>  		{ GSS_S_BAD_NAMETYPE, "GSS_S_BAD_NAMETYPE" },		\

Reviewed-by: Jeff Layton <jlayton@kernel.org>