Re: [Xenomai-core] [PULL] native: Fix msendq fastlock leakage

[Jan Kiszka <jan.kiszka@domain.hid>]

[-- Attachment #1: Type: text/plain, Size: 2684 bytes --]

On 2011-06-20 19:46, Gilles Chanteperdrix wrote:
> On 06/20/2011 07:07 PM, Jan Kiszka wrote:
>>>  static inline void do_cleanup_event(struct mm_struct *mm)
>>>  {
>>> +	struct task_struct *p = current;
>>> +	struct mm_struct *old;
>>> +
>>> +	old = xnshadow_mm(p);
>>> +	xnshadow_mmptd(p) = mm;
>>> +
>>>  	ppd_remove_mm(mm, &detach_ppd);
>>> +
>>> +	xnshadow_mmptd(p) = old;
>>
>> I don't have the full picture yet, but that feels racy: If the context
>> over which we clean up that foreign mm is also using xnshadow_mmptd,
>> other threads in that process may dislike this temporary change.
> 
> This mmptd is only used by xnshadow_ppd_get (which never dereferences it
> by the way

I know that the current code is safe. Avoiding mm_struct types is about
keeping it safe in the future. Why track the type if its just an opaque
key and should be handled as such?

). And if the current thread is running the cleanup event, it
> is not running any other service at the same time. So, this is safe.

Ah, xnshadow_mmptd is per-thread, not per-process as I incorrectly
assumed. Then it's safe.

> 
> An alternative implementation would be to use some global per-cpu
> variable and disable the preemption during cleanup. But that would not
> fix the case of the shadow cleanups where current->mm is already null.
> 
> The remaining problem is an irq interrupting the cleanup, and using
> xnshadow_ppd_get. But I would not expect that to happen. I mean,
> xnshadow_ppd_get is more a service to be used for implementation of
> system calls.

Yes, that kind of usage would be a bug in the first place.

> 
>>> diff --git a/ksrc/skins/posix/mutex.c b/ksrc/skins/posix/mutex.c
>>> index 6ce75e5..cc86852 100644
>>> --- a/ksrc/skins/posix/mutex.c
>>> +++ b/ksrc/skins/posix/mutex.c
>>> @@ -219,10 +219,6 @@ void pse51_mutex_destroy_internal(pse51_mutex_t *mutex,
>>>  	xnlock_put_irqrestore(&nklock, s);
>>>  
>>>  #ifdef CONFIG_XENO_FASTSYNCH
>>> -	/* We call xnheap_free even if the mutex is not pshared; when
>>> -	   this function is called from pse51_mutexq_cleanup, the
>>> -	   sem_heap is destroyed, or not the one to which the fastlock
>>> -	   belongs, xnheap will simply return an error. */
>>
>> I think this comment is not completely obsolete. It still applies /wrt
>> shared/non-shared.
> 
> If we apply this patch after the one changing the ppd cleanup order,
> everything falls in place (I can even put a warning in xnheap_destroy if
> the heap has some bytes still in use when destroyed).

"We call xnheap_free even if the mutex is not pshared." This still
applies and is unrelated to the ppd changes.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]