2023-05-29 15:42:51

by Luis Henriques

Subject: [PATCH] ocfs2: check new file size on fallocate call

When changing a file size with fallocate() the new size isn't being
checked. In particular, the FSIZE ulimit isn't being checked, which makes
fstest generic/228 fail. Simply adding a call to inode_newsize_ok() fixes
this issue.

Signed-off-by: Luís Henriques <[email protected]>
---
fs/ocfs2/file.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index efb09de4343d..b173c36bcab3 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -2100,14 +2100,20 @@ static long ocfs2_fallocate(struct file *file, int mode, loff_t offset,
struct ocfs2_space_resv sr;
int change_size = 1;
int cmd = OCFS2_IOC_RESVSP64;
+ int ret = 0;

if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
return -EOPNOTSUPP;
if (!ocfs2_writes_unwritten_extents(osb))
return -EOPNOTSUPP;

- if (mode & FALLOC_FL_KEEP_SIZE)
+ if (mode & FALLOC_FL_KEEP_SIZE) {
change_size = 0;
+ } else {
+ ret = inode_newsize_ok(inode, offset + len);
+ if (ret)
+ return ret;
+ }

if (mode & FALLOC_FL_PUNCH_HOLE)
cmd = OCFS2_IOC_UNRESVSP64;

2023-05-31 04:06:05

by Mark Fasheh

Subject: Re: [PATCH] ocfs2: check new file size on fallocate call

On Mon, May 29, 2023 at 8:26 AM Luís Henriques <[email protected]> wrote:
>
> When changing a file size with fallocate() the new size isn't being
> checked. In particular, the FSIZE ulimit isn't being checked, which makes
> fstest generic/228 fail. Simply adding a call to inode_newsize_ok() fixes
> this issue.
>
> Signed-off-by: Luís Henriques <[email protected]>

Looks good, thanks Luis.

Reviewed-by: Mark Fasheh <[email protected]>

2023-05-31 06:25:04

by Joseph Qi

Subject: Re: [PATCH] ocfs2: check new file size on fallocate call

On 5/29/23 11:26 PM, Luís Henriques wrote:
> When changing a file size with fallocate() the new size isn't being
> checked. In particular, the FSIZE ulimit isn't being checked, which makes
> fstest generic/228 fail. Simply adding a call to inode_newsize_ok() fixes
> this issue.
>
> Signed-off-by: Luís Henriques <[email protected]>
> ---
> fs/ocfs2/file.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index efb09de4343d..b173c36bcab3 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -2100,14 +2100,20 @@ static long ocfs2_fallocate(struct file *file, int mode, loff_t offset,
> struct ocfs2_space_resv sr;
> int change_size = 1;
> int cmd = OCFS2_IOC_RESVSP64;
> + int ret = 0;
>
> if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
> return -EOPNOTSUPP;

This means we only support keep-size and pouch_hole.
And it seems pouch_hole will also imply keep-size.

> if (!ocfs2_writes_unwritten_extents(osb))
> return -EOPNOTSUPP;
>
> - if (mode & FALLOC_FL_KEEP_SIZE)
> + if (mode & FALLOC_FL_KEEP_SIZE) {
> change_size = 0;
> + } else {

Seems this will be a dead branch?

Thanks,
Joseph

> + ret = inode_newsize_ok(inode, offset + len);
> + if (ret)
> + return ret;
> + }
>
> if (mode & FALLOC_FL_PUNCH_HOLE)
> cmd = OCFS2_IOC_UNRESVSP64;

2023-05-31 08:38:21

by Luis Henriques

Subject: Re: [PATCH] ocfs2: check new file size on fallocate call

Joseph Qi <[email protected]> writes:

> On 5/29/23 11:26 PM, Luís Henriques wrote:
>> When changing a file size with fallocate() the new size isn't being
>> checked. In particular, the FSIZE ulimit isn't being checked, which makes
>> fstest generic/228 fail. Simply adding a call to inode_newsize_ok() fixes
>> this issue.
>>
>> Signed-off-by: Luís Henriques <[email protected]>
>> ---
>> fs/ocfs2/file.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
>> index efb09de4343d..b173c36bcab3 100644
>> --- a/fs/ocfs2/file.c
>> +++ b/fs/ocfs2/file.c
>> @@ -2100,14 +2100,20 @@ static long ocfs2_fallocate(struct file *file, int mode, loff_t offset,
>> struct ocfs2_space_resv sr;
>> int change_size = 1;
>> int cmd = OCFS2_IOC_RESVSP64;
>> + int ret = 0;
>>
>> if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
>> return -EOPNOTSUPP;
>
> This means we only support keep-size and pouch_hole.
> And it seems pouch_hole will also imply keep-size.

I think you're forgetting about mode = 0, which is also valid. And the
default '0' will allow size to be changed.

>> if (!ocfs2_writes_unwritten_extents(osb))
>> return -EOPNOTSUPP;
>>
>> - if (mode & FALLOC_FL_KEEP_SIZE)
>> + if (mode & FALLOC_FL_KEEP_SIZE) {
>> change_size = 0;
>> + } else {
>
> Seems this will be a dead branch?

Again, you need to consider '0' as a valid mode value. If you run
generic/228 without this patch you'll see that test failing because it
*does* hit this branch.

Cheers,
--
Luís

>
> Thanks,
> Joseph
>
>> + ret = inode_newsize_ok(inode, offset + len);
>> + if (ret)
>> + return ret;
>> + }
>>
>> if (mode & FALLOC_FL_PUNCH_HOLE)
>> cmd = OCFS2_IOC_UNRESVSP64;

2023-05-31 08:39:36

by Joseph Qi

Subject: Re: [PATCH] ocfs2: check new file size on fallocate call

On 5/29/23 11:26 PM, Luís Henriques wrote:
> When changing a file size with fallocate() the new size isn't being
> checked. In particular, the FSIZE ulimit isn't being checked, which makes
> fstest generic/228 fail. Simply adding a call to inode_newsize_ok() fixes
> this issue.
>
> Signed-off-by: Luís Henriques <[email protected]>

Reviewed-by: Joseph Qi <[email protected]>
> ---
> fs/ocfs2/file.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index efb09de4343d..b173c36bcab3 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -2100,14 +2100,20 @@ static long ocfs2_fallocate(struct file *file, int mode, loff_t offset,
> struct ocfs2_space_resv sr;
> int change_size = 1;
> int cmd = OCFS2_IOC_RESVSP64;
> + int ret = 0;
>
> if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
> return -EOPNOTSUPP;
> if (!ocfs2_writes_unwritten_extents(osb))
> return -EOPNOTSUPP;
>
> - if (mode & FALLOC_FL_KEEP_SIZE)
> + if (mode & FALLOC_FL_KEEP_SIZE) {
> change_size = 0;
> + } else {
> + ret = inode_newsize_ok(inode, offset + len);
> + if (ret)
> + return ret;
> + }
>
> if (mode & FALLOC_FL_PUNCH_HOLE)
> cmd = OCFS2_IOC_UNRESVSP64;

2023-05-31 08:44:46

by Joseph Qi

Subject: Re: [PATCH] ocfs2: check new file size on fallocate call

On 5/31/23 4:29 PM, Luís Henriques wrote:
> Joseph Qi <[email protected]> writes:
>
>> On 5/29/23 11:26 PM, Luís Henriques wrote:
>>> When changing a file size with fallocate() the new size isn't being
>>> checked. In particular, the FSIZE ulimit isn't being checked, which makes
>>> fstest generic/228 fail. Simply adding a call to inode_newsize_ok() fixes
>>> this issue.
>>>
>>> Signed-off-by: Luís Henriques <[email protected]>
>>> ---
>>> fs/ocfs2/file.c | 8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
>>> index efb09de4343d..b173c36bcab3 100644
>>> --- a/fs/ocfs2/file.c
>>> +++ b/fs/ocfs2/file.c
>>> @@ -2100,14 +2100,20 @@ static long ocfs2_fallocate(struct file *file, int mode, loff_t offset,
>>> struct ocfs2_space_resv sr;
>>> int change_size = 1;
>>> int cmd = OCFS2_IOC_RESVSP64;
>>> + int ret = 0;
>>>
>>> if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
>>> return -EOPNOTSUPP;
>>
>> This means we only support keep-size and pouch_hole.
>> And it seems pouch_hole will also imply keep-size.
>
> I think you're forgetting about mode = 0, which is also valid. And the
> default '0' will allow size to be changed.
>

Oops... You are right.

>>> if (!ocfs2_writes_unwritten_extents(osb))
>>> return -EOPNOTSUPP;
>>>
>>> - if (mode & FALLOC_FL_KEEP_SIZE)
>>> + if (mode & FALLOC_FL_KEEP_SIZE) {
>>> change_size = 0;
>>> + } else {
>>
>> Seems this will be a dead branch?
>
> Again, you need to consider '0' as a valid mode value. If you run
> generic/228 without this patch you'll see that test failing because it
> *does* hit this branch.
>
> Cheers,

2023-05-31 22:29:32

by Andrew Morton

Subject: Re: [Ocfs2-devel] [PATCH] ocfs2: check new file size on fallocate call

On Mon, 29 May 2023 16:26:45 +0100 Lu?s Henriques via Ocfs2-devel <[email protected]> wrote:

> When changing a file size with fallocate() the new size isn't being
> checked. In particular, the FSIZE ulimit isn't being checked, which makes
> fstest generic/228 fail. Simply adding a call to inode_newsize_ok() fixes
> this issue.
>
> ...
>
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -2100,14 +2100,20 @@ static long ocfs2_fallocate(struct file *file, int mode, loff_t offset,
> struct ocfs2_space_resv sr;
> int change_size = 1;
> int cmd = OCFS2_IOC_RESVSP64;
> + int ret = 0;
>
> if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
> return -EOPNOTSUPP;
> if (!ocfs2_writes_unwritten_extents(osb))
> return -EOPNOTSUPP;
>
> - if (mode & FALLOC_FL_KEEP_SIZE)
> + if (mode & FALLOC_FL_KEEP_SIZE) {
> change_size = 0;
> + } else {
> + ret = inode_newsize_ok(inode, offset + len);
> + if (ret)
> + return ret;
> + }
>

So userspace can exceed rlimit(RLIMIT_FSIZE).

Do we think this flaw is serious enough to justify backporting the fix
into earlier -stable kernels?

2023-06-01 01:31:37

by Joseph Qi

Subject: Re: [Ocfs2-devel] [PATCH] ocfs2: check new file size on fallocate call

On 6/1/23 6:11 AM, Andrew Morton wrote:
> On Mon, 29 May 2023 16:26:45 +0100 Luís Henriques via Ocfs2-devel <[email protected]> wrote:
>
>> When changing a file size with fallocate() the new size isn't being
>> checked. In particular, the FSIZE ulimit isn't being checked, which makes
>> fstest generic/228 fail. Simply adding a call to inode_newsize_ok() fixes
>> this issue.
>>
>> ...
>>
>> --- a/fs/ocfs2/file.c
>> +++ b/fs/ocfs2/file.c
>> @@ -2100,14 +2100,20 @@ static long ocfs2_fallocate(struct file *file, int mode, loff_t offset,
>> struct ocfs2_space_resv sr;
>> int change_size = 1;
>> int cmd = OCFS2_IOC_RESVSP64;
>> + int ret = 0;
>>
>> if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
>> return -EOPNOTSUPP;
>> if (!ocfs2_writes_unwritten_extents(osb))
>> return -EOPNOTSUPP;
>>
>> - if (mode & FALLOC_FL_KEEP_SIZE)
>> + if (mode & FALLOC_FL_KEEP_SIZE) {
>> change_size = 0;
>> + } else {
>> + ret = inode_newsize_ok(inode, offset + len);
>> + if (ret)
>> + return ret;
>> + }
>>
>
> So userspace can exceed rlimit(RLIMIT_FSIZE).
>
> Do we think this flaw is serious enough to justify backporting the fix
> into earlier -stable kernels?

I think it's worth ccing stable kernel as well.

Thanks,
Joseph