2015-04-08 04:46:59

by Davide Italiano

Subject: [PATCH] ext4: move check under lock to avoid race

I originally thought that ext4_zero_range() and ext4_collapse() range
duplicated the check in fallocate(), performing it under the lock.
Dmitry explained to me how I was wrong, because there's nothing to
prevent ioctl() to convert indirect <==> extent, so the check needs to
be done with the inode lock held.
Further inspection showed that ext4_fallocate() doesn't re-check inside
the lock scope so I'm not entirely sure it's safe.
I propose this patch that moves the check inside the lock scope to
guarantee safeness. My original point remains, i.e. there's no need to
duplicate it, in particular if there's nothing that prevents things to
change.
I hope this (take two) is slightly more correct.

Davide Italiano (1):
ext4: Move check under lock scope to close a race.

fs/ext4/extents.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)

--
2.3.4

2015-04-08 04:47:04

by Davide Italiano

Subject: [PATCH] ext4: Move check under lock scope to close a race.

fallocate() checks that the file is extent-based and returns
EOPNOTSUPP in case is not. Other tasks can convert from and to
indirect and extent so it's safe to check only after grabbing
the inode mutex.

Signed-off-by: Davide Italiano <[email protected]>
---
fs/ext4/extents.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index bed4308..8a9ee08 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4934,13 +4934,6 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
if (ret)
return ret;

- /*
- * currently supporting (pre)allocate mode for extent-based
- * files _only_
- */
- if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
- return -EOPNOTSUPP;

2015-04-20 20:50:20

by Davide Italiano

Subject: Re: [PATCH] ext4: Move check under lock scope to close a race.

On Tue, Apr 7, 2015 at 9:46 PM, Davide Italiano <[email protected]> wrote:
> fallocate() checks that the file is extent-based and returns
> EOPNOTSUPP in case is not. Other tasks can convert from and to
> indirect and extent so it's safe to check only after grabbing
> the inode mutex.
>
> Signed-off-by: Davide Italiano <[email protected]>
> ---
> fs/ext4/extents.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index bed4308..8a9ee08 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4934,13 +4934,6 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
> if (ret)
> return ret;
>
> - /*
> - * currently supporting (pre)allocate mode for extent-based
> - * files _only_
> - */
> - if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)))
> - return -EOPNOTSUPP;
> -
> if (mode & FALLOC_FL_COLLAPSE_RANGE)
> return ext4_collapse_range(inode, offset, len);
>
> @@ -4962,6 +4955,15 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>
> mutex_lock(&inode->i_mutex);
>
> + /*
> + * currently supporting (pre)allocate mode for extent-based
> + * files _only_
> + */
> + if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
> + ret = -EOPNOTSUPP;
> + goto out;
> + }
> +
> if (!(mode & FALLOC_FL_KEEP_SIZE) &&
> offset + len > i_size_read(inode)) {
> new_size = offset + len;
> --
> 2.3.4
>

Any comment on this? Is it correct?

2015-05-03 03:21:47

by Theodore Ts'o

Subject: Re: [PATCH] ext4: Move check under lock scope to close a race.

On Tue, Apr 07, 2015 at 09:46:50PM -0700, Davide Italiano wrote:
> fallocate() checks that the file is extent-based and returns
> EOPNOTSUPP in case is not. Other tasks can convert from and to
> indirect and extent so it's safe to check only after grabbing
> the inode mutex.
>
> Signed-off-by: Davide Italiano <[email protected]>

Thanks, applied.

- Ted