| Message ID | 5A29E00D83AB84E3+20240706031101.637601-1-wangyuli@uniontech.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF |
| Headers | show |
| Series | Revert "bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()" for linux-6.6.37 | expand |
On Sat, Jul 06, 2024 at 11:11:01AM +0800, WangYuli wrote: > This reverts commit 08f6c05feb1db21653e98ca84ea04ca032d014c7. > > Upstream commit e60adf513275 ("bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()") > depends on > upstream commit 1dad391daef1 ("bpf, arm64: use bpf_prog_pack for memory management"). > > It will cause a compilation warning on the arm64 if it's not merged: > arch/arm64/net/bpf_jit_comp.c: In function ‘bpf_int_jit_compile’: > arch/arm64/net/bpf_jit_comp.c:1651:17: warning: ignoring return value of ‘bpf_jit_binary_lock_ro’ declared with attribute ‘warn_unused_result’ [-Wunused-result] > 1651 | bpf_jit_binary_lock_ro(header); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > This will prevent the kernel with the '-Werror' compile option from > being compiled successfully. > > We might as well revert this commit in linux-6.6.37 to solve the > problem in a simple way. 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 ? If not, why not just backport the single missing arm64 commit, and why didn't this show up in testing? confused, greg k-h
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. > 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, but it's not the simplest solution. > and why > didn't this show up in testing? Thanks for the tip. I'll be sure to keep a closer eye on the stable kernel testing phase in the future and hopefully catch any problems more timely. > confused, > > greg k-h > Sincerely
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? > > 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, but 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? thanks, greg k-h
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(). Christophe
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. thanks, greg k-h
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
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@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 ? 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
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@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 ? > > 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
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c index ac8e4d9bf954..6a1c9fca5260 100644 --- a/arch/arm/net/bpf_jit_32.c +++ b/arch/arm/net/bpf_jit_32.c @@ -1982,21 +1982,28 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) /* If building the body of the JITed code fails somehow, * we fall back to the interpretation. */ - if (build_body(&ctx) < 0) - goto out_free; + if (build_body(&ctx) < 0) { + image_ptr = NULL; + bpf_jit_binary_free(header); + prog = orig_prog; + goto out_imms; + } build_epilogue(&ctx); /* 3.) Extra pass to validate JITed Code */ - if (validate_code(&ctx)) - goto out_free; + if (validate_code(&ctx)) { + image_ptr = NULL; + bpf_jit_binary_free(header); + prog = orig_prog; + goto out_imms; + } flush_icache_range((u32)header, (u32)(ctx.target + ctx.idx)); if (bpf_jit_enable > 1) /* there are 2 passes here */ bpf_jit_dump(prog->len, image_size, 2, ctx.target); - if (bpf_jit_binary_lock_ro(header)) - goto out_free; + bpf_jit_binary_lock_ro(header); prog->bpf_func = (void *)ctx.target; prog->jited = 1; prog->jited_len = image_size; @@ -2013,11 +2020,5 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) bpf_jit_prog_release_other(prog, prog == orig_prog ? tmp : orig_prog); return prog; - -out_free: - image_ptr = NULL; - bpf_jit_binary_free(header); - prog = orig_prog; - goto out_imms; } diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c index 13cd480385ca..9eb7753d117d 100644 --- a/arch/loongarch/net/bpf_jit.c +++ b/arch/loongarch/net/bpf_jit.c @@ -1206,19 +1206,16 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) flush_icache_range((unsigned long)header, (unsigned long)(ctx.image + ctx.idx)); if (!prog->is_func || extra_pass) { - int err; - if (extra_pass && ctx.idx != jit_data->ctx.idx) { pr_err_once("multi-func JIT bug %d != %d\n", ctx.idx, jit_data->ctx.idx); - goto out_free; - } - err = bpf_jit_binary_lock_ro(header); - if (err) { - pr_err_once("bpf_jit_binary_lock_ro() returned %d\n", - err); - goto out_free; + bpf_jit_binary_free(header); + prog->bpf_func = NULL; + prog->jited = 0; + prog->jited_len = 0; + goto out_offset; } + bpf_jit_binary_lock_ro(header); } else { jit_data->ctx = ctx; jit_data->image = image_ptr; @@ -1249,13 +1246,6 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) out_offset = -1; return prog; - -out_free: - bpf_jit_binary_free(header); - prog->bpf_func = NULL; - prog->jited = 0; - prog->jited_len = 0; - goto out_offset; } /* Indicate the JIT backend supports mixing bpf2bpf and tailcalls. */ diff --git a/arch/mips/net/bpf_jit_comp.c b/arch/mips/net/bpf_jit_comp.c index e355dfca4400..a40d926b6513 100644 --- a/arch/mips/net/bpf_jit_comp.c +++ b/arch/mips/net/bpf_jit_comp.c @@ -1012,8 +1012,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) bpf_prog_fill_jited_linfo(prog, &ctx.descriptors[1]); /* Set as read-only exec and flush instruction cache */ - if (bpf_jit_binary_lock_ro(header)) - goto out_err; + bpf_jit_binary_lock_ro(header); flush_icache_range((unsigned long)header, (unsigned long)&ctx.target[ctx.jit_index]); diff --git a/arch/parisc/net/bpf_jit_core.c b/arch/parisc/net/bpf_jit_core.c index 979f45d4d1fb..d6ee2fd45550 100644 --- a/arch/parisc/net/bpf_jit_core.c +++ b/arch/parisc/net/bpf_jit_core.c @@ -167,13 +167,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) bpf_flush_icache(jit_data->header, ctx->insns + ctx->ninsns); if (!prog->is_func || extra_pass) { - if (bpf_jit_binary_lock_ro(jit_data->header)) { - bpf_jit_binary_free(jit_data->header); - prog->bpf_func = NULL; - prog->jited = 0; - prog->jited_len = 0; - goto out_offset; - } + bpf_jit_binary_lock_ro(jit_data->header); prologue_len = ctx->epilogue_offset - ctx->body_len; for (i = 0; i < prog->len; i++) ctx->offset[i] += prologue_len; diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c index 05746e22fe79..62ee557d4b49 100644 --- a/arch/s390/net/bpf_jit_comp.c +++ b/arch/s390/net/bpf_jit_comp.c @@ -1973,11 +1973,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp) print_fn_code(jit.prg_buf, jit.size_prg); } if (!fp->is_func || extra_pass) { - if (bpf_jit_binary_lock_ro(header)) { - bpf_jit_binary_free(header); - fp = orig_fp; - goto free_addrs; - } + bpf_jit_binary_lock_ro(header); } else { jit_data->header = header; jit_data->ctx = jit; diff --git a/arch/sparc/net/bpf_jit_comp_64.c b/arch/sparc/net/bpf_jit_comp_64.c index 73bf0aea8baf..fa0759bfe498 100644 --- a/arch/sparc/net/bpf_jit_comp_64.c +++ b/arch/sparc/net/bpf_jit_comp_64.c @@ -1602,11 +1602,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) bpf_flush_icache(header, (u8 *)header + header->size); if (!prog->is_func || extra_pass) { - if (bpf_jit_binary_lock_ro(header)) { - bpf_jit_binary_free(header); - prog = orig_prog; - goto out_off; - } + bpf_jit_binary_lock_ro(header); } else { jit_data->ctx = ctx; jit_data->image = image_ptr; diff --git a/arch/x86/net/bpf_jit_comp32.c b/arch/x86/net/bpf_jit_comp32.c index f2fc8c38629b..429a89c5468b 100644 --- a/arch/x86/net/bpf_jit_comp32.c +++ b/arch/x86/net/bpf_jit_comp32.c @@ -2600,7 +2600,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) if (bpf_jit_enable > 1) bpf_jit_dump(prog->len, proglen, pass + 1, image); - if (image && !bpf_jit_binary_lock_ro(header)) { + if (image) { + bpf_jit_binary_lock_ro(header); prog->bpf_func = (void *)image; prog->jited = 1; prog->jited_len = proglen; diff --git a/include/linux/filter.h b/include/linux/filter.h index a74d97114a54..5a2800ec94ea 100644 --- a/include/linux/filter.h +++ b/include/linux/filter.h @@ -853,11 +853,10 @@ static inline int __must_check bpf_prog_lock_ro(struct bpf_prog *fp) return 0; } -static inline int __must_check -bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr) +static inline void bpf_jit_binary_lock_ro(struct bpf_binary_header *hdr) { set_vm_flush_reset_perms(hdr); - return set_memory_rox((unsigned long)hdr, hdr->size >> PAGE_SHIFT); + set_memory_rox((unsigned long)hdr, hdr->size >> PAGE_SHIFT); } int sk_filter_trim_cap(struct sock *sk, struct sk_buff *skb, unsigned int cap);
This reverts commit 08f6c05feb1db21653e98ca84ea04ca032d014c7. Upstream commit e60adf513275 ("bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()") depends on upstream commit 1dad391daef1 ("bpf, arm64: use bpf_prog_pack for memory management"). It will cause a compilation warning on the arm64 if it's not merged: arch/arm64/net/bpf_jit_comp.c: In function ‘bpf_int_jit_compile’: arch/arm64/net/bpf_jit_comp.c:1651:17: warning: ignoring return value of ‘bpf_jit_binary_lock_ro’ declared with attribute ‘warn_unused_result’ [-Wunused-result] 1651 | bpf_jit_binary_lock_ro(header); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This will prevent the kernel with the '-Werror' compile option from being compiled successfully. We might as well revert this commit in linux-6.6.37 to solve the problem in a simple way. Signed-off-by: WangYuli <wangyuli@uniontech.com> --- arch/arm/net/bpf_jit_32.c | 25 +++++++++++++------------ arch/loongarch/net/bpf_jit.c | 22 ++++++---------------- arch/mips/net/bpf_jit_comp.c | 3 +-- arch/parisc/net/bpf_jit_core.c | 8 +------- arch/s390/net/bpf_jit_comp.c | 6 +----- arch/sparc/net/bpf_jit_comp_64.c | 6 +----- arch/x86/net/bpf_jit_comp32.c | 3 ++- include/linux/filter.h | 5 ++--- 8 files changed, 27 insertions(+), 51 deletions(-)