* [PATCH] SUNRPC: fix sign error causing rpcsec_gss drops
@ 2021-10-01 13:59 J. Bruce Fields
2021-10-01 16:00 ` Chuck Lever III
2021-10-01 16:50 ` Daniel Kobras
0 siblings, 2 replies; 5+ messages in thread
From: J. Bruce Fields @ 2021-10-01 13:59 UTC (permalink / raw)
To: Chuck Lever; +Cc: linux-nfs, Volodymyr Khomenko
From: "J. Bruce Fields" <bfields@redhat.com>
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: stable@vger.kernel.org
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] SUNRPC: fix sign error causing rpcsec_gss drops
2021-10-01 13:59 [PATCH] SUNRPC: fix sign error causing rpcsec_gss drops J. Bruce Fields
@ 2021-10-01 16:00 ` Chuck Lever III
2021-10-01 16:50 ` Daniel Kobras
1 sibling, 0 replies; 5+ messages in thread
From: Chuck Lever III @ 2021-10-01 16:00 UTC (permalink / raw)
To: Bruce Fields; +Cc: Linux NFS Mailing List, Volodymyr Khomenko
> On Oct 1, 2021, at 9:59 AM, J. Bruce Fields <bfields@fieldses.org> wrote:
>
> From: "J. Bruce Fields" <bfields@redhat.com>
>
> 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: stable@vger.kernel.org
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] SUNRPC: fix sign error causing rpcsec_gss drops
2021-10-01 13:59 [PATCH] SUNRPC: fix sign error causing rpcsec_gss drops J. Bruce Fields
2021-10-01 16:00 ` Chuck Lever III
@ 2021-10-01 16:50 ` Daniel Kobras
2021-10-01 17:44 ` J. Bruce Fields
1 sibling, 1 reply; 5+ messages in thread
From: Daniel Kobras @ 2021-10-01 16:50 UTC (permalink / raw)
To: J. Bruce Fields, Chuck Lever; +Cc: linux-nfs, Volodymyr Khomenko
Hi Bruce!
Am 01.10.21 um 15:59 schrieb J. Bruce Fields:
> From: "J. Bruce Fields" <bfields@redhat.com>
>
> 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: stable@vger.kernel.org
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
> 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
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] SUNRPC: fix sign error causing rpcsec_gss drops
2021-10-01 16:50 ` Daniel Kobras
@ 2021-10-01 17:44 ` J. Bruce Fields
2021-10-04 6:50 ` Daniel Kobras
0 siblings, 1 reply; 5+ messages in thread
From: J. Bruce Fields @ 2021-10-01 17:44 UTC (permalink / raw)
To: Daniel Kobras; +Cc: Chuck Lever, linux-nfs, Volodymyr Khomenko
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" <bfields@redhat.com>
> >
> > 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] SUNRPC: fix sign error causing rpcsec_gss drops
2021-10-01 17:44 ` J. Bruce Fields
@ 2021-10-04 6:50 ` Daniel Kobras
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Kobras @ 2021-10-04 6:50 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Chuck Lever, linux-nfs, Volodymyr Khomenko
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" <bfields@redhat.com>
>>>
>>> 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
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-04 6:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-01 13:59 [PATCH] SUNRPC: fix sign error causing rpcsec_gss drops J. Bruce Fields
2021-10-01 16:00 ` Chuck Lever III
2021-10-01 16:50 ` Daniel Kobras
2021-10-01 17:44 ` J. Bruce Fields
2021-10-04 6:50 ` Daniel Kobras
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.