Messages in this thread | | | Date | Sat, 30 May 2009 11:03:33 -0700 | From | "Larry H." <> | Subject | Re: [patch 0/5] Support for sanitization flag in low-level page allocator |
| |
On 19:34 Sat 30 May , Ingo Molnar wrote: > You need to provide a more sufficient and more constructive answer > than that, if you propose upstream patches that impact the SLAB > subsystem.
Impact? If you mean introducing changes, definitely. If the word has negative connotations in this context, definitely not ;)
> FYI Pekka is one of the SLAB subsystem maintainers so you need to > convince him that your patches are the right approach. Trying to > teach Pekka about SLAB internals in a condescending tone will only > cause your patches to be ignored.
I've never tried to teach you anything but security matters, so far. And I've been quite unsuccessful at it, apparently. That said, please let me explain why kzfree was broken (as of 2.6.29.4, I've been told 30-rc2 already has users of it).
The first issue is that SLOB has a broken ksize, which won't take into consideration compound pages AFAIK. To fix this you will need to introduce some changes in the way the slob_page structure is handled, and add real size tracking to it. You will find these problems if you try to implement a reliable kmem_ptr_validate for SLOB, too.
The second is that I've experienced issues with kzfree on 2.6.29.4, in which something (apparently the freelist pointer) is overwritten and leads to a NULL pointer deference in the next allocation in the affected cache. I didn't fully analyze what was broken, besides that for sanitizing the objects on kfree I needed to rely on the inuse size and not the one reported by ksize, if I wanted to avoid hitting that trailing meta-data.
I just noticed Johannes Weiner's patch from February 16.
BTW, talking about branches and call depth, you are proposing using kzfree() which involves further test and call branches (including those inside the specific ksize implementation of the allocator being used) and it duplicates the check for ZERO_SIZE_PTR/NULL too. The function is so simple that it should be a static inline declared in slab.h. It also lacks any validation checks as performed in kfree (besides the zero size/null ptr one).
Also, users of unconditional sanitization would see unnecessary duplication of the clearing, causing a real performance hit (which would be almost non existent otherwise). That will make kzfree unsuitable for most hot spots like the crypto api and the mac80211 wep code.
Honestly your proposed approach seems a little weak.
Larry
| |