Hi Geert, I got unnecessarily annoyed/angry in my previous reply, I'm sorry for that. It clouded my judgement a bit then but I still have some comments now
On 2026-08-25 at 14:15:34 +0200, Geert Uytterhoeven wrote: > Hi Krzysztof, > > On Tue, 25 Aug 2026 at 13:39, Krzysztof Niemiec > <[email protected]> wrote: > > On 2026-08-24 at 17:12:30 +0200, Geert Uytterhoeven wrote: > > > Enabling a (modular) test should not silently enable additional kernel > > > functionality, as that may increase the attack vector for a product. > > > > What vector? What product, exactly? As far as I know buddy allocator code is > > Any product that uses DRM and has CONFIG_KUNIT_ALL_TESTS=m, so > developers can easily run any relevant test when the need arises. > > > currently loaded when a) loading a drm driver (which would need that code > > Not just "a drm driver": very few DRM drivers use it. > Hence one can have a product with DRM that does not need the buddy > allocator. > And the buddy allocator was moved out of DRM in anticipation of a possible non-DRM driver that wants the buddy allocator. So there's no implication between DRM and buddy in either way anymore. Currently it's only DRM drivers that actually use it, but there's already scaffolding in rust for it that does not depend on DRM, and once there's a non-DRM driver that wants to use the allocator it would have to undo the dependency added in this patch anyway. > > to function anyway) and/or b) loading the kunit test for the buddy > > allocator, > > which would need that code to function anyway. And AFAIK neither are > > built unconditionally into the kernel. So the amount of potential vuln > > surface is not exposed beyond what is reasonably required for the system to > > function as intended. > > > > If you've found a concrete bug/exploit/"attack vector" utilizing buddy code, > > then please address that directly. > > So let's include all available code in the kernel image anyway, unless > you can point at a concrete bug/exploit/"attack vector"??? > > > > Fix this by reverting the select to a dependency. > > > > The way drm kunit tests do it is by select-ing the appropriate > > functionality out of the available ones, with "broad" depends-ons for > > DRM and such (say "depends on DRM\n\n select DRM_BUDDY"). DRM_BUDDY, > > for example, is not directly selectable in .config, as it's meant to be > > selected automatically if a module uses it. > > In general, test code must not select the code to be tested, but depend > on it, to avoid that enabling CONFIG_KUNIT_ALL_TESTS pulls in lots of > unneeded code. > Yeah and this didn't click for me when replying, of course this increases available symbols in the kernel which increases attack surface. And the DRM test case is different since I guess if DRM is selected it's an implicit agreement from the user to pull in smaller DRM components as needed? I still think the statement about the attack vector in the commit log is a bit broad and vague though, maybe just stating that "building with KUNIT_ALL_TESTS unconditionally pulls in code from gpu buddy, hence increasing the attack surface" would say that same thing but more to the point? > > This is also the case for drivers, with Xe having depends on DRM and > > then like 40 lines of selects for specific functionality, same for i915, > > amdgpu, so on. > > For drivers, it is different: if you want the driver, you obviously > need the helper or library code. > > > > Make sure the GPU > > > buddy allocator can be tested without enabling another driver that uses > > > DRM_BUDDY by making GPU_BUDDY visible if KUNIT is enabled. > > > > This was the case even without your patch, and was the entire point of > > 8fe3fc37564920ae mentioned in Fixes. The kunit test kernel can be built with > > Yes, that was the intent of commit 8fe3fc37564920ae, but it used the > wrong method. > > It is customary to make library code that is only selected by its > users, and cannot be enabled explicitly, visible for testing. > See e.g. commit bf2d44d07de726b0 ("ASoC: wm_adsp: Add kunit test for > firmware file search") > > > a rather minimal .kunitconfig (included in the gpu/tests directory). So > > it only loads whatever kunit_tool would load, plus CONFIG_KUNIT, plus > > CONFIG_GPU_BUDDY_KUNIT_TEST, plus (implicitly, which I discussed above) > > CONFIG_GPU_BUDDY. It doesn't load DRM_BUDDY at all (which was the bug > > being fixed in that commit, you used to be forced to load it, now you > > aren't) > > > > > The page based buddy allocator for GPU memory is only used by DRM. > > > Hence add a dependency on DRM to GPU_BUDDY, to prevent asking the user > > > about this code when configuring a kernel without DRM support. > > > > It seems to be *technically* true, however, the entire point of moving the > > buddy allocator outside the DRM directory is to expose it to other non-DRM > > drivers, so the lack of dependency is kind of by design. > > > > The change was rather anticipatory, but it was clearly well reviewed and > > sent by a Joel, a nova-core dev, I assume it's gonna be needed at least > > by them at one point or another. > > > > If, say, nova decides to use the "communal" buddy allocator (if it > > doesn't do it already, I only briefly skimmed it looking for kernel::gpu) > > then the DRM dependency from this patch would have to be reverted (yet > > again). Maybe moving the buddy allocator up from DRM should have been done > > along with the first non-DRM user and only then, but it just didn't go that > > way. > > > > If the issue is that this option is too annoying during make or in > > menuconfig, then I don't think this is the way to address it. > > Once the symbol is visible, it needs a proper dependency, to avoid > asking the user irrelevant questions. > If it's visible and I understand now that it should be and how, then of course. > > > Fixes: 8fe3fc37564920ae ("gpu: Fix dependencies in > > > CONFIG_GPU_BUDDY_KUNIT_TEST") > > > Signed-off-by: Geert Uytterhoeven <[email protected]> > > > > --- a/drivers/gpu/Kconfig > > > +++ b/drivers/gpu/Kconfig > > > @@ -1,14 +1,15 @@ > > > # SPDX-License-Identifier: GPL-2.0 > > > > > > config GPU_BUDDY > > > - bool > > > + bool "Page based buddy allocator for GPU memory" if KUNIT > > > + depends on DRM || COMPILE_TEST > > > > What's the justification for COMPILE_TEST here? It might be needed but > > you didn't explain why or mention it in the patch description at all. > > It is customary to add "|| COMPILE_TEST" when adding dependencies that > are not strictly needed for a successful build. > I can't tell if this is something so obvious it needs no comment; I feel like it should be documented explicitly in the commit log. If the DRM dependency would be dropped would the COMPILE_TEST have to stay? Because then we're forcing a non-drm user to enable it if they want to use the buddy alloc at all. I think just leaving the "if KUNIT" part would check all the boxes, since the tests are actually runnable with KUnit via .kunitconfig without building an entire DRM driver, GPU_BUDDY also won't be randomly enabled with KUNIT_ALL_TESTS on systems that don't care about it, and we're not forcing anyone to enable DRM on a feature that was explictly made independent from it. Thanks Krzysztof
