On Wed, 25 Sep 2024, Dan Carpenter wrote:
> Hello NeilBrown,
>
> Commit 86ab08beb3f0 ("SUNRPC: replace program list with program
> array") from Sep 5, 2024 (linux-next), leads to the following Smatch
> static checker warning:
>
> net/sunrpc/svc.c:1368 svc_process_common()
> error: uninitialized symbol 'progp'.
>
> net/sunrpc/svc.c
> 1320 static int
> 1321 svc_process_common(struct svc_rqst *rqstp)
> 1322 {
> 1323 struct xdr_stream *xdr = &rqstp->rq_res_stream;
> 1324 struct svc_program *progp;
> 1325 const struct svc_procedure *procp = NULL;
> 1326 struct svc_serv *serv = rqstp->rq_server;
> 1327 struct svc_process_info process;
> 1328 enum svc_auth_status auth_res;
> 1329 unsigned int aoffset;
> 1330 int pr, rc;
> 1331 __be32 *p;
> 1332
> 1333 /* Will be turned off only when NFSv4 Sessions are used */
> 1334 set_bit(RQ_USEDEFERRAL, &rqstp->rq_flags);
> 1335 clear_bit(RQ_DROPME, &rqstp->rq_flags);
> 1336
> 1337 /* Construct the first words of the reply: */
> 1338 svcxdr_init_encode(rqstp);
> 1339 xdr_stream_encode_be32(xdr, rqstp->rq_xid);
> 1340 xdr_stream_encode_be32(xdr, rpc_reply);
> 1341
> 1342 p = xdr_inline_decode(&rqstp->rq_arg_stream, XDR_UNIT * 4);
> 1343 if (unlikely(!p))
> 1344 goto err_short_len;
> 1345 if (*p++ != cpu_to_be32(RPC_VERSION))
> 1346 goto err_bad_rpc;
> 1347
> 1348 xdr_stream_encode_be32(xdr, rpc_msg_accepted);
> 1349
> 1350 rqstp->rq_prog = be32_to_cpup(p++);
> 1351 rqstp->rq_vers = be32_to_cpup(p++);
> 1352 rqstp->rq_proc = be32_to_cpup(p);
> 1353
> 1354 for (pr = 0; pr < serv->sv_nprogs; pr++) {
>
> This loop used to iterate until we hit a break statement or until progp was NULL
>
> 1355 progp = &serv->sv_programs[pr];
>
> But now progp is always non-NULL. (Smatch is concerned that ->sv_nprogrs is
> <= 0 but I doubt that's possible?)
It is unsigned so <0 is certainly out, and in practice it is never zero.
But we shouldn't depend on that.
The code is also wrong in that an unknown program number will cause the
last program in the array to be used.
I'll send a patch.
Thanks,
NeilBrown
>
> 1356
> 1357 if (rqstp->rq_prog == progp->pg_prog)
> 1358 break;
> 1359 }
> 1360
> 1361 /*
> 1362 * Decode auth data, and add verifier to reply buffer.
> 1363 * We do this before anything else in order to get a decent
> 1364 * auth verifier.
> 1365 */
> 1366 auth_res = svc_authenticate(rqstp);
> 1367 /* Also give the program a chance to reject this call: */
> --> 1368 if (auth_res == SVC_OK && progp)
> ^^^^^
> So this condition doesn't make sense.
>
> 1369 auth_res = progp->pg_authenticate(rqstp);
> 1370 trace_svc_authenticate(rqstp, auth_res);
> 1371 switch (auth_res) {
> 1372 case SVC_OK:
> 1373 break;
> 1374 case SVC_GARBAGE:
> 1375 goto err_garbage_args;
> 1376 case SVC_SYSERR:
> 1377 goto err_system_err;
> 1378 case SVC_DENIED:
> 1379 goto err_bad_auth;
> 1380 case SVC_CLOSE:
> 1381 goto close;
> 1382 case SVC_DROP:
> 1383 goto dropit;
> 1384 case SVC_COMPLETE:
> 1385 goto sendit;
> 1386 default:
> 1387 pr_warn_once("Unexpected svc_auth_status (%d)\n", auth_res);
> 1388 goto err_system_err;
> 1389 }
> 1390
> 1391 if (progp == NULL)
> ^^^^^^^^^^^^^
> Same
>
> 1392 goto err_bad_prog;
> 1393
> 1394 switch (progp->pg_init_request(rqstp, progp, &process)) {
> 1395 case rpc_success:
> 1396 break;
> 1397 case rpc_prog_unavail:
> 1398 goto err_bad_prog;
> 1399 case rpc_prog_mismatch:
>
> regards,
> dan carpenter
>
