2023-01-06 15:34:26

by Jeff Layton

Subject: [PATCH v2 1/2] nfsd: allow nfsd_file_get to sanely handle a NULL pointer

...and remove some now-useless NULL pointer checks in its callers.

Suggested-by: NeilBrown <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/filecache.c | 5 ++---
fs/nfsd/nfs4state.c | 4 +---
2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 0ef070349014..58ac93e7e680 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -452,7 +452,7 @@ static bool nfsd_file_lru_remove(struct nfsd_file *nf)
struct nfsd_file *
nfsd_file_get(struct nfsd_file *nf)
{
- if (likely(refcount_inc_not_zero(&nf->nf_ref)))
+ if (nf && refcount_inc_not_zero(&nf->nf_ref))
return nf;
return NULL;
}
@@ -1096,8 +1096,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
rcu_read_lock();
nf = rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
nfsd_file_rhash_params);
- if (nf)
- nf = nfsd_file_get(nf);
+ nf = nfsd_file_get(nf);
rcu_read_unlock();

if (nf) {
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4809ae0f0138..655fcfec0ace 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -602,9 +602,7 @@ put_nfs4_file(struct nfs4_file *fi)
static struct nfsd_file *
__nfs4_get_fd(struct nfs4_file *f, int oflag)
{
- if (f->fi_fds[oflag])
- return nfsd_file_get(f->fi_fds[oflag]);
- return NULL;
+ return nfsd_file_get(f->fi_fds[oflag]);
}

static struct nfsd_file *
--
2.39.0

2023-01-06 15:34:26

by Jeff Layton

Subject: [PATCH v2 2/2] nfsd: fix potential race in nfs4_find_file

The WARN_ON_ONCE check is not terribly useful. It also seems possible
for nfs4_find_file to race with the destruction of an fi_deleg_file
while trying to take a reference to it.

Now that it's safe to pass nfs_get_file a NULL pointer, remove the WARN
and NULL pointer check. Take the fi_lock when fetching fi_deleg_file.

Cc: NeilBrown <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/nfs4state.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 655fcfec0ace..f923bab09a31 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6415,23 +6415,26 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
static struct nfsd_file *
nfs4_find_file(struct nfs4_stid *s, int flags)
{
+ struct nfsd_file *ret = NULL;
+
if (!s)
return NULL;

switch (s->sc_type) {
case NFS4_DELEG_STID:
- if (WARN_ON_ONCE(!s->sc_file->fi_deleg_file))
- return NULL;
- return nfsd_file_get(s->sc_file->fi_deleg_file);
+ spin_lock(&s->sc_file->fi_lock);
+ ret = nfsd_file_get(s->sc_file->fi_deleg_file);
+ spin_unlock(&s->sc_file->fi_lock);
+ break;
case NFS4_OPEN_STID:
case NFS4_LOCK_STID:
if (flags & RD_STATE)
- return find_readable_file(s->sc_file);
+ ret = find_readable_file(s->sc_file);
else
- return find_writeable_file(s->sc_file);
+ ret = find_writeable_file(s->sc_file);
}

- return NULL;
+ return ret;
}

static __be32
--
2.39.0

2023-01-08 19:38:55

by Chuck Lever III

Subject: Re: [PATCH v2 1/2] nfsd: allow nfsd_file_get to sanely handle a NULL pointer

> On Jan 6, 2023, at 10:33 AM, Jeff Layton <[email protected]> wrote:
>
> ...and remove some now-useless NULL pointer checks in its callers.
>
> Suggested-by: NeilBrown <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/filecache.c | 5 ++---
> fs/nfsd/nfs4state.c | 4 +---
> 2 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 0ef070349014..58ac93e7e680 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -452,7 +452,7 @@ static bool nfsd_file_lru_remove(struct nfsd_file *nf)
> struct nfsd_file *
> nfsd_file_get(struct nfsd_file *nf)
> {
> - if (likely(refcount_inc_not_zero(&nf->nf_ref)))
> + if (nf && refcount_inc_not_zero(&nf->nf_ref))
> return nf;
> return NULL;
> }
> @@ -1096,8 +1096,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> rcu_read_lock();
> nf = rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
> nfsd_file_rhash_params);
> - if (nf)
> - nf = nfsd_file_get(nf);
> + nf = nfsd_file_get(nf);
> rcu_read_unlock();
>
> if (nf) {
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 4809ae0f0138..655fcfec0ace 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -602,9 +602,7 @@ put_nfs4_file(struct nfs4_file *fi)
> static struct nfsd_file *
> __nfs4_get_fd(struct nfs4_file *f, int oflag)
> {
> - if (f->fi_fds[oflag])
> - return nfsd_file_get(f->fi_fds[oflag]);
> - return NULL;
> + return nfsd_file_get(f->fi_fds[oflag]);
> }
>
> static struct nfsd_file *
> --
> 2.39.0

Hi Jeff-

I've applied v2 of 1/2 and 2/2 to nfsd's for-next.

--
Chuck Lever