On Sun, 15 Sept 2024 at 14:05, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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 >s->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.
Linus
