[Ocfs2-devel] ocfs2: fix sparse file & data ordering issue in direct io

[Ocfs2-devel] ocfs2: fix sparse file & data ordering issue in direct io

[Dan Carpenter]

Hello Ryan Ding,

The patch fbe25fb91af5: "ocfs2: fix sparse file & data ordering issue
in direct io" from Feb 25, 2016, leads to the following static
checker warning:

	fs/ocfs2/aops.c:2242 ocfs2_dio_get_block()
	error: potentially dereferencing uninitialized 'wc'.

fs/ocfs2/aops.c
  2235  
  2236          ret = ocfs2_write_begin_nolock(inode->i_mapping, pos, len,
  2237                                         OCFS2_WRITE_DIRECT, NULL,
  2238                                         (void **)&wc, di_bh, NULL);
                                               ^^^^^^^^^^^^

See commit 5cffff9e2986 ('ocfs2: Fix ocfs2_page_mkwrite()') for an
explanation why a zero return here does not imply that "wc" has been
initialized.

  2239          if (ret) {
  2240                  mlog_errno(ret);
  2241                  goto unlock;
  2242          }
  2243  
  2244          desc = &wc->w_desc[0];
  2245  
  2246          p_blkno = ocfs2_clusters_to_blocks(inode->i_sb, desc->c_phys);

regards,
dan carpenter

[Ocfs2-devel] ocfs2: fix sparse file & data ordering issue in direct io

[Dan Carpenter]

I never got a response on this.  I was looking at it today and it still
looks buggy to me.

regards,
dan carpenter

On Wed, Mar 09, 2016 at 01:25:05PM +0300, Dan Carpenter wrote:
> Hello Ryan Ding,
> 
> The patch fbe25fb91af5: "ocfs2: fix sparse file & data ordering issue
> in direct io" from Feb 25, 2016, leads to the following static
> checker warning:
> 
> 	fs/ocfs2/aops.c:2242 ocfs2_dio_get_block()
> 	error: potentially dereferencing uninitialized 'wc'.
> 
> fs/ocfs2/aops.c
>   2235  
>   2236          ret = ocfs2_write_begin_nolock(inode->i_mapping, pos, len,
>   2237                                         OCFS2_WRITE_DIRECT, NULL,
>   2238                                         (void **)&wc, di_bh, NULL);
>                                                ^^^^^^^^^^^^
> 
> See commit 5cffff9e2986 ('ocfs2: Fix ocfs2_page_mkwrite()') for an
> explanation why a zero return here does not imply that "wc" has been
> initialized.
> 
>   2239          if (ret) {
>   2240                  mlog_errno(ret);
>   2241                  goto unlock;
>   2242          }
>   2243  
>   2244          desc = &wc->w_desc[0];
>   2245  
>   2246          p_blkno = ocfs2_clusters_to_blocks(inode->i_sb, desc->c_phys);
> 
> regards,
> dan carpenter

[Ocfs2-devel] ocfs2: fix sparse file & data ordering issue in direct io

[Dan Carpenter]

Ryan's email is dead.  But this is buggy.  Someone please fix it.

regards,
dan carpenter

On Tue, Nov 15, 2016 at 01:33:30PM +0300, Dan Carpenter wrote:
> I never got a response on this.  I was looking at it today and it still
> looks buggy to me.
> 
> regards,
> dan carpenter
> 
> On Wed, Mar 09, 2016 at 01:25:05PM +0300, Dan Carpenter wrote:
> > Hello Ryan Ding,
> > 
> > The patch fbe25fb91af5: "ocfs2: fix sparse file & data ordering issue
> > in direct io" from Feb 25, 2016, leads to the following static
> > checker warning:
> > 
> > 	fs/ocfs2/aops.c:2242 ocfs2_dio_get_block()
> > 	error: potentially dereferencing uninitialized 'wc'.
> > 
> > fs/ocfs2/aops.c
> >   2235  
> >   2236          ret = ocfs2_write_begin_nolock(inode->i_mapping, pos, len,
> >   2237                                         OCFS2_WRITE_DIRECT, NULL,
> >   2238                                         (void **)&wc, di_bh, NULL);
> >                                                ^^^^^^^^^^^^
> > 
> > See commit 5cffff9e2986 ('ocfs2: Fix ocfs2_page_mkwrite()') for an
> > explanation why a zero return here does not imply that "wc" has been
> > initialized.
> > 
> >   2239          if (ret) {
> >   2240                  mlog_errno(ret);
> >   2241                  goto unlock;
> >   2242          }
> >   2243  
> >   2244          desc = &wc->w_desc[0];
> >   2245  
> >   2246          p_blkno = ocfs2_clusters_to_blocks(inode->i_sb, desc->c_phys);
> > 
> > regards,
> > dan carpenter

