| Message ID | 1248219723-832-1-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Fix seems logical, although would like to see the maxbytes field the correct size. If it really is a loff_t rather than unsigned why wasn't sparse warning on the vfs in sendfile when it did this incorrect cast? When did this start breaking, am a little surprised that connectathon (and the usual dbench, fsstress, fsx etc.) didn't break if sendfile was broken, and I don't think that cifs has changed in this area in a long time. Shouldn't this cc stable ... sendfile is important. On Tue, Jul 21, 2009 at 6:42 PM, Jeff Layton<jlayton@redhat.com> wrote: > This off-by-one bug causes sendfile() to not work properly. When a task > calls sendfile() on a file on a CIFS filesystem, the syscall returns -1 > and sets errno to EOVERFLOW. > > do_sendfile uses s_maxbytes to verify the returned offset of the file. > The problem there is that this value is cast to a signed value (loff_t). > When this is done on the s_maxbytes value that cifs uses, it becomes > negative and the comparisons against it fail. > > Even though s_maxbytes is an unsigned value, it seems that it's not OK > to set it in such a way that it'll end up negative when it's cast to a > signed value. These casts happen in other codepaths besides sendfile > too, but the VFS is a little hard to follow in this area and I can't > be sure if there are other bugs that this will fix. > > It's not clear to me why s_maxbytes isn't just declared as loff_t in the > first place, but either way we still need to fix these values to make > sendfile work properly. This is also an opportunity to replace the magic > bit-shift values here with the standard #defines for this. > > This fixes the reproducer program I have that does a sendfile and > will probably also fix the situation where apache is serving from a > CIFS share. > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > Â fs/cifs/connect.c | Â Â 8 ++++---- > Â 1 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index 3e9936d..82ad2a8 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -2446,10 +2446,10 @@ try_mount_again: > Â Â Â Â Â Â Â Â tcon->local_lease = volume_info->local_lease; > Â Â Â Â } > Â Â Â Â if (pSesInfo) { > - Â Â Â Â Â Â Â if (pSesInfo->capabilities & CAP_LARGE_FILES) { > - Â Â Â Â Â Â Â Â Â Â Â sb->s_maxbytes = (u64) 1 << 63; > - Â Â Â Â Â Â Â } else > - Â Â Â Â Â Â Â Â Â Â Â sb->s_maxbytes = (u64) 1 << 31; /* 2 GB */ > + Â Â Â Â Â Â Â if (pSesInfo->capabilities & CAP_LARGE_FILES) > + Â Â Â Â Â Â Â Â Â Â Â sb->s_maxbytes = MAX_LFS_FILESIZE; > + Â Â Â Â Â Â Â else > + Â Â Â Â Â Â Â Â Â Â Â sb->s_maxbytes = MAX_NON_LFS; > Â Â Â Â } > > Â Â Â Â /* BB FIXME fix time_gran to be larger for LANMAN sessions */ > -- > 1.6.0.6 > >
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 3e9936d..82ad2a8 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -2446,10 +2446,10 @@ try_mount_again: tcon->local_lease = volume_info->local_lease; } if (pSesInfo) { - if (pSesInfo->capabilities & CAP_LARGE_FILES) { - sb->s_maxbytes = (u64) 1 << 63; - } else - sb->s_maxbytes = (u64) 1 << 31; /* 2 GB */ + if (pSesInfo->capabilities & CAP_LARGE_FILES) + sb->s_maxbytes = MAX_LFS_FILESIZE; + else + sb->s_maxbytes = MAX_NON_LFS; } /* BB FIXME fix time_gran to be larger for LANMAN sessions */
This off-by-one bug causes sendfile() to not work properly. When a task calls sendfile() on a file on a CIFS filesystem, the syscall returns -1 and sets errno to EOVERFLOW. do_sendfile uses s_maxbytes to verify the returned offset of the file. The problem there is that this value is cast to a signed value (loff_t). When this is done on the s_maxbytes value that cifs uses, it becomes negative and the comparisons against it fail. Even though s_maxbytes is an unsigned value, it seems that it's not OK to set it in such a way that it'll end up negative when it's cast to a signed value. These casts happen in other codepaths besides sendfile too, but the VFS is a little hard to follow in this area and I can't be sure if there are other bugs that this will fix. It's not clear to me why s_maxbytes isn't just declared as loff_t in the first place, but either way we still need to fix these values to make sendfile work properly. This is also an opportunity to replace the magic bit-shift values here with the standard #defines for this. This fixes the reproducer program I have that does a sendfile and will probably also fix the situation where apache is serving from a CIFS share. Signed-off-by: Jeff Layton <jlayton@redhat.com> --- fs/cifs/connect.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)