* Re: [PATCH] ocfs2: check new file size on fallocate call
2023-05-29 15:26 [PATCH] ocfs2: check new file size on fallocate call Luís Henriques
@ 2023-05-31 3:32 ` Mark Fasheh
2023-05-31 6:00 ` Joseph Qi
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Mark Fasheh @ 2023-05-31 3:32 UTC (permalink / raw)
To: Luís Henriques
Cc: Joel Becker, Joseph Qi, Heming Zhao, ocfs2-devel, linux-kernel
On Mon, May 29, 2023 at 8:26 AM Luís Henriques <lhenriques@suse.de> 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 <lhenriques@suse.de>
Looks good, thanks Luis.
Reviewed-by: Mark Fasheh <mark@fasheh.com>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ocfs2: check new file size on fallocate call
2023-05-29 15:26 [PATCH] ocfs2: check new file size on fallocate call Luís Henriques
2023-05-31 3:32 ` Mark Fasheh
@ 2023-05-31 6:00 ` Joseph Qi
2023-05-31 8:29 ` Luís Henriques
2023-05-31 8:32 ` Joseph Qi
2023-05-31 22:11 ` [Ocfs2-devel] " Andrew Morton
3 siblings, 1 reply; 8+ messages in thread
From: Joseph Qi @ 2023-05-31 6:00 UTC (permalink / raw)
To: Luís Henriques, Mark Fasheh, Joel Becker, Heming Zhao
Cc: ocfs2-devel, linux-kernel
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 <lhenriques@suse.de>
> ---
> 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;
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ocfs2: check new file size on fallocate call
2023-05-31 6:00 ` Joseph Qi
@ 2023-05-31 8:29 ` Luís Henriques
2023-05-31 8:31 ` Joseph Qi
0 siblings, 1 reply; 8+ messages in thread
From: Luís Henriques @ 2023-05-31 8:29 UTC (permalink / raw)
To: Joseph Qi
Cc: Mark Fasheh, Joel Becker, Heming Zhao, ocfs2-devel, linux-kernel
Joseph Qi <joseph.qi@linux.alibaba.com> 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 <lhenriques@suse.de>
>> ---
>> 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;
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ocfs2: check new file size on fallocate call
2023-05-31 8:29 ` Luís Henriques
@ 2023-05-31 8:31 ` Joseph Qi
0 siblings, 0 replies; 8+ messages in thread
From: Joseph Qi @ 2023-05-31 8:31 UTC (permalink / raw)
To: Luís Henriques
Cc: Mark Fasheh, Joel Becker, Heming Zhao, ocfs2-devel, linux-kernel
On 5/31/23 4:29 PM, Luís Henriques wrote:
> Joseph Qi <joseph.qi@linux.alibaba.com> 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 <lhenriques@suse.de>
>>> ---
>>> 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,
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ocfs2: check new file size on fallocate call
2023-05-29 15:26 [PATCH] ocfs2: check new file size on fallocate call Luís Henriques
2023-05-31 3:32 ` Mark Fasheh
2023-05-31 6:00 ` Joseph Qi
@ 2023-05-31 8:32 ` Joseph Qi
2023-05-31 22:11 ` [Ocfs2-devel] " Andrew Morton
3 siblings, 0 replies; 8+ messages in thread
From: Joseph Qi @ 2023-05-31 8:32 UTC (permalink / raw)
To: Luís Henriques, Mark Fasheh, Joel Becker, Heming Zhao, akpm
Cc: ocfs2-devel, linux-kernel
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 <lhenriques@suse.de>
Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> ---
> 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;
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Ocfs2-devel] [PATCH] ocfs2: check new file size on fallocate call
2023-05-29 15:26 [PATCH] ocfs2: check new file size on fallocate call Luís Henriques
` (2 preceding siblings ...)
2023-05-31 8:32 ` Joseph Qi
@ 2023-05-31 22:11 ` Andrew Morton
2023-06-01 1:23 ` Joseph Qi
3 siblings, 1 reply; 8+ messages in thread
From: Andrew Morton @ 2023-05-31 22:11 UTC (permalink / raw)
To: Luís Henriques
Cc: Luís Henriques via Ocfs2-devel, Mark Fasheh, Joel Becker,
Joseph Qi, Heming Zhao, linux-kernel
On Mon, 29 May 2023 16:26:45 +0100 Luís Henriques via Ocfs2-devel <ocfs2-devel@oss.oracle.com> 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?
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Ocfs2-devel] [PATCH] ocfs2: check new file size on fallocate call
2023-05-31 22:11 ` [Ocfs2-devel] " Andrew Morton
@ 2023-06-01 1:23 ` Joseph Qi
0 siblings, 0 replies; 8+ messages in thread
From: Joseph Qi @ 2023-06-01 1:23 UTC (permalink / raw)
To: Andrew Morton, Luís Henriques
Cc: Luís Henriques via Ocfs2-devel, Mark Fasheh, Joel Becker,
Heming Zhao, linux-kernel
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 <ocfs2-devel@oss.oracle.com> 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
^ permalink raw reply [flat|nested] 8+ messages in thread