| Message ID | 172724928945.17050.3126216882032780036@noble.neil.brown.name (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | sunrpc: fix prog selection loop in svc_process_common | expand |
Hi Neil - On Wed, Sep 25, 2024 at 05:28:09PM +1000, NeilBrown wrote: > > If the rq_prog is not in the list of programs, then we use the last > program in the list and subsequent tests on 'progp' being NULL are > useless. That's the logic error, but what is the observed unexpected behavior? > We should only assign progp when we find the right program, and we > should initialize it to NULL > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Fixes: 86ab08beb3f0 ("SUNRPC: replace program list with program array") > Signed-off-by: NeilBrown <neilb@suse.de> IIRC commit 86ab08beb3f0 went through Anna's tree during this merge window. It would be easier (for me) if Anna took this one. Acked-by: Chuck Lever <chuck.lever@oracle.com> > --- > net/sunrpc/svc.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > index 7e7f4e0390c7..79879b7d39cb 100644 > --- a/net/sunrpc/svc.c > +++ b/net/sunrpc/svc.c > @@ -1321,7 +1321,7 @@ static int > svc_process_common(struct svc_rqst *rqstp) > { > struct xdr_stream *xdr = &rqstp->rq_res_stream; > - struct svc_program *progp; > + struct svc_program *progp = NULL; > const struct svc_procedure *procp = NULL; > struct svc_serv *serv = rqstp->rq_server; > struct svc_process_info process; > @@ -1351,12 +1351,9 @@ svc_process_common(struct svc_rqst *rqstp) > rqstp->rq_vers = be32_to_cpup(p++); > rqstp->rq_proc = be32_to_cpup(p); > > - for (pr = 0; pr < serv->sv_nprogs; pr++) { > - progp = &serv->sv_programs[pr]; > - > - if (rqstp->rq_prog == progp->pg_prog) > - break; > - } > + for (pr = 0; pr < serv->sv_nprogs; pr++) > + if (rqstp->rq_prog == serv->sv_programs[pr].pg_prog) > + progp = &serv->sv_programs[pr]; > > /* > * Decode auth data, and add verifier to reply buffer. > -- > 2.46.0 >
[I fixed Dan's address - sorry about that] On Thu, 26 Sep 2024, Chuck Lever wrote: > Hi Neil - > > On Wed, Sep 25, 2024 at 05:28:09PM +1000, NeilBrown wrote: > > > > If the rq_prog is not in the list of programs, then we use the last > > program in the list and subsequent tests on 'progp' being NULL are > > useless. > > That's the logic error, but what is the observed unexpected > behavior? The unexpected behaviour is that "if rq_prog is not in the list of programs, then we use the last program in the list". Isn't that a behaviour? Should I add that "we don't get the expected rpc_prog_unavail error? What am I missing? > > > > We should only assign progp when we find the right program, and we > > should initialize it to NULL > > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > Fixes: 86ab08beb3f0 ("SUNRPC: replace program list with program array") > > Signed-off-by: NeilBrown <neilb@suse.de> > > IIRC commit 86ab08beb3f0 went through Anna's tree during this merge > window. It would be easier (for me) if Anna took this one. I don't entirely understand the logic, but ok. > > Acked-by: Chuck Lever <chuck.lever@oracle.com> Thanks, NeilBrown > > > > --- > > net/sunrpc/svc.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > > index 7e7f4e0390c7..79879b7d39cb 100644 > > --- a/net/sunrpc/svc.c > > +++ b/net/sunrpc/svc.c > > @@ -1321,7 +1321,7 @@ static int > > svc_process_common(struct svc_rqst *rqstp) > > { > > struct xdr_stream *xdr = &rqstp->rq_res_stream; > > - struct svc_program *progp; > > + struct svc_program *progp = NULL; > > const struct svc_procedure *procp = NULL; > > struct svc_serv *serv = rqstp->rq_server; > > struct svc_process_info process; > > @@ -1351,12 +1351,9 @@ svc_process_common(struct svc_rqst *rqstp) > > rqstp->rq_vers = be32_to_cpup(p++); > > rqstp->rq_proc = be32_to_cpup(p); > > > > - for (pr = 0; pr < serv->sv_nprogs; pr++) { > > - progp = &serv->sv_programs[pr]; > > - > > - if (rqstp->rq_prog == progp->pg_prog) > > - break; > > - } > > + for (pr = 0; pr < serv->sv_nprogs; pr++) > > + if (rqstp->rq_prog == serv->sv_programs[pr].pg_prog) > > + progp = &serv->sv_programs[pr]; > > > > /* > > * Decode auth data, and add verifier to reply buffer. > > -- > > 2.46.0 > > > > -- > Chuck Lever >
> On Sep 25, 2024, at 5:25 PM, NeilBrown <neilb@suse.de> wrote: > > > [I fixed Dan's address - sorry about that] > > On Thu, 26 Sep 2024, Chuck Lever wrote: >> Hi Neil - >> >> On Wed, Sep 25, 2024 at 05:28:09PM +1000, NeilBrown wrote: >>> >>> If the rq_prog is not in the list of programs, then we use the last >>> program in the list and subsequent tests on 'progp' being NULL are >>> useless. >> >> That's the logic error, but what is the observed unexpected >> behavior? > > The unexpected behaviour is that "if rq_prog is not in the list of > programs, then we use the last program in the list". Isn't that a > behaviour? Should I add that "we don't get the expected > rpc_prog_unavail error? I'm thinking of something that would catch the eye of some overworked support engineer who might not be deeply familiar with NFS or RPC. Clients won't see RPC_PROG_UNAVAIL, but what would they see instead? Under what conditions would they see this misbehavior? It's no big deal, since this bug will never reach a stable kernel. I was thinking out loud, and forgot to label the remark as such. -- Chuck Lever
On Wed, 2024-09-25 at 17:28 +1000, NeilBrown wrote: > If the rq_prog is not in the list of programs, then we use the last > program in the list and subsequent tests on 'progp' being NULL are > useless. > > We should only assign progp when we find the right program, and we > should initialize it to NULL > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Fixes: 86ab08beb3f0 ("SUNRPC: replace program list with program array") > Signed-off-by: NeilBrown <neilb@suse.de> > --- > net/sunrpc/svc.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > index 7e7f4e0390c7..79879b7d39cb 100644 > --- a/net/sunrpc/svc.c > +++ b/net/sunrpc/svc.c > @@ -1321,7 +1321,7 @@ static int > svc_process_common(struct svc_rqst *rqstp) > { > struct xdr_stream *xdr = &rqstp->rq_res_stream; > - struct svc_program *progp; > + struct svc_program *progp = NULL; > const struct svc_procedure *procp = NULL; > struct svc_serv *serv = rqstp->rq_server; > struct svc_process_info process; > @@ -1351,12 +1351,9 @@ svc_process_common(struct svc_rqst *rqstp) > rqstp->rq_vers = be32_to_cpup(p++); > rqstp->rq_proc = be32_to_cpup(p); > > - for (pr = 0; pr < serv->sv_nprogs; pr++) { > - progp = &serv->sv_programs[pr]; > - > - if (rqstp->rq_prog == progp->pg_prog) > - break; > - } > + for (pr = 0; pr < serv->sv_nprogs; pr++) > + if (rqstp->rq_prog == serv->sv_programs[pr].pg_prog) > + progp = &serv->sv_programs[pr]; > > /* > * Decode auth data, and add verifier to reply buffer. Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Sun, 29 Sep 2024, Chuck Lever III wrote: > > > > On Sep 25, 2024, at 5:25 PM, NeilBrown <neilb@suse.de> wrote: > > > > > > [I fixed Dan's address - sorry about that] > > > > On Thu, 26 Sep 2024, Chuck Lever wrote: > >> Hi Neil - > >> > >> On Wed, Sep 25, 2024 at 05:28:09PM +1000, NeilBrown wrote: > >>> > >>> If the rq_prog is not in the list of programs, then we use the last > >>> program in the list and subsequent tests on 'progp' being NULL are > >>> useless. > >> > >> That's the logic error, but what is the observed unexpected > >> behavior? > > > > The unexpected behaviour is that "if rq_prog is not in the list of > > programs, then we use the last program in the list". Isn't that a > > behaviour? Should I add that "we don't get the expected > > rpc_prog_unavail error? > > I'm thinking of something that would catch the eye of some > overworked support engineer who might not be deeply familiar > with NFS or RPC. > > Clients won't see RPC_PROG_UNAVAIL, but what would they see > instead? Under what conditions would they see this misbehavior? I had thought of this being a bug with no observable consequences in normal circumstance. But upon reflection I realise that if an nfs server was running on a kernel which didn't have NFS_ACL_SUPPORT then a ping of the nfs_acl service on port 2049 would incorrectly report success. Do clients do an RPC ping without checking with portmap first? Maybe, but only at mount time. So if you rebooted a server that had ACL support so that now it doesn't - the result might confuse the client. Maybe. I agree that it can be valuable decoding the likely user-visible effect of a bug. Not always easy. I'll take your suggested way-out of noting that the bug should never appear in a released kernel - only an -rc1 Thanks, NeilBrown > > It's no big deal, since this bug will never reach a stable > kernel. I was thinking out loud, and forgot to label the > remark as such. > > > -- > Chuck Lever > > >
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c index 7e7f4e0390c7..79879b7d39cb 100644 --- a/net/sunrpc/svc.c +++ b/net/sunrpc/svc.c @@ -1321,7 +1321,7 @@ static int svc_process_common(struct svc_rqst *rqstp) { struct xdr_stream *xdr = &rqstp->rq_res_stream; - struct svc_program *progp; + struct svc_program *progp = NULL; const struct svc_procedure *procp = NULL; struct svc_serv *serv = rqstp->rq_server; struct svc_process_info process; @@ -1351,12 +1351,9 @@ svc_process_common(struct svc_rqst *rqstp) rqstp->rq_vers = be32_to_cpup(p++); rqstp->rq_proc = be32_to_cpup(p); - for (pr = 0; pr < serv->sv_nprogs; pr++) { - progp = &serv->sv_programs[pr]; - - if (rqstp->rq_prog == progp->pg_prog) - break; - } + for (pr = 0; pr < serv->sv_nprogs; pr++) + if (rqstp->rq_prog == serv->sv_programs[pr].pg_prog) + progp = &serv->sv_programs[pr]; /* * Decode auth data, and add verifier to reply buffer.
If the rq_prog is not in the list of programs, then we use the last program in the list and subsequent tests on 'progp' being NULL are useless. We should only assign progp when we find the right program, and we should initialize it to NULL Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Fixes: 86ab08beb3f0 ("SUNRPC: replace program list with program array") Signed-off-by: NeilBrown <neilb@suse.de> --- net/sunrpc/svc.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)