| Message ID | 27526e921037d6217bdfc6a078c53d37ae9effab.1720711381.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | SUNRPC: Fixup gss_status tracepoint error output | expand |
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 > >
> 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
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
> 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
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
> 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
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>
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" }, \
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(-)