Re: [Bugme-new] [Bug 33502] New: Caught 64-bit read from uninitialized memory in __alloc_skb

Eric Dumazet
2011-05-10T19:32:32Z

Le mardi 10 mai 2011 à 13:28 -0500, Christoph Lameter a écrit :
> On Tue, 10 May 2011, Eric Dumazet wrote:
>
> > > + else
> > > + p = get_freepointer(s, object);
> > > +
> > > + local_irq_restore(flags);
> > > +#else
> > > + p = get_freepointer(s, object);
> > > +#endif
> > > + return p;
> > > +}
> > > +
> > > static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp)
> > > {
> > > *(void **)(object + s->offset) = fp;
> > > @@ -1933,7 +1954,7 @@ redo:
> > > if (unlikely(!irqsafe_cpu_cmpxchg_double(
> > > s->cpu_slab->freelist, s->cpu_slab->tid,
> > > object, tid,
> > > - get_freepointer(s, object), next_tid(tid)))) {
> > > + get_freepointer_safe(s, object), next_tid(tid)))) {
> > >
> > > note_cmpxchg_failure("slab_alloc", s, tid);
> > > goto redo;
> >
> >
> > Really this wont work Stephen
>
> I am not Stephen.
>

Yes, sorry Christoph.

> > You have to disable IRQ _before_ even fetching 'object'
>
> The object pointer is being obtained from a per cpu structure and
> not from the page. What is the problem with fetching the object pointer?
>
> > Or else, you can have an IRQ, allocate this object, pass to another cpu.
>
> If that occurs then TID is being incremented and we will restart the loop
> getting the new object pointer from the per cpu structure. The object
> pointer that we were considering is irrelevant.
>

Problem is not restart the loop, but avoiding accessing a non valid
memory area.

> > This other cpu can free the object and unmap page right after you did
> > the probe_kernel_address(object) (successfully), and before your cpu :
> >
> > p = get_freepointer(s, object); << BUG >>
>
> If the other cpu frees the object and unmaps the page then
> get_freepointer_safe() can obtain an arbitrary value since the TID was
> incremented. We will restart the loop and discard the value retrieved.
>

In current code I see :

tid = c->tid;
barrier();
object = c->freelist;

There is no guarantee c->tid is fetched before c->freelist by cpu.

You need rmb() here.

I claim all this would be far more simple disabling IRQ before fetching
c->tid and c->freelist, in DEBUG_PAGE_ALLOC case.

You would not even need to use magic probe_kernel_read()

Why do you try so _hard_ trying to optimize this, I really wonder.
Nobody is able to read this code anymore and prove its correct.