On Tue, 27 Nov 2012 16:41:28 +0530 Suresh Jayaraman <[email protected]> wrote:
> On 11/27/2012 04:34 PM, Jeff Layton wrote: > > On Tue, 27 Nov 2012 12:35:37 +0530 > > Suresh Jayaraman <[email protected]> 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 <[email protected]> > >>> Signed-off-by: Jeff Layton <[email protected]> > >>> --- > >>> 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. > > > > Yeah, I too was thinking it is too small a race window but thought > anyway I'll ask. > > Steve, I think this has to be pushed to Linus asap before the merge > window begins to ensure that 3.7 includes this fix. > > Agreed -- this shouldn't wait... -- Jeff Layton <[email protected]> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
