[PATCH v2] SUNRPC: Check if we need to recalculate slack estimates

[PATCH v2] SUNRPC: Check if we need to recalculate slack estimates

[Scott Mayhew]

If the incoming GSS verifier is larger than what we previously recorded
on the gss_auth, that would indicate the GSS cred/context used for that
RPC is using a different enctype than the one used by the machine
cred/context, and we should recalculate the slack variables accordingly.

Link: https://bugs.debian.org/1120598
Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 net/sunrpc/auth_gss/auth_gss.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 5c095cb8cb20..bff5f10581a2 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -1721,6 +1721,18 @@ gss_validate(struct rpc_task *task, struct xdr_stream *xdr)
 	if (maj_stat)
 		goto bad_mic;
 
+	/*
+	 * Normally we only recalculate the slack variables once after
+	 * creating a new gss_auth, but we should also do it if the incoming
+	 * verifier has a larger size than what was previously recorded.
+	 * When the incoming verifier is larger than expected, the
+	 * GSS context is using a different enctype than the one used
+	 * initially by the machine credential. Force a slack size update
+	 * to maintain good payload alignment.
+	 */
+	if (cred->cr_auth->au_verfsize < (XDR_QUADLEN(len) + 2))
+		__set_bit(RPCAUTH_AUTH_UPDATE_SLACK, &cred->cr_auth->au_flags);
+
 	/* We leave it to unwrap to calculate au_rslack. For now we just
 	 * calculate the length of the verifier: */
 	if (test_bit(RPCAUTH_AUTH_UPDATE_SLACK, &cred->cr_auth->au_flags))
-- 
2.51.0

Re: [PATCH v2] SUNRPC: Check if we need to recalculate slack estimates

[Chuck Lever]

On 11/20/25 7:12 AM, Scott Mayhew wrote:
> If the incoming GSS verifier is larger than what we previously recorded
> on the gss_auth, that would indicate the GSS cred/context used for that
> RPC is using a different enctype than the one used by the machine
> cred/context, and we should recalculate the slack variables accordingly.
> 
> Link: https://bugs.debian.org/1120598

Since there is a bug link, a Fixes: tag is recommended.


> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
>  net/sunrpc/auth_gss/auth_gss.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> index 5c095cb8cb20..bff5f10581a2 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -1721,6 +1721,18 @@ gss_validate(struct rpc_task *task, struct xdr_stream *xdr)
>  	if (maj_stat)
>  		goto bad_mic;
>  
> +	/*
> +	 * Normally we only recalculate the slack variables once after
> +	 * creating a new gss_auth, but we should also do it if the incoming
> +	 * verifier has a larger size than what was previously recorded.
> +	 * When the incoming verifier is larger than expected, the
> +	 * GSS context is using a different enctype than the one used
> +	 * initially by the machine credential. Force a slack size update
> +	 * to maintain good payload alignment.
> +	 */
> +	if (cred->cr_auth->au_verfsize < (XDR_QUADLEN(len) + 2))
> +		__set_bit(RPCAUTH_AUTH_UPDATE_SLACK, &cred->cr_auth->au_flags);

set_bit() rather than __set_bit is a better choice for a lockless update
where multiple concurrent threads can have access to the flags field.


> +
>  	/* We leave it to unwrap to calculate au_rslack. For now we just
>  	 * calculate the length of the verifier: */
>  	if (test_bit(RPCAUTH_AUTH_UPDATE_SLACK, &cred->cr_auth->au_flags))

Thanks for pursuing this one, Scott.

Reviewed-by: Chuck Lever <chuck.lever@oracle.com>


-- 
Chuck Lever

Re: [PATCH v2] SUNRPC: Check if we need to recalculate slack estimates

[Jeff Layton]

On Thu, 2025-11-20 at 08:44 -0500, Chuck Lever wrote:
> On 11/20/25 7:12 AM, Scott Mayhew wrote:
> > If the incoming GSS verifier is larger than what we previously recorded
> > on the gss_auth, that would indicate the GSS cred/context used for that
> > RPC is using a different enctype than the one used by the machine
> > cred/context, and we should recalculate the slack variables accordingly.
> > 
> > Link: https://bugs.debian.org/1120598
> 
> Since there is a bug link, a Fixes: tag is recommended.
> 
> 
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> >  net/sunrpc/auth_gss/auth_gss.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> > index 5c095cb8cb20..bff5f10581a2 100644
> > --- a/net/sunrpc/auth_gss/auth_gss.c
> > +++ b/net/sunrpc/auth_gss/auth_gss.c
> > @@ -1721,6 +1721,18 @@ gss_validate(struct rpc_task *task, struct xdr_stream *xdr)
> >  	if (maj_stat)
> >  		goto bad_mic;
> >  
> > +	/*
> > +	 * Normally we only recalculate the slack variables once after
> > +	 * creating a new gss_auth, but we should also do it if the incoming
> > +	 * verifier has a larger size than what was previously recorded.
> > +	 * When the incoming verifier is larger than expected, the
> > +	 * GSS context is using a different enctype than the one used
> > +	 * initially by the machine credential. Force a slack size update
> > +	 * to maintain good payload alignment.
> > +	 */
> > +	if (cred->cr_auth->au_verfsize < (XDR_QUADLEN(len) + 2))
> > +		__set_bit(RPCAUTH_AUTH_UPDATE_SLACK, &cred->cr_auth->au_flags);
> 
> set_bit() rather than __set_bit is a better choice for a lockless update
> where multiple concurrent threads can have access to the flags field.
> 
> 

This function tests for that flag just below though. Could another task
do the test_and_clear_bit() in gss_update_rslack(), such that the
au_verfsize doesn't get updated below ?

If that is a possibility, maybe you should update the au_verfsize
first, and then the flag just afterward (with a barrier between).

> > +
> >  	/* We leave it to unwrap to calculate au_rslack. For now we just
> >  	 * calculate the length of the verifier: */
> >  	if (test_bit(RPCAUTH_AUTH_UPDATE_SLACK, &cred->cr_auth->au_flags))
> 
> Thanks for pursuing this one, Scott.
> 
> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2] SUNRPC: Check if we need to recalculate slack estimates

[Scott Mayhew]

On Thu, 20 Nov 2025, Jeff Layton wrote:

> On Thu, 2025-11-20 at 08:44 -0500, Chuck Lever wrote:
> > On 11/20/25 7:12 AM, Scott Mayhew wrote:
> > > If the incoming GSS verifier is larger than what we previously recorded
> > > on the gss_auth, that would indicate the GSS cred/context used for that
> > > RPC is using a different enctype than the one used by the machine
> > > cred/context, and we should recalculate the slack variables accordingly.
> > > 
> > > Link: https://bugs.debian.org/1120598
> > 
> > Since there is a bug link, a Fixes: tag is recommended.
> > 
> > 
> > > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > > ---
> > >  net/sunrpc/auth_gss/auth_gss.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> > > index 5c095cb8cb20..bff5f10581a2 100644
> > > --- a/net/sunrpc/auth_gss/auth_gss.c
> > > +++ b/net/sunrpc/auth_gss/auth_gss.c
> > > @@ -1721,6 +1721,18 @@ gss_validate(struct rpc_task *task, struct xdr_stream *xdr)
> > >  	if (maj_stat)
> > >  		goto bad_mic;
> > >  
> > > +	/*
> > > +	 * Normally we only recalculate the slack variables once after
> > > +	 * creating a new gss_auth, but we should also do it if the incoming
> > > +	 * verifier has a larger size than what was previously recorded.
> > > +	 * When the incoming verifier is larger than expected, the
> > > +	 * GSS context is using a different enctype than the one used
> > > +	 * initially by the machine credential. Force a slack size update
> > > +	 * to maintain good payload alignment.
> > > +	 */
> > > +	if (cred->cr_auth->au_verfsize < (XDR_QUADLEN(len) + 2))
> > > +		__set_bit(RPCAUTH_AUTH_UPDATE_SLACK, &cred->cr_auth->au_flags);
> > 
> > set_bit() rather than __set_bit is a better choice for a lockless update
> > where multiple concurrent threads can have access to the flags field.
> > 
> > 
> 
> This function tests for that flag just below though. Could another task
> do the test_and_clear_bit() in gss_update_rslack(), such that the
> au_verfsize doesn't get updated below ?
> 
> If that is a possibility, maybe you should update the au_verfsize
> first, and then the flag just afterward (with a barrier between).

Yes, I suppose that is a possibility.  But in that case we are pretty
much screwed (at least in the case of krb5i and krb5p) because it's
likely that the fields used to generate the 'before' and 'after' values
passed gss_update_rslack() don't coincide with the verifier anyways.

IOW if we're going to recalculate rslack and ralign based on a larger
verifier, then it pretty much has to be done by the same task that sets
the flag in the first place.

