linux-kernel - Re: [PATCH] mm: fix uninitialized variable warnings

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 5 Nov 2018 10:01:14 +0100
From:   Jan Kara <jack@...e.cz>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Andrew Morton <akpm@...ux-foundation.org>, Jan Kara <jack@...e.cz>,
        Michal Hocko <mhocko@...e.com>,
        Wang Long <wanglong19@...tuan.com>,
        Matthew Wilcox <willy@...radead.org>,
        Dave Chinner <dchinner@...hat.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, Tejun Heo <tj@...nel.org>
Subject: Re: [PATCH] mm: fix uninitialized variable warnings

On Fri 02-11-18 16:31:06, Arnd Bergmann wrote:
> In a rare randconfig build, I got a warning about possibly uninitialized
> variables:
> 
> mm/page-writeback.c: In function 'balance_dirty_pages':
> mm/page-writeback.c:1623:16: error: 'writeback' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>     mdtc->dirty += writeback;
>                 ^~
> mm/page-writeback.c:1624:4: error: 'filepages' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>     mdtc_calc_avail(mdtc, filepages, headroom);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> mm/page-writeback.c:1624:4: error: 'headroom' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> The compiler evidently fails to notice that the usage is in dead code
> after 'mdtc' is set to NULL when CONFIG_CGROUP_WRITEBACK is disabled.
> Adding an IS_ENABLED() check makes this clear to the compiler.
> 
> Signed-off-by: Arnd Bergmann <arnd@...db.de>

I'm surprised the compiler was not able to infer this since:

struct dirty_throttle_control * const mdtc = mdtc_valid(&mdtc_stor) ?
                                                     &mdtc_stor : NULL;

and if CONFIG_CGROUP_WRITEBACK is disabled, mdtc_valid() is defined to
'false'.  But possibly the function is just too big and the problematic
condition is in the loop so maybe it all confuses the compiler too much.

> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 3f690bae6b78..f02535b7731a 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -1611,7 +1611,7 @@ static void balance_dirty_pages(struct bdi_writeback *wb,
>  			bg_thresh = gdtc->bg_thresh;
>  		}
>  
> -		if (mdtc) {
> +		if (IS_ENABLED(CONFIG_CGROUP_WRITEBACK) && mdtc) {
>  			unsigned long filepages, headroom, writeback;

Honestly, I don't like the IS_ENABLED(CONFIG_CGROUP_WRITEBACK) check here.
It just looks too arbitrary. Could we perhaps change the code like

struct dirty_throttle_control * const mdtc = &mdtc_stor;

And then replace checks for !mtdc in the function to !mdtc_valid(mdtc)?
That is the same thing as currently and it should make it obvious to the
compiler as well as human what is going on... Tejun?

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR