[PATCH] cifs: fix writeback race with file that is growing

[PATCH] cifs: fix writeback race with file that is growing

[Jeff Layton]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 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);
 		wdata->bytes = ((nr_pages - 1) * PAGE_CACHE_SIZE) +
 					wdata->tailsz;
-- 
1.7.11.7

Re: [PATCH] cifs: fix writeback race with file that is growing

[Suresh Jayaraman]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  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?


-- 
Suresh Jayaraman

Re: [PATCH] cifs: fix writeback race with file that is growing

[Jeff Layton]

On Tue, 27 Nov 2012 12:35:37 +0530
Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: fix writeback race with file that is growing

[Suresh Jayaraman]

On 11/27/2012 04:34 PM, Jeff Layton wrote:
> On Tue, 27 Nov 2012 12:35:37 +0530
> Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  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.


-- 
Suresh Jayaraman

Re: [PATCH] cifs: fix writeback race with file that is growing

[Jeff Layton]

On Tue, 27 Nov 2012 16:41:28 +0530
Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org> wrote:

> On 11/27/2012 04:34 PM, Jeff Layton wrote:
> > On Tue, 27 Nov 2012 12:35:37 +0530
> > Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>> ---
> >>>  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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: fix writeback race with file that is growing

[Steve French]

Ok - reviewing now

On Tue, Nov 27, 2012 at 6:00 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, 27 Nov 2012 16:41:28 +0530
> Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org> wrote:
>
>> On 11/27/2012 04:34 PM, Jeff Layton wrote:
>> > On Tue, 27 Nov 2012 12:35:37 +0530
>> > Suresh Jayaraman <sjayaraman-IBi9RG/b67k@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >>> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> >>> ---
>> >>>  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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>



-- 
Thanks,

Steve