| Message ID | 20230525101624.15814-6-jack@suse.cz (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | fs: Fix directory corruption when moving directories | expand |
On Thu, May 25, 2023 at 1:17 PM Jan Kara <jack@suse.cz> wrote: > > Currently lock_two_nondirectories() is skipping any passed directories. > After vfs_rename() uses lock_two_inodes(), all the remaining four users > of this function pass only regular files to it. So drop the somewhat > unusual "skip directory" logic and instead warn if anybody passes > directory to it. This also allows us to use lock_two_inodes() in > lock_two_nondirectories() to concentrate the lock ordering logic in less > places. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/inode.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/fs/inode.c b/fs/inode.c > index 2015fa50d34a..accf5125a049 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -1140,7 +1140,7 @@ void lock_two_inodes(struct inode *inode1, struct inode *inode2, > /** > * lock_two_nondirectories - take two i_mutexes on non-directory objects > * > - * Lock any non-NULL argument that is not a directory. > + * Lock any non-NULL argument. Passed objects must not be directories. > * Zero, one or two objects may be locked by this function. > * > * @inode1: first inode to lock > @@ -1148,13 +1148,9 @@ void lock_two_inodes(struct inode *inode1, struct inode *inode2, > */ > void lock_two_nondirectories(struct inode *inode1, struct inode *inode2) > { > - if (inode1 > inode2) > - swap(inode1, inode2); > - > - if (inode1 && !S_ISDIR(inode1->i_mode)) > - inode_lock(inode1); > - if (inode2 && !S_ISDIR(inode2->i_mode) && inode2 != inode1) > - inode_lock_nested(inode2, I_MUTEX_NONDIR2); > + WARN_ON_ONCE(S_ISDIR(inode1->i_mode)); > + WARN_ON_ONCE(S_ISDIR(inode2->i_mode)); > + lock_two_inodes(inode1, inode2, I_MUTEX_NORMAL, I_MUTEX_NONDIR2); > } > EXPORT_SYMBOL(lock_two_nondirectories); > Need the same treatment to unlock_two_nondirectories() because now if someone does pass a directory they will get a warning but also a leaked lock. Thanks, Amir.
On Fri 26-05-23 15:13:06, Amir Goldstein wrote: > On Thu, May 25, 2023 at 1:17 PM Jan Kara <jack@suse.cz> wrote: > > > > Currently lock_two_nondirectories() is skipping any passed directories. > > After vfs_rename() uses lock_two_inodes(), all the remaining four users > > of this function pass only regular files to it. So drop the somewhat > > unusual "skip directory" logic and instead warn if anybody passes > > directory to it. This also allows us to use lock_two_inodes() in > > lock_two_nondirectories() to concentrate the lock ordering logic in less > > places. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/inode.c | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/fs/inode.c b/fs/inode.c > > index 2015fa50d34a..accf5125a049 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -1140,7 +1140,7 @@ void lock_two_inodes(struct inode *inode1, struct inode *inode2, > > /** > > * lock_two_nondirectories - take two i_mutexes on non-directory objects > > * > > - * Lock any non-NULL argument that is not a directory. > > + * Lock any non-NULL argument. Passed objects must not be directories. > > * Zero, one or two objects may be locked by this function. > > * > > * @inode1: first inode to lock > > @@ -1148,13 +1148,9 @@ void lock_two_inodes(struct inode *inode1, struct inode *inode2, > > */ > > void lock_two_nondirectories(struct inode *inode1, struct inode *inode2) > > { > > - if (inode1 > inode2) > > - swap(inode1, inode2); > > - > > - if (inode1 && !S_ISDIR(inode1->i_mode)) > > - inode_lock(inode1); > > - if (inode2 && !S_ISDIR(inode2->i_mode) && inode2 != inode1) > > - inode_lock_nested(inode2, I_MUTEX_NONDIR2); > > + WARN_ON_ONCE(S_ISDIR(inode1->i_mode)); > > + WARN_ON_ONCE(S_ISDIR(inode2->i_mode)); > > + lock_two_inodes(inode1, inode2, I_MUTEX_NORMAL, I_MUTEX_NONDIR2); > > } > > EXPORT_SYMBOL(lock_two_nondirectories); > > > > Need the same treatment to unlock_two_nondirectories() because now if > someone does pass a directory they will get a warning but also a leaked lock. Yes, probably that is good defensive programming. I'll update the patch. Honza
diff --git a/fs/inode.c b/fs/inode.c index 2015fa50d34a..accf5125a049 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -1140,7 +1140,7 @@ void lock_two_inodes(struct inode *inode1, struct inode *inode2, /** * lock_two_nondirectories - take two i_mutexes on non-directory objects * - * Lock any non-NULL argument that is not a directory. + * Lock any non-NULL argument. Passed objects must not be directories. * Zero, one or two objects may be locked by this function. * * @inode1: first inode to lock @@ -1148,13 +1148,9 @@ void lock_two_inodes(struct inode *inode1, struct inode *inode2, */ void lock_two_nondirectories(struct inode *inode1, struct inode *inode2) { - if (inode1 > inode2) - swap(inode1, inode2); - - if (inode1 && !S_ISDIR(inode1->i_mode)) - inode_lock(inode1); - if (inode2 && !S_ISDIR(inode2->i_mode) && inode2 != inode1) - inode_lock_nested(inode2, I_MUTEX_NONDIR2); + WARN_ON_ONCE(S_ISDIR(inode1->i_mode)); + WARN_ON_ONCE(S_ISDIR(inode2->i_mode)); + lock_two_inodes(inode1, inode2, I_MUTEX_NORMAL, I_MUTEX_NONDIR2); } EXPORT_SYMBOL(lock_two_nondirectories);
Currently lock_two_nondirectories() is skipping any passed directories. After vfs_rename() uses lock_two_inodes(), all the remaining four users of this function pass only regular files to it. So drop the somewhat unusual "skip directory" logic and instead warn if anybody passes directory to it. This also allows us to use lock_two_inodes() in lock_two_nondirectories() to concentrate the lock ordering logic in less places. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/inode.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)