Received-SPF: pass (google.com: domain of linux-kernel+bounces-119048-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Tue, 26 Mar 2024 15:18:21 +0200
Message-Id: <D03PM9A6IS79.3D6BW7KBLH9C3@kernel.org>
To: "Jarkko Sakkinen" <jarkko@kernel.org>, "Masami Hiramatsu"
 <mhiramat@kernel.org>
Cc: <linux-riscv@lists.infradead.org>, "Paul Walmsley"
 <paul.walmsley@sifive.com>, "Palmer Dabbelt" <palmer@dabbelt.com>, "Albert
 Ou" <aou@eecs.berkeley.edu>, <linux-kernel@vger.kernel.org>, "Luis
 Chamberlain" <mcgrof@kernel.org>, <linux-modules@vger.kernel.org>, "Naveen
 N . Rao" <naveen.n.rao@linux.ibm.com>, "Anil S Keshavamurthy"
 <anil.s.keshavamurthy@intel.com>, "David S . Miller" <davem@davemloft.net>
Subject: Re: [PATCH v5 1/2] kprobes: textmem API
From: "Jarkko Sakkinen" <jarkko@kernel.org>
References: <20240325215502.660-1-jarkko@kernel.org>
 <20240326095836.f43d259b7747269a7c0b9d23@kernel.org>
 <D03AL7A5G3M2.3UK4ASWILGBJS@kernel.org>
 <D03B7XJYRFC1.2L3I2TO5HNQD3@kernel.org>
In-Reply-To: <D03B7XJYRFC1.2L3I2TO5HNQD3@kernel.org>

On Tue Mar 26, 2024 at 4:01 AM EET, Jarkko Sakkinen wrote:
> On Tue Mar 26, 2024 at 3:31 AM EET, Jarkko Sakkinen wrote:
> > > > +#endif /* _LINUX_EXECMEM_H */
> > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > > > index 9d9095e81792..87fd8c14a938 100644
> > > > --- a/kernel/kprobes.c
> > > > +++ b/kernel/kprobes.c
> > > > @@ -44,6 +44,7 @@
> > > >  #include <asm/cacheflush.h>
> > > >  #include <asm/errno.h>
> > > >  #include <linux/uaccess.h>
> > > > +#include <linux/execmem.h>
> > > > =20
> > > >  #define KPROBE_HASH_BITS 6
> > > >  #define KPROBE_TABLE_SIZE (1 << KPROBE_HASH_BITS)
> > > > @@ -113,17 +114,17 @@ enum kprobe_slot_state {
> > > >  void __weak *alloc_insn_page(void)
> > > >  {
> > > >  	/*
> > > > -	 * Use module_alloc() so this page is within +/- 2GB of where the
> > > > +	 * Use alloc_execmem() so this page is within +/- 2GB of where th=
e
> > > >  	 * kernel image and loaded module images reside. This is required
> > > >  	 * for most of the architectures.
> > > >  	 * (e.g. x86-64 needs this to handle the %rip-relative fixups.)
> > > >  	 */
> > > > -	return module_alloc(PAGE_SIZE);
> > > > +	return alloc_execmem(PAGE_SIZE, GFP_KERNEL);
> > > >  }
> > > > =20
> > > >  static void free_insn_page(void *page)
> > > >  {
> > > > -	module_memfree(page);
> > > > +	free_execmem(page);
> > > >  }
> > > > =20
> > > >  struct kprobe_insn_cache kprobe_insn_slots =3D {
> > > > @@ -1580,6 +1581,7 @@ static int check_kprobe_address_safe(struct k=
probe *p,
> > > >  		goto out;
> > > >  	}
> > > > =20
> > > > +#ifdef CONFIG_MODULES
> > >
> > > You don't need this block, because these APIs have dummy functions.
> >
> > Hmm... I'll verify this tomorrow.
>
> It depends on having struct module available given "(*probed_mod)->state"=
.
>
> It is non-existent unless CONFIG_MODULES is set given how things are
> flagged in include/linux/module.h.

Hey, noticed kconfig issue.

According to kconfig-language.txt:

"select should be used with care. select will force a symbol to a value
without visiting the dependencies."

So the problem here lies in KPROBES config entry using select statement
to pick ALLOC_EXECMEM. It will not take the depends on statement into
account and thus will allow to select kprobes without any allocator in
place.

So to address this I'd suggest to use depends on statement also for
describing relation between KPROBES and ALLOC_EXECMEM. It does not make
life worse than before for anyone because even with the current kernel
you have to select MODULES before you can move forward with kprobes.

BR, Jarkko