Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191204080039.ixjqetefkzzlldyt@kili.mountain>
In-Reply-To: <20191204080039.ixjqetefkzzlldyt@kili.mountain>
From:   Olga Kornievskaia <olga.kornievskaia@gmail.com>
Date:   Wed, 4 Dec 2019 15:11:01 -0500
Message-ID: <CAN-5tyEG3C_Ebdr6dpMJ+gQ1pEAMNqbTv76dKu=KK9rspREr1A@mail.gmail.com>
Subject: Re: [bug report] NFSD: allow inter server COPY to have a STALE source
 server fh
To:     Dan Carpenter <dan.carpenter@oracle.com>,
        "J. Bruce Fields" <bfields@redhat.com>
Cc:     linux-nfs <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Wed, Dec 4, 2019 at 3:00 AM Dan Carpenter <dan.carpenter@oracle.com> 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