Date:   Wed, 16 Nov 2022 22:57:16 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>,
        Ira Weiny <ira.weiny@...el.com>
Cc:     jarkko@...nel.org, dave.hansen@...ux.intel.com,
        linux-sgx@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
        Borislav Petkov <bp@...en8.de>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>,
        Kristen Carlson Accardi <kristen@...ux.intel.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] x86/sgx: Replace kmap/kunmap_atomic calls

On Wed, Nov 16 2022 at 21:00, Fabio M. De Francesco wrote:
> On mercoledì 16 novembre 2022 18:21:50 CET Ira Weiny wrote:
>> > Despite I suppose that nobody should rely on those kmap_atomic() side
>> > effects, I have been observing that a large part of the users of the
>> > Highmem API used to call kmap_atomic() for its implicit
>> > pagefault_disable()
>> > and preempt_disable() side effects.
>>
>> Fabio I think you missed the point here.  Just because we have found _some_
>> places where the side effect was required does not mean that "a large part
>> of the users..." do.
>
> You are right. Numbers don't support that "a large part of the users..." but
> that it happened and will happen again.

There is a fundamental misconception here. You argue about requirement
instead of dependency. Those are absolutely not the same.

Technically there is no requirement at all to use kmap_atomic().

kmap_atomic() in it's original implementation had the requirement of
disabling preemption to protect the temporary mapping. kmap_atomic()
is nothing else than a conveniance wrapper:

   preempt_disable();
   establish_mapping();

and later on when pagefault_disable() was separated from
preempt_disable() it became:

   preempt_disable();
   pagefault_disable();
   establish_mapping();

which is again technically not required at all.

It was done because there were usage sites where the implementation of
the kmap_atomic() section _depended_ on the implicit pagefault_disable()
of preempt_disable(). So the pagefault_disable() was added in order not
to break these usage sites.

But did any of the usage sites have a hard technical requirment to have
pagefaults or preemption disabled by kmap_atomic(). No. Not a single
one. All of them could have been converted to disable preemption and/or
pagefaults explicitely.

The implementation dependencies are a consequence of the original
kmap_atomic() implementation and not the other way round.

There is no single context where the implicit preemption and pagefault
disable of kmap_atomic() is required due to the context itself.

If there is code which runs, e.g. with interrupts disabled and needs a
temporary mapping which is then used for a copy to user operation, which
can obviously fault, then there is still no requirement for
kmap_atomic() to disable preemption or pagefaults.

The requirement to disable pagefaults comes from the
copy_to_user_inatomic() which is in turn necessary because the code runs
with interrupts disabled.

So now we have kmap_local() which does not rely on preemption disable
anymore because the protection of the temporary mapping is done
differently.

So you can convert any instance of kmap_atomic() over to kmap_local(),
but you have to analyze whether any code inside the mapped section
requires protection against preemption or pagefault handling.

If there is code which requires that, then you have to figure out
whether that code has been written in that way due to the semantics of
kmap_atomic() or if there is a different requirement for it.

In the previous example I gave you vs. copy_to_user_inatomic()

again:
    kmap_atomic();
    remaining = copy_to_user_inatomic();
    kunmap_atomic();
    if (remaining) {
	if (try_to_handle_fault())
	    goto again;
	ret = -EFAULT;
    }

is clearly no requirement neither for preemption nor for pagefault
disable. The semantics of kmap_atomic() enforced to write the code this
way. The replacement code:

    kmap_local();
    ret = copy_to_user();
    kunmap_local();

is completely equivalent. It can be written this way because
kmap_local() has no implicit side effects which prevent using
copy_to_user() which in turn can fault, handle the fault ...

This is all about implementation details which depend on the original
kmap_atomic() semantics and have been enforced by them.

E.g. something like this:

    kmap_atomic();
    smp_processor_id();
    kunmap_atomic();

has a dependency, but not a requirement.

    kmap_local();
    preempt_disable();
    smp_processor_id();
    preempt_enable();
    kunmap_local();
    
is completely equivalent and it makes it entirely clear why preemption
needs to be disabled: because smp_processor_id() requires it.

There are only the three classes of usage sites which are affected:

   1) The code which is enforced to be stupid because of kmap_atomic()

   2) The code which (ab)uses the side effects of kmap_atomic()

   3) The combination of #1 and #2

All of them can be replaced by functionaly equivalent code.

There is not a single usage site which has a hard requirement on the
kmap_atomic() semantics due to the context in which kmap_atomic() is
invoked.

Ergo _ALL_ usage of kmap_atomic() and related interfaces can be
eliminated completely.

Claiming that even a single place

>> > used to call kmap_atomic() for its implicit pagefault_disable() and
>> > preempt_disable() side effects.

belongs into fairy tale territory, but has nothing to do with technical
reality and facts. Why? Simply because there was no other solution to
handle highmem in a hotpath or in a non-sleepable context.

Ergo developers were forced to use kmap_atomic() and then obviously had
to accomodate the code in the mapping section. Of course they utilized
the side effects because that's what we do with spinlocks and other
primitives as well.

So can we just go and fix these places instead of indulging in
handwaving and bikeshedding?

Thanks,

        tglx

Powered by blists - more mailing lists