diff mbox series

Revert "bpf: Take return from set_memory_rox() into account with bpf_jit_binary_lock_ro()" for linux-6.6.37

Message ID 5A29E00D83AB84E3+20240706031101.637601-1-wangyuli@uniontech.com (mailing list archive)
State Handled Elsewhere
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

Commit Message

WangYuli July 6, 2024, 3:11 a.m. UTC
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(-)

Comments

Greg KH July 6, 2024, 9:30 a.m. UTC | #1
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
WangYuli July 7, 2024, 7:34 a.m. UTC | #2
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
Greg KH July 8, 2024, 12:36 p.m. UTC | #3
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
LEROY Christophe July 8, 2024, 3:12 p.m. UTC | #4
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
Greg KH July 9, 2024, 9:15 a.m. UTC | #5
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
LEROY Christophe July 9, 2024, 9:24 a.m. UTC | #6
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
Greg KH July 9, 2024, 9:39 a.m. UTC | #7
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
LEROY Christophe July 9, 2024, 10:03 a.m. UTC | #8
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 mbox series

Patch

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);