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@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. > 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.
