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