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.
David
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
