| 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 >
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(-)