On Fri, Aug 2, 2024 at 11:57 AM Jann Horn <jannh@xxxxxxxxxx> wrote: > > > > > Let's reword this to: > > > > kasan_slab_pre_free - Check whether freeing a slab object is safe. > > @object: Object to be freed. > > > > This function checks whether freeing the given object is safe. It > > performs checks to detect double-free and invalid-free bugs and > > reports them. > > > > This function is intended only for use by the slab allocator. > > > > @Return true if freeing the object is not safe; false otherwise. > > Ack, will apply this for v6. But I'll replace "not safe" with > "unsafe", and change "It performs checks to detect double-free and > invalid-free bugs and reports them" to "It may check for double-free > and invalid-free bugs and report them.", since KASAN only sometimes > performs such checks (depending on CONFIG_KASAN, kasan_enabled(), > kasan_arch_is_ready(), and so on). Ok! > > kasan_slab_free - Poison, initialize, and quarantine a slab object. > > @object: Object to be freed. > > @init: Whether to initialize the object. > > > > This function poisons a slab object and saves a free stack trace for > > it, except for SLAB_TYPESAFE_BY_RCU caches. > > > > For KASAN modes that have integrated memory initialization > > (kasan_has_integrated_init() == true), this function also initializes > > the object's memory. For other modes, the @init argument is ignored. > > As an aside: Is this actually reliably true? It would be false for > kfence objects, but luckily we can't actually get kfence objects > passed to this function (which I guess maybe we should maybe document > here as part of the API). It would also be wrong if > __kasan_slab_free() can be reached while kasan_arch_is_ready() is > false, which I guess would happen if you ran a CONFIG_KASAN=y kernel > on a powerpc machine without radix or something like that? > > (And similarly I wonder if the check of kasan_has_integrated_init() in > slab_post_alloc_hook() is racy, but I haven't checked in which phase > of boot KASAN is enabled for HWASAN.) > > But I guess that's out of scope for this series. Yeah, valid concerns. Documenting all of them is definitely too much, though. > > For the Generic mode, this function might also quarantine the object. > > When this happens, KASAN will defer freeing the object to a later > > stage and handle it internally then. The return value indicates > > whether the object was quarantined. > > > > This function is intended only for use by the slab allocator. > > > > @Return true if KASAN quarantined the object; false otherwise. > > Same thing as I wrote on patch 2/2: To me this seems like too much > implementation detail for the documentation of an API between > components of the kernel? I agree that the meaning of the "init" > argument is important to document here, and it should be documented > that the hook can take ownership of the object (and I guess it's fine > to mention that this is for quarantine purposes), but I would leave > out details about differences in behavior between KASAN modes. > Basically my heuristic here is that in my opinion, this header comment > should mostly describe as much of the function as SLUB has to know to > properly use it. > > So I'd do something like: > > <<< > kasan_slab_free - Poison, initialize, and quarantine a slab object. > @object: Object to be freed. > @init: Whether to initialize the object. > > This function informs that a slab object has been freed and is not > supposed to be accessed anymore, except for objects in > SLAB_TYPESAFE_BY_RCU caches. > > For KASAN modes that have integrated memory initialization > (kasan_has_integrated_init() == true), this function also initializes > the object's memory. For other modes, the @init argument is ignored. > > This function might also take ownership of the object to quarantine it. > When this happens, KASAN will defer freeing the object to a later > stage and handle it internally until then. The return value indicates > whether KASAN took ownership of the object. > > This function is intended only for use by the slab allocator. > > @Return true if KASAN took ownership of the object; false otherwise. > >>> Looks good to me.
