On Wed, Dec 4, 2019 at 3:00 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> Hello Olga Kornievskaia,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 4e48f1cccab3: "NFSD: allow inter server COPY to have a
> STALE source server fh" from Oct 7, 2019, leads to the following
> Smatch complaint:
>
> fs/nfsd/nfs4proc.c:2371 nfsd4_proc_compound()
> error: we previously assumed 'current_fh->fh_export' could be null (see line 2325)
>
> fs/nfsd/nfs4proc.c
> 2324 }
> 2325 } else if (current_fh->fh_export &&
> ^^^^^^^^^^^^^^^^^^^^^
> The patch adds a check for NULL
>
> 2326 current_fh->fh_export->ex_fslocs.migrated &&
> 2327 !(op->opdesc->op_flags & ALLOWED_ON_ABSENT_FS)) {
> 2328 op->status = nfserr_moved;
> 2329 goto encode_op;
> 2330 }
> 2331
> 2332 fh_clear_wcc(current_fh);
> 2333
> 2334 /* If op is non-idempotent */
> 2335 if (op->opdesc->op_flags & OP_MODIFIES_SOMETHING) {
> 2336 /*
> 2337 * Don't execute this op if we couldn't encode a
> 2338 * succesful reply:
> 2339 */
> 2340 u32 plen = op->opdesc->op_rsize_bop(rqstp, op);
> 2341 /*
> 2342 * Plus if there's another operation, make sure
> 2343 * we'll have space to at least encode an error:
> 2344 */
> 2345 if (resp->opcnt < args->opcnt)
> 2346 plen += COMPOUND_ERR_SLACK_SPACE;
> 2347 op->status = nfsd4_check_resp_size(resp, plen);
> 2348 }
> 2349
> 2350 if (op->status)
> 2351 goto encode_op;
> 2352
> 2353 if (op->opdesc->op_get_currentstateid)
> 2354 op->opdesc->op_get_currentstateid(cstate, &op->u);
> 2355 op->status = op->opdesc->op_func(rqstp, cstate, &op->u);
> 2356
> 2357 /* Only from SEQUENCE */
> 2358 if (cstate->status == nfserr_replay_cache) {
> 2359 dprintk("%s NFS4.1 replay from cache\n", __func__);
> 2360 status = op->status;
> 2361 goto out;
> 2362 }
> 2363 if (!op->status) {
> 2364 if (op->opdesc->op_set_currentstateid)
> 2365 op->opdesc->op_set_currentstateid(cstate, &op->u);
> 2366
> 2367 if (op->opdesc->op_flags & OP_CLEAR_STATEID)
> 2368 clear_current_stateid(cstate);
> 2369
> 2370 if (need_wrongsec_check(rqstp))
> 2371 op->status = check_nfsd_access(current_fh->fh_export, rqstp);
> ^^^^^^^^^^^^^^^^^^^^^
> Is it required here as well?
Bruce, correct me if I'm wrong but I think we are ok here. Because for
the COPY operation for which the current_fh->fh_export can be null,
need_wrongsec_check() would be false.
>
> 2372 }
> 2373 encode_op:
>
> regards,
> dan carpenter
