[bug report] f2fs: issue small discard by LBA order

[bug report] f2fs: issue small discard by LBA order

[Dan Carpenter]

Hello Chao Yu,

The patch 20ee4382322c: "f2fs: issue small discard by LBA order" from
Jul 8, 2018, leads to the following Smatch warning:

	fs/f2fs/segment.c:1277 __issue_discard_cmd_orderly()
	warn: 'dc' was already freed.

fs/f2fs/segment.c
  1260          while (dc) {
  1261                  struct rb_node *node;
  1262  
  1263                  if (dc->state != D_PREP)
  1264                          goto next;
  1265  
  1266                  if (dpolicy->io_aware && !is_idle(sbi)) {
  1267                          io_interrupted = true;
  1268                          break;
  1269                  }
  1270  
  1271                  dcc->next_pos = dc->lstart + dc->len;
  1272                  __submit_discard_cmd(sbi, dpolicy, dc, &issued);
                                                           ^^
Smatch is complaining that __submit_discard_cmd() frees dc on the
error path.

  1273  
  1274                  if (issued >= dpolicy->max_requests)
  1275                          break;
  1276  next:
  1277                  node = rb_next(&dc->rb_node);
                                        ^^^^^^^^^^^
Dereference.

  1278                  dc = rb_entry_safe(node, struct discard_cmd, rb_node);
  1279          }
  1280  
  1281          blk_finish_plug(&plug);
  1282  

See also:
fs/f2fs/segment.c:2550 __issue_discard_cmd_range() warn: 'dc' was already freed.

regards,
dan carpenter

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [bug report] f2fs: issue small discard by LBA order

[Chao Yu]

Hi Dan,

Thanks for your report, let me try fixing this issue.

Out of curiosity, the bug was introduced in __issue_discard_cmd_range() early
instead of recent commit 20ee4382322c ("f2fs: issue small discard by LBA
order"), did Smatch missed to check original commit?

Thanks,

On 2018/8/7 16:17, Dan Carpenter wrote:
> Hello Chao Yu,
> 
> The patch 20ee4382322c: "f2fs: issue small discard by LBA order" from
> Jul 8, 2018, leads to the following Smatch warning:
> 
> 	fs/f2fs/segment.c:1277 __issue_discard_cmd_orderly()
> 	warn: 'dc' was already freed.
> 
> fs/f2fs/segment.c
>   1260          while (dc) {
>   1261                  struct rb_node *node;
>   1262  
>   1263                  if (dc->state != D_PREP)
>   1264                          goto next;
>   1265  
>   1266                  if (dpolicy->io_aware && !is_idle(sbi)) {
>   1267                          io_interrupted = true;
>   1268                          break;
>   1269                  }
>   1270  
>   1271                  dcc->next_pos = dc->lstart + dc->len;
>   1272                  __submit_discard_cmd(sbi, dpolicy, dc, &issued);
>                                                            ^^
> Smatch is complaining that __submit_discard_cmd() frees dc on the
> error path.
> 
>   1273  
>   1274                  if (issued >= dpolicy->max_requests)
>   1275                          break;
>   1276  next:
>   1277                  node = rb_next(&dc->rb_node);
>                                         ^^^^^^^^^^^
> Dereference.
> 
>   1278                  dc = rb_entry_safe(node, struct discard_cmd, rb_node);
>   1279          }
>   1280  
>   1281          blk_finish_plug(&plug);
>   1282  
> 
> See also:
> fs/f2fs/segment.c:2550 __issue_discard_cmd_range() warn: 'dc' was already freed.
> 
> regards,
> dan carpenter
> 
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [bug report] f2fs: issue small discard by LBA order

[Dan Carpenter]

On Tue, Aug 07, 2018 at 06:52:45PM +0800, Chao Yu wrote:
> Hi Dan,
> 
> Thanks for your report, let me try fixing this issue.
> 
> Out of curiosity, the bug was introduced in __issue_discard_cmd_range() early
> instead of recent commit 20ee4382322c ("f2fs: issue small discard by LBA
> order"), did Smatch missed to check original commit?
>

It does warn about __issue_discard_cmd_range() but I didn't look at
which patch introduced that warning...

This code is several months old, but I don't know why it's only warning
about it now.

regards,
dan carpenter


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [bug report] f2fs: issue small discard by LBA order

[Chao Yu]

Hi Dan,

On 2018/8/7 19:19, Dan Carpenter wrote:
> On Tue, Aug 07, 2018 at 06:52:45PM +0800, Chao Yu wrote:
>> Hi Dan,
>>
>> Thanks for your report, let me try fixing this issue.
>>
>> Out of curiosity, the bug was introduced in __issue_discard_cmd_range() early
>> instead of recent commit 20ee4382322c ("f2fs: issue small discard by LBA
>> order"), did Smatch missed to check original commit?
>>
> 
> It does warn about __issue_discard_cmd_range() but I didn't look at
> which patch introduced that warning...

Actually, it was introduced at 2017, I guess Smatch has missed to check below
patch due to some reason, if we have already setup to check each commit of
released linux.

8412663d177d9 ("f2fs: support issuing/waiting discard in range")
Author's commit time: 2017-10-04 09:08:32 +0800

> 
> This code is several months old, but I don't know why it's only warning
> about it now.

Anyway, I sent a patch to fix this issue, I have simply tested with fault
injection in discard flow, it looks there is no further error reported.

[PATCH] f2fs: fix use-after-free of dicard command entry

Thanks,

> 
> regards,
> dan carpenter
> 
> 
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot