From: "J. Bruce Fields" <[email protected]>
If sd_max is unsigned, then sd_max - GSS_SEQ_WIN is a very large number
whenever sd_max is less than GSS_SEQ_WIN, and the comparison:
seq_num <= sd->sd_max - GSS_SEQ_WIN
in gss_check_seq_num is pretty much always true, even when that's
clearly not what was intended.
This was causing pynfs to hang when using krb5, because pynfs uses zero
as the initial gss sequence number. That's perfectly legal, but this
logic error causes knfsd to drop the rpc in that case. Out-of-order
sequence IDs in the first GSS_SEQ_WIN (128) calls will also cause this.
Fixes: 10b9d99a3dbb ("SUNRPC: Augment server-side rpcgss tracepoints")
Cc: [email protected]
Signed-off-by: J. Bruce Fields <[email protected]>
---
net/sunrpc/auth_gss/svcauth_gss.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
index 7dba6a9c213a..b87565b64928 100644
--- a/net/sunrpc/auth_gss/svcauth_gss.c
+++ b/net/sunrpc/auth_gss/svcauth_gss.c
@@ -645,7 +645,7 @@ static bool gss_check_seq_num(const struct svc_rqst *rqstp, struct rsc *rsci,
}
__set_bit(seq_num % GSS_SEQ_WIN, sd->sd_win);
goto ok;
- } else if (seq_num <= sd->sd_max - GSS_SEQ_WIN) {
+ } else if (seq_num + GSS_SEQ_WIN <= sd->sd_max) {
goto toolow;
}
if (__test_and_set_bit(seq_num % GSS_SEQ_WIN, sd->sd_win))
--
2.31.1
> On Oct 1, 2021, at 9:59 AM, J. Bruce Fields <[email protected]> wrote:
>
> From: "J. Bruce Fields" <[email protected]>
>
> If sd_max is unsigned, then sd_max - GSS_SEQ_WIN is a very large number
> whenever sd_max is less than GSS_SEQ_WIN, and the comparison:
>
> seq_num <= sd->sd_max - GSS_SEQ_WIN
>
> in gss_check_seq_num is pretty much always true, even when that's
> clearly not what was intended.
>
> This was causing pynfs to hang when using krb5, because pynfs uses zero
> as the initial gss sequence number. That's perfectly legal, but this
> logic error causes knfsd to drop the rpc in that case. Out-of-order
> sequence IDs in the first GSS_SEQ_WIN (128) calls will also cause this.
>
> Fixes: 10b9d99a3dbb ("SUNRPC: Augment server-side rpcgss tracepoints")
> Cc: [email protected]
> Signed-off-by: J. Bruce Fields <[email protected]>
This will be included in the next NFSD 5.15-rc. Thanks!
See the for-next branch at
git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
> ---
> net/sunrpc/auth_gss/svcauth_gss.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 7dba6a9c213a..b87565b64928 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -645,7 +645,7 @@ static bool gss_check_seq_num(const struct svc_rqst *rqstp, struct rsc *rsci,
> }
> __set_bit(seq_num % GSS_SEQ_WIN, sd->sd_win);
> goto ok;
> - } else if (seq_num <= sd->sd_max - GSS_SEQ_WIN) {
> + } else if (seq_num + GSS_SEQ_WIN <= sd->sd_max) {
> goto toolow;
> }
> if (__test_and_set_bit(seq_num % GSS_SEQ_WIN, sd->sd_win))
> --
> 2.31.1
>
--
Chuck Lever
Hi Bruce!
Am 01.10.21 um 15:59 schrieb J. Bruce Fields:
> From: "J. Bruce Fields" <[email protected]>
>
> If sd_max is unsigned, then sd_max - GSS_SEQ_WIN is a very large number
> whenever sd_max is less than GSS_SEQ_WIN, and the comparison:
>
> seq_num <= sd->sd_max - GSS_SEQ_WIN
>
> in gss_check_seq_num is pretty much always true, even when that's
> clearly not what was intended.
>
> This was causing pynfs to hang when using krb5, because pynfs uses zero
> as the initial gss sequence number. That's perfectly legal, but this
> logic error causes knfsd to drop the rpc in that case. Out-of-order
> sequence IDs in the first GSS_SEQ_WIN (128) calls will also cause this.
>
> Fixes: 10b9d99a3dbb ("SUNRPC: Augment server-side rpcgss tracepoints")
I wonder about the Fixes tag: That changeset added tracepoints to the
exit path, but the buggy logic seems to have been present since the
pre-git ages. Or am I missing something about 10b9d99a3dbb? (This might
explain some reports of--as you stated elsewhere--"once in a blue moon
my krb5 mounts hang" we've investigated, albeit on kernels that predate
10b9d99a3dbb.)
Kind regards,
Daniel
> Cc: [email protected]
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> net/sunrpc/auth_gss/svcauth_gss.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> index 7dba6a9c213a..b87565b64928 100644
> --- a/net/sunrpc/auth_gss/svcauth_gss.c
> +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> @@ -645,7 +645,7 @@ static bool gss_check_seq_num(const struct svc_rqst *rqstp, struct rsc *rsci,
> }
> __set_bit(seq_num % GSS_SEQ_WIN, sd->sd_win);
> goto ok;
> - } else if (seq_num <= sd->sd_max - GSS_SEQ_WIN) {
> + } else if (seq_num + GSS_SEQ_WIN <= sd->sd_max) {
> goto toolow;
> }
> if (__test_and_set_bit(seq_num % GSS_SEQ_WIN, sd->sd_win))
>
--
Daniel Kobras
Principal Architect
Puzzle ITC Deutschland
+49 7071 14316 0
http://www.puzzle-itc.de
--
Puzzle ITC Deutschland GmbH
Sitz der Gesellschaft: Eisenbahnstraße 1, 72072
Tübingen
Eingetragen am Amtsgericht Stuttgart HRB 765802
Geschäftsführer:
Lukas Kallies, Daniel Kobras, Mark Pröhl
On Fri, Oct 01, 2021 at 06:50:12PM +0200, Daniel Kobras wrote:
> Hi Bruce!
>
> Am 01.10.21 um 15:59 schrieb J. Bruce Fields:
> > From: "J. Bruce Fields" <[email protected]>
> >
> > If sd_max is unsigned, then sd_max - GSS_SEQ_WIN is a very large number
> > whenever sd_max is less than GSS_SEQ_WIN, and the comparison:
> >
> > seq_num <= sd->sd_max - GSS_SEQ_WIN
> >
> > in gss_check_seq_num is pretty much always true, even when that's
> > clearly not what was intended.
> >
> > This was causing pynfs to hang when using krb5, because pynfs uses zero
> > as the initial gss sequence number. That's perfectly legal, but this
> > logic error causes knfsd to drop the rpc in that case. Out-of-order
> > sequence IDs in the first GSS_SEQ_WIN (128) calls will also cause this.
> >
> > Fixes: 10b9d99a3dbb ("SUNRPC: Augment server-side rpcgss tracepoints")
>
> I wonder about the Fixes tag: That changeset added tracepoints to the
> exit path, but the buggy logic seems to have been present since the
> pre-git ages. Or am I missing something about 10b9d99a3dbb?
The relevant parts of 10b9d99a3dbb were:
struct gss_svc_seq_data {
/* highest seq number seen so far: */
- int sd_max;
+ u32 sd_max;
and
-gss_check_seq_num(struct rsc *rsci, int seq_num)
+static bool gss_check_seq_num(const struct svc_rqst *rqstp, struct rsc *rsci,
+ u32 seq_num)
Together, they mean the comparison
seq_num <= sd->sd_max - GSS_SEQ_WIN
in the case sd_max is zero, effectively ends up being
seq_num <= 4294967168
instead of what was intended,
seq_num <= -128
.
> (This might explain some reports of--as you stated elsewhere--"once in
> a blue moon my krb5 mounts hang" we've investigated, albeit on kernels
> that predate 10b9d99a3dbb.)
Sounds like it was something else, I'm afraid!
--b.
Hi Bruce!
Am 01.10.21 um 19:44 schrieb J. Bruce Fields:
> On Fri, Oct 01, 2021 at 06:50:12PM +0200, Daniel Kobras wrote:
>> Am 01.10.21 um 15:59 schrieb J. Bruce Fields:
>>> From: "J. Bruce Fields" <[email protected]>
>>>
>>> If sd_max is unsigned, then sd_max - GSS_SEQ_WIN is a very large number
>>> whenever sd_max is less than GSS_SEQ_WIN, and the comparison:
>>>
>>> seq_num <= sd->sd_max - GSS_SEQ_WIN
>>>
>>> in gss_check_seq_num is pretty much always true, even when that's
>>> clearly not what was intended.
>>>
>>> This was causing pynfs to hang when using krb5, because pynfs uses zero
>>> as the initial gss sequence number. That's perfectly legal, but this
>>> logic error causes knfsd to drop the rpc in that case. Out-of-order
>>> sequence IDs in the first GSS_SEQ_WIN (128) calls will also cause this.
>>>
>>> Fixes: 10b9d99a3dbb ("SUNRPC: Augment server-side rpcgss tracepoints")
>>
>> I wonder about the Fixes tag: That changeset added tracepoints to the
>> exit path, but the buggy logic seems to have been present since the
>> pre-git ages. Or am I missing something about 10b9d99a3dbb?
>
> The relevant parts of 10b9d99a3dbb were:
>
> struct gss_svc_seq_data {
> /* highest seq number seen so far: */
> - int sd_max;
> + u32 sd_max;
>
> and
>
> -gss_check_seq_num(struct rsc *rsci, int seq_num)
> +static bool gss_check_seq_num(const struct svc_rqst *rqstp, struct rsc *rsci,
> + u32 seq_num)
>
> Together, they mean the comparison
>
> seq_num <= sd->sd_max - GSS_SEQ_WIN
>
> in the case sd_max is zero, effectively ends up being
>
> seq_num <= 4294967168
>
> instead of what was intended,
>
> seq_num <= -128
>
> .
Sorry, indeed I had missed the change in signedness. Thanks for elaborating!
>> (This might explain some reports of--as you stated elsewhere--"once in
>> a blue moon my krb5 mounts hang" we've investigated, albeit on kernels
>> that predate 10b9d99a3dbb.)
>
> Sounds like it was something else, I'm afraid!
Darn! ;-)
Kind regards,
Daniel
--
Daniel Kobras
Principal Architect
Puzzle ITC Deutschland
+49 7071 14316 0
http://www.puzzle-itc.de
--
Puzzle ITC Deutschland GmbH
Sitz der Gesellschaft: Eisenbahnstraße 1, 72072
Tübingen
Eingetragen am Amtsgericht Stuttgart HRB 765802
Geschäftsführer:
Lukas Kallies, Daniel Kobras, Mark Pröhl