linux-kernel - Re: f2fs: factor out discard_cmd usage from general rb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZB3WDpunfgJZhhQy@google.com>
Date:   Fri, 24 Mar 2023 09:55:42 -0700
From:   Jaegeuk Kim <jaegeuk@...nel.org>
To:     "Colin King (gmail)" <colin.i.king@...il.com>
Cc:     Chao Yu <chao@...nel.org>, linux-f2fs-devel@...ts.sourceforge.net,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: f2fs: factor out discard_cmd usage from general rb_tree use

On 03/24, Colin King (gmail) wrote:
> Hi,
> 
> static analysis with clang scan build has detected a potential issue
> introduced by the following commit:
> 
> commit 7e9775a516ff6c1e73ee2b42ec563cafee38f42f
> Author: Jaegeuk Kim <jaegeuk@...nel.org>
> Date:   Fri Mar 10 11:12:35 2023 -0800
> 
> f2fs: factor out discard_cmd usage from general rb_tree use

Good catch!
I found the bug and will post v2 soon.

> 
> 
> The warning is as follows:
> 
> fs/f2fs/segment.c:1425:4: warning: Value stored to 'tdc' is never read
> [deadcode.DeadStores]
> 
> The while loop in function __update_discard_tree_range is as follows (+ my
> annotations):
> 
> 
>         while (1) {
>                 struct rb_node *node;
>                 struct discard_cmd *tdc = NULL;
> 
> ### tdc is set to NULL
> 
>                 if (prev_dc) {
>                         di.lstart = prev_dc->di.lstart + prev_dc->di.len;
>                         if (di.lstart < lstart)
>                                 di.lstart = lstart;
>                         if (di.lstart >= end)
>                                 break;
> 
>                         if (!next_dc || next_dc->di.lstart > end)
>                                 di.len = end - di.lstart;
>                         else
>                                 di.len = next_dc->di.lstart - di.lstart;
>                         di.start = start + di.lstart - lstart;
>                 }
> 
>                 if (!di.len)
>                         goto next;
> 
>                 if (prev_dc && prev_dc->state == D_PREP &&
>                         prev_dc->bdev == bdev &&
>                         __is_discard_back_mergeable(&di, &prev_dc->di,
> 
> max_discard_blocks)) {
>                         prev_dc->di.len += di.len;
>                         dcc->undiscard_blks += di.len;
>                         __relocate_discard_cmd(dcc, prev_dc);
>                         di = prev_dc->di;
>                         tdc = prev_dc;
> 
> ### tdc is set to prev_dc, however, it is not not read any more with th
> introduction of the "goto next"" statement introduced in the commit
> mentioned earlier
> 
>                         goto next;
>                 }
> 
>                 if (next_dc && next_dc->state == D_PREP &&
>                         next_dc->bdev == bdev &&
>                         __is_discard_front_mergeable(&di, &next_dc->di,
> 
> max_discard_blocks)) {
>                         next_dc->di.lstart = di.lstart;
>                         next_dc->di.len += di.len;
>                         next_dc->di.start = di.start;
>                         dcc->undiscard_blks += di.len;
>                         __relocate_discard_cmd(dcc, next_dc);
> 
> ### tdc is always NULL, there is no path to this code where tdc is ever set
> to a non-NULL value.
> 
>                         if (tdc)
>                                 __remove_discard_cmd(sbi, tdc);
>                         goto next;
>                 }
> 
>                 __insert_discard_cmd(sbi, bdev, di.lstart, di.start,
> di.len);
>  next:
>                 prev_dc = next_dc;
>                 if (!prev_dc)
>                         break;
> 
>                 node = rb_next(&prev_dc->rb_node);
>                 next_dc = rb_entry_safe(node, struct discard_cmd, rb_node);
>         }
>