On Mon, 2023-09-11 at 18:26 -0400, Chuck Lever wrote:
> On Mon, Sep 11, 2023 at 02:30:27PM -0400, trondmy@xxxxxxxxx wrote:
> > From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> >
> > The nfsd_open code handles EOPENSTALE correctly, by retrying the
> > call to
> > fh_verify() and __nfsd_open(). However the filecache just drops the
> > error on the floor, and immediately returns nfserr_stale to the
> > caller.
> >
> > This patch ensures that we propagate the EOPENSTALE code back to
> > nfsd_file_do_acquire, and that we handle it correctly.
> >
> > Fixes: 65294c1f2c5e ("nfsd: add a new struct file caching facility
> > to nfsd")
> > Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> > ---
> > fs/nfsd/filecache.c | 27 +++++++++++++++++++--------
> > fs/nfsd/vfs.c | 28 +++++++++++++---------------
> > fs/nfsd/vfs.h | 2 +-
> > 3 files changed, 33 insertions(+), 24 deletions(-)
> >
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index ee9c923192e0..07bf219f9ae4 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -989,22 +989,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp,
> > struct svc_fh *fhp,
> > unsigned char need = may_flags & NFSD_FILE_MAY_MASK;
> > struct net *net = SVC_NET(rqstp);
> > struct nfsd_file *new, *nf;
> > - const struct cred *cred;
> > + bool stale_retry = true;
> > bool open_retry = true;
> > struct inode *inode;
> > __be32 status;
> > int ret;
> >
> > +retry:
> > status = fh_verify(rqstp, fhp, S_IFREG,
> > may_flags|NFSD_MAY_OWNER_OVERRIDE);
> > if (status != nfs_ok)
> > return status;
> > inode = d_inode(fhp->fh_dentry);
> > - cred = get_current_cred();
>
> If memory serves, we have to hold a reference to the current cred
> through the acquire process. Otherwise, the cred can be released
> while nfsd_file_do_acquire() is still running, which results in an
> oops.
>
> What's the reason to replace get_current_cred() with current_cred()?
In both cases, you're holding the rcu_read_lock() across the entire
call, so it is quite safe to use current_cred().
>
>
> > -retry:
> > rcu_read_lock();
> > - nf = nfsd_file_lookup_locked(net, cred, inode, need,
> > want_gc);
> > + nf = nfsd_file_lookup_locked(net, current_cred(), inode,
> > need, want_gc);
> > rcu_read_unlock();
> >
> > if (nf) {
> > @@ -1026,7 +1025,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp,
> > struct svc_fh *fhp,
> >
> > rcu_read_lock();
> > spin_lock(&inode->i_lock);
> > - nf = nfsd_file_lookup_locked(net, cred, inode, need,
> > want_gc);
> > + nf = nfsd_file_lookup_locked(net, current_cred(), inode,
> > need, want_gc);
> > if (unlikely(nf)) {
> > spin_unlock(&inode->i_lock);
> > rcu_read_unlock();
> > @@ -1058,6 +1057,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp,
> > struct svc_fh *fhp,
> > goto construction_err;
> > }
> > open_retry = false;
> > + fh_put(fhp);
> > goto retry;
> > }
> > this_cpu_inc(nfsd_file_cache_hits);
> > @@ -1074,7 +1074,6 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp,
> > struct svc_fh *fhp,
> > nfsd_file_check_write_error(nf);
> > *pnf = nf;
> > }
> > - put_cred(cred);
> > trace_nfsd_file_acquire(rqstp, inode, may_flags, nf,
> > status);
> > return status;
> >
> > @@ -1088,8 +1087,20 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp,
> > struct svc_fh *fhp,
> > status = nfs_ok;
> > trace_nfsd_file_opened(nf, status);
> > } else {
> > - status = nfsd_open_verified(rqstp, fhp,
> > may_flags,
> > - &nf->nf_file);
> > + ret = nfsd_open_verified(rqstp, fhp,
> > may_flags,
> > + &nf->nf_file);
> > + if (ret == -EOPENSTALE && stale_retry) {
> > + stale_retry = false;
> > + nfsd_file_unhash(nf);
> > + clear_and_wake_up_bit(NFSD_FILE_PEN
> > DING,
> > + &nf-
> > >nf_flags);
> > + if (refcount_dec_and_test(&nf-
> > >nf_ref))
> > + nfsd_file_free(nf);
> > + nf = NULL;
> > + fh_put(fhp);
> > + goto retry;
> > + }
> > + status = nfserrno(ret);
> > trace_nfsd_file_open(nf, status);
> > }
> > } else
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 2c9074ab2315..98fa4fd0556d 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -823,7 +823,7 @@ int nfsd_open_break_lease(struct inode *inode,
> > int access)
> > * and additional flags.
> > * N.B. After this call fhp needs an fh_put
> > */
> > -static __be32
> > +static int
> > __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t
> > type,
> > int may_flags, struct file **filp)
> > {
> > @@ -831,14 +831,12 @@ __nfsd_open(struct svc_rqst *rqstp, struct
> > svc_fh *fhp, umode_t type,
> > struct inode *inode;
> > struct file *file;
> > int flags = O_RDONLY|O_LARGEFILE;
> > - __be32 err;
> > - int host_err = 0;
> > + int host_err = -EPERM;
> >
> > path.mnt = fhp->fh_export->ex_path.mnt;
> > path.dentry = fhp->fh_dentry;
> > inode = d_inode(path.dentry);
> >
> > - err = nfserr_perm;
> > if (IS_APPEND(inode) && (may_flags & NFSD_MAY_WRITE))
> > goto out;
> >
> > @@ -847,7 +845,7 @@ __nfsd_open(struct svc_rqst *rqstp, struct
> > svc_fh *fhp, umode_t type,
> >
> > host_err = nfsd_open_break_lease(inode, may_flags);
> > if (host_err) /* NOMEM or WOULDBLOCK */
> > - goto out_nfserr;
> > + goto out;
> >
> > if (may_flags & NFSD_MAY_WRITE) {
> > if (may_flags & NFSD_MAY_READ)
> > @@ -859,13 +857,13 @@ __nfsd_open(struct svc_rqst *rqstp, struct
> > svc_fh *fhp, umode_t type,
> > file = dentry_open(&path, flags, current_cred());
> > if (IS_ERR(file)) {
> > host_err = PTR_ERR(file);
> > - goto out_nfserr;
> > + goto out;
> > }
> >
> > host_err = ima_file_check(file, may_flags);
> > if (host_err) {
> > fput(file);
> > - goto out_nfserr;
> > + goto out;
> > }
> >
> > if (may_flags & NFSD_MAY_64BIT_COOKIE)
> > @@ -874,10 +872,8 @@ __nfsd_open(struct svc_rqst *rqstp, struct
> > svc_fh *fhp, umode_t type,
> > file->f_mode |= FMODE_32BITHASH;
> >
> > *filp = file;
> > -out_nfserr:
> > - err = nfserrno(host_err);
> > out:
> > - return err;
> > + return host_err;
> > }
> >
> > __be32
> > @@ -885,6 +881,7 @@ nfsd_open(struct svc_rqst *rqstp, struct svc_fh
> > *fhp, umode_t type,
> > int may_flags, struct file **filp)
> > {
> > __be32 err;
> > + int host_err;
> > bool retried = false;
> >
> > validate_process_creds();
> > @@ -904,12 +901,13 @@ nfsd_open(struct svc_rqst *rqstp, struct
> > svc_fh *fhp, umode_t type,
> > retry:
> > err = fh_verify(rqstp, fhp, type, may_flags);
> > if (!err) {
> > - err = __nfsd_open(rqstp, fhp, type, may_flags,
> > filp);
> > - if (err == nfserr_stale && !retried) {
> > + host_err = __nfsd_open(rqstp, fhp, type, may_flags,
> > filp);
> > + if (host_err == -EOPENSTALE && !retried) {
> > retried = true;
> > fh_put(fhp);
> > goto retry;
> > }
> > + err = nfserrno(host_err);
> > }
> > validate_process_creds();
> > return err;
> > @@ -922,13 +920,13 @@ nfsd_open(struct svc_rqst *rqstp, struct
> > svc_fh *fhp, umode_t type,
> > * @may_flags: internal permission flags
> > * @filp: OUT: open "struct file *"
> > *
> > - * Returns an nfsstat value in network byte order.
> > + * Returns a posix error.
> > */
> > -__be32
> > +int
> > nfsd_open_verified(struct svc_rqst *rqstp, struct svc_fh *fhp, int
> > may_flags,
> > struct file **filp)
> > {
> > - __be32 err;
> > + int err;
> >
> > validate_process_creds();
> > err = __nfsd_open(rqstp, fhp, S_IFREG, may_flags, filp);
> > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> > index a6890ea7b765..e4b7207ef2e0 100644
> > --- a/fs/nfsd/vfs.h
> > +++ b/fs/nfsd/vfs.h
> > @@ -104,7 +104,7 @@ __be32 nfsd_setxattr(struct
> > svc_rqst *rqstp, struct svc_fh *fhp,
> > int nfsd_open_break_lease(struct inode *, int);
> > __be32 nfsd_open(struct svc_rqst *, struct svc_fh *,
> > umode_t,
> > int, struct file **);
> > -__be32 nfsd_open_verified(struct svc_rqst *, struct svc_fh
> > *,
> > +int nfsd_open_verified(struct svc_rqst *, struct svc_fh
> > *,
> > int, struct file **);
> > __be32 nfsd_splice_read(struct svc_rqst *rqstp, struct
> > svc_fh *fhp,
> > struct file *file, loff_t offset,
> > --
> > 2.41.0
> >
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
