lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2024070908-glade-granny-1137@gregkh>
Date: Tue, 9 Jul 2024 11:15:57 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: LEROY Christophe <christophe.leroy2@...soprasteria.com>
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

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