Le 09/07/2024 à 11:39, Greg KH a écrit : > 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 :) > In fact this change is part of a long-term hardening effort as described in https://github.com/KSPP/linux/issues/7 I'm not sure it's worth picking it up on its own, so I propose to not do anything unless someone raises their hand. Christophe
