[Bug]: Re-evaluate TPL/Atomic Sequencing

[joschock]

Is there an existing issue for this?

• I have searched existing issues

Current Behavior

The present ordering of TplMutex acquire is:

set atomic lock = true
<preemption window>
raise TPL_NOTIFY

and release is:

restore previous/lower TPL
<queued callbacks may run here>
set atomic lock = false

This means that there is a window where event handlers running at a callback equal to the lock level (which would normally be prevented by the TPL raise can observe the Mutex as Locked (due to the atomic). This could lead to re-entrancy/deadlock.

Expected Behavior

It should not be possible for a callback at or below the TplMutex lock level to observe the Mutex as locked().

Steps To Reproduce

By inspection.

Build Environment

- OS(s):
- Targets Impacted:
N/A

Version Information

Commit: 517de6ea31857538d65c2fedf39d24fa1fb2b1ba

Urgency

Medium

Are you going to fix this?

I will fix it

Do you need maintainer feedback?

Maintainer feedback requested

Anything else?

No response

[joschock]

@makubacki / @os-d / @cfernald / @Javagedes - second opinion on this one? I think it opens a small window for unexpected TplMutex reentrancy/deadlock, but would like some extra review on it.

[os-d]

You want to flip this to:

raise TPL_NOTIFY
set atomic lock = true

And

set atomic lock = false
restore previous/lower TPL

?

[joschock]

You want to flip this to:

raise TPL_NOTIFY set atomic lock = true

And

set atomic lock = false restore previous/lower TPL

?

yes, that's the idea.

[joschock]

You want to flip this to:
raise TPL_NOTIFY set atomic lock = true
And
set atomic lock = false restore previous/lower TPL
?

yes, that's the idea.

well, something more like

raise TPL
compare-change-atomic lock
if fail - restore tpl and fail lock

and

release atomic lock
restore tpl

[cfernald]

@joschock I agree with your assessment, oddly enough though the dxe_core version already does this.

    pub fn try_lock(&self) -> Option<TplGuard<'_, T>> {
        let release_tpl = raise_tpl(self.tpl_lock_level);
        if self.lock.compare_exchange(false, true, Ordering::Acquire, Ordering::Relaxed).is_ok() {
            Some(TplGuard { release_tpl, mutex: self })
        } else {
            restore_tpl(release_tpl);
            None
        }
    }

Perhaps this is a good reason to try to find a way to unify these implementations.

[os-d]

Yeah, that makes sense to me and I agree with Chris’s assertion on unifying these.

[makubacki]

@joschock, I assume you're willing to do the consolidation?

[joschock]

@joschock, I assume you're willing to do the consolidation?

I'm not sure how to do the consolidation with breaking some of the rules around dependency directionality between core and SDK. The TPL primitives are in core, and the SDK version invokes them thru BootServices; but the core also needs TplMutex lit up before the BootServices primitives are fully built. I'll study that a bit more and see if there is a clean way to unify them somehow, perhaps through some abstraction of TPL.

[cfernald]

@joschock, I was thinking we could have a single verison in the SDK whose generic could be satisfied by the static functions for dxe_core, such that the core version is just a specific type of the same generic the SDK uses.

e.g.

// patina_dxe_core::tpl_mutex

pub(crate) struct TpmMutex<T> {
    inner: patina::TplMutex<B = StaticBootService, T = T>
}

Struct StaticBootServices...

Now it may make more since to have a core layer within this or something to make that abstraction less cumbersome, but you get my point