Re: [PATCH] ipc,sem block sem_lock on sma->lock during sma initialization

[Davidlohr Bueso <dave@stgolabs.net>]

On Fri, 2014-11-21 at 18:03 -0500, Rik van Riel wrote:
> On 11/21/2014 03:42 PM, Andrew Morton wrote:
> > On Fri, 21 Nov 2014 15:29:27 -0500 Rik van Riel <riel@redhat.com>
> > wrote:
> > 
> >> On 11/21/2014 03:09 PM, Andrew Morton wrote:
> >>> On Fri, 21 Nov 2014 14:52:26 -0500 Rik van Riel
> >>> <riel@redhat.com> wrote:
> >>> 
> >>>> When manipulating just one semaphore with semop, sem_lock
> >>>> only takes that single semaphore's lock. This creates a
> >>>> problem during initialization of the semaphore array, when
> >>>> the data structures used by sem_lock have not been set up
> >>>> yet. The sma->lock is already held by newary, and we just
> >>>> have to make sure everything else waits on that lock during
> >>>> initialization.
> >>>> 
> >>>> Luckily it is easy to make sem_lock wait on the sma->lock,
> >>>> by pretending there is a complex operation in progress while
> >>>> the sma is being initialized.
> >>>> 
> >>>> The newary function already zeroes sma->complex_count before 
> >>>> unlocking the sma->lock.
> >>> 
> >>> What are the runtime effects of the bug?
> >>> 
> >> 
> >> NULL pointer dereference in spin_lock from sem_lock, if it is
> >> called before sma->sem_base has been pointed somewhere valid.
> > 
> > Help us out here.  People need to use this description to work out 
> > which kernel versions need the patch and whether to backport the
> > fix into their various kernels.  Other people will be starting at
> > this changelog wondering "will this fix the bug my customer has
> > reported".
> > 
> > Is there some bug report people can look at?

This would be nice for the changelog...

> > 
> > What userspace actions trigger this bug?
> 
> The reason the bug took almost two years to get noticed is that
> it takes one task doing a semop on a semaphore in an array that
> is still getting instantiated by newary (getsem) from another
> task.
> 
> In other words, if you try to use a semaphore array before
> getsem returns, you can oops the task that calls semop.

This seems bogus from an application level: how can you call semop if
you don't have the semid yet returned from semget? And the fact that the
race is with newary, means that the call is in fact creating a *new*
set, as opposed to plugging into an already existing set.

The fix in newary() being before the actual creation of the id seems
even stranger:

	sma->complex_count = 1;
	id = ipc_addid(&sem_ids(ns), &sma->sem_perm, ns->sc_semmni);

As for semtimedop() before even getting to sem_lock(), we first call:

	sma = sem_obtain_object_check(ns, semid);

So shouldn't that fail anyway before we even consider acquiring the lock?

Thanks,
Davidlohr