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

[Eric Dumazet <eric.dumazet@gmail.com>]

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.