[PATCH] icpp remedy

[Sebastian Huber]

On 15.09.22 00:06, Gedare Bloom wrote:
> On Tue, Sep 13, 2022 at 12:42 PM Kuan-Hsun Chen<c0066c at gmail.com>  wrote:
>> Thanks for the prompt reply. Yes, I will guide Junjie to make a ticket and go through the issue.
>>
>>> Is there a test case for this?
>> Yes, a test case is also ready to be reviewed and can be part of the testsuite to test out ICPP (MrsP should also pass).
>>
>>> If it applies to 5 or 4.11, there needs to be another ticket to get the fix back ported.
>> So each release version with one ticket? We only check 5.1 in this work. My intuitive guess is that if the functionality does not change over the versions, the remedy should be similar.
>> Let us figure it out.
>>
>> On Tue, Sep 13, 2022 at 8:21 PM Joel Sherrill<joel at rtems.org>  wrote:
>>>
>>>
>>> On Tue, Sep 13, 2022, 1:14 PM Kuan-Hsun Chen<c0066c at gmail.com>  wrote:
>>>> (Oh, I just noticed that the patch was submitted faster than my mail to share this patch :P).
>>>
>>> No problem. That's why I asked what the issue was.
>>>
>>> It needs at least a much better log message. It needs a ticket given the gravity of the issue. That ticket can explain the situation and include links. Probably needs a comment above the change explaining what's going on.
>>>
>>> Is there a test case for this?
>>>
>>> If it applies to 5 or 4.11, there needs to be another ticket to get the fix back ported.
>>>
>>> Great catch. Just need to do a good job on the log, test, ticket(s), etc
>>>
>>> --joel
>>>
>>>> Hi Joel,
>>>>
>>>> Let me clarify what happened here. In our recent study, we adopt Frama-C to formally verify the implementation of ICPP (also MrsP) and note that the current flow may lead to a deadlock. The patch is a simple remedy to fix the issue.
>>>>
>>>> The resource’s ceiling is not checked against the thread’s base priority, but against its current dynamic priority derived from the task’s priority aggregation. However, a resource’s ceiling is required to be set as the highest base priority of all tasks that are requesting it. This mismatch may lead to a deadlock by denying legitimate nested resource access if resources are requested in descending order of priority. So this adaption is needed to resolve the issue. A similar issue can also be found in MrsP due to the usage of this function. A detailed explanation can be found at the end of Section V in the attached preprint, and a toy example is provided there to reveal the issue.
>>>>
> By changing this to check the base priority instead of the dynamic
> priority, the mutex seize will now allow a task to acquire a ceiling
> mutex even though its dynamic priority is higher than the ceiling. The
> specification to check against the base priority of tasks comes from
> the ICPP without nested lock behavior as I recall, and I'm not 100%
> sure where the requirement comes from that "a resource’s ceiling is
> required to be set as the highest base priority of all tasks that are
> requesting it" -- is that requirement proved anywhere for ICPP with
> nested resources? I'm a little concerned about making this kind of
> fundamental change deep within the (shared) locking protocol code,
> especially since it primarily seems to cater to a uniprocessor corner
> case.
> 
> This is a tricky situation, and clearly a lot of work has already gone
> into discovering it. I think we need to discuss this patch a bit more.
> If I understood correctly, this deadlock can't happen if locks are
> always acquired in the increasing priority order. Changing this code
> allows for applications to acquire locks in arbitrary priority order.
> I'm not sure that is permissible in multiprocessor systems to still
> ensure correctness of the locking protocol correctness. It would be
> therefore also valid to state this as an explicit requirement for
> applications to make correct use of ICPP. The same thing is also true
> for MrsP, and for multiprocessor locking protocols in general I
> believe it is required for locks to be acquired in priority order? I'd
> be inclined to instead say that we should confirm that locks are
> acquired in the proper priority ordering as a way to avoid the
> identified deadlock situation.

The check against the dynamic priority is there for a long time (at 
least RTEMS 3.6.0). The questions is how much policy we want to enforce 
through such checks. I am not sure if acquiring mutexes in an arbitrary 
priority order is a good application design. However, from an 
implementation point of view we could remove the check completely. Each 
priority ceiling mutex provides a priority node for the task, so it is 
independent of other priority contributions. We also have a dedicated 
deadlock detection.

> 
> Either way this discussion/patch goes, we should document these issues
> inhttps://docs.rtems.org/branches/master/c-user/key_concepts.html#priorityceiling

Yes.

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber at embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/