On Thu, 2012-08-16 at 21:28 +0100, David Howells wrote: > Linus Torvalds <[email protected]> wrote: > > > David? Why is there a BUG_ON() there? Killing the machine helps us exactly > > how? > > > > There seems to be some missing locking wrt idmap->idmap_key_cons > > accesses. > > I was thinking that it shouldn't be possible to get there with a non-NULL > value in idmap->idmap_key_cons, but I left an assertion in there just to be > sure - after all, we have a leak or other problems if we just overwrite the > value. > > The reason for I thought this is that there's a mutex (idmap->idmap_mutex) > guarding access to the legacy upcall-to-running-daemon method. This should be > held over the entire life of the upcall since request_key_with_auxdata() waits > for the request to be completed (it calls wait_for_key_construction()). > > > Instead, it uses the insane > > > > + cons = ACCESS_ONCE(idmap->idmap_key_cons); > > + idmap->idmap_key_cons = NULL; > > > > sequence that makes no sense. What's so special about it that it needs > > ACCESS_ONCE()? If that access is unlocked, then setting the field to > > NULL directly afterwards seems singularly unsafe. What's the logic > > behind such an access pattern? > > I did try and explain that in the preceding comment. However, you're right, > ACCESS_ONCE() isn't necessary as we write to the value immediately after. I > wonder if I needed some sort of memory barrier instead. At one point during > the development of the patch, I was clearing idmap_key_cons later in the > function and I added the ACCESS_ONCE() at that point. > > The problem I tried to fix is that once nfs_idmap_read_message() is called, > the request_key_with_auxdata() may wake up immediately and then the mutex can > get released - which means someone else can get in *before* we call > complete_request_key(). > > Previously, complete_request_key() was being called with the pointer in > idmap->idmap_key_cons - but that may not be touched from the point the key is > instantiated, so I was trying to ensure idmap->idmap_key_cons was only read > once. > > The call sequence looks something like this: > > {REQUESTER} > -->nfs_map_name_to_uid() > -->nfs_idmap_lookup_id() > -->nfs_idmap_get_key() > -->nfs_idmap_request_key(&key_type_id_resolver) > <--[fails] > mutex_lock(&idmap->idmap_mutex) > -->nfs_idmap_request_key(&key_type_id_resolver_legacy, ...) > -->request_key_with_auxdata() > -->request_key_and_link() > -->construct_key_and_link() > -->construct_key() > -->key->type->request_key: nfs_idmap_legacy_upcall() > BUG_ON(idmap->idmap_key_cons != NULL); > idmap->idmap_key_cons = cons; > -->rpc_queue_upcall() > [WAKE DAEMON] > <--[ok] rpc_queue_upcall() > <--[ok] nfs_idmap_legacy_upcall() > <--[ok] construct_key() > <--[ok] construct_key_and_link() > <--[ok] request_key_and_link() > -->wait_for_key_construction() > [SLEEP] > > {DAEMON} > -->sys_write() > -->idmap_pipe_downcall() > cons = ACCESS_ONCE(idmap->idmap_key_cons); > idmap->idmap_key_cons = NULL; > -->nfs_idmap_read_message() > -->nfs_idmap_instantiate() > -->key_instantiate_and_link() > -->__key_instantiate_and_link() > [WAKE UP REQUESTER] > > <--[ok] wait_for_key_construction() > <--[ok] request_key_with_auxdata() > [USE KEY PAYLOAD] > key_put() > <--[ok] nfs_idmap_request_key() > mutex_unlock(&idmap->idmap_mutex) > > {NEXT REQUESTER} > -->nfs_map_name_to_uid() > -->nfs_idmap_lookup_id() > -->nfs_idmap_get_key() > -->nfs_idmap_request_key(&key_type_id_resolver) > <--[fails] > mutex_lock(&idmap->idmap_mutex) > -->nfs_idmap_request_key(&key_type_id_resolver_legacy, ...) > -->request_key_with_auxdata() > -->request_key_and_link() > -->construct_key_and_link() > -->construct_key() > -->key->type->request_key: nfs_idmap_legacy_upcall() > BUG_ON(idmap->idmap_key_cons != NULL); > > So it looks like it oughtn't to be possible to get here with idmap_key_cons > being non-NULL.
Sure it is. Look at what happens if rpc_queue_upcall() fails, then look at the 2 patches at http://git.linux-nfs.org/?p=trondmy/linux-nfs.git;a=commitdiff;h=7f26d74498ca000ccfa28515b86998f0a0c2298a and http://git.linux-nfs.org/?p=trondmy/linux-nfs.git;a=commitdiff;h=d0f6eca7eb175373a61173d86f3cf4ec4a9166fa -- Trond Myklebust Linux NFS client maintainer NetApp [email protected] www.netapp.com N�����r��y����b�X��ǧv�^�){.n�+������z)����w* jg��� �����ݢj/���z�ޖ��2�ޙ����&�)ߡ�a���� �G���h��j:+v���w��٥
