Re: [PATCH] Revert "bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()" for linux-6.6.37

[LEROY Christophe <christophe.leroy2@cs-soprasteria.com>]

Le 09/07/2024 à 11:15, Greg KH a écrit :
> On Mon, Jul 08, 2024 at 03:12:55PM +0000, LEROY Christophe wrote:
>>
>>
>> Le 08/07/2024 à 14:36, Greg KH a écrit :
>>> On Sun, Jul 07, 2024 at 03:34:15PM +0800, WangYuli wrote:
>>>>
>>>> On 2024/7/6 17:30, Greg KH wrote:
>>>>> This makes it sound like you are reverting this because of a build
>>>>> error, which is not the case here, right?  Isn't this because of the
>>>>> powerpc issue reported here:
>>>>>      https://lore.kernel.org/r/20240705203413.wbv2nw3747vjeibk@altlinux.org
>>>>> ?
>>>>
>>>> No, it only occurs on ARM64 architecture. The reason is that before being
>>>> modified, the function
>>>>
>>>> bpf_jit_binary_lock_ro() in arch/arm64/net/bpf_jit_comp.c +1651
>>>>
>>>> was introduced with __must_check, which is defined as
>>>> __attribute__((__warn_unused_result__)).
>>>>
>>>>
>>>> However, at this point, calling bpf_jit_binary_lock_ro(header)
>>>> coincidentally results in an unused-result
>>>>
>>>> warning.
>>>
>>> Ok, thanks, but why is no one else seeing this in their testing?
>>
>> Probably the configs used by robots do not activate BPF JIT ?
>>
>>>
>>>>> If not, why not just backport the single missing arm64 commit,
>>>>
>>>> Upstream commit 1dad391daef1 ("bpf, arm64: use bpf_prog_pack for memory
>>>> management") is part of
>>>>
>>>> a larger change that involves multiple commits. It's not an isolated commit.
>>>>
>>>>
>>>> We could certainly backport all of them to solve this problem, buthas it's not
>>>> the simplest solution.
>>>
>>> reverting the change feels wrong in that you will still have the bug
>>> present that it was trying to solve, right?  If so, can you then provide
>>> a working version?
>>
>> Indeed, by reverting the change you "punish" all architectures because
>> arm64 hasn't properly been backported, is it fair ?
>>
>> In fact, when I implemented commit e60adf513275 ("bpf: Take return from
>> set_memory_rox() into account with bpf_jit_binary_lock_ro()"), we had
>> the following users for function bpf_jit_binary_lock_ro() :
>>
>> $ git grep bpf_jit_binary_lock_ro e60adf513275~
>> e60adf513275~:arch/arm/net/bpf_jit_32.c:
>> bpf_jit_binary_lock_ro(header);
>> e60adf513275~:arch/loongarch/net/bpf_jit.c:
>> bpf_jit_binary_lock_ro(header);
>> e60adf513275~:arch/mips/net/bpf_jit_comp.c:
>> bpf_jit_binary_lock_ro(header);
>> e60adf513275~:arch/parisc/net/bpf_jit_core.c:
>> bpf_jit_binary_lock_ro(jit_data->header);
>> e60adf513275~:arch/s390/net/bpf_jit_comp.c:
>> bpf_jit_binary_lock_ro(header);
>> e60adf513275~:arch/sparc/net/bpf_jit_comp_64.c:
>> bpf_jit_binary_lock_ro(header);
>> e60adf513275~:arch/x86/net/bpf_jit_comp32.c:
>> bpf_jit_binary_lock_ro(header);
>> e60adf513275~:include/linux/filter.h:static inline void
>> bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr)
>>
>> But when commit 08f6c05feb1d ("bpf: Take return from set_memory_rox()
>> into account with bpf_jit_binary_lock_ro()") was applied, we had one
>> more user which is arm64:
>>
>> $ git grep bpf_jit_binary_lock_ro 08f6c05feb1d~
>> 08f6c05feb1d~:arch/arm/net/bpf_jit_32.c:
>> bpf_jit_binary_lock_ro(header);
>> 08f6c05feb1d~:arch/arm64/net/bpf_jit_comp.c:
>> bpf_jit_binary_lock_ro(header);
>> 08f6c05feb1d~:arch/loongarch/net/bpf_jit.c:
>> bpf_jit_binary_lock_ro(header);
>> 08f6c05feb1d~:arch/mips/net/bpf_jit_comp.c:
>> bpf_jit_binary_lock_ro(header);
>> 08f6c05feb1d~:arch/parisc/net/bpf_jit_core.c:
>> bpf_jit_binary_lock_ro(jit_data->header);
>> 08f6c05feb1d~:arch/s390/net/bpf_jit_comp.c:
>> bpf_jit_binary_lock_ro(header);
>> 08f6c05feb1d~:arch/sparc/net/bpf_jit_comp_64.c:
>> bpf_jit_binary_lock_ro(header);
>> 08f6c05feb1d~:arch/x86/net/bpf_jit_comp32.c:
>> bpf_jit_binary_lock_ro(header);
>> 08f6c05feb1d~:include/linux/filter.h:static inline void
>> bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr)
>>
>> Therefore, commit 08f6c05feb1d should have included a backport for arm64.
>>
>> So yes, I agree with Greg, the correct fix should be to backport to
>> ARM64 the changes done on other architectures in order to properly
>> handle return of set_memory_rox() in bpf_jit_binary_lock_ro().
>
> Ok, but it looks like due to this series, the powerpc tree is crashing
> at the first bpf load, so something went wrong.  Let me go revert these
> 4 patches for now, and then I will be glad to queue them up if you can
> provide a working series for all arches.
>

Fair enough, indeed I think for powerpc it probably also relies on more
changes, so both ARM and POWERPC need a carefull backport.

I can look at it, but can you tell why it was decided to apply that
commit on stable at the first place ? Is there a particular need ?

Thanks
Christophe