Message-ID: <23b480a7-fc50-44a9-b6f7-9d112059e7c3@cs-soprasteria.com>
Date: Tue, 9 Jul 2024 10:03:37 +0000
From: LEROY Christophe <christophe.leroy2@...soprasteria.com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: WangYuli <wangyuli@...ontech.com>, "stable@...r.kernel.org"
	<stable@...r.kernel.org>, "sashal@...nel.org" <sashal@...nel.org>,
	"ast@...nel.org" <ast@...nel.org>, "keescook@...omium.org"
	<keescook@...omium.org>, "linux-hardening@...r.kernel.org"
	<linux-hardening@...r.kernel.org>, "catalin.marinas@....com"
	<catalin.marinas@....com>, "song@...nel.org" <song@...nel.org>,
	"puranjay12@...il.com" <puranjay12@...il.com>, "daniel@...earbox.net"
	<daniel@...earbox.net>, "andrii@...nel.org" <andrii@...nel.org>,
	"martin.lau@...ux.dev" <martin.lau@...ux.dev>, "yonghong.song@...ux.dev"
	<yonghong.song@...ux.dev>, "john.fastabend@...il.com"
	<john.fastabend@...il.com>, "kpsingh@...nel.org" <kpsingh@...nel.org>,
	"sdf@...gle.com" <sdf@...gle.com>, "haoluo@...gle.com" <haoluo@...gle.com>,
	"jolsa@...nel.org" <jolsa@...nel.org>, "illusionist.neo@...il.com"
	<illusionist.neo@...il.com>, "linux@...linux.org.uk" <linux@...linux.org.uk>,
	"bpf@...r.kernel.org" <bpf@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "chenhuacai@...nel.org"
	<chenhuacai@...nel.org>, "kernel@...0n.name" <kernel@...0n.name>,
	"loongarch@...ts.linux.dev" <loongarch@...ts.linux.dev>,
	"johan.almbladh@...finetworks.com" <johan.almbladh@...finetworks.com>,
	"paulburton@...nel.org" <paulburton@...nel.org>, "tsbogend@...ha.franken.de"
	<tsbogend@...ha.franken.de>, "linux-mips@...r.kernel.org"
	<linux-mips@...r.kernel.org>, "deller@....de" <deller@....de>,
	"linux-parisc@...r.kernel.org" <linux-parisc@...r.kernel.org>,
	"iii@...ux.ibm.com" <iii@...ux.ibm.com>, "hca@...ux.ibm.com"
	<hca@...ux.ibm.com>, "gor@...ux.ibm.com" <gor@...ux.ibm.com>,
	"agordeev@...ux.ibm.com" <agordeev@...ux.ibm.com>,
	"borntraeger@...ux.ibm.com" <borntraeger@...ux.ibm.com>,
	"svens@...ux.ibm.com" <svens@...ux.ibm.com>, "linux-s390@...r.kernel.org"
	<linux-s390@...r.kernel.org>, "davem@...emloft.net" <davem@...emloft.net>,
	"sparclinux@...r.kernel.org" <sparclinux@...r.kernel.org>, "kuba@...nel.org"
	<kuba@...nel.org>, "hawk@...nel.org" <hawk@...nel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "dsahern@...nel.org"
	<dsahern@...nel.org>, "tglx@...utronix.de" <tglx@...utronix.de>,
	"mingo@...hat.com" <mingo@...hat.com>, "bp@...en8.de" <bp@...en8.de>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>, "x86@...nel.org"
	<x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>, "guanwentao@...ontech.com"
	<guanwentao@...ontech.com>, "baimingcong@...ontech.com"
	<baimingcong@...ontech.com>
Subject: Re: [PATCH] Revert "bpf: Take return from set_memory_rox() into
 account with bpf_jit_binary_lock_ro()" for linux-6.6.37



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

Powered by blists - more mailing lists