On Tue, 27 Nov 2012 12:35:37 +0530
Suresh Jayaraman <sjayaraman@xxxxxxxx> wrote:
> On 11/26/2012 09:28 PM, Jeff Layton wrote:
> > Commit eddb079deb4 created a regression in the writepages codepath.
> > Previously, whenever it needed to check the size of the file, it did so
> > by consulting the inode->i_size field directly. With that patch, the
> > i_size was fetched once on entry into the writepages code and that value
> > was used henceforth.
> >
> > If the file is changing size though (for instance, if someone is writing
> > to it or has truncated it), then that value is likely to be wrong. This
> > can lead to data corruption. Pages past the EOF at the time that the
> > writepages call was issued may be silently dropped and ignored because
> > cifs_writepages wrongly assumes that the file must have been truncated
> > in the interim.
> >
> > Fix cifs_writepages to properly fetch the size from the inode->i_size
> > field instead to properly account for this possibility.
> >
> > Reported-and-Tested-by: Maxim Britov <ungifted01@xxxxxxxxx>
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > fs/cifs/file.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index edb25b4..70b6f4c 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -1794,7 +1794,6 @@ static int cifs_writepages(struct address_space *mapping,
> > struct TCP_Server_Info *server;
> > struct page *page;
> > int rc = 0;
> > - loff_t isize = i_size_read(mapping->host);
> >
> > /*
> > * If wsize is smaller than the page cache size, default to writing
> > @@ -1899,7 +1898,7 @@ retry:
> > */
> > set_page_writeback(page);
> >
> > - if (page_offset(page) >= isize) {
> > + if (page_offset(page) >= i_size_read(mapping->host)) {
> > done = true;
> > unlock_page(page);
> > end_page_writeback(page);
> > @@ -1932,7 +1931,8 @@ retry:
> > wdata->offset = page_offset(wdata->pages[0]);
> > wdata->pagesz = PAGE_CACHE_SIZE;
> > wdata->tailsz =
> > - min(isize - page_offset(wdata->pages[nr_pages - 1]),
> > + min(i_size_read(mapping->host) -
> > + page_offset(wdata->pages[nr_pages - 1]),
> > (loff_t)PAGE_CACHE_SIZE);
>
> Good catch. Looks correct to me. Wondering whether we would need a
> similar fix in cifs_write_begin() where we get the i_size and then
> immediately do check whether the page lies beyond EOF?
>
>
Thanks, I should also mention that this was reported here:
https://bugzilla.kernel.org/show_bug.cgi?id=50991
Regarding cifs_write_begin, I don't think so. The code does not sleep
between the point where the i_size is fetched and then later used. If
there is a race window there, it's very small.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
