On Thu, Sep 01, 2022 at 10:31:10AM +1000, NeilBrown wrote: > Thanks for this list. Keep in mind that this list is just off the top of my head - it's nowhere near complete. > d_splice_alias() happens at ->lookup time so it is already under a > shared lock. I don't see that it depends on i_rwsem - it uses i_lock > for the important locking. Nope. Process 1: rmdir foo/bar foo found and locked exclusive [*] dentry of bar found ->rmdir() instance called Process 2: stat foo/splat foo found and locked shared [*] dentry of splat does not exist anywhere dentry allocated, marked in-lookup ->lookup() instance called inode found and passed to d_splice_alias() ... ... which finds that it's a directory inode ... ... and foo/bar refers to it. E.g. it's on NFS and another client has just done mv bar splat __d_unalias() is called, to try and move existing alias (foo/bar) into the right place. It sees that no change of parent is involved, so it can just proceed to __d_move(). Process 1: forms an rmdir request to server, using ->d_name (and possibly ->d_parent) of dentry of foo/bar. It knows that ->d_name is stable, since the caller holds foo locked exclusive and all callers of __d_move() hold the old parent at least shared. In mainline process 2 will block (or, in case if it deals with different parent, try to grab the old parent of the existing alias shared and fail and with -ESTALE). With your changes process 1 will be holding foo/ locked shared, so process 2 will succeed and proceed to __d_move(), right under the nose of process 1 accessing ->d_name. If the names involved had been longer than 32 characters, it would risk accessing kfreed memory. Or fetching the length from old name and pointer from new one, walking past the end of kmalloc'ed object, etc. Sure, assuming that we are talking about NFS, server would have probably failed the RMDIR request - if you managed to form that request without oopsing, that is.
