* [PATCH] fs, mm: fix race in unlinking swapfile
@ 2021-07-07 20:48 Hugh Dickins
2021-07-12 9:06 ` Jan Kara
0 siblings, 1 reply; 2+ messages in thread
From: Hugh Dickins @ 2021-07-07 20:48 UTC (permalink / raw)
To: Andrew Morton
Cc: Hugh Dickins, Al Viro, Christoph Hellwig, Willy Tarreau,
linux-kernel, linux-fsdevel, linux-mm
We had a recurring situation in which admin procedures setting up
swapfiles would race with test preparation clearing away swapfiles;
and just occasionally that got stuck on a swapfile "(deleted)" which
could never be swapped off. That is not supposed to be possible.
2.6.28 commit f9454548e17c ("don't unlink an active swapfile") admitted
that it was leaving a race window open: now close it.
may_delete() makes the IS_SWAPFILE check (amongst many others) before
inode_lock has been taken on target: now repeat just that simple check
in vfs_unlink() and vfs_rename(), after taking inode_lock.
Which goes most of the way to fixing the race, but swapon() must also
check after it acquires inode_lock, that the file just opened has not
already been unlinked.
Fixes: f9454548e17c ("don't unlink an active swapfile")
Signed-off-by: Hugh Dickins <hughd@google.com>
---
fs/namei.c | 8 +++++++-
mm/swapfile.c | 6 ++++++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/fs/namei.c b/fs/namei.c
index bf6d8a738c59..ff866c07f4d2 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4024,7 +4024,9 @@ int vfs_unlink(struct user_namespace *mnt_userns, struct inode *dir,
return -EPERM;
inode_lock(target);
- if (is_local_mountpoint(dentry))
+ if (IS_SWAPFILE(target))
+ error = -EPERM;
+ else if (is_local_mountpoint(dentry))
error = -EBUSY;
else {
error = security_inode_unlink(dir, dentry);
@@ -4526,6 +4528,10 @@ int vfs_rename(struct renamedata *rd)
else if (target)
inode_lock(target);
+ error = -EPERM;
+ if (IS_SWAPFILE(source) || (target && IS_SWAPFILE(target)))
+ goto out;
+
error = -EBUSY;
if (is_local_mountpoint(old_dentry) || is_local_mountpoint(new_dentry))
goto out;
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 1e07d1c776f2..7527afd95284 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -3130,6 +3130,7 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
struct filename *name;
struct file *swap_file = NULL;
struct address_space *mapping;
+ struct dentry *dentry;
int prio;
int error;
union swap_header *swap_header;
@@ -3173,6 +3174,7 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
p->swap_file = swap_file;
mapping = swap_file->f_mapping;
+ dentry = swap_file->f_path.dentry;
inode = mapping->host;
error = claim_swapfile(p, inode);
@@ -3180,6 +3182,10 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
goto bad_swap;
inode_lock(inode);
+ if (d_unlinked(dentry) || cant_mount(dentry)) {
+ error = -ENOENT;
+ goto bad_swap_unlock_inode;
+ }
if (IS_SWAPFILE(inode)) {
error = -EBUSY;
goto bad_swap_unlock_inode;
--
2.26.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] fs, mm: fix race in unlinking swapfile
2021-07-07 20:48 [PATCH] fs, mm: fix race in unlinking swapfile Hugh Dickins
@ 2021-07-12 9:06 ` Jan Kara
0 siblings, 0 replies; 2+ messages in thread
From: Jan Kara @ 2021-07-12 9:06 UTC (permalink / raw)
To: Hugh Dickins
Cc: Andrew Morton, Al Viro, Christoph Hellwig, Willy Tarreau,
linux-kernel, linux-fsdevel, linux-mm
On Wed 07-07-21 13:48:31, Hugh Dickins wrote:
> We had a recurring situation in which admin procedures setting up
> swapfiles would race with test preparation clearing away swapfiles;
> and just occasionally that got stuck on a swapfile "(deleted)" which
> could never be swapped off. That is not supposed to be possible.
>
> 2.6.28 commit f9454548e17c ("don't unlink an active swapfile") admitted
> that it was leaving a race window open: now close it.
>
> may_delete() makes the IS_SWAPFILE check (amongst many others) before
> inode_lock has been taken on target: now repeat just that simple check
> in vfs_unlink() and vfs_rename(), after taking inode_lock.
>
> Which goes most of the way to fixing the race, but swapon() must also
> check after it acquires inode_lock, that the file just opened has not
> already been unlinked.
>
> Fixes: f9454548e17c ("don't unlink an active swapfile")
> Signed-off-by: Hugh Dickins <hughd@google.com>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/namei.c | 8 +++++++-
> mm/swapfile.c | 6 ++++++
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index bf6d8a738c59..ff866c07f4d2 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4024,7 +4024,9 @@ int vfs_unlink(struct user_namespace *mnt_userns, struct inode *dir,
> return -EPERM;
>
> inode_lock(target);
> - if (is_local_mountpoint(dentry))
> + if (IS_SWAPFILE(target))
> + error = -EPERM;
> + else if (is_local_mountpoint(dentry))
> error = -EBUSY;
> else {
> error = security_inode_unlink(dir, dentry);
> @@ -4526,6 +4528,10 @@ int vfs_rename(struct renamedata *rd)
> else if (target)
> inode_lock(target);
>
> + error = -EPERM;
> + if (IS_SWAPFILE(source) || (target && IS_SWAPFILE(target)))
> + goto out;
> +
> error = -EBUSY;
> if (is_local_mountpoint(old_dentry) || is_local_mountpoint(new_dentry))
> goto out;
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 1e07d1c776f2..7527afd95284 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -3130,6 +3130,7 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
> struct filename *name;
> struct file *swap_file = NULL;
> struct address_space *mapping;
> + struct dentry *dentry;
> int prio;
> int error;
> union swap_header *swap_header;
> @@ -3173,6 +3174,7 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
>
> p->swap_file = swap_file;
> mapping = swap_file->f_mapping;
> + dentry = swap_file->f_path.dentry;
> inode = mapping->host;
>
> error = claim_swapfile(p, inode);
> @@ -3180,6 +3182,10 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
> goto bad_swap;
>
> inode_lock(inode);
> + if (d_unlinked(dentry) || cant_mount(dentry)) {
> + error = -ENOENT;
> + goto bad_swap_unlock_inode;
> + }
> if (IS_SWAPFILE(inode)) {
> error = -EBUSY;
> goto bad_swap_unlock_inode;
> --
> 2.26.2
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-07-12 9:09 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-07 20:48 [PATCH] fs, mm: fix race in unlinking swapfile Hugh Dickins
2021-07-12 9:06 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).