> 
> > > +
> > >  	/* We leave it to unwrap to calculate au_rslack. For now we just
> > >  	 * calculate the length of the verifier: */
> > >  	if (test_bit(RPCAUTH_AUTH_UPDATE_SLACK, &cred->cr_auth->au_flags))
> > 
> > Thanks for pursuing this one, Scott.
> > 
> > Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
> > 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
> 

Re: [PATCH v2] SUNRPC: Check if we need to recalculate slack estimates

[Trond Myklebust]

Hi Scott,

On Thu, 2025-11-20 at 07:12 -0500, Scott Mayhew wrote:
> If the incoming GSS verifier is larger than what we previously
> recorded
> on the gss_auth, that would indicate the GSS cred/context used for
> that
> RPC is using a different enctype than the one used by the machine
> cred/context, and we should recalculate the slack variables
> accordingly.
> 
> Link: https://bugs.debian.org/1120598
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
>  net/sunrpc/auth_gss/auth_gss.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/net/sunrpc/auth_gss/auth_gss.c
> b/net/sunrpc/auth_gss/auth_gss.c
> index 5c095cb8cb20..bff5f10581a2 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -1721,6 +1721,18 @@ gss_validate(struct rpc_task *task, struct
> xdr_stream *xdr)
>  	if (maj_stat)
>  		goto bad_mic;
>  
> +	/*
> +	 * Normally we only recalculate the slack variables once
> after
> +	 * creating a new gss_auth, but we should also do it if the
> incoming
> +	 * verifier has a larger size than what was previously
> recorded.
> +	 * When the incoming verifier is larger than expected, the
> +	 * GSS context is using a different enctype than the one
> used
> +	 * initially by the machine credential. Force a slack size
> update
> +	 * to maintain good payload alignment.
> +	 */
> +	if (cred->cr_auth->au_verfsize < (XDR_QUADLEN(len) + 2))
> +		__set_bit(RPCAUTH_AUTH_UPDATE_SLACK, &cred->cr_auth-
> >au_flags);
> +
>  	/* We leave it to unwrap to calculate au_rslack. For now we
> just
>  	 * calculate the length of the verifier: */
>  	if (test_bit(RPCAUTH_AUTH_UPDATE_SLACK, &cred->cr_auth-
> >au_flags))

What's the status here? Are you planning to put out a new version with
the non-atomic __set_bit() -> atomic set_bit() change?

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trondmy@kernel.org, trond.myklebust@hammerspace.com

Re: [PATCH v2] SUNRPC: Check if we need to recalculate slack estimates

[Scott Mayhew]

On Wed, 03 Dec 2025, Trond Myklebust wrote:

> Hi Scott,
> 
> On Thu, 2025-11-20 at 07:12 -0500, Scott Mayhew wrote:
> > If the incoming GSS verifier is larger than what we previously
> > recorded
> > on the gss_auth, that would indicate the GSS cred/context used for
> > that
> > RPC is using a different enctype than the one used by the machine
> > cred/context, and we should recalculate the slack variables
> > accordingly.
> > 
> > Link: https://bugs.debian.org/1120598
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> >  net/sunrpc/auth_gss/auth_gss.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/net/sunrpc/auth_gss/auth_gss.c
> > b/net/sunrpc/auth_gss/auth_gss.c
> > index 5c095cb8cb20..bff5f10581a2 100644
> > --- a/net/sunrpc/auth_gss/auth_gss.c
> > +++ b/net/sunrpc/auth_gss/auth_gss.c
> > @@ -1721,6 +1721,18 @@ gss_validate(struct rpc_task *task, struct
> > xdr_stream *xdr)
> >  	if (maj_stat)
> >  		goto bad_mic;
> >  
> > +	/*
> > +	 * Normally we only recalculate the slack variables once
> > after
> > +	 * creating a new gss_auth, but we should also do it if the
> > incoming
> > +	 * verifier has a larger size than what was previously
> > recorded.
> > +	 * When the incoming verifier is larger than expected, the
> > +	 * GSS context is using a different enctype than the one
> > used
> > +	 * initially by the machine credential. Force a slack size
> > update
> > +	 * to maintain good payload alignment.
> > +	 */
> > +	if (cred->cr_auth->au_verfsize < (XDR_QUADLEN(len) + 2))
> > +		__set_bit(RPCAUTH_AUTH_UPDATE_SLACK, &cred->cr_auth-
> > >au_flags);
> > +
> >  	/* We leave it to unwrap to calculate au_rslack. For now we
> > just
> >  	 * calculate the length of the verifier: */
> >  	if (test_bit(RPCAUTH_AUTH_UPDATE_SLACK, &cred->cr_auth-
> > >au_flags))
> 
> What's the status here? Are you planning to put out a new version with
> the non-atomic __set_bit() -> atomic set_bit() change?

