On 8/11/22 11:52, Marco Elver wrote: > On Thu, 11 Aug 2022 at 11:31, <vbabka@xxxxxxx> wrote: >> >> On 8/11/22 10:59, Imran Khan wrote: >> > By default kfence allocation can happen for any slab object, whose size >> > is up to PAGE_SIZE, as long as that allocation is the first allocation >> > after expiration of kfence sample interval. But in certain debugging >> > scenarios we may be interested in debugging corruptions involving >> > some specific slub objects like dentry or ext4_* etc. In such cases >> > limiting kfence for allocations involving only specific slub objects >> > will increase the probablity of catching the issue since kfence pool >> > will not be consumed by other slab objects. >> >> So you want to enable specific caches for kfence. >> >> > This patch introduces a sysfs interface '/sys/kernel/slab/<name>/skip_kfence' >> > to disable kfence for specific slabs. Having the interface work in this >> > way does not impact current/default behavior of kfence and allows us to >> > use kfence for specific slabs (when needed) as well. The decision to >> > skip/use kfence is taken depending on whether kmem_cache.flags has >> > (newly introduced) SLAB_SKIP_KFENCE flag set or not. >> >> But this seems everything is still enabled and you can selectively disable. >> Isn't that rather impractical? > > A script just iterates through all the caches that they don't want, > and sets skip_kfence? It doesn't look more complicated. Well, yeah, it's possible. >> How about making this cache flag rather denote that KFENCE is enabled (not >> skipped), set it by default only for for caches with size <= 1024, then you > > Where does 1024 come from? PAGE_SIZE? You're right, the existing check in __kfence_alloc() uses PAGE_SIZE, not 1024, which probably came from lack of coffee :) > The problem with that opt-in vs. opt-out is that it becomes more > complex to maintain opt-in (as the first RFC of this did). With the I see. There was a kfence_global_alloc_enabled and slub_kfence[=slabs] ... that probably wouldn't be necessary even in an opt-in scenario as I described. > new flag SLAB_SKIP_KFENCE, it also can serve a dual purpose, where > someone might want to explicitly opt out by default and pass it to > kmem_cache_create() (for whatever reason; not that we'd encourage > that). Right, not be able to do that would be a downside (although it should be possible even with opt-in to add an opt-out cache flag that would just make sure the opt-in flag is not set even if eligible by global defaults). > I feel that the real use cases for selectively enabling caches for > KFENCE are very narrow, and a design that introduces lots of > complexity elsewhere, just to support this feature cannot be justified > (which is why I suggested the simpler design here back in > https://lore.kernel.org/lkml/CANpmjNNmD9z7oRqSaP72m90kWL7jYH+cxNAZEGpJP8oLrDV-vw@xxxxxxxxxxxxxx/ > ) I don't mind strongly either way, just a suggestion to consider.
