From: Andrey Ryabinin <ryabinin.a.a@gmail.com> To: Alexander Potapenko <glider@google.com> Cc: Andrey Konovalov <adech.fo@gmail.com>, Christoph Lameter <cl@linux.com>, Dmitry Vyukov <dvyukov@google.com>, Andrew Morton <akpm@linux-foundation.org>, Steven Rostedt <rostedt@goodmis.org>, Joonsoo Kim <iamjoonsoo.kim@lge.com>, Joonsoo Kim <js1304@gmail.com>, Kostya Serebryany <kcc@google.com>, kasan-dev <kasan-dev@googlegroups.com>, LKML <linux-kernel@vger.kernel.org>, "linux-mm@kvack.org" <linux-mm@kvack.org> Subject: Re: [PATCH v8 7/7] mm: kasan: Initial memory quarantine implementation Date: Tue, 10 May 2016 22:57:37 +0300 [thread overview] Message-ID: <CAPAsAGyxo9vHUM63tEKBS_edmYSHkxXhX-zrzaKP4QG4Vbf3FA@mail.gmail.com> (raw) In-Reply-To: <CAG_fn=UFvCVRwQ8uPHvabAuRmGEBOXsga-yfA+bz=MtmFZBeqg@mail.gmail.com> 2016-05-10 20:17 GMT+03:00 Alexander Potapenko <glider@google.com>: > On Tue, May 10, 2016 at 5:39 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote: >> 2016-03-15 13:10 GMT+03:00 Alexander Potapenko <glider@google.com>: >> >>> >>> static inline int kasan_module_alloc(void *addr, size_t size) { return 0; } >>> static inline void kasan_free_shadow(const struct vm_struct *vm) {} >>> diff --git a/lib/test_kasan.c b/lib/test_kasan.c >>> index 82169fb..799c98e 100644 >>> --- a/lib/test_kasan.c >>> +++ b/lib/test_kasan.c >>> @@ -344,6 +344,32 @@ static noinline void __init kasan_stack_oob(void) >>> *(volatile char *)p; >>> } >>> >>> +#ifdef CONFIG_SLAB >>> +static noinline void __init kasan_quarantine_cache(void) >>> +{ >>> + struct kmem_cache *cache = kmem_cache_create( >>> + "test", 137, 8, GFP_KERNEL, NULL); >>> + int i; >>> + >>> + for (i = 0; i < 100; i++) { >>> + void *p = kmem_cache_alloc(cache, GFP_KERNEL); >>> + >>> + kmem_cache_free(cache, p); >>> + p = kmalloc(sizeof(u64), GFP_KERNEL); >>> + kfree(p); >>> + } >>> + kmem_cache_shrink(cache); >>> + for (i = 0; i < 100; i++) { >>> + u64 *p = kmem_cache_alloc(cache, GFP_KERNEL); >>> + >>> + kmem_cache_free(cache, p); >>> + p = kmalloc(sizeof(u64), GFP_KERNEL); >>> + kfree(p); >>> + } >>> + kmem_cache_destroy(cache); >>> +} >>> +#endif >>> + >> >> Test looks quite useless. The kernel does allocations/frees all the >> time, so I don't think that this test >> adds something valuable. > Agreed. >> And what's the result that we expect from this test? No crashes? >> I'm thinking it would better to remove it. > Do you think it may make sense to improve it by introducing an actual > use-after-free? > Or perhaps we could insert a loop doing 1000 kmalloc()/kfree() calls > into the existing UAF tests. You don't need to do an actual UAF, all you need is to make sure that repeated kmalloc() + kfree() produces new addresses. But I personally wouldn't bother with testing this at all. So, unless you care, just remove the test. >> >>> + >>> +/* smp_load_acquire() here pairs with smp_store_release() in >>> + * quarantine_reduce(). >>> + */ >>> +#define QUARANTINE_LOW_SIZE (smp_load_acquire(&quarantine_size) * 3 / 4) >> >> I'd prefer open coding barrier with a proper comment int place, >> instead of sneaking it into macros. > Ack. >> [...] >> >>> + >>> +void quarantine_reduce(void) >>> +{ >>> + size_t new_quarantine_size; >>> + unsigned long flags; >>> + struct qlist to_free = QLIST_INIT; >>> + size_t size_to_free = 0; >>> + void **last; >>> + >>> + /* smp_load_acquire() here pairs with smp_store_release() below. */ >> >> Besides pairing rules, the comment should also explain *why* we need >> this and for what >> load/stores it provides memory ordering guarantees. For example take a >> look at other >> comments near barriers in the kernel tree. > Something along the lines of "We must load A before B, hence the barrier"? Yes. BTW, do we really need these barriers? I didn't tried to understand this, thus could be wrong here, but it seems that READ_ONCE/WRITE_ONCE would be enough. >>> + if (likely(ACCESS_ONCE(global_quarantine.bytes) <= >>> + smp_load_acquire(&quarantine_size))) >>> + return; >>> + >>>
WARNING: multiple messages have this Message-ID (diff)
From: Andrey Ryabinin <ryabinin.a.a@gmail.com> To: Alexander Potapenko <glider@google.com> Cc: Andrey Konovalov <adech.fo@gmail.com>, Christoph Lameter <cl@linux.com>, Dmitry Vyukov <dvyukov@google.com>, Andrew Morton <akpm@linux-foundation.org>, Steven Rostedt <rostedt@goodmis.org>, Joonsoo Kim <iamjoonsoo.kim@lge.com>, Joonsoo Kim <js1304@gmail.com>, Kostya Serebryany <kcc@google.com>, kasan-dev <kasan-dev@googlegroups.com>, LKML <linux-kernel@vger.kernel.org>, "linux-mm@kvack.org" <linux-mm@kvack.org> Subject: Re: [PATCH v8 7/7] mm: kasan: Initial memory quarantine implementation Date: Tue, 10 May 2016 22:57:37 +0300 [thread overview] Message-ID: <CAPAsAGyxo9vHUM63tEKBS_edmYSHkxXhX-zrzaKP4QG4Vbf3FA@mail.gmail.com> (raw) In-Reply-To: <CAG_fn=UFvCVRwQ8uPHvabAuRmGEBOXsga-yfA+bz=MtmFZBeqg@mail.gmail.com> 2016-05-10 20:17 GMT+03:00 Alexander Potapenko <glider@google.com>: > On Tue, May 10, 2016 at 5:39 PM, Andrey Ryabinin <ryabinin.a.a@gmail.com> wrote: >> 2016-03-15 13:10 GMT+03:00 Alexander Potapenko <glider@google.com>: >> >>> >>> static inline int kasan_module_alloc(void *addr, size_t size) { return 0; } >>> static inline void kasan_free_shadow(const struct vm_struct *vm) {} >>> diff --git a/lib/test_kasan.c b/lib/test_kasan.c >>> index 82169fb..799c98e 100644 >>> --- a/lib/test_kasan.c >>> +++ b/lib/test_kasan.c >>> @@ -344,6 +344,32 @@ static noinline void __init kasan_stack_oob(void) >>> *(volatile char *)p; >>> } >>> >>> +#ifdef CONFIG_SLAB >>> +static noinline void __init kasan_quarantine_cache(void) >>> +{ >>> + struct kmem_cache *cache = kmem_cache_create( >>> + "test", 137, 8, GFP_KERNEL, NULL); >>> + int i; >>> + >>> + for (i = 0; i < 100; i++) { >>> + void *p = kmem_cache_alloc(cache, GFP_KERNEL); >>> + >>> + kmem_cache_free(cache, p); >>> + p = kmalloc(sizeof(u64), GFP_KERNEL); >>> + kfree(p); >>> + } >>> + kmem_cache_shrink(cache); >>> + for (i = 0; i < 100; i++) { >>> + u64 *p = kmem_cache_alloc(cache, GFP_KERNEL); >>> + >>> + kmem_cache_free(cache, p); >>> + p = kmalloc(sizeof(u64), GFP_KERNEL); >>> + kfree(p); >>> + } >>> + kmem_cache_destroy(cache); >>> +} >>> +#endif >>> + >> >> Test looks quite useless. The kernel does allocations/frees all the >> time, so I don't think that this test >> adds something valuable. > Agreed. >> And what's the result that we expect from this test? No crashes? >> I'm thinking it would better to remove it. > Do you think it may make sense to improve it by introducing an actual > use-after-free? > Or perhaps we could insert a loop doing 1000 kmalloc()/kfree() calls > into the existing UAF tests. You don't need to do an actual UAF, all you need is to make sure that repeated kmalloc() + kfree() produces new addresses. But I personally wouldn't bother with testing this at all. So, unless you care, just remove the test. >> >>> + >>> +/* smp_load_acquire() here pairs with smp_store_release() in >>> + * quarantine_reduce(). >>> + */ >>> +#define QUARANTINE_LOW_SIZE (smp_load_acquire(&quarantine_size) * 3 / 4) >> >> I'd prefer open coding barrier with a proper comment int place, >> instead of sneaking it into macros. > Ack. >> [...] >> >>> + >>> +void quarantine_reduce(void) >>> +{ >>> + size_t new_quarantine_size; >>> + unsigned long flags; >>> + struct qlist to_free = QLIST_INIT; >>> + size_t size_to_free = 0; >>> + void **last; >>> + >>> + /* smp_load_acquire() here pairs with smp_store_release() below. */ >> >> Besides pairing rules, the comment should also explain *why* we need >> this and for what >> load/stores it provides memory ordering guarantees. For example take a >> look at other >> comments near barriers in the kernel tree. > Something along the lines of "We must load A before B, hence the barrier"? Yes. BTW, do we really need these barriers? I didn't tried to understand this, thus could be wrong here, but it seems that READ_ONCE/WRITE_ONCE would be enough. >>> + if (likely(ACCESS_ONCE(global_quarantine.bytes) <= >>> + smp_load_acquire(&quarantine_size))) >>> + return; >>> + >>> -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2016-05-10 19:57 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-03-15 10:10 [PATCH v8 0/7] SLAB support for KASAN Alexander Potapenko 2016-03-15 10:10 ` Alexander Potapenko 2016-03-15 10:10 ` [PATCH v8 1/7] kasan: Modify kmalloc_large_oob_right(), add kmalloc_pagealloc_oob_right() Alexander Potapenko 2016-03-15 10:10 ` Alexander Potapenko 2016-03-15 10:10 ` [PATCH v8 2/7] mm, kasan: SLAB support Alexander Potapenko 2016-03-15 10:10 ` Alexander Potapenko 2016-03-15 10:10 ` [PATCH v8 3/7] mm, kasan: Added GFP flags to KASAN API Alexander Potapenko 2016-03-15 10:10 ` Alexander Potapenko 2016-03-15 10:10 ` [PATCH v8 4/7] arch, ftrace: For KASAN put hard/soft IRQ entries into separate sections Alexander Potapenko 2016-03-15 10:10 ` Alexander Potapenko 2016-03-15 10:10 ` [PATCH v8 5/7] mm, kasan: Stackdepot implementation. Enable stackdepot for SLAB Alexander Potapenko 2016-03-15 10:10 ` Alexander Potapenko 2016-03-15 10:10 ` [PATCH v8 6/7] kasan: Test fix: Warn if the UAF could not be detected in kmalloc_uaf2 Alexander Potapenko 2016-03-15 10:10 ` Alexander Potapenko 2016-03-15 10:10 ` [PATCH v8 7/7] mm: kasan: Initial memory quarantine implementation Alexander Potapenko 2016-03-15 10:10 ` Alexander Potapenko 2016-05-10 15:39 ` Andrey Ryabinin 2016-05-10 15:39 ` Andrey Ryabinin 2016-05-10 17:17 ` Alexander Potapenko 2016-05-10 17:17 ` Alexander Potapenko 2016-05-10 19:57 ` Andrey Ryabinin [this message] 2016-05-10 19:57 ` Andrey Ryabinin 2016-05-11 9:04 ` Alexander Potapenko 2016-05-11 9:04 ` Alexander Potapenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAPAsAGyxo9vHUM63tEKBS_edmYSHkxXhX-zrzaKP4QG4Vbf3FA@mail.gmail.com \ --to=ryabinin.a.a@gmail.com \ --cc=adech.fo@gmail.com \ --cc=akpm@linux-foundation.org \ --cc=cl@linux.com \ --cc=dvyukov@google.com \ --cc=glider@google.com \ --cc=iamjoonsoo.kim@lge.com \ --cc=js1304@gmail.com \ --cc=kasan-dev@googlegroups.com \ --cc=kcc@google.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=rostedt@goodmis.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.