No.  After discussing with Chuck and Jeff I'm not sure this is the
right approach.

I was under the impression that the slack and ralign values were more
like estimates and we could afford to be conservative, i.e. I was
thinking that as long as we were accommodating the enctype with the
largest space requirements then we'd be okay.  But if that's not the
case, then  updating the values when a user cred is using a SHA2
enctype would mean the values are incorrect if the machine cred is using
a SHA1 enctype.

Maybe we should instead just emit some sort of a warning when we
encounter a verifier with a different size that what we previously
recorded on the auth handle?

> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trondmy@kernel.org, trond.myklebust@hammerspace.com
> 

Re: [PATCH v2] SUNRPC: Check if we need to recalculate slack estimates

[Chuck Lever]

On 12/4/25 8:53 AM, Scott Mayhew wrote:
> On Wed, 03 Dec 2025, Trond Myklebust wrote:
> 
>> Hi Scott,
>>
>> On Thu, 2025-11-20 at 07:12 -0500, Scott Mayhew wrote:
>>> If the incoming GSS verifier is larger than what we previously
>>> recorded
>>> on the gss_auth, that would indicate the GSS cred/context used for
>>> that
>>> RPC is using a different enctype than the one used by the machine
>>> cred/context, and we should recalculate the slack variables
>>> accordingly.
>>>
>>> Link: https://bugs.debian.org/1120598
>>> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
>>> ---
>>>  net/sunrpc/auth_gss/auth_gss.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/net/sunrpc/auth_gss/auth_gss.c
>>> b/net/sunrpc/auth_gss/auth_gss.c
>>> index 5c095cb8cb20..bff5f10581a2 100644
>>> --- a/net/sunrpc/auth_gss/auth_gss.c
>>> +++ b/net/sunrpc/auth_gss/auth_gss.c
>>> @@ -1721,6 +1721,18 @@ gss_validate(struct rpc_task *task, struct
>>> xdr_stream *xdr)
>>>  	if (maj_stat)
>>>  		goto bad_mic;
>>>  
>>> +	/*
>>> +	 * Normally we only recalculate the slack variables once
>>> after
>>> +	 * creating a new gss_auth, but we should also do it if the
>>> incoming
>>> +	 * verifier has a larger size than what was previously
>>> recorded.
>>> +	 * When the incoming verifier is larger than expected, the
>>> +	 * GSS context is using a different enctype than the one
>>> used
>>> +	 * initially by the machine credential. Force a slack size
>>> update
>>> +	 * to maintain good payload alignment.
>>> +	 */
>>> +	if (cred->cr_auth->au_verfsize < (XDR_QUADLEN(len) + 2))
>>> +		__set_bit(RPCAUTH_AUTH_UPDATE_SLACK, &cred->cr_auth-
>>>> au_flags);
>>> +
>>>  	/* We leave it to unwrap to calculate au_rslack. For now we
>>> just
>>>  	 * calculate the length of the verifier: */
>>>  	if (test_bit(RPCAUTH_AUTH_UPDATE_SLACK, &cred->cr_auth-
>>>> au_flags))
>>
>> What's the status here? Are you planning to put out a new version with
>> the non-atomic __set_bit() -> atomic set_bit() change?
> 
> No.  After discussing with Chuck and Jeff I'm not sure this is the
> right approach.

We were discussing the issue of how to protect the update in the slack
value.

Scott observed that, with RPC_AUTH_GSS, multiple enctypes share the same
rpc_auth. Slack values are stored in the rpc_auth. The SHA-1 and SHA-2
based enctypes might have different slack values. For one set of
enctypes sharing that rpc_auth, the slack values will then always be
wrong.

So perhaps a better solution to this specific issue is to move
the slack values for RPC_AUTH_GSS into the gss_cred? </hand_wave>


> I was under the impression that the slack and ralign values were more
> like estimates and we could afford to be conservative, i.e. I was
> thinking that as long as we were accommodating the enctype with the
> largest space requirements then we'd be okay.  But if that's not the
> case, then  updating the values when a user cred is using a SHA2
> enctype would mean the values are incorrect if the machine cred is using
> a SHA1 enctype.
> 
> Maybe we should instead just emit some sort of a warning when we
> encounter a verifier with a different size that what we previously
> recorded on the auth handle?

-- 
Chuck Lever