On 08/07/2012 11:44 AM, Myklebust, Trond wrote: > On Tue, 2012-08-07 at 11:30 -0400, bjschuma@xxxxxxxxxx wrote: >> From: Bryan Schumaker <bjschuma@xxxxxxxxxx> >> >> 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@xxxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
