On 21/09/2023 1:38 pm, Jan Beulich wrote:
> On 20.09.2023 21:21, Andrew Cooper wrote:
>> Nicola reports that the XSA-438 fix introduced new MISRA violations because
>> of
>> some incidental tidying it tried to do. The parameter is useless, so resolve
>> the MISRA regression by removing it.
>>
>> hap_update_cr3() discards the parameter entirely, while sh_update_cr3() uses
>> it to distinguish internal and external callers and therefore whether the
>> paging lock should be taken.
>>
>> However, we have paging_lock_recursive() for this purpose, which also avoids
>> the ability for the shadow internal callers to accidentally not hold the
>> lock.
>>
>> Fixes: fb0ff49fe9f7 ("x86/shadow: defer releasing of PV's top-level shadow
>> reference")
>> Reported-by: Nicola Vetrini <[email protected]>
>> Signed-off-by: Andrew Cooper <[email protected]>
>> ---
>> CC: Jan Beulich <[email protected]>
>> CC: Roger Pau Monné <[email protected]>
>> CC: Wei Liu <[email protected]>
>> CC: George Dunlap <[email protected]>
>> CC: Tim Deegan <[email protected]>
>> CC: Stefano Stabellini <[email protected]>
>> CC: Nicola Vetrini <[email protected]>
>>
>> Slightly RFC. Only compile tested so far.
> With shadow/none.c also suitably edited
> Reviewed-by: Jan Beulich <[email protected]>
Ah yes - I did forget about none.c. Thanks.
> I'm a little surprised you introduce new uses of the (kind of odd) recursive
> lock,
> when previously you voiced your dislike for our use of such. ("Kind of odd"
> because
> unlike spin_lock_recursive(), only the potentially inner caller needs to use
> the
> recursive form of the acquire.)
I do very much dislike recursive locks, and I do think that an
alternative universe without them would be better code. But a stream of
int/bool params are a similarly bad antipattern too.
As paging_lock_recursive() is used for this exact purpose elsewhere,
it's silly not to use fix one of the problems when it doesn't really
make the other problem worse.
~Andrew
