| Message ID | 20180809085245.22448-1-vbabka@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm, slub: restore the original intention of prefetch_freepointer() | expand |
On Thu, Aug 9, 2018 at 1:52 AM, Vlastimil Babka <vbabka@suse.cz> wrote: > In SLUB, prefetch_freepointer() is used when allocating an object from cache's > freelist, to make sure the next object in the list is cache-hot, since it's > probable it will be allocated soon. > > Commit 2482ddec670f ("mm: add SLUB free list pointer obfuscation") has > unintentionally changed the prefetch in a way where the prefetch is turned to a > real fetch, and only the next->next pointer is prefetched. In case there is not > a stream of allocations that would benefit from prefetching, the extra real > fetch might add a useless cache miss to the allocation. Restore the previous > behavior. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > Cc: Kees Cook <keescook@chromium.org> > Cc: Daniel Micay <danielmicay@gmail.com> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Christoph Lameter <cl@linux.com> > Cc: Pekka Enberg <penberg@kernel.org> > Cc: David Rientjes <rientjes@google.com> > Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> > --- > While I don't expect this to be causing the bug at hand, it's worth fixing. > For the bug it might mean that the page fault moves elsewhere. > > mm/slub.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index 51258eff4178..ce2b9e5cea77 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -271,8 +271,7 @@ static inline void *get_freepointer(struct kmem_cache *s, void *object) > > static void prefetch_freepointer(const struct kmem_cache *s, void *object) > { > - if (object) > - prefetch(freelist_dereference(s, object + s->offset)); > + prefetch(object + s->offset); Ah -- gotcha. I think I misunderstood the purpose here. You're not prefetching what is being pointed at, you're literally prefetching what is stored there. That wouldn't require dereferencing the freelist pointer, no. Thanks! Acked-by: Kees Cook <keescook@chromium.org> > } > > static inline void *get_freepointer_safe(struct kmem_cache *s, void *object) > -- > 2.18.0 >
diff --git a/mm/slub.c b/mm/slub.c index 51258eff4178..ce2b9e5cea77 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -271,8 +271,7 @@ static inline void *get_freepointer(struct kmem_cache *s, void *object) static void prefetch_freepointer(const struct kmem_cache *s, void *object) { - if (object) - prefetch(freelist_dereference(s, object + s->offset)); + prefetch(object + s->offset); } static inline void *get_freepointer_safe(struct kmem_cache *s, void *object)
In SLUB, prefetch_freepointer() is used when allocating an object from cache's freelist, to make sure the next object in the list is cache-hot, since it's probable it will be allocated soon. Commit 2482ddec670f ("mm: add SLUB free list pointer obfuscation") has unintentionally changed the prefetch in a way where the prefetch is turned to a real fetch, and only the next->next pointer is prefetched. In case there is not a stream of allocations that would benefit from prefetching, the extra real fetch might add a useless cache miss to the allocation. Restore the previous behavior. Signed-off-by: Vlastimil Babka <vbabka@suse.cz> Cc: Kees Cook <keescook@chromium.org> Cc: Daniel Micay <danielmicay@gmail.com> Cc: Eric Dumazet <edumazet@google.com> Cc: Christoph Lameter <cl@linux.com> Cc: Pekka Enberg <penberg@kernel.org> Cc: David Rientjes <rientjes@google.com> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> --- While I don't expect this to be causing the bug at hand, it's worth fixing. For the bug it might mean that the page fault moves elsewhere. mm/slub.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)