Linux-NFS Archive on lore.kernel.org
 help / color / Atom feed
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 21:38:26 -0500
Message-ID: <20191205023826.GA43279@pick.fieldses.org> (raw)
In-Reply-To: <20191204220435.GG40361@pick.fieldses.org>

On Wed, Dec 04, 2019 at 05:04:35PM -0500, J. Bruce Fields wrote:
> 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.

Seems like a compound like this should trigger a NULL dereference:

	PUTFH(foreign filehandle)
	GETATTR
	SAVEFH
	COPY

First, check_if_stalefh_allowed sets no_verify on the first (PUTFH) op.
Then op_func = nfsd4_putfh runs and leaves current_fh->fh_export NULL.
need_wrongsec_check returns true, since this PUTFH has OP_IS_PUTFH_LIKE
set and GETATTR does not have OP_HANDLES_WRONGSEC set.

Haven't actually tested that, maybe I'm missing something.

So, stuff we could do:

	- add an extra check of fh_export or something here.
	- make check_if_stalefh_allowed more careful--it'd be easy for
	  it to spot that a compound like the above is going to fail.
	- double-check that we don't assume the filehandle is verified
	  elsewhere in nfsd_compound.
	- ?

> 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.

Also nervous about cases like the above where we'll be passing the
foreign filehandle into GETATTR and counting on fh_verify failing in a
useful way.  I mean, maybe the client gets what it deserves for sending
an obviously nonsensical compound, but still it'd probably be better to
catch that earlier.

--b.


  reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-04  8:00 Dan Carpenter
2019-12-04 20:11 ` Olga Kornievskaia
2019-12-04 22:04   ` J. Bruce Fields
2019-12-05  2:38     ` J. Bruce Fields [this message]
2019-12-06 21:14       ` bfields
2019-12-06 21:15         ` J. Bruce Fields
2019-12-06 21:27           ` Olga Kornievskaia

Reply instructions:

You may reply publically 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=20191205023826.GA43279@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

Linux-NFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nfs/0 linux-nfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nfs linux-nfs/ https://lore.kernel.org/linux-nfs \
		linux-nfs@vger.kernel.org
	public-inbox-index linux-nfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-nfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git