Re: [autofs] [RFC PATCH]autofs4: hang and proposed fix

["William H. Taber" <wtaber@us.ibm.com>]

Ian Kent wrote:
> On Wed, 30 Nov 2005, William H. Taber wrote:
> 
> 
>>Trond Myklebust wrote:
>>
>>>On Wed, 2005-11-30 at 15:32 -0500, William H. Taber wrote:
>>>
>>>
>>>
>>>>Not only is there this case, but the original premise is wrong as well.
>>>>There is a second case in which a d_revalidate function is called with the
>>>>parent i_sem and that is when it is called from inside of lookup_one_len.
>>>>What makes this tricky is that lookup_one_len is called from
>>>>nfs_sillyrename from inside of nfs_rename which is called, naturally
>>>>enough by sys_rename.  The rename code is very careful about the order in
>>>>which it obtains the parent semaphores because it needs to get two of
>>>>them.  It must always obtain the locks in the same order so that does not
>>>>get into a deadly embrace.  If we start arbitrarily releasing a parent
>>>>semaphore in cached_lookup and taking it again after the revalidate, we
>>>>risk breaking the lock ordering and creating a deadly embrace.
>>>>
>>>>When I started writing this I thought that it would be safe for the autofs
>>>>revalidate code to release the parent semaphore because they do not have a
>>>>rename callback.  But I looked again at the rename code and it calls
>>>>lookup_hash on the final source and destination files after locking the
>>>>parents so the potential for a deadly embrace still exists unless there is
>>>>some other assurance that these final lookups will never pend waiting on
>>>>the automounter in either their revalidate or lookup routines.  (Actually
>>>>the requirement is that they never give up the parent i_sem lock, but the
>>>>lookup code has to give up the lock so that the autofs demon can run and
>>>>perform the mount so it amounts to the same thing.)
>>>>
>>>>The same issue exists for devfs which also releases the parent i_sem lock
>>>>so that it can wait inside its revalidation routine.
>>>
>>>
>>>So exactly why does autofs4 want to hold the dir->i_sem in d_revalidate
>>>in the first place? Can't we move any code that requires dir->i_sem to
>>>be held into a ->lookup() method?
>>
>>It's not that d_revalidate wants or doesn't want to hold the lock.  The caller
>>of lookup_one_len is required to get the lock and this function calls
>>lookup_hash which calls cached_lookup which calls d_revalidate.
>>
>>
>>>Trivially, if you have a d_revalidate that does something like
>>>
>>>int autofs_revalidate(struct dentry *dentry, struct nameidata *nd)
>>>{
>>>  d_drop(dentry);
>>>  return 0;
>>>}
>>>
>>>then the VFS will currently allocate a new dentry with the same name,
>>>and call ->lookup() on it without dropping dir->i_sem. If you still need
>>>to reference the old dentry, then put it on a private list somewhere.
>>>That would also allow you to return the old dentry as the result of the
>>>->lookup() operation if that is desirable.
>>
>>Problem with that, as I understand it and Ian Kent knows better than I, is
>>that the autofs lookup code creates the dentry and fills it in partially and
>>marks it as waiting for mounting and wakes up the automount demon.  The demon
>>completes the mount and finishes filling in the dentry.  So we cannot have
>>some other lookup coming in and removing the dentry on us.  At least that is
>>what I understand from Ian's answer when I proposed the same sort of thing to
>>him.   Even if  they end up doing something like that in a future version of
>>the automounter, I would still like a simple patch that can be applied to
>>existing systems as an interim fix.
> 
> 
> Lets see if I can keep this explaination simple.
> 
> The user space process using the autofs filesystem (autodir or automount) 
> needs to be able to call mkdir at mount time as a result of a callback 
> from revalidate. Sometimes this comes indirectly from lookup (if the 
> directory does not already exist).
> 
> lookup_one_len requires the i_sem to be held so two instances of a 
> filesystem calling it lead to a deadlock when mkdir is called from 
> userspace (the third process). In the case we are discussing this happens 
> because the first process calls lookup which releases the i_sem and 
> calls revalidate itself. The second calls revalidate which doesn't release 
> the i_sem and is places on a wait queue for mount completion. Consequently 
> the mkdir blocks.
> 
> So the requirement is that autofs release the i_sem during the callback, 
> not obtain it.
> 
> Will believes that it is not safe for autofs to release i_sem for 
> the callback to user space because it is possible that path that aquired 
> it may not be the path that has called revalidate and I can see his point.
> 
> Never the less I'm still not convinced that this is possible given the 
> restrictions of autofs.
> 
> Let me try and describe this, hopefully more clearly than I've done so 
> far.
> 
> The only operations defined for autofs are:
> 
> mkdir, rmdir, symlink and unlink 
> 
> and the only processes that can do these operations must be in the same 
> process group that mounted the filesystem. EACCESS is returned for all 
> other processes attempting these operations.
> 
> The other functionality is read-only (and perhaps triggers a mount) 
> being lookup, revalidate and readdir.
> 
> So the question is, can anyone provide an example of a path that, upon 
> calling autofs revalidate or lookup with the i_sem held, not be the path 
> that aquired it?

Any other process calling lookup_one_len on a file in /net.

Will