On Mon, Mar 29, 2021 at 04:18:09PM +0100, Colin Ian King wrote: > Hi, > > Static analysis on linux-next with Coverity has found a potential > uninitialized variable issue in function __alloc_pages_bulk with the > following commit: > > commit b0e0a469733fa571ddd8fe147247c9561b51b2da > Author: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx> > Date: Mon Mar 29 11:12:24 2021 +1100 > > mm/page_alloc: add a bulk page allocator > > The analysis is as follows: > > > <SNIP> > > 5050 if (nr_pages - nr_populated == 1) > 5051 goto failed; > 5052 > 5053 /* May set ALLOC_NOFRAGMENT, fragmentation will return 1 > page. */ > 5054 gfp &= gfp_allowed_mask; > 5055 alloc_gfp = gfp; > > Uninitialized scalar variable (UNINIT) > 15. uninit_use_in_call: Using uninitialized value alloc_flags when > calling prepare_alloc_pages. > > 5056 if (!prepare_alloc_pages(gfp, 0, preferred_nid, nodemask, > &ac, &alloc_gfp, &alloc_flags)) Ok, so Coverity thinks that alloc_flags is potentially uninitialised and without digging into every part of the report, Coverity is right. > <SNIP> > > So alloc_flags in gfp_to_alloc_flags_cma is being updated with the |= > operator and we managed to get to this path with uninitialized > alloc_flags. Should alloc_flags be initialized to zero in > __alloc_page_bulk()? > You are correct about the |= updating an initial value, but I think the initialized value should be ALLOC_WMARK_LOW. A value of 0 would be the same as ALLOC_WMARK_MIN and that would allow the bulk allocator to potentially consume too many pages without waking kswapd. I'll put together a patch shortly. Thanks Colin! -- Mel Gorman SUSE Labs
