From: "J. Bruce Fields" <bfields@redhat.com>
To: Olga Kornievskaia <olga.kornievskaia@gmail.com>
Cc: Dan Carpenter <dan.carpenter@oracle.com>,
linux-nfs <linux-nfs@vger.kernel.org>
Subject: Re: [bug report] NFSD: allow inter server COPY to have a STALE source server fh
Date: Wed, 4 Dec 2019 17:04:35 -0500 [thread overview]
Message-ID: <20191204220435.GG40361@pick.fieldses.org> (raw)
In-Reply-To: <CAN-5tyEG3C_Ebdr6dpMJ+gQ1pEAMNqbTv76dKu=KK9rspREr1A@mail.gmail.com>
On Wed, Dec 04, 2019 at 03:11:01PM -0500, Olga Kornievskaia wrote:
> 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.
Honestly.... I've spent a few minutes thinking about it, but haven't
been able to come up either with an example where this will attempt a
NULL dereference, or a convincing argument that it never will.
I'll think about it some more and I'll figure it out. But I worry that
the the logic is fragile.
One other thing I noticed: in the no_verify case, we're depending on
fh_verify returning a stale error on a foreign filehandle. But I don't
think we can count on it. It might, by coincidence, turn out that
fh_verify returns some other error, and then a legitimate COPY could
fail for no reason.
--b.
next prev parent reply other threads:[~2019-12-04 22:04 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-04 8:00 [bug report] NFSD: allow inter server COPY to have a STALE source server fh Dan Carpenter
2019-12-04 20:11 ` Olga Kornievskaia
2019-12-04 22:04 ` J. Bruce Fields [this message]
2019-12-05 2:38 ` J. Bruce Fields
2019-12-06 21:14 ` J. Bruce Fields
2019-12-06 21:15 ` J. Bruce Fields
2019-12-06 21:27 ` Olga Kornievskaia
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191204220435.GG40361@pick.fieldses.org \
--to=bfields@redhat.com \
--cc=dan.carpenter@oracle.com \
--cc=linux-nfs@vger.kernel.org \
--cc=olga.kornievskaia@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).