On Tue, Jul 15, 2014 at 11:37:56AM +0400, Andrey Ryabinin wrote: > On 07/15/14 10:04, Joonsoo Kim wrote: > > On Wed, Jul 09, 2014 at 03:30:08PM +0400, Andrey Ryabinin wrote: > >> Some code in slub could validly touch memory marked by kasan as > >> unaccessible. > >> Even though slub.c doesn't instrumented, functions called in it are > >> instrumented, > >> so to avoid false positive reports such places are protected by > >> kasan_disable_local()/kasan_enable_local() calls. > >> > >> Signed-off-by: Andrey Ryabinin <[email protected]> > >> --- > >> mm/slub.c | 21 +++++++++++++++++++-- > >> 1 file changed, 19 insertions(+), 2 deletions(-) > >> > >> diff --git a/mm/slub.c b/mm/slub.c > >> index 6ddedf9..c8dbea7 100644 > >> --- a/mm/slub.c > >> +++ b/mm/slub.c > >> @@ -560,8 +560,10 @@ static void print_tracking(struct kmem_cache *s, void > >> *object) > >> if (!(s->flags & SLAB_STORE_USER)) > >> return; > >> > >> + kasan_disable_local(); > >> print_track("Allocated", get_track(s, object, TRACK_ALLOC)); > >> print_track("Freed", get_track(s, object, TRACK_FREE)); > >> + kasan_enable_local(); > > > > I don't think that this is needed since print_track() doesn't call > > external function with object pointer. print_track() call pr_err(), but, > > before calling, it retrieve t->addrs[i] so memory access only occurs > > in slub.c. > > > Agree. > > >> } > >> > >> static void print_page_info(struct page *page) > >> @@ -604,6 +606,8 @@ static void print_trailer(struct kmem_cache *s, struct > >> page *page, u8 *p) > >> unsigned int off; /* Offset of last byte */ > >> u8 *addr = page_address(page); > >> > >> + kasan_disable_local(); > >> + > >> print_tracking(s, p); > >> > >> print_page_info(page); > >> @@ -632,6 +636,8 @@ static void print_trailer(struct kmem_cache *s, struct > >> page *page, u8 *p) > >> /* Beginning of the filler is the free pointer */ > >> print_section("Padding ", p + off, s->size - off); > >> > >> + kasan_enable_local(); > >> + > >> dump_stack(); > >> } > > > > And, I recommend that you put this hook on right place. > > At a glance, the problematic function is print_section() which have > > external function call, print_hex_dump(), with object pointer. > > If you disable kasan in print_section, all the below thing won't be > > needed, I guess. > > > > Nope, at least memchr_inv() call in slab_pad_check will be a problem. > > I think putting disable/enable only where we strictly need them might be a > problem for future maintenance of slub. > If someone is going to add a new function call somewhere, he must ensure that > it this call won't be a problem > for kasan.
I don't agree with this. If someone is going to add a slab_pad_check() in other places in slub.c, we should disable/enable kasan there, too. This looks same maintenance problem to me. Putting disable/enable only where we strictly need at least ensures that we don't need to care when using slub internal functions. And, if memchr_inv() is problem, I think that you also need to add hook into validate_slab_cache(). validate_slab_cache() -> validate_slab_slab() -> validate_slab() -> check_object() -> check_bytes_and_report() -> memchr_inv() Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
