| Message ID | 1344353431-28831-1-git-send-email-bjschuma@netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2012-08-07 at 11:30 -0400, bjschuma@netapp.com wrote: > From: Bryan Schumaker <bjschuma@netapp.com> > > idmap_pipe_downcall already clears this field if the upcall succeeds, > but if it fails (rpc.idmapd isn't running) the field will still be set > on the next call triggering a BUG_ON(). > > Signed-off-by: Bryan Schumaker <bjschuma@netapp.com> > --- > fs/nfs/idmap.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c > index b701358..645cfe7 100644 > --- a/fs/nfs/idmap.c > +++ b/fs/nfs/idmap.c > @@ -683,10 +683,12 @@ static int nfs_idmap_legacy_upcall(struct key_construction *cons, > > ret = rpc_queue_upcall(idmap->idmap_pipe, msg); > if (ret < 0) > - goto out2; > + goto out3; > > return ret; > > +out3: > + idmap->idmap_key_cons = NULL; > out2: > kfree(im); > out1: That fixes rpc_queue_upcall failure path, but we still need to deal with timeouts and close. How about the following alternative: Set idmap->idmap_key_cons after grabbing the mutex in nfs_idmap_get_key(), then clear it before you release that mutex. That leaves one possible race in which the idmap->idmap_key_cons is cleared while we're in idmap_pipe_downcall. One way to solve that race is to use a second mutex (see the original idmapper design from before your changes) to ensure that nothing clears idmap_key_cons while we're in idmap_pipe_downcall. -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
On 08/07/2012 11:44 AM, Myklebust, Trond wrote: > On Tue, 2012-08-07 at 11:30 -0400, bjschuma@netapp.com wrote: >> From: Bryan Schumaker <bjschuma@netapp.com> >> >> idmap_pipe_downcall already clears this field if the upcall succeeds, >> but if it fails (rpc.idmapd isn't running) the field will still be set >> on the next call triggering a BUG_ON(). >> >> Signed-off-by: Bryan Schumaker <bjschuma@netapp.com> >> --- >> fs/nfs/idmap.c | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c >> index b701358..645cfe7 100644 >> --- a/fs/nfs/idmap.c >> +++ b/fs/nfs/idmap.c >> @@ -683,10 +683,12 @@ static int nfs_idmap_legacy_upcall(struct key_construction *cons, >> >> ret = rpc_queue_upcall(idmap->idmap_pipe, msg); >> if (ret < 0) >> - goto out2; >> + goto out3; >> >> return ret; >> >> +out3: >> + idmap->idmap_key_cons = NULL; >> out2: >> kfree(im); >> out1: > > That fixes rpc_queue_upcall failure path, but we still need to deal with > timeouts and close. > > How about the following alternative: > > Set idmap->idmap_key_cons after grabbing the mutex in > nfs_idmap_get_key(), then clear it before you release that mutex. That The key construction data doesn't exist during nfs_idmap_get_key(). request_key() sets it up to pass to our functions as it works through the keyrings code. - Bryan > leaves one possible race in which the idmap->idmap_key_cons is cleared > while we're in idmap_pipe_downcall. One way to solve that race is to use > a second mutex (see the original idmapper design from before your > changes) to ensure that nothing clears idmap_key_cons while we're in > idmap_pipe_downcall. > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2012-08-07 at 11:52 -0400, Bryan Schumaker wrote: > On 08/07/2012 11:44 AM, Myklebust, Trond wrote: > > On Tue, 2012-08-07 at 11:30 -0400, bjschuma@netapp.com wrote: > >> From: Bryan Schumaker <bjschuma@netapp.com> > >> > >> idmap_pipe_downcall already clears this field if the upcall succeeds, > >> but if it fails (rpc.idmapd isn't running) the field will still be set > >> on the next call triggering a BUG_ON(). > >> > >> Signed-off-by: Bryan Schumaker <bjschuma@netapp.com> > >> --- > >> fs/nfs/idmap.c | 16 +++++++++------- > >> 1 file changed, 9 insertions(+), 7 deletions(-) > >> > >> diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c > >> index b701358..645cfe7 100644 > >> --- a/fs/nfs/idmap.c > >> +++ b/fs/nfs/idmap.c > >> @@ -683,10 +683,12 @@ static int nfs_idmap_legacy_upcall(struct key_construction *cons, > >> > >> ret = rpc_queue_upcall(idmap->idmap_pipe, msg); > >> if (ret < 0) > >> - goto out2; > >> + goto out3; > >> > >> return ret; > >> > >> +out3: > >> + idmap->idmap_key_cons = NULL; > >> out2: > >> kfree(im); > >> out1: > > > > That fixes rpc_queue_upcall failure path, but we still need to deal with > > timeouts and close. > > > > How about the following alternative: > > > > Set idmap->idmap_key_cons after grabbing the mutex in > > nfs_idmap_get_key(), then clear it before you release that mutex. That > > The key construction data doesn't exist during nfs_idmap_get_key(). request_key() sets it up to pass to our functions as it works through the keyrings code. OK, however you can still clear idmap_key_cons there... > > leaves one possible race in which the idmap->idmap_key_cons is cleared > > while we're in idmap_pipe_downcall. One way to solve that race is to use > > a second mutex (see the original idmapper design from before your > > changes) to ensure that nothing clears idmap_key_cons while we're in > > idmap_pipe_downcall. This would still apply. -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c index b701358..645cfe7 100644 --- a/fs/nfs/idmap.c +++ b/fs/nfs/idmap.c @@ -683,10 +683,12 @@ static int nfs_idmap_legacy_upcall(struct key_construction *cons, ret = rpc_queue_upcall(idmap->idmap_pipe, msg); if (ret < 0) - goto out2; + goto out3; return ret; +out3: + idmap->idmap_key_cons = NULL; out2: kfree(im); out1: