On Thu, Oct 15, 2020 at 11:44:15AM +0200, Vlastimil Babka wrote: > On 10/15/20 10:23 AM, Christopher Lameter wrote: > > On Wed, 14 Oct 2020, Kees Cook wrote: > > > > > Note on patch 2: Christopher NAKed it, but I actually think this is a > > > reasonable thing to add -- the "too small" check is only made when built > > > with CONFIG_DEBUG_VM, so it *is* actually possible for someone to trip > > > over this directly, even if it would never make it into a released > > > kernel. I see no reason to just leave this foot-gun in place, though, so > > > we might as well just fix it too. (Which seems to be what Longman was > > > similarly supporting, IIUC.) > > > > Well then remove the duplication of checks. The NAK was there because it > > seems that you were not aware of the existing checks. > > > > > Anyway, if patch 2 stays NAKed, that's fine. It's entirely separable, > > > and the other 2 can land. :) > > > > Just deal with the old checks too and it will be fine. > > Yeah, the existing check is under CONFIG_DEBUG_VM, which means it's not > active on some configurations. Creating a cache is not exactly fast path > operation, so I would remove this guard. > As for the minimum size check, I would probably remove it (but watch out if > SLAB/SLOB can handle it). It's not effective to use slab cache for 4-byte > objects, but why make it an error. Err, why did the check exist to begin with? If the check isn't wanted, that's one thing, but I was just trying to fix what I saw in the redzone handling. What is preferred here? 1) drop patch 2 2) keep patch 2, but also: a) validate slab/slob can handle < word-sized allocations b) remove check in kmem_cache_sanity_check option 2a seems like it could be fragile if I miss something. I think I'd rather just take option 1. -- Kees Cook
