lkml.org 
[lkml]   [2016]   [May]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH] kasan: improve double-free detection
Date
> >> >> I missed that Alexander already landed patches that reduce header size
> >> >> to 16 bytes.
> >> >> It is not OK to increase them again. Please leave state as bitfield
> >> >> and update it with CAS (if we introduce helper functions for state
> >> >> manipulation, they will hide the CAS loop, which is nice).
> >> >>
> >> >
> >> > Available CAS primitives/compiler do not support CAS with bitfield. I
> propose
> >> > to change kasan_alloc_meta to:
> >> >
> >> > struct kasan_alloc_meta {
> >> > struct kasan_track track;
> >> > u16 size_delta; /* object_size - alloc size */
> >> > u8 state; /* enum kasan_state */
> >> > u8 reserved1;
> >> > u32 reserved2;
> >> > }
> >> >
> >> > This shrinks _used_ meta object by 1 byte wrt the original. (btw, patch v1
> does
> >> > not increase overall alloc meta object size). "Alloc size", where needed, is
> >> > easily calculated as a delta from cache->object_size.
> >>
> >>
> >> What is the maximum size that slab can allocate?
> >> I remember seeing slabs as large as 4MB some time ago (or did I
> >> confuse it with something else?). If there are such large objects,
> >> that 2 bytes won't be able to hold even delta.
> >> However, now on my desktop I don't see slabs larger than 16KB in
> >> /proc/slabinfo.
> >
> > max size for SLAB's slab is 32MB; default is 4MB. I must have gotten confused
> by
> > SLUB's 8KB limit. Anyway, new kasan_alloc_meta in patch V2:
> >
> > struct kasan_alloc_meta {
> > struct kasan_track track;
> > union {
> > u8 lock;
> > struct {
> > u32 dummy : 8;
> > u32 size_delta : 24; /* object_size - alloc size */
> > };
> > };
> > u32 state : 2; /* enum kasan_alloc_state */
> > u32 unused : 30;
> > };
> >
> > This uses 2 more bits than current, but given the constraints I think this is
> > close to optimal.
>
>
> We plan to use the unused part for another depot_stack_handle_t (u32)
> to memorize stack of the last call_rcu on the object (this will
> greatly simplify debugging of use-after-free for objects freed by
> rcu). So we need that unused part.
>
> I would would simply put all these fields into a single u32:
>
> struct kasan_alloc_meta {
> struct kasan_track track;
> u32 status; // contains lock, state and size
> u32 unused; // reserved for call_rcu stack handle
> };
>
> And then separately a helper type to pack/unpack status:
>
> union kasan_alloc_status {
> u32 raw;
> struct {
> u32 lock : 1;
> u32 state : 2;
> u32 unused : 5;
> u32 size : 24;
> };
> };
>
>
> Then, when we need to read/update the header we do something like:
>
> kasan_alloc_status status, new_status;
>
> for (;;) {
> status.raw = READ_ONCE(header->status);
> // read status, form new_status, for example:
> if (status.lock)
> continue;
> new_status.raw = status.raw;
> new_status.lock = 1;
> if (cas(&header->status, status.raw, new_status.raw))
> break;
> }
>
>
> This will probably make state manipulation functions few lines longer,
> but since there are like 3 such functions I don't afraid that. And we
> still can use bitfield magic to extract fields and leave whole 5 bits
> unused bits for future.

The difficulty is that the lock managed by CAS needs 1 byte, mininum; TAS bit
is even 'worse': address must be that of an unsigned long.

Might it be possible for you to employ the 'kasan_free_meta' header for your
RCU stack handle instead since KASAN does not currently store state for RCU
slabs on free?

Kuthonuzo
\
 
 \ /
  Last update: 2016-05-05 08:41    [W:0.075 / U:1.196 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site