[Ocfs2-devel] ocfs2: fix sparse file & data ordering issue in direct io

[Eric Ren]

Hi Dan,

On 11/15/2016 06:36 PM, Dan Carpenter wrote:
> Ryan's email is dead.  But this is buggy.  Someone please fix it.
>
> regards,
> dan carpenter
>
> On Tue, Nov 15, 2016 at 01:33:30PM +0300, Dan Carpenter wrote:
>> I never got a response on this.  I was looking at it today and it still
>> looks buggy to me.
>>
>> regards,
>> dan carpenter
>>
>> On Wed, Mar 09, 2016 at 01:25:05PM +0300, Dan Carpenter wrote:
>>> Hello Ryan Ding,
>>>
>>> The patch fbe25fb91af5: "ocfs2: fix sparse file & data ordering issue
>>> in direct io" from Feb 25, 2016, leads to the following static
>>> checker warning:
>>>
>>> 	fs/ocfs2/aops.c:2242 ocfs2_dio_get_block()
>>> 	error: potentially dereferencing uninitialized 'wc'.
>>>
>>> fs/ocfs2/aops.c
>>>    2235
>>>    2236          ret = ocfs2_write_begin_nolock(inode->i_mapping, pos, len,
>>>    2237                                         OCFS2_WRITE_DIRECT, NULL,
>>>    2238                                         (void **)&wc, di_bh, NULL);
>>>                                                 ^^^^^^^^^^^^
How do you perform the static checker? Please tech me;-)

Regarding this warning, please try to make this line 
(https://github.com/torvalds/linux/blob/master/fs/ocfs2/aops.c#L2128)
into:

struct ocfs2_write_ctxt *wc = NULL;

It should work, and haven't any side effect.

Eric
>>>
>>> See commit 5cffff9e2986 ('ocfs2: Fix ocfs2_page_mkwrite()') for an
>>> explanation why a zero return here does not imply that "wc" has been
>>> initialized.
>>>
>>>    2239          if (ret) {
>>>    2240                  mlog_errno(ret);
>>>    2241                  goto unlock;
>>>    2242          }
>>>    2243
>>>    2244          desc = &wc->w_desc[0];
>>>    2245
>>>    2246          p_blkno = ocfs2_clusters_to_blocks(inode->i_sb, desc->c_phys);
>>>
>>> regards,
>>> dan carpenter
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>

[Ocfs2-devel] ocfs2: fix sparse file & data ordering issue in direct io

[Dan Carpenter]

On Wed, Nov 16, 2016 at 10:33:49AM +0800, Eric Ren wrote:
> >>>fs/ocfs2/aops.c
> >>>   2235
> >>>   2236          ret = ocfs2_write_begin_nolock(inode->i_mapping, pos, len,
> >>>   2237                                         OCFS2_WRITE_DIRECT, NULL,
> >>>   2238                                         (void **)&wc, di_bh, NULL);
> >>>                                                ^^^^^^^^^^^^
> How do you perform the static checker? Please tech me;-)
> 

It's Smatch things that's not public yet.  Soon.

> Regarding this warning, please try to make this line
> (https://github.com/torvalds/linux/blob/master/fs/ocfs2/aops.c#L2128)
> into:
> 
> struct ocfs2_write_ctxt *wc = NULL;
> 
> It should work, and haven't any side effect.

That silences the warning, of course, but I feel like the code is buggy.
How do we know that we don't hit that exit path?

fs/ocfs2/aops.c
  1808          /*
  1809           * ocfs2_grab_pages_for_write() returns -EAGAIN if it could not lock
  1810           * the target page. In this case, we exit with no error and no target
  1811           * page. This will trigger the caller, page_mkwrite(), to re-try
  1812           * the operation.
  1813           */
  1814          if (ret == -EAGAIN) {
  1815                  BUG_ON(wc->w_target_page);
  1816                  ret = 0;
  1817                  goto out_quota;
  1818          }

regards,
dan carpenter

[Ocfs2-devel] ocfs2: fix sparse file & data ordering issue in direct io

[Eric Ren]

Hi,

On 11/16/2016 06:45 PM, Dan Carpenter wrote:
> On Wed, Nov 16, 2016 at 10:33:49AM +0800, Eric Ren wrote:
> That silences the warning, of course, but I feel like the code is buggy.
> How do we know that we don't hit that exit path?
Sorry, I missed your point. Do you mean the below?

"1817 goto out_quota; " will free (*wc), but with "ret = 0". Thus, the caller
think it's OK to use (*wc), but...

Do I understand you correctly?

Eric
>
> fs/ocfs2/aops.c
>    1808          /*
>    1809           * ocfs2_grab_pages_for_write() returns -EAGAIN if it could not lock
>    1810           * the target page. In this case, we exit with no error and no target
>    1811           * page. This will trigger the caller, page_mkwrite(), to re-try
>    1812           * the operation.
>    1813           */
>    1814          if (ret == -EAGAIN) {
>    1815                  BUG_ON(wc->w_target_page);
>    1816                  ret = 0;
>    1817                  goto out_quota;
>    1818          }
>
> regards,
> dan carpenter
>
>
>

[Ocfs2-devel] ocfs2: fix sparse file & data ordering issue in direct io

[Dan Carpenter]

On Thu, Nov 17, 2016 at 11:08:08AM +0800, Eric Ren wrote:
> Hi,
> 
> On 11/16/2016 06:45 PM, Dan Carpenter wrote:
> >On Wed, Nov 16, 2016 at 10:33:49AM +0800, Eric Ren wrote:
> >That silences the warning, of course, but I feel like the code is buggy.
> >How do we know that we don't hit that exit path?
> Sorry, I missed your point. Do you mean the below?
> 
> "1817 goto out_quota; " will free (*wc), but with "ret = 0". Thus, the caller
> think it's OK to use (*wc), but...
> 
> Do I understand you correctly?
> 

It doesn't free it.  It frees "wc" but not "*fsdata".  So it leaves it
unintialized on that path.  That's the issue, yes.

It could be that it's impossible to reach that path from here, but it's
not clear to me.

regards,
dan carpenter

[Ocfs2-devel] ocfs2: fix sparse file & data ordering issue in direct io

[piaojun]

Hi Carpenter,

I guess this may cause illegal memory access error as follows:

generic_perform_write
--a_ops->write_begin(file, mapping, pos, bytes, flags, &page, &fsdata);
  --ocfs2_write_begin_nolock
    --ocfs2_grab_pages_for_write
	--return -EAGAIN if not enough pages
    --'fsdata' will be uninitialized

--a_ops->write_end(file, mapping, pos, bytes, copied, page, fsdata);
  --ocfs2_write_end_nolock
    --the access of wc->w_di_bh->b_data will cause error.

if so, I suggest not eating error code and let upper level to handle
this problem.

On 2016-3-9 18:25, Dan Carpenter wrote:
> Hello Ryan Ding,
> 
> The patch fbe25fb91af5: "ocfs2: fix sparse file & data ordering issue
> in direct io" from Feb 25, 2016, leads to the following static
> checker warning:
> 
> 	fs/ocfs2/aops.c:2242 ocfs2_dio_get_block()
> 	error: potentially dereferencing uninitialized 'wc'.
> 
> fs/ocfs2/aops.c
>   2235  
>   2236          ret = ocfs2_write_begin_nolock(inode->i_mapping, pos, len,
>   2237                                         OCFS2_WRITE_DIRECT, NULL,
>   2238                                         (void **)&wc, di_bh, NULL);
>                                                ^^^^^^^^^^^^
> 
> See commit 5cffff9e2986 ('ocfs2: Fix ocfs2_page_mkwrite()') for an
> explanation why a zero return here does not imply that "wc" has been
> initialized.
> 
>   2239          if (ret) {
>   2240                  mlog_errno(ret);
>   2241                  goto unlock;
>   2242          }
>   2243  
>   2244          desc = &wc->w_desc[0];
>   2245  
>   2246          p_blkno = ocfs2_clusters_to_blocks(inode->i_sb, desc->c_phys);
> 
> regards,
> dan carpenter
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
> 
> 

[Ocfs2-devel] ocfs2: fix sparse file & data ordering issue in direct io

[piaojun]

sorry, my analysis is wrong. -EAGAIN will be eaten only when 'type' is
OCFS2_WRITE_MMAP in ocfs2_write_begin_nolock(). so uninitialized 'wc'
will not be dereferenced in ocfs2_dio_get_block() as abnormal branch
can catch error 'ret'.

thanks,
Jun

On 2016-11-17 19:58, piaojun wrote:
> Hi Carpenter,
> 
> I guess this may cause illegal memory access error as follows:
> 
> generic_perform_write
> --a_ops->write_begin(file, mapping, pos, bytes, flags, &page, &fsdata);
>   --ocfs2_write_begin_nolock
>     --ocfs2_grab_pages_for_write
> 	--return -EAGAIN if not enough pages
>     --'fsdata' will be uninitialized
> 
> --a_ops->write_end(file, mapping, pos, bytes, copied, page, fsdata);
>   --ocfs2_write_end_nolock
>     --the access of wc->w_di_bh->b_data will cause error.
> 
> if so, I suggest not eating error code and let upper level to handle
> this problem.
> 
> On 2016-3-9 18:25, Dan Carpenter wrote:
>> Hello Ryan Ding,
>>
>> The patch fbe25fb91af5: "ocfs2: fix sparse file & data ordering issue
>> in direct io" from Feb 25, 2016, leads to the following static
>> checker warning:
>>
>> 	fs/ocfs2/aops.c:2242 ocfs2_dio_get_block()
>> 	error: potentially dereferencing uninitialized 'wc'.
>>
>> fs/ocfs2/aops.c
>>   2235  
>>   2236          ret = ocfs2_write_begin_nolock(inode->i_mapping, pos, len,
>>   2237                                         OCFS2_WRITE_DIRECT, NULL,
>>   2238                                         (void **)&wc, di_bh, NULL);
>>                                                ^^^^^^^^^^^^
>>
>> See commit 5cffff9e2986 ('ocfs2: Fix ocfs2_page_mkwrite()') for an
>> explanation why a zero return here does not imply that "wc" has been
>> initialized.
>>
>>   2239          if (ret) {
>>   2240                  mlog_errno(ret);
>>   2241                  goto unlock;
>>   2242          }
>>   2243  
>>   2244          desc = &wc->w_desc[0];
>>   2245  
>>   2246          p_blkno = ocfs2_clusters_to_blocks(inode->i_sb, desc->c_phys);
>>
>> regards,
>> dan carpenter
>>
>> _______________________________________________
>> Ocfs2-devel mailing list
>> Ocfs2-devel at oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>>
>>

[Ocfs2-devel] ocfs2: fix sparse file & data ordering issue in direct io

[Junxiao Bi]

Hi Dan,

It will not cause a real issue. -EAGAIN can be only returned in
__ocfs2_page_mkwrite() path where "locked_page" is NULL, so that
function will return VM_FAULT_NOPAGE before accessing "fsdata".

Thanks,
Junxiao.

On 11/17/2016 06:03 PM, Dan Carpenter wrote:
> On Thu, Nov 17, 2016 at 11:08:08AM +0800, Eric Ren wrote:
>> Hi,
>>
>> On 11/16/2016 06:45 PM, Dan Carpenter wrote:
>>> On Wed, Nov 16, 2016 at 10:33:49AM +0800, Eric Ren wrote:
>>> That silences the warning, of course, but I feel like the code is buggy.
>>> How do we know that we don't hit that exit path?
>> Sorry, I missed your point. Do you mean the below?
>>
>> "1817 goto out_quota; " will free (*wc), but with "ret = 0". Thus, the caller
>> think it's OK to use (*wc), but...
>>
>> Do I understand you correctly?
>>
> 
> It doesn't free it.  It frees "wc" but not "*fsdata".  So it leaves it
> unintialized on that path.  That's the issue, yes.
> 
> It could be that it's impossible to reach that path from here, but it's
> not clear to me.
> 
> regards,
> dan carpenter
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
> 

[Ocfs2-devel] ocfs2: fix sparse file & data ordering issue in direct io

[Dan Carpenter]

On Tue, Nov 22, 2016 at 10:29:52AM +0800, Junxiao Bi wrote:
> Hi Dan,
> 
> It will not cause a real issue. -EAGAIN can be only returned in
> __ocfs2_page_mkwrite() path where "locked_page" is NULL, so that
> function will return VM_FAULT_NOPAGE before accessing "fsdata".
> 

Good enough.  Thanks for looking at this.

regards,
dan carpenter