| Message ID | 20221001095918.7546-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] nfsd: nfsd_do_file_acquire should hold rcu_read_lock while getting refs | expand |
Hi Jeff- > On Oct 1, 2022, at 5:59 AM, Jeff Layton <jlayton@kernel.org> wrote: > > nfsd_file is RCU-freed, so it's possible that one could be found that's > in the process of being freed and the memory recycled. Ensure we hold > the rcu_read_lock while attempting to get a reference on the object. I'm OK with entertaining clean-up patches in this code, but I am skeptical that this patch addresses the concern enumerated in bug #394. As you've pointed out to me before, the "UAF on DELEGRETURN crashes" appeared well before v5.19, which is the kernel release where this bit of code was introduced. > Cc: NeilBrown <neilb@suse.de> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/filecache.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > index d5c57360b418..f4f75ae2e4ea 100644 > --- a/fs/nfsd/filecache.c > +++ b/fs/nfsd/filecache.c > @@ -1077,10 +1077,12 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > > retry: > /* Avoid allocation if the item is already in cache */ > + rcu_read_lock(); > nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key, > nfsd_file_rhash_params); > if (nf) > nf = nfsd_file_get(nf); > + rcu_read_unlock(); Again: 657 static inline void *rhashtable_lookup_fast( 658 struct rhashtable *ht, const void *key, 659 const struct rhashtable_params params) 660 { 661 void *obj; 662 663 rcu_read_lock(); 664 obj = rhashtable_lookup(ht, key, params); 665 rcu_read_unlock(); 666 667 return obj; 668 } Since rhashtable_lookup() itself is a public API, please just call that in nfsd_file_do_acquire() after explicitly taking the RCU read lock. > if (nf) > goto wait_for_construction; > > @@ -1090,21 +1092,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > goto out_status; > } > > + rcu_read_lock(); > nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl, > &key, &new->nf_rhash, > nfsd_file_rhash_params); > + if (!IS_ERR_OR_NULL(nf)) { > + nf = nfsd_file_get(nf); Note that nfsd_file_get() can still return NULL. > + nfsd_file_slab_free(&new->nf_rcu); Why is the slab_free call now inside the RCU critical section? Granted this should be a rare case, but this adds unnecessary latency while the read lock is held. > + } > + rcu_read_unlock(); Is there a problem replacing rhashtable_lookup_get_insert_key() with rhashtable_lookup_insert_key() and just retrying the normal lookup if insertion returns EEXIST? That way, an nfsd_file_get() is necessary only at the rhashtable_lookup() call site above. > if (!nf) { > nf = new; @new was just released above, so won't this set @nf to point to freed memory in some cases? > goto open_file; > } > if (IS_ERR(nf)) > goto insert_err; > - nf = nfsd_file_get(nf); > - if (nf == NULL) { > - nf = new; > - goto open_file; > - } > - nfsd_file_slab_free(&new->nf_rcu); > > wait_for_construction: > wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE); > -- > 2.37.3 -- Chuck Lever
On Sat, 2022-10-01 at 15:33 +0000, Chuck Lever III wrote: > Hi Jeff- > > > On Oct 1, 2022, at 5:59 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > nfsd_file is RCU-freed, so it's possible that one could be found that's > > in the process of being freed and the memory recycled. Ensure we hold > > the rcu_read_lock while attempting to get a reference on the object. > > I'm OK with entertaining clean-up patches in this code, but I > am skeptical that this patch addresses the concern enumerated > in bug #394. As you've pointed out to me before, the "UAF on > DELEGRETURN crashes" appeared well before v5.19, which is the > kernel release where this bit of code was introduced. > Yeah, there may be more than one bug here. In any case, these patches should close potential races, so I think we ought to take them. > > > Cc: NeilBrown <neilb@suse.de> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/filecache.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c > > index d5c57360b418..f4f75ae2e4ea 100644 > > --- a/fs/nfsd/filecache.c > > +++ b/fs/nfsd/filecache.c > > @@ -1077,10 +1077,12 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > > > > retry: > > /* Avoid allocation if the item is already in cache */ > > + rcu_read_lock(); > > nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key, > > nfsd_file_rhash_params); > > if (nf) > > nf = nfsd_file_get(nf); > > + rcu_read_unlock(); > > Again: > > 657 static inline void *rhashtable_lookup_fast( > 658 struct rhashtable *ht, const void *key, > 659 const struct rhashtable_params params) > 660 { > 661 void *obj; > 662 > 663 rcu_read_lock(); > 664 obj = rhashtable_lookup(ht, key, params); > 665 rcu_read_unlock(); > 666 > 667 return obj; > 668 } > > Since rhashtable_lookup() itself is a public API, please > just call that in nfsd_file_do_acquire() after explicitly > taking the RCU read lock. > > Understood. Sorry I missed your point. I'll fix that up. > > if (nf) > > goto wait_for_construction; > > > > @@ -1090,21 +1092,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, > > goto out_status; > > } > > > > + rcu_read_lock(); > > nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl, > > &key, &new->nf_rhash, > > nfsd_file_rhash_params); > > + if (!IS_ERR_OR_NULL(nf)) { > > + nf = nfsd_file_get(nf); > > Note that nfsd_file_get() can still return NULL. > True, and that would leak. Good catch. > > > + nfsd_file_slab_free(&new->nf_rcu); > > Why is the slab_free call now inside the RCU critical section? > Granted this should be a rare case, but this adds unnecessary > latency while the read lock is held. > Fair point. > > > + } > > + rcu_read_unlock(); > > Is there a problem replacing rhashtable_lookup_get_insert_key() > with rhashtable_lookup_insert_key() and just retrying the normal > lookup if insertion returns EEXIST? That way, an nfsd_file_get() > is necessary only at the rhashtable_lookup() call site above. > > I like this idea, and it allows for a rather nice cleanup of the code. I'll send a v3 set after I've had a chance to do some testing. > > if (!nf) { > > nf = new; > > @new was just released above, so won't this set @nf to point > to freed memory in some cases? > > > > goto open_file; > > } > > if (IS_ERR(nf)) > > goto insert_err; > > - nf = nfsd_file_get(nf); > > - if (nf == NULL) { > > - nf = new; > > - goto open_file; > > - } > > - nfsd_file_slab_free(&new->nf_rcu); > > > > wait_for_construction: > > wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE); > > -- > > 2.37.3 > > > -- > Chuck Lever > > >
> On Oct 1, 2022, at 12:40 PM, Jeff Layton <jlayton@kernel.org> wrote: > > On Sat, 2022-10-01 at 15:33 +0000, Chuck Lever III wrote: >> Hi Jeff- >> >>> On Oct 1, 2022, at 5:59 AM, Jeff Layton <jlayton@kernel.org> wrote: >>> >>> nfsd_file is RCU-freed, so it's possible that one could be found that's >>> in the process of being freed and the memory recycled. Ensure we hold >>> the rcu_read_lock while attempting to get a reference on the object. >> >> I'm OK with entertaining clean-up patches in this code, but I >> am skeptical that this patch addresses the concern enumerated >> in bug #394. As you've pointed out to me before, the "UAF on >> DELEGRETURN crashes" appeared well before v5.19, which is the >> kernel release where this bit of code was introduced. >> > > Yeah, there may be more than one bug here. In any case, these patches > should close potential races, so I think we ought to take them. Agreed, all of these are valid and desirable improvements. I've applied the two from yesterday to my internal tree for more testing. I plan to apply this one as well once the wrinkles are ironed out. Since these are a bit late in the cycle, I plan to push the set to Linus after the initial nfsd-6.1 PR, either near the end of the merge window or in -rc1. -- Chuck Lever
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c index d5c57360b418..f4f75ae2e4ea 100644 --- a/fs/nfsd/filecache.c +++ b/fs/nfsd/filecache.c @@ -1077,10 +1077,12 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, retry: /* Avoid allocation if the item is already in cache */ + rcu_read_lock(); nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key, nfsd_file_rhash_params); if (nf) nf = nfsd_file_get(nf); + rcu_read_unlock(); if (nf) goto wait_for_construction; @@ -1090,21 +1092,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp, goto out_status; } + rcu_read_lock(); nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl, &key, &new->nf_rhash, nfsd_file_rhash_params); + if (!IS_ERR_OR_NULL(nf)) { + nf = nfsd_file_get(nf); + nfsd_file_slab_free(&new->nf_rcu); + } + rcu_read_unlock(); if (!nf) { nf = new; goto open_file; } if (IS_ERR(nf)) goto insert_err; - nf = nfsd_file_get(nf); - if (nf == NULL) { - nf = new; - goto open_file; - } - nfsd_file_slab_free(&new->nf_rcu); wait_for_construction: wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
nfsd_file is RCU-freed, so it's possible that one could be found that's in the process of being freed and the memory recycled. Ensure we hold the rcu_read_lock while attempting to get a reference on the object. Cc: NeilBrown <neilb@suse.de> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/filecache.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)