| Message ID | 20210326195229.114110-1-lsahlber@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: add support for FALLOC_FL_COLLAPSE_RANGE | expand |
Ronnie Sahlberg <lsahlber@redhat.com> writes: > +static long smb3_collapse_range(struct file *file, struct cifs_tcon *tcon, > + loff_t off, loff_t len) > +{ > + int rc; > + unsigned int xid; > + struct cifsFileInfo *cfile = file->private_data; > + __le64 eof; > + > + xid = get_xid(); > + > + if (off + len < off) { > + rc = -EFBIG; > + goto out; > + } loff_t is defined as 'long long' for me which is signed, and signed overflow is Undefined Behaviour, unless we compile with -fwrapv which I'm not sure it is something we can assume. Also, vfs_fallocate() in fs/open.c already does an overflow check before calling f_op->falloc(), this is probably not needed. (It's also relying on signed overflow so I guess it is ok...?) Rest of the patch looks good otherwise. Cheers,
On Wed, Mar 31, 2021 at 12:22 AM Aurélien Aptel <aaptel@suse.com> wrote: > > Ronnie Sahlberg <lsahlber@redhat.com> writes: > > +static long smb3_collapse_range(struct file *file, struct cifs_tcon *tcon, > > + loff_t off, loff_t len) > > +{ > > + int rc; > > + unsigned int xid; > > + struct cifsFileInfo *cfile = file->private_data; > > + __le64 eof; > > + > > + xid = get_xid(); > > + > > + if (off + len < off) { > > + rc = -EFBIG; > > + goto out; > > + } > > loff_t is defined as 'long long' for me which is signed, and signed > overflow is Undefined Behaviour, unless we compile with -fwrapv which > I'm not sure it is something we can assume. > > Also, vfs_fallocate() in fs/open.c already does an overflow check before > calling f_op->falloc(), this is probably not needed. (It's also relying > on signed overflow so I guess it is ok...?) Thanks. Steve, can you drop this check from the patch? > > Rest of the patch looks good otherwise. > > Cheers, > -- > Aurélien Aptel / SUSE Labs Samba Team > GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München) >
On Tue, Mar 30, 2021 at 5:09 PM ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > > On Wed, Mar 31, 2021 at 12:22 AM Aurélien Aptel <aaptel@suse.com> wrote: > > > > Ronnie Sahlberg <lsahlber@redhat.com> writes: > > > +static long smb3_collapse_range(struct file *file, struct cifs_tcon *tcon, > > > + loff_t off, loff_t len) > > > +{ > > > + int rc; > > > + unsigned int xid; > > > + struct cifsFileInfo *cfile = file->private_data; > > > + __le64 eof; > > > + > > > + xid = get_xid(); > > > + > > > + if (off + len < off) { > > > + rc = -EFBIG; > > > + goto out; > > > + } > > > > loff_t is defined as 'long long' for me which is signed, and signed > > overflow is Undefined Behaviour, unless we compile with -fwrapv which > > I'm not sure it is something we can assume. > > > > Also, vfs_fallocate() in fs/open.c already does an overflow check before > > calling f_op->falloc(), this is probably not needed. (It's also relying > > on signed overflow so I guess it is ok...?) > > Thanks. > Steve, can you drop this check from the patch? Yes. I was trying to run some xfstests on it vs. Windows with REFS (and Samba with BTRFS after that). Any observations on these two patches and xfstests? > > Rest of the patch looks good otherwise. > > > > Cheers, > > -- > > Aurélien Aptel / SUSE Labs Samba Team
Running xfstests on this patch (along with the previous INSERT_RANGE patch) fixes at least six xfstests which were skipped before: # ./check -cifs generic/072 generic/145 generic/147 generic/153 generic/351 generic/458 FSTYP -- cifs PLATFORM -- Linux/x86_64 smfrench-Virtual-Machine 5.12.0-051200rc4-generic #202103212230 SMP Sun Mar 21 22:33:27 UTC 2021 generic/072 7s ... 6s generic/145 0s ... 1s generic/147 1s ... 0s generic/153 0s ... 1s generic/351 5s ... 3s generic/458 1s ... 1s Ran: generic/072 generic/145 generic/147 generic/153 generic/351 generic/458 Passed all 6 tests Promising ... there are additional fallocate related tests that still fail or are skipped, but good progress ... On Fri, Mar 26, 2021 at 2:52 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/smb2ops.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index 9bae7e8deb09..3bb18944aaa4 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -3640,6 +3640,44 @@ static long smb3_simple_falloc(struct file *file, struct cifs_tcon *tcon, > return rc; > } > > +static long smb3_collapse_range(struct file *file, struct cifs_tcon *tcon, > + loff_t off, loff_t len) > +{ > + int rc; > + unsigned int xid; > + struct cifsFileInfo *cfile = file->private_data; > + __le64 eof; > + > + xid = get_xid(); > + > + if (off + len < off) { > + rc = -EFBIG; > + goto out; > + } > + > + if (off >= i_size_read(file->f_inode) || > + off + len >= i_size_read(file->f_inode)) { > + rc = -EINVAL; > + goto out; > + } > + > + rc = smb2_copychunk_range(xid, cfile, cfile, off + len, > + i_size_read(file->f_inode) - off - len, off); > + if (rc < 0) > + goto out; > + > + eof = i_size_read(file->f_inode) - len; > + rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid, > + cfile->fid.volatile_fid, cfile->pid, &eof); > + if (rc < 0) > + goto out; > + > + rc = 0; > + out: > + free_xid(xid); > + return rc; > +} > + > static loff_t smb3_llseek(struct file *file, struct cifs_tcon *tcon, loff_t offset, int whence) > { > struct cifsFileInfo *wrcfile, *cfile = file->private_data; > @@ -3811,6 +3849,8 @@ static long smb3_fallocate(struct file *file, struct cifs_tcon *tcon, int mode, > return smb3_zero_range(file, tcon, off, len, false); > } else if (mode == FALLOC_FL_KEEP_SIZE) > return smb3_simple_falloc(file, tcon, off, len, true); > + else if (mode == FALLOC_FL_COLLAPSE_RANGE) > + return smb3_collapse_range(file, tcon, off, len); > else if (mode == 0) > return smb3_simple_falloc(file, tcon, off, len, false); > > -- > 2.29.2 > -- Thanks, Steve
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index 9bae7e8deb09..3bb18944aaa4 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -3640,6 +3640,44 @@ static long smb3_simple_falloc(struct file *file, struct cifs_tcon *tcon, return rc; } +static long smb3_collapse_range(struct file *file, struct cifs_tcon *tcon, + loff_t off, loff_t len) +{ + int rc; + unsigned int xid; + struct cifsFileInfo *cfile = file->private_data; + __le64 eof; + + xid = get_xid(); + + if (off + len < off) { + rc = -EFBIG; + goto out; + } + + if (off >= i_size_read(file->f_inode) || + off + len >= i_size_read(file->f_inode)) { + rc = -EINVAL; + goto out; + } + + rc = smb2_copychunk_range(xid, cfile, cfile, off + len, + i_size_read(file->f_inode) - off - len, off); + if (rc < 0) + goto out; + + eof = i_size_read(file->f_inode) - len; + rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid, + cfile->fid.volatile_fid, cfile->pid, &eof); + if (rc < 0) + goto out; + + rc = 0; + out: + free_xid(xid); + return rc; +} + static loff_t smb3_llseek(struct file *file, struct cifs_tcon *tcon, loff_t offset, int whence) { struct cifsFileInfo *wrcfile, *cfile = file->private_data; @@ -3811,6 +3849,8 @@ static long smb3_fallocate(struct file *file, struct cifs_tcon *tcon, int mode, return smb3_zero_range(file, tcon, off, len, false); } else if (mode == FALLOC_FL_KEEP_SIZE) return smb3_simple_falloc(file, tcon, off, len, true); + else if (mode == FALLOC_FL_COLLAPSE_RANGE) + return smb3_collapse_range(file, tcon, off, len); else if (mode == 0) return smb3_simple_falloc(file, tcon, off, len, false);
Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/smb2ops.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)