| Message ID | 20230529152645.32680-1-lhenriques@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ocfs2: check new file size on fallocate call | expand |
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>
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;
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,
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,
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;
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?
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
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;
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(-)