Re: [PATCH 1/9] x86: vdso: Introduce asm/vdso/mman.h

[Vincenzo Frascino <vincenzo.frascino@arm.com>]

Hi Christophe,

Thank you for your review.

On 04/09/2024 17:56, Christophe Leroy wrote:
> 
> 
> Le 03/09/2024 à 17:14, Vincenzo Frascino a écrit :
...

>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __ASM_VDSO_MMAN_H
>> +#define __ASM_VDSO_MMAN_H
>> +
>> +#ifndef __ASSEMBLY__
>> +
>> +#include <uapi/linux/mman.h>
>> +
>> +#define VDSO_MMAP_PROT    PROT_READ | PROT_WRITE
>> +#define VDSO_MMAP_FLAGS    MAP_DROPPABLE | MAP_ANONYMOUS
> 
> I still can't see the benefit of duplicating those two defines in every arch.
> 
> I understand your point that some arch might in the future want to use different
> flags, but unless we already have one such architecture in mind we shouldn't
> make things more complicated than needed.
> 
> In case such an architecture is already identified it should be said in the
> commit message
> 

I do not have such an architecture in mind, hence I did not add it to the commit
message.

Apart being future proof the real problem here is to handle the mman.h header.
As Arnd was saying [1] (and I agree), including it on some architectures might
be problematic if they change it in a way that is incompatible with compat vdso.

In this way we make sure that the each architecture that decides to include it
specifically validates it for this purpose. I am not a fan of complicating code
either but this seems the lesser evil. I am open to any solution you can come up
that is better then this one.

The other issue I see is that being these defines in a uapi header that is
included directly by userspace splitting it requires some validation to make
sure we do not break the status quo.

[1]
https://lore.kernel.org/lkml/cb66b582-ba63-4a5a-9df8-b07288f1f66d@app.fastmail.com/

>> +
>> +#endif /* !__ASSEMBLY__ */
>> +
>> +#endif /* __ASM_VDSO_MMAN_H */

-- 
Regards,
Vincenzo