Re: [PATCH] x86/sgx: Replace kmap/kunmap_atomic calls

[Fabio M. De Francesco]

On Thursday, September 29, 2022 6:06:46 PM CEST Kristen Carlson Accardi wrote:
> It is not necessary to disable page faults or preemption when
> using kmap calls

Do you refer to the page faults disabling that kmap_atomic() provides as a 
side effect? Can you please clarify a little more? kmap_atomic() disables 
always only page faults, instead it might not disable preemption; it depends 
on CONFIG_PREEMPT_RT. Therefore, why are you also talking about preemption?

Are you converting code which runs in atomic context regardless kmap_atomic()? 
If so, between kmap() and kmap_atomic(), the author(s) had only one choice, it 
correctly was kmap_atomic(), otherwise we might end up with a perfect recipe
for triggering SAC bugs.

kmap() were not suited in those cases because it might sleep. If the intents 
of the author are simply map a page while in atomic, so to avoid sleeping in 
atomic bugs, your conversions looks good. 

For the reasons above, can you please say something more about why this code 
needed a kmap_atomic() instead of calling kmap()?

A different case is in choosing kmap_atomic() is there because of its side 
effects, despite the code is running in non atomic context until the mapping, 
but it needs to disable pagefaults only somewhere between the mapping and 
unmapping. This is a trickier case than the above-mentioned one because along 
with conversion developers should at least disable the pagefaults and 
probably, although not necessarily, also disable preemption.

> , so replace kmap_atomic() and kunmap_atomic()
> calls with more the more appropriate kmap_local_page() and
> kunmap_local() calls.

Why is kmap_local_page() better suited in general and is safe in 
this specific case? 

I think that we should provide the maintainer with well supported reasons why 
they should change any piece of code which is still doing what it is thought 
for. A mere deprecation in favour of a newer API may not be enough to change 
code that is still working properly (like in the old "if it's not broken, 
don't fix it!", or something like this :)).

Thanks,

Fabio


> 
> Signed-off-by: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/sgx/encl.c  | 12 ++++++------
>  arch/x86/kernel/cpu/sgx/ioctl.c |  4 ++--
>  arch/x86/kernel/cpu/sgx/main.c  |  8 ++++----
>  3 files changed, 12 insertions(+), 12 deletions(-)
> 

[snip]