Re: [bug report] mm: avoid leaving partial pfn mappings around in error case

[Dimitri Sivanich <sivanich@hpe.com>]

On Sun, Sep 15, 2024 at 03:14:06PM +0200, Linus Torvalds wrote:
> On Sun, 15 Sept 2024 at 14:05, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > Yep.  You're right.  Smatch doesn't count allocations as sleeping when we pass a
> > variable to for the gfp flags and those functions do "get_zeroed_page(gfp)".
> > I've been intending for years to handle bitmasks better but I've never
> > implemented that code.
> 
> That explains it. In this case, the 'variable' gfp values are
> basically identical: GFP_PGTABLE_KERNEL and GFP_PGTABLE_USER depending
> on which mm they get allocated to, and both are just variations of
> GFP_KERNEL (with __GFP_ZERO and a __GFP_ACCOUNT for the user case), so
> the exact details of th egfp flags are conditional, but they are
> unconditionally sleeping allocations.
> 
> But yeah, if smatch doesn't follow that kind of conditional gfp flags,
> it explains why smatch thought the odd gru_fault() code used to be ok,
> even though it clearly isn't.
> 
> I'm not sure why gru_fault does that preempt-disable at all, since the
> real locking seems to be that &gts->ts_ctxlock mutex, but it has done
> it since its initial commit.
> 
> And it's been very much wrong since that initial commit, but I suspect
> it never triggers in real life because the page tables probably are
> already set up.
> 
> Probably more importantly, it doesn't trigger in real life because SGI
> UV machines with that GRU hardware are probably hard to find.
> 
> Anyway, maybe somebody can explain why gru_fault() has that
> preempt_disable() in it, but it is very wrong. It always has been, and
> the recent change just made smatch notice a new case.
> 
> I don't actually see any reason for that preemption disable, and I
> suspect it could just be removed. But maybe somebody else who knows
> that odd driver can pipe up.. Adding Dimirti and Arnd just in case.
>

Removing preemption disable from gru_fault, and the rest of the driver, seems
to be OK, and is probably the best way to fix this in light of the other
instance that Dan found in gru_get_cpu_resources.

I will submit a patch to do this.