On Tue, Jul 09, 2024 at 09:24:54AM +0000, LEROY Christophe wrote: > > > 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@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(). > > > > 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 ? Based on the changelog text itself, it fixes an issue and was flagged as something to be backported. If this isn't needed in 6.6.y, then no worries at all, we can just drop them and be happy :) thanks, greg k-h
