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@xxxxxxxxxxxx >>> ? >> >> 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(). Christophe
