Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1416327 > unrolled thread
| Started by | Oleg Drokin <green@linuxhacker.ru> |
|---|---|
| First post | 2016-06-07 17:40 +0200 |
| Last post | 2016-06-09 14:30 +0200 |
| Articles | 5 on this page of 45 — 4 participants |
Back to article view | Back to linux.kernel
Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-07 17:40 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Jeff Layton <jlayton@poochiereds.net> - 2016-06-07 19:20 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-07 19:40 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Jeff Layton <jlayton@poochiereds.net> - 2016-06-07 22:10 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-08 01:40 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Jeff Layton <jlayton@poochiereds.net> - 2016-06-08 02:10 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-08 02:50 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-08 04:30 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-08 06:00 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Jeff Layton <jlayton@poochiereds.net> - 2016-06-08 13:00 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-08 16:50 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-08 18:20 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Jeff Layton <jlayton@poochiereds.net> - 2016-06-08 19:30 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Oleg Drokin <green@linuxhacker.ru> - 2016-06-08 19:40 +0200
[PATCH] nfsd: Always lock state exclusively. Oleg Drokin <green@linuxhacker.ru> - 2016-06-09 05:00 +0200
Re: [PATCH] nfsd: Always lock state exclusively. Jeff Layton <jlayton@poochiereds.net> - 2016-06-09 12:20 +0200
[PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Oleg Drokin <green@linuxhacker.ru> - 2016-06-09 23:10 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Oleg Drokin <green@linuxhacker.ru> - 2016-06-10 06:20 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Jeff Layton <jlayton@poochiereds.net> - 2016-06-10 13:00 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file "J . Bruce Fields" <bfields@fieldses.org> - 2016-06-10 23:00 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Oleg Drokin <green@linuxhacker.ru> - 2016-06-11 17:50 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Jeff Layton <jlayton@poochiereds.net> - 2016-06-12 03:40 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Oleg Drokin <green@linuxhacker.ru> - 2016-06-12 04:10 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Jeff Layton <jlayton@poochiereds.net> - 2016-06-12 05:00 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Oleg Drokin <green@linuxhacker.ru> - 2016-06-12 05:20 +0200
Re: [PATCH] nfsd: Close a race between access checking/setting in nfs4_get_vfs_file Jeff Layton <jlayton@poochiereds.net> - 2016-06-12 15:20 +0200
[PATCH v2] nfsd: Always lock state exclusively. Oleg Drokin <green@linuxhacker.ru> - 2016-06-13 03:30 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. "J . Bruce Fields" <bfields@fieldses.org> - 2016-06-14 17:40 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. Oleg Drokin <green@linuxhacker.ru> - 2016-06-14 18:00 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. "J . Bruce Fields" <bfields@fieldses.org> - 2016-06-14 21:00 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. Jeff Layton <jlayton@poochiereds.net> - 2016-06-15 01:00 +0200
[PATCH 3/3] nfsd: Make init_open_stateid() a bit more whole Oleg Drokin <green@linuxhacker.ru> - 2016-06-15 05:30 +0200
[PATCH 0/3] nfsd state handling fixes Oleg Drokin <green@linuxhacker.ru> - 2016-06-15 05:30 +0200
[PATCH 2/3] nfsd: Extend the mutex holding region around in nfsd4_process_open2() Oleg Drokin <green@linuxhacker.ru> - 2016-06-15 05:40 +0200
[PATCH 1/3] nfsd: Always lock state exclusively. Oleg Drokin <green@linuxhacker.ru> - 2016-06-15 05:40 +0200
Re: [PATCH 0/3] nfsd state handling fixes Oleg Drokin <green@linuxhacker.ru> - 2016-06-16 04:00 +0200
Re: [PATCH 0/3] nfsd state handling fixes "J . Bruce Fields" <bfields@fieldses.org> - 2016-06-16 04:10 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. Oleg Drokin <green@linuxhacker.ru> - 2016-06-15 01:00 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. Jeff Layton <jlayton@poochiereds.net> - 2016-06-15 01:00 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. "J . Bruce Fields" <bfields@fieldses.org> - 2016-06-14 17:50 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. Oleg Drokin <green@linuxhacker.ru> - 2016-06-14 18:00 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. "J . Bruce Fields" <bfields@fieldses.org> - 2016-06-14 20:50 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. Oleg Drokin <green@linuxhacker.ru> - 2016-06-15 04:30 +0200
Re: [PATCH v2] nfsd: Always lock state exclusively. "J . Bruce Fields" <bfields@fieldses.org> - 2016-06-15 15:40 +0200
Re: Files leak from nfsd in 4.7.1-rc1 (and more?) Andrew W Elble <aweits@rit.edu> - 2016-06-09 14:30 +0200
Page 3 of 3 — ← Prev page 1 2 [3]
| From | Oleg Drokin <green@linuxhacker.ru> |
|---|---|
| Date | 2016-06-14 18:00 +0200 |
| Subject | Re: [PATCH v2] nfsd: Always lock state exclusively. |
| Message-ID | <rJYUV-6QY-7@gated-at.bofh.it> |
| In reply to | #1422026 |
On Jun 14, 2016, at 11:46 AM, J . Bruce Fields wrote:
> On Sun, Jun 12, 2016 at 09:26:27PM -0400, Oleg Drokin wrote:
>> It used to be the case that state had an rwlock that was locked for write
>> by downgrades, but for read for upgrades (opens). Well, the problem is
>> if there are two competing opens for the same state, they step on
>> each other toes potentially leading to leaking file descriptors
>> from the state structure, since access mode is a bitmap only set once.
>>
>> Extend the holding region around in nfsd4_process_open2() to avoid
>> racing entry into nfs4_get_vfs_file().
>> Make init_open_stateid() return with locked stateid to be unlocked
>> by the caller.
>>
>> Now this version held up pretty well in my testing for 24 hours.
>> It still does not address the situation if during one of the racing
>> nfs4_get_vfs_file() calls we are getting an error from one (first?)
>> of them. This is to be addressed in a separate patch after having a
>> solid reproducer (potentially using some fault injection).
>>
>> Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
>> ---
>> fs/nfsd/nfs4state.c | 47 +++++++++++++++++++++++++++--------------------
>> fs/nfsd/state.h | 2 +-
>> 2 files changed, 28 insertions(+), 21 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index f5f82e1..fa5fb5a 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -3487,6 +3487,10 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
>> struct nfs4_openowner *oo = open->op_openowner;
>> struct nfs4_ol_stateid *retstp = NULL;
>>
>> + /* We are moving these outside of the spinlocks to avoid the warnings */
>> + mutex_init(&stp->st_mutex);
>> + mutex_lock(&stp->st_mutex);
>
> A mutex_init_locked() primitive might also be convenient here.
I know! I would be able to do it under spinlock then without moving this around too.
But alas, not only there is not one, mutex documentation states this is disallowed.
> You could also take the two previous lines from the caller into this
> function instead of passing in stp, that might simplify the code.
> (Haven't checked.)
I am not really sure what do you mean here.
These lines are moved from further away in this function )well, just the init, anyway).
Having half initialisation of stp here and half in the caller sounds kind of strange
to me.
>
> --b.
>
>> +
>> spin_lock(&oo->oo_owner.so_client->cl_lock);
>> spin_lock(&fp->fi_lock);
>>
>> @@ -3502,13 +3506,14 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
>> stp->st_access_bmap = 0;
>> stp->st_deny_bmap = 0;
>> stp->st_openstp = NULL;
>> - init_rwsem(&stp->st_rwsem);
>> list_add(&stp->st_perstateowner, &oo->oo_owner.so_stateids);
>> list_add(&stp->st_perfile, &fp->fi_stateids);
>>
>> out_unlock:
>> spin_unlock(&fp->fi_lock);
>> spin_unlock(&oo->oo_owner.so_client->cl_lock);
>> + if (retstp)
>> + mutex_lock(&retstp->st_mutex);
>> return retstp;
>> }
>>
>> @@ -4335,32 +4340,34 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
>> */
>> if (stp) {
>> /* Stateid was found, this is an OPEN upgrade */
>> - down_read(&stp->st_rwsem);
>> + mutex_lock(&stp->st_mutex);
>> status = nfs4_upgrade_open(rqstp, fp, current_fh, stp, open);
>> if (status) {
>> - up_read(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> goto out;
>> }
>> } else {
>> stp = open->op_stp;
>> open->op_stp = NULL;
>> + /*
>> + * init_open_stateid() either returns a locked stateid
>> + * it found, or initializes and locks the new one we passed in
>> + */
>> swapstp = init_open_stateid(stp, fp, open);
>> if (swapstp) {
>> nfs4_put_stid(&stp->st_stid);
>> stp = swapstp;
>> - down_read(&stp->st_rwsem);
>> status = nfs4_upgrade_open(rqstp, fp, current_fh,
>> stp, open);
>> if (status) {
>> - up_read(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> goto out;
>> }
>> goto upgrade_out;
>> }
>> - down_read(&stp->st_rwsem);
>> status = nfs4_get_vfs_file(rqstp, fp, current_fh, stp, open);
>> if (status) {
>> - up_read(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> release_open_stateid(stp);
>> goto out;
>> }
>> @@ -4372,7 +4379,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
>> }
>> upgrade_out:
>> nfs4_inc_and_copy_stateid(&open->op_stateid, &stp->st_stid);
>> - up_read(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>>
>> if (nfsd4_has_session(&resp->cstate)) {
>> if (open->op_deleg_want & NFS4_SHARE_WANT_NO_DELEG) {
>> @@ -4977,12 +4984,12 @@ static __be32 nfs4_seqid_op_checks(struct nfsd4_compound_state *cstate, stateid_
>> * revoked delegations are kept only for free_stateid.
>> */
>> return nfserr_bad_stateid;
>> - down_write(&stp->st_rwsem);
>> + mutex_lock(&stp->st_mutex);
>> status = check_stateid_generation(stateid, &stp->st_stid.sc_stateid, nfsd4_has_session(cstate));
>> if (status == nfs_ok)
>> status = nfs4_check_fh(current_fh, &stp->st_stid);
>> if (status != nfs_ok)
>> - up_write(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> return status;
>> }
>>
>> @@ -5030,7 +5037,7 @@ static __be32 nfs4_preprocess_confirmed_seqid_op(struct nfsd4_compound_state *cs
>> return status;
>> oo = openowner(stp->st_stateowner);
>> if (!(oo->oo_flags & NFS4_OO_CONFIRMED)) {
>> - up_write(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> nfs4_put_stid(&stp->st_stid);
>> return nfserr_bad_stateid;
>> }
>> @@ -5062,12 +5069,12 @@ nfsd4_open_confirm(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> oo = openowner(stp->st_stateowner);
>> status = nfserr_bad_stateid;
>> if (oo->oo_flags & NFS4_OO_CONFIRMED) {
>> - up_write(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> goto put_stateid;
>> }
>> oo->oo_flags |= NFS4_OO_CONFIRMED;
>> nfs4_inc_and_copy_stateid(&oc->oc_resp_stateid, &stp->st_stid);
>> - up_write(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> dprintk("NFSD: %s: success, seqid=%d stateid=" STATEID_FMT "\n",
>> __func__, oc->oc_seqid, STATEID_VAL(&stp->st_stid.sc_stateid));
>>
>> @@ -5143,7 +5150,7 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
>> nfs4_inc_and_copy_stateid(&od->od_stateid, &stp->st_stid);
>> status = nfs_ok;
>> put_stateid:
>> - up_write(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> nfs4_put_stid(&stp->st_stid);
>> out:
>> nfsd4_bump_seqid(cstate, status);
>> @@ -5196,7 +5203,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> if (status)
>> goto out;
>> nfs4_inc_and_copy_stateid(&close->cl_stateid, &stp->st_stid);
>> - up_write(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>>
>> nfsd4_close_open_stateid(stp);
>>
>> @@ -5422,7 +5429,7 @@ init_lock_stateid(struct nfs4_ol_stateid *stp, struct nfs4_lockowner *lo,
>> stp->st_access_bmap = 0;
>> stp->st_deny_bmap = open_stp->st_deny_bmap;
>> stp->st_openstp = open_stp;
>> - init_rwsem(&stp->st_rwsem);
>> + mutex_init(&stp->st_mutex);
>> list_add(&stp->st_locks, &open_stp->st_locks);
>> list_add(&stp->st_perstateowner, &lo->lo_owner.so_stateids);
>> spin_lock(&fp->fi_lock);
>> @@ -5591,7 +5598,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> &open_stp, nn);
>> if (status)
>> goto out;
>> - up_write(&open_stp->st_rwsem);
>> + mutex_unlock(&open_stp->st_mutex);
>> open_sop = openowner(open_stp->st_stateowner);
>> status = nfserr_bad_stateid;
>> if (!same_clid(&open_sop->oo_owner.so_client->cl_clientid,
>> @@ -5600,7 +5607,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> status = lookup_or_create_lock_state(cstate, open_stp, lock,
>> &lock_stp, &new);
>> if (status == nfs_ok)
>> - down_write(&lock_stp->st_rwsem);
>> + mutex_lock(&lock_stp->st_mutex);
>> } else {
>> status = nfs4_preprocess_seqid_op(cstate,
>> lock->lk_old_lock_seqid,
>> @@ -5704,7 +5711,7 @@ out:
>> seqid_mutating_err(ntohl(status)))
>> lock_sop->lo_owner.so_seqid++;
>>
>> - up_write(&lock_stp->st_rwsem);
>> + mutex_unlock(&lock_stp->st_mutex);
>>
>> /*
>> * If this is a new, never-before-used stateid, and we are
>> @@ -5874,7 +5881,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> fput:
>> fput(filp);
>> put_stateid:
>> - up_write(&stp->st_rwsem);
>> + mutex_unlock(&stp->st_mutex);
>> nfs4_put_stid(&stp->st_stid);
>> out:
>> nfsd4_bump_seqid(cstate, status);
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index 986e51e..64053ea 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -535,7 +535,7 @@ struct nfs4_ol_stateid {
>> unsigned char st_access_bmap;
>> unsigned char st_deny_bmap;
>> struct nfs4_ol_stateid *st_openstp;
>> - struct rw_semaphore st_rwsem;
>> + struct mutex st_mutex;
>> };
>>
>> static inline struct nfs4_ol_stateid *openlockstateid(struct nfs4_stid *s)
>> --
>> 2.7.4
[toc] | [prev] | [next] | [standalone]
| From | "J . Bruce Fields" <bfields@fieldses.org> |
|---|---|
| Date | 2016-06-14 20:50 +0200 |
| Subject | Re: [PATCH v2] nfsd: Always lock state exclusively. |
| Message-ID | <rK1zr-bN-5@gated-at.bofh.it> |
| In reply to | #1422038 |
On Tue, Jun 14, 2016 at 11:56:20AM -0400, Oleg Drokin wrote:
>
> On Jun 14, 2016, at 11:46 AM, J . Bruce Fields wrote:
>
> > On Sun, Jun 12, 2016 at 09:26:27PM -0400, Oleg Drokin wrote:
> >> It used to be the case that state had an rwlock that was locked for write
> >> by downgrades, but for read for upgrades (opens). Well, the problem is
> >> if there are two competing opens for the same state, they step on
> >> each other toes potentially leading to leaking file descriptors
> >> from the state structure, since access mode is a bitmap only set once.
> >>
> >> Extend the holding region around in nfsd4_process_open2() to avoid
> >> racing entry into nfs4_get_vfs_file().
> >> Make init_open_stateid() return with locked stateid to be unlocked
> >> by the caller.
> >>
> >> Now this version held up pretty well in my testing for 24 hours.
> >> It still does not address the situation if during one of the racing
> >> nfs4_get_vfs_file() calls we are getting an error from one (first?)
> >> of them. This is to be addressed in a separate patch after having a
> >> solid reproducer (potentially using some fault injection).
> >>
> >> Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
> >> ---
> >> fs/nfsd/nfs4state.c | 47 +++++++++++++++++++++++++++--------------------
> >> fs/nfsd/state.h | 2 +-
> >> 2 files changed, 28 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index f5f82e1..fa5fb5a 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -3487,6 +3487,10 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
> >> struct nfs4_openowner *oo = open->op_openowner;
> >> struct nfs4_ol_stateid *retstp = NULL;
> >>
> >> + /* We are moving these outside of the spinlocks to avoid the warnings */
> >> + mutex_init(&stp->st_mutex);
> >> + mutex_lock(&stp->st_mutex);
> >
> > A mutex_init_locked() primitive might also be convenient here.
>
> I know! I would be able to do it under spinlock then without moving this around too.
>
> But alas, not only there is not one, mutex documentation states this is disallowed.
You're just talking about this comment?:
* It is not allowed to initialize an already locked mutex.
That's a weird comment. You're proably right that what they meant was
something like "It is not allowed to initialize a mutex to locked
state". But, I don't know, taken literally that comment doesn't make
sense (how could you even distinguish between an already-locked mutex
and an uninitialized mutex?), so maybe it'd be worth asking.
> > You could also take the two previous lines from the caller into this
> > function instead of passing in stp, that might simplify the code.
> > (Haven't checked.)
>
> I am not really sure what do you mean here.
> These lines are moved from further away in this function )well, just the init, anyway).
>
> Having half initialisation of stp here and half in the caller sounds kind of strange
> to me.
I was thinking of something like the following--so init_open_stateid
hides more of the details of the swapping. Untested. Does it look like
an improvement to you?
There's got to be a way to make this code a little less convoluted....
--b.
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index fa5fb5aa4847..41b59854c40f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3480,13 +3480,15 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
}
static struct nfs4_ol_stateid *
-init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
- struct nfsd4_open *open)
+init_open_stateid(struct nfs4_file *fp, struct nfsd4_open *open)
{
struct nfs4_openowner *oo = open->op_openowner;
struct nfs4_ol_stateid *retstp = NULL;
+ struct nfs4_ol_stateid *stp;
+ stp = open->op_stp;
+ open->op_stp = NULL;
/* We are moving these outside of the spinlocks to avoid the warnings */
mutex_init(&stp->st_mutex);
mutex_lock(&stp->st_mutex);
@@ -3512,9 +3514,12 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
out_unlock:
spin_unlock(&fp->fi_lock);
spin_unlock(&oo->oo_owner.so_client->cl_lock);
- if (retstp)
- mutex_lock(&retstp->st_mutex);
- return retstp;
+ if (retstp) {
+ nfs4_put_stid(&stp->st_stid);
+ stp = retstp;
+ mutex_lock(&stp->st_mutex);
+ }
+ return stp;
}
/*
@@ -4310,7 +4315,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
struct nfs4_client *cl = open->op_openowner->oo_owner.so_client;
struct nfs4_file *fp = NULL;
struct nfs4_ol_stateid *stp = NULL;
- struct nfs4_ol_stateid *swapstp = NULL;
struct nfs4_delegation *dp = NULL;
__be32 status;
@@ -4347,16 +4351,9 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
goto out;
}
} else {
- stp = open->op_stp;
- open->op_stp = NULL;
- /*
- * init_open_stateid() either returns a locked stateid
- * it found, or initializes and locks the new one we passed in
- */
- swapstp = init_open_stateid(stp, fp, open);
- if (swapstp) {
- nfs4_put_stid(&stp->st_stid);
- stp = swapstp;
+ /* stp is returned locked: */
+ stp = init_open_stateid(fp, open);
+ if (stp->st_access_bmap == 0) {
status = nfs4_upgrade_open(rqstp, fp, current_fh,
stp, open);
if (status) {
[toc] | [prev] | [next] | [standalone]
| From | Oleg Drokin <green@linuxhacker.ru> |
|---|---|
| Date | 2016-06-15 04:30 +0200 |
| Subject | Re: [PATCH v2] nfsd: Always lock state exclusively. |
| Message-ID | <rK8KH-4TL-27@gated-at.bofh.it> |
| In reply to | #1422216 |
On Jun 14, 2016, at 2:46 PM, J . Bruce Fields wrote:
> On Tue, Jun 14, 2016 at 11:56:20AM -0400, Oleg Drokin wrote:
>>
>> On Jun 14, 2016, at 11:46 AM, J . Bruce Fields wrote:
>>
>>> On Sun, Jun 12, 2016 at 09:26:27PM -0400, Oleg Drokin wrote:
>>>> It used to be the case that state had an rwlock that was locked for write
>>>> by downgrades, but for read for upgrades (opens). Well, the problem is
>>>> if there are two competing opens for the same state, they step on
>>>> each other toes potentially leading to leaking file descriptors
>>>> from the state structure, since access mode is a bitmap only set once.
>>>>
>>>> Extend the holding region around in nfsd4_process_open2() to avoid
>>>> racing entry into nfs4_get_vfs_file().
>>>> Make init_open_stateid() return with locked stateid to be unlocked
>>>> by the caller.
>>>>
>>>> Now this version held up pretty well in my testing for 24 hours.
>>>> It still does not address the situation if during one of the racing
>>>> nfs4_get_vfs_file() calls we are getting an error from one (first?)
>>>> of them. This is to be addressed in a separate patch after having a
>>>> solid reproducer (potentially using some fault injection).
>>>>
>>>> Signed-off-by: Oleg Drokin <green@linuxhacker.ru>
>>>> ---
>>>> fs/nfsd/nfs4state.c | 47 +++++++++++++++++++++++++++--------------------
>>>> fs/nfsd/state.h | 2 +-
>>>> 2 files changed, 28 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index f5f82e1..fa5fb5a 100644
>>>> --- a/fs/nfsd/nfs4state.c
>>>> +++ b/fs/nfsd/nfs4state.c
>>>> @@ -3487,6 +3487,10 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
>>>> struct nfs4_openowner *oo = open->op_openowner;
>>>> struct nfs4_ol_stateid *retstp = NULL;
>>>>
>>>> + /* We are moving these outside of the spinlocks to avoid the warnings */
>>>> + mutex_init(&stp->st_mutex);
>>>> + mutex_lock(&stp->st_mutex);
>>>
>>> A mutex_init_locked() primitive might also be convenient here.
>>
>> I know! I would be able to do it under spinlock then without moving this around too.
>>
>> But alas, not only there is not one, mutex documentation states this is disallowed.
>
> You're just talking about this comment?:
>
> * It is not allowed to initialize an already locked mutex.
>
> That's a weird comment. You're proably right that what they meant was
> something like "It is not allowed to initialize a mutex to locked
> state". But, I don't know, taken literally that comment doesn't make
> sense (how could you even distinguish between an already-locked mutex
> and an uninitialized mutex?), so maybe it'd be worth asking.
I think this is because of the strict ownership tracking or something.
I guess I can ask.
>>> You could also take the two previous lines from the caller into this
>>> function instead of passing in stp, that might simplify the code.
>>> (Haven't checked.)
>>
>> I am not really sure what do you mean here.
>> These lines are moved from further away in this function )well, just the init, anyway).
>>
>> Having half initialisation of stp here and half in the caller sounds kind of strange
>> to me.
>
> I was thinking of something like the following--so init_open_stateid
> hides more of the details of the swapping. Untested. Does it look like
> an improvement to you?
>
> There's got to be a way to make this code a little less convoluted....
>
> --b.
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index fa5fb5aa4847..41b59854c40f 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3480,13 +3480,15 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
> }
>
> static struct nfs4_ol_stateid *
> -init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
> - struct nfsd4_open *open)
> +init_open_stateid(struct nfs4_file *fp, struct nfsd4_open *open)
> {
>
> struct nfs4_openowner *oo = open->op_openowner;
> struct nfs4_ol_stateid *retstp = NULL;
> + struct nfs4_ol_stateid *stp;
>
> + stp = open->op_stp;
> + open->op_stp = NULL;
> /* We are moving these outside of the spinlocks to avoid the warnings */
> mutex_init(&stp->st_mutex);
> mutex_lock(&stp->st_mutex);
> @@ -3512,9 +3514,12 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
> out_unlock:
> spin_unlock(&fp->fi_lock);
> spin_unlock(&oo->oo_owner.so_client->cl_lock);
> - if (retstp)
> - mutex_lock(&retstp->st_mutex);
> - return retstp;
> + if (retstp) {
> + nfs4_put_stid(&stp->st_stid);
So as I am trying to integrate this into my patchset,
do we really need this?
We don't if we took the other path and left this one
hanging off the struct nfsd4_open (why do we need to
assign it NULL before the search?) I imagine then
we'd save some free/realloc churn as well?
I assume struct nfsd4_open cannot be shared between threads?
Otherwise we have bigger problems at hand like mutex init on a locked
mutex from another thread and stuff.
I'll try this theory I guess.
> + stp = retstp;
> + mutex_lock(&stp->st_mutex);
> + }
> + return stp;
> }
>
> /*
> @@ -4310,7 +4315,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
> struct nfs4_client *cl = open->op_openowner->oo_owner.so_client;
> struct nfs4_file *fp = NULL;
> struct nfs4_ol_stateid *stp = NULL;
> - struct nfs4_ol_stateid *swapstp = NULL;
> struct nfs4_delegation *dp = NULL;
> __be32 status;
>
> @@ -4347,16 +4351,9 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
> goto out;
> }
> } else {
> - stp = open->op_stp;
> - open->op_stp = NULL;
> - /*
> - * init_open_stateid() either returns a locked stateid
> - * it found, or initializes and locks the new one we passed in
> - */
> - swapstp = init_open_stateid(stp, fp, open);
> - if (swapstp) {
> - nfs4_put_stid(&stp->st_stid);
> - stp = swapstp;
> + /* stp is returned locked: */
> + stp = init_open_stateid(fp, open);
> + if (stp->st_access_bmap == 0) {
> status = nfs4_upgrade_open(rqstp, fp, current_fh,
> stp, open);
> if (status) {
[toc] | [prev] | [next] | [standalone]
| From | "J . Bruce Fields" <bfields@fieldses.org> |
|---|---|
| Date | 2016-06-15 15:40 +0200 |
| Subject | Re: [PATCH v2] nfsd: Always lock state exclusively. |
| Message-ID | <rKjcZ-37G-17@gated-at.bofh.it> |
| In reply to | #1422499 |
On Tue, Jun 14, 2016 at 10:19:49PM -0400, Oleg Drokin wrote:
> On Jun 14, 2016, at 2:46 PM, J . Bruce Fields wrote:
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index fa5fb5aa4847..41b59854c40f 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -3480,13 +3480,15 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
> > }
> >
> > static struct nfs4_ol_stateid *
> > -init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
> > - struct nfsd4_open *open)
> > +init_open_stateid(struct nfs4_file *fp, struct nfsd4_open *open)
> > {
> >
> > struct nfs4_openowner *oo = open->op_openowner;
> > struct nfs4_ol_stateid *retstp = NULL;
> > + struct nfs4_ol_stateid *stp;
> >
> > + stp = open->op_stp;
> > + open->op_stp = NULL;
> > /* We are moving these outside of the spinlocks to avoid the warnings */
> > mutex_init(&stp->st_mutex);
> > mutex_lock(&stp->st_mutex);
> > @@ -3512,9 +3514,12 @@ init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp,
> > out_unlock:
> > spin_unlock(&fp->fi_lock);
> > spin_unlock(&oo->oo_owner.so_client->cl_lock);
> > - if (retstp)
> > - mutex_lock(&retstp->st_mutex);
> > - return retstp;
> > + if (retstp) {
> > + nfs4_put_stid(&stp->st_stid);
>
> So as I am trying to integrate this into my patchset,
> do we really need this?
> We don't if we took the other path and left this one
> hanging off the struct nfsd4_open (why do we need to
> assign it NULL before the search?) I imagine then
> we'd save some free/realloc churn as well?
Yes, good idea.
> I assume struct nfsd4_open cannot be shared between threads?
Right.
> Otherwise we have bigger problems at hand like mutex init on a locked
> mutex from another thread and stuff.
>
> I'll try this theory I guess.
Sounds good!
--b.
[toc] | [prev] | [next] | [standalone]
| From | Andrew W Elble <aweits@rit.edu> |
|---|---|
| Date | 2016-06-09 14:30 +0200 |
| Message-ID | <rI7fX-6hH-19@gated-at.bofh.it> |
| In reply to | #1417279 |
> But...suppose we have two racing OPEN calls. They're both in > nfs4_get_vfs_file. One opens the file and succeeds and the other fails > and ends up in out_put_access. At that point, you could end up > clobbering the successful update to st_access_bmap from the other task, > and we'd end up not putting the file access references in > release_all_access. Pondering further (with an assumption about the workload), isn't this path also likely to involve a lease break in progress on that file? Thanks, Andy -- Andrew W. Elble aweits@discipline.rit.edu Infrastructure Engineer, Communications Technical Lead Rochester Institute of Technology PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912
[toc] | [prev] | [standalone]
Page 3 of 3 — ← Prev page 1 2 [3]
Back to top | Article view | linux.kernel
csiph-web