Ok - reviewing now
On Tue, Nov 27, 2012 at 6:00 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Tue, 27 Nov 2012 16:41:28 +0530
> Suresh Jayaraman <sjayaraman@xxxxxxxx> wrote:
>
>> On 11/27/2012 04:34 PM, Jeff Layton wrote:
>> > 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.
>> >
>>
>> 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 <jlayton@xxxxxxxxxx>
--
Thanks,
Steve
--
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
