| Message ID | db0980d0-8c99-940a-1748-04e679a366d1@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: memory corruption in nfsd4_lock() | expand |
On Mon, 2020-03-23 at 10:55 +0300, Vasily Averin wrote: > New struct nfsd4_blocked_lock allocated in find_or_allocate_block() > does not initialised nbl_list and nbl_lru. > If conflock allocation fails rollback can call list_del_init() > access uninitialized fields and corrupt memory. > > Fixes: 76d348fadff5 ("nfsd: have nfsd4_lock use blocking locks for v4.1+ lock") > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> > --- > fs/nfsd/nfs4state.c | 32 +++++++++++++++----------------- > 1 file changed, 15 insertions(+), 17 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 369e574c5092..176ef8d24fae 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -6524,6 +6524,13 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > goto out; > } > > + conflock = locks_alloc_lock(); > + if (!conflock) { > + dprintk("NFSD: %s: unable to allocate lock!\n", __func__); > + status = nfserr_jukebox; > + goto out; > + } > + > nbl = find_or_allocate_block(lock_sop, &fp->fi_fhandle, nn); > if (!nbl) { > dprintk("NFSD: %s: unable to allocate block!\n", __func__); > @@ -6542,13 +6549,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length); > nfs4_transform_lock_offset(file_lock); > > - conflock = locks_alloc_lock(); > - if (!conflock) { > - dprintk("NFSD: %s: unable to allocate lock!\n", __func__); > - status = nfserr_jukebox; > - goto out; > - } > - > if (fl_flags & FL_SLEEP) { > nbl->nbl_time = jiffies; > spin_lock(&nn->blocked_locks_lock); > @@ -6581,17 +6581,15 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > status = nfserrno(err); > break; > } > -out: > - if (nbl) { > - /* dequeue it if we queued it before */ > - if (fl_flags & FL_SLEEP) { > - spin_lock(&nn->blocked_locks_lock); > - list_del_init(&nbl->nbl_list); > - list_del_init(&nbl->nbl_lru); > - spin_unlock(&nn->blocked_locks_lock); > - } > - free_blocked_lock(nbl); > + /* dequeue it if we queued it before */ > + if (fl_flags & FL_SLEEP) { > + spin_lock(&nn->blocked_locks_lock); > + list_del_init(&nbl->nbl_list); > + list_del_init(&nbl->nbl_lru); > + spin_unlock(&nn->blocked_locks_lock); > } > + free_blocked_lock(nbl); > +out: > if (nf) > nfsd_file_put(nf); > if (lock_stp) { Good catch! Is there any reason not to just fix this by initializing the list_heads in find_or_allocate_block? That seems like it'd be a simpler fix.
> On Mar 23, 2020, at 3:55 AM, Vasily Averin <vvs@virtuozzo.com> wrote: > > New struct nfsd4_blocked_lock allocated in find_or_allocate_block() > does not initialised nbl_list and nbl_lru. > If conflock allocation fails rollback can call list_del_init() > access uninitialized fields and corrupt memory. > > Fixes: 76d348fadff5 ("nfsd: have nfsd4_lock use blocking locks for v4.1+ lock") > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> > --- > fs/nfsd/nfs4state.c | 32 +++++++++++++++----------------- > 1 file changed, 15 insertions(+), 17 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 369e574c5092..176ef8d24fae 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -6524,6 +6524,13 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > goto out; > } > > + conflock = locks_alloc_lock(); > + if (!conflock) { > + dprintk("NFSD: %s: unable to allocate lock!\n", __func__); > + status = nfserr_jukebox; > + goto out; > + } Nit: What do people think about removing this dprintk() as part of the fix? > nbl = find_or_allocate_block(lock_sop, &fp->fi_fhandle, nn); > if (!nbl) { > dprintk("NFSD: %s: unable to allocate block!\n", __func__); > @@ -6542,13 +6549,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length); > nfs4_transform_lock_offset(file_lock); > > - conflock = locks_alloc_lock(); > - if (!conflock) { > - dprintk("NFSD: %s: unable to allocate lock!\n", __func__); > - status = nfserr_jukebox; > - goto out; > - } > - > if (fl_flags & FL_SLEEP) { > nbl->nbl_time = jiffies; > spin_lock(&nn->blocked_locks_lock); > @@ -6581,17 +6581,15 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > status = nfserrno(err); > break; > } > -out: > - if (nbl) { > - /* dequeue it if we queued it before */ > - if (fl_flags & FL_SLEEP) { > - spin_lock(&nn->blocked_locks_lock); > - list_del_init(&nbl->nbl_list); > - list_del_init(&nbl->nbl_lru); > - spin_unlock(&nn->blocked_locks_lock); > - } > - free_blocked_lock(nbl); > + /* dequeue it if we queued it before */ > + if (fl_flags & FL_SLEEP) { > + spin_lock(&nn->blocked_locks_lock); > + list_del_init(&nbl->nbl_list); > + list_del_init(&nbl->nbl_lru); > + spin_unlock(&nn->blocked_locks_lock); > } > + free_blocked_lock(nbl); > +out: > if (nf) > nfsd_file_put(nf); > if (lock_stp) { > -- > 2.17.1 > -- Chuck Lever
On Mon, Mar 23, 2020 at 09:50:34AM -0400, Chuck Lever wrote: > > > > On Mar 23, 2020, at 3:55 AM, Vasily Averin <vvs@virtuozzo.com> wrote: > > > > New struct nfsd4_blocked_lock allocated in find_or_allocate_block() > > does not initialised nbl_list and nbl_lru. > > If conflock allocation fails rollback can call list_del_init() > > access uninitialized fields and corrupt memory. > > > > Fixes: 76d348fadff5 ("nfsd: have nfsd4_lock use blocking locks for v4.1+ lock") > > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> > > --- > > fs/nfsd/nfs4state.c | 32 +++++++++++++++----------------- > > 1 file changed, 15 insertions(+), 17 deletions(-) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index 369e574c5092..176ef8d24fae 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -6524,6 +6524,13 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > > goto out; > > } > > > > + conflock = locks_alloc_lock(); > > + if (!conflock) { > > + dprintk("NFSD: %s: unable to allocate lock!\n", __func__); > > + status = nfserr_jukebox; > > + goto out; > > + } > > Nit: What do people think about removing this dprintk() as part of the fix? I don't think we want a dprintk every place we kmalloc. All for removing them.--b.
On 3/23/20 3:18 PM, Jeff Layton wrote: > On Mon, 2020-03-23 at 10:55 +0300, Vasily Averin wrote: >> New struct nfsd4_blocked_lock allocated in find_or_allocate_block() >> does not initialised nbl_list and nbl_lru. >> If conflock allocation fails rollback can call list_del_init() >> access uninitialized fields and corrupt memory. >> >> Fixes: 76d348fadff5 ("nfsd: have nfsd4_lock use blocking locks for v4.1+ lock") >> Signed-off-by: Vasily Averin <vvs@virtuozzo.com> > > Good catch! Is there any reason not to just fix this by initializing the > list_heads in find_or_allocate_block? That seems like it'd be a simpler > fix. > Rollback in nfsd4_lock() is not optimal, I've tried to improve it too, However I agree such improvement is not a simplest fix and it anyway does not make whole rollback perfect. I think it's better to re-send small fix for the found problem, and prepare separate patches for rollback improvements, Thank you, Vasily Averin
Howdy- > On Mar 23, 2020, at 3:55 AM, Vasily Averin <vvs@virtuozzo.com> wrote: > > New struct nfsd4_blocked_lock allocated in find_or_allocate_block() > does not initialised nbl_list and nbl_lru. > If conflock allocation fails rollback can call list_del_init() > access uninitialized fields and corrupt memory. > > Fixes: 76d348fadff5 ("nfsd: have nfsd4_lock use blocking locks for v4.1+ lock") > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> I'm not certain where we stand with this one. Jeff, are you OK with me taking this for v5.7, or is there additional work needed? I can drop the dprintk when I merge it. > --- > fs/nfsd/nfs4state.c | 32 +++++++++++++++----------------- > 1 file changed, 15 insertions(+), 17 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 369e574c5092..176ef8d24fae 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -6524,6 +6524,13 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > goto out; > } > > + conflock = locks_alloc_lock(); > + if (!conflock) { > + dprintk("NFSD: %s: unable to allocate lock!\n", __func__); > + status = nfserr_jukebox; > + goto out; > + } > + > nbl = find_or_allocate_block(lock_sop, &fp->fi_fhandle, nn); > if (!nbl) { > dprintk("NFSD: %s: unable to allocate block!\n", __func__); > @@ -6542,13 +6549,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length); > nfs4_transform_lock_offset(file_lock); > > - conflock = locks_alloc_lock(); > - if (!conflock) { > - dprintk("NFSD: %s: unable to allocate lock!\n", __func__); > - status = nfserr_jukebox; > - goto out; > - } > - > if (fl_flags & FL_SLEEP) { > nbl->nbl_time = jiffies; > spin_lock(&nn->blocked_locks_lock); > @@ -6581,17 +6581,15 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > status = nfserrno(err); > break; > } > -out: > - if (nbl) { > - /* dequeue it if we queued it before */ > - if (fl_flags & FL_SLEEP) { > - spin_lock(&nn->blocked_locks_lock); > - list_del_init(&nbl->nbl_list); > - list_del_init(&nbl->nbl_lru); > - spin_unlock(&nn->blocked_locks_lock); > - } > - free_blocked_lock(nbl); > + /* dequeue it if we queued it before */ > + if (fl_flags & FL_SLEEP) { > + spin_lock(&nn->blocked_locks_lock); > + list_del_init(&nbl->nbl_list); > + list_del_init(&nbl->nbl_lru); > + spin_unlock(&nn->blocked_locks_lock); > } > + free_blocked_lock(nbl); > +out: > if (nf) > nfsd_file_put(nf); > if (lock_stp) { > -- > 2.17.1 > -- Chuck Lever
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 369e574c5092..176ef8d24fae 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -6524,6 +6524,13 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, goto out; } + conflock = locks_alloc_lock(); + if (!conflock) { + dprintk("NFSD: %s: unable to allocate lock!\n", __func__); + status = nfserr_jukebox; + goto out; + } + nbl = find_or_allocate_block(lock_sop, &fp->fi_fhandle, nn); if (!nbl) { dprintk("NFSD: %s: unable to allocate block!\n", __func__); @@ -6542,13 +6549,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length); nfs4_transform_lock_offset(file_lock); - conflock = locks_alloc_lock(); - if (!conflock) { - dprintk("NFSD: %s: unable to allocate lock!\n", __func__); - status = nfserr_jukebox; - goto out; - } - if (fl_flags & FL_SLEEP) { nbl->nbl_time = jiffies; spin_lock(&nn->blocked_locks_lock); @@ -6581,17 +6581,15 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, status = nfserrno(err); break; } -out: - if (nbl) { - /* dequeue it if we queued it before */ - if (fl_flags & FL_SLEEP) { - spin_lock(&nn->blocked_locks_lock); - list_del_init(&nbl->nbl_list); - list_del_init(&nbl->nbl_lru); - spin_unlock(&nn->blocked_locks_lock); - } - free_blocked_lock(nbl); + /* dequeue it if we queued it before */ + if (fl_flags & FL_SLEEP) { + spin_lock(&nn->blocked_locks_lock); + list_del_init(&nbl->nbl_list); + list_del_init(&nbl->nbl_lru); + spin_unlock(&nn->blocked_locks_lock); } + free_blocked_lock(nbl); +out: if (nf) nfsd_file_put(nf); if (lock_stp) {
New struct nfsd4_blocked_lock allocated in find_or_allocate_block() does not initialised nbl_list and nbl_lru. If conflock allocation fails rollback can call list_del_init() access uninitialized fields and corrupt memory. Fixes: 76d348fadff5 ("nfsd: have nfsd4_lock use blocking locks for v4.1+ lock") Signed-off-by: Vasily Averin <vvs@virtuozzo.com> --- fs/nfsd/nfs4state.c | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-)