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 <nicola.vetr...@bugseng.com>
>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Roger Pau Monné <roger....@citrix.com>
>> CC: Wei Liu <w...@xen.org>
>> CC: George Dunlap <george.dun...@eu.citrix.com>
>> CC: Tim Deegan <t...@xen.org>
>> CC: Stefano Stabellini <sstabell...@kernel.org>
>> CC: Nicola Vetrini <nicola.vetr...@bugseng.com>
>>
>> Slightly RFC. Only compile tested so far.
> With shadow/none.c also suitably edited
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
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
