Re: [PATCH v4 05/15] mm: introduce execmem_alloc() and execmem_free()
On Sat, 20 Apr 2024 07:22:50 +0300
Mike Rapoport wrote:
> On Fri, Apr 19, 2024 at 02:42:16PM -0700, Song Liu wrote:
> > On Fri, Apr 19, 2024 at 1:00 PM Mike Rapoport wrote:
> > >
> > > On Fri, Apr 19, 2024 at 10:32:39AM -0700, Song Liu wrote:
> > > > On Fri, Apr 19, 2024 at 10:03 AM Mike Rapoport wrote:
> > > > [...]
> > > > > > >
> > > > > > > [1]
> > > > > > > https://lore.kernel.org/all/20240411160526.2093408-1-r...@kernel.org
> > > > > >
> > > > > > For the ROX to work, we need different users (module text, kprobe,
> > > > > > etc.) to have
> > > > > > the same execmem_range. From [1]:
> > > > > >
> > > > > > static void *execmem_cache_alloc(struct execmem_range *range,
> > > > > > size_t size)
> > > > > > {
> > > > > > ...
> > > > > >p = __execmem_cache_alloc(size);
> > > > > >if (p)
> > > > > >return p;
> > > > > > err = execmem_cache_populate(range, size);
> > > > > > ...
> > > > > > }
> > > > > >
> > > > > > We are calling __execmem_cache_alloc() without range. For this to
> > > > > > work,
> > > > > > we can only call execmem_cache_alloc() with one execmem_range.
> > > > >
> > > > > Actually, on x86 this will "just work" because everything shares the
> > > > > same
> > > > > address space :)
> > > > >
> > > > > The 2M pages in the cache will be in the modules space, so
> > > > > __execmem_cache_alloc() will always return memory from that address
> > > > > space.
> > > > >
> > > > > For other architectures this indeed needs to be fixed with passing the
> > > > > range to __execmem_cache_alloc() and limiting search in the cache for
> > > > > that
> > > > > range.
> > > >
> > > > I think we at least need the "map to" concept (initially proposed by
> > > > Thomas)
> > > > to get this work. For example, EXECMEM_BPF and EXECMEM_KPROBE
> > > > maps to EXECMEM_MODULE_TEXT, so that all these actually share
> > > > the same range.
> > >
> > > Why?
> >
> > IIUC, we need to update __execmem_cache_alloc() to take a range pointer as
> > input. module text will use "range" for EXECMEM_MODULE_TEXT, while kprobe
> > will use "range" for EXECMEM_KPROBE. Without "map to" concept or sharing
> > the "range" object, we will have to compare different range parameters to
> > check
> > we can share cached pages between module text and kprobe, which is not
> > efficient. Did I miss something?
Song, thanks for trying to eplain. I think I need to explain why I used
module_alloc() originally.
This depends on how kprobe features are implemented on the architecture, and
how much features are supported on kprobes.
Because kprobe jump optimization and kprobe jump-back optimization need to
use a jump instruction to jump into the trampoline and jump back from the
trampoline directly, if the architecuture jmp instruction supports +-2GB range
like x86, it needs to allocate the trampoline buffer inside such address space.
This requirement is similar to the modules (because module function needs to
call other functions in the kernel etc.), at least kprobes on x86 used
module_alloc().
However, if an architecture only supports breakpoint/trap based kprobe,
it does not need to consider whether the execmem is allocated.
>
> We can always share large ROX pages as long as they are within the correct
> address space. The permissions for them are ROX and the alignment
> differences are due to KASAN and this is handled during allocation of the
> large page to refill the cache. __execmem_cache_alloc() only needs to limit
> the search for the address space of the range.
So I don't think EXECMEM_KPROBE always same as EXECMEM_MODULE_TEXT, it
should be configured for each arch. Especially, if it is only used for
searching parameter, it looks OK to me.
Thank you,
>
> And regardless, they way we deal with sharing of the cache can be sorted
> out later.
>
> > Thanks,
> > Song
>
> --
> Sincerely yours,
> Mike.
>
--
Masami Hiramatsu (Google)
Re: [PATCH v4 14/15] kprobes: remove dependency on CONFIG_MODULES
On Fri, Apr 19, 2024 at 03:59:40PM +, Christophe Leroy wrote:
>
>
> Le 19/04/2024 à 17:49, Mike Rapoport a écrit :
> > Hi Masami,
> >
> > On Thu, Apr 18, 2024 at 06:16:15AM +0900, Masami Hiramatsu wrote:
> >> Hi Mike,
> >>
> >> On Thu, 11 Apr 2024 19:00:50 +0300
> >> Mike Rapoport wrote:
> >>
> >>> From: "Mike Rapoport (IBM)"
> >>>
> >>> kprobes depended on CONFIG_MODULES because it has to allocate memory for
> >>> code.
> >>>
> >>> Since code allocations are now implemented with execmem, kprobes can be
> >>> enabled in non-modular kernels.
> >>>
> >>> Add #ifdef CONFIG_MODULE guards for the code dealing with kprobes inside
> >>> modules, make CONFIG_KPROBES select CONFIG_EXECMEM and drop the
> >>> dependency of CONFIG_KPROBES on CONFIG_MODULES.
> >>
> >> Thanks for this work, but this conflicts with the latest fix in v6.9-rc4.
> >> Also, can you use IS_ENABLED(CONFIG_MODULES) instead of #ifdefs in
> >> function body? We have enough dummy functions for that, so it should
> >> not make a problem.
> >
> > The code in check_kprobe_address_safe() that gets the module and checks for
> > __init functions does not compile with IS_ENABLED(CONFIG_MODULES).
> > I can pull it out to a helper or leave #ifdef in the function body,
> > whichever you prefer.
>
> As far as I can see, the only problem is MODULE_STATE_COMING.
> Can we move 'enum module_state' out of #ifdef CONFIG_MODULES in module.h ?
There's dereference of 'struct module' there:
(*probed_mod)->state != MODULE_STATE_COMING) {
...
}
so moving out 'enum module_state' won't be enough.
> >
> >> --
> >> Masami Hiramatsu
> >
--
Sincerely yours,
Mike.
Re: [PATCH v4 14/15] kprobes: remove dependency on CONFIG_MODULES
On Sat, 20 Apr 2024 10:33:38 +0300
Mike Rapoport wrote:
> On Fri, Apr 19, 2024 at 03:59:40PM +, Christophe Leroy wrote:
> >
> >
> > Le 19/04/2024 à 17:49, Mike Rapoport a écrit :
> > > Hi Masami,
> > >
> > > On Thu, Apr 18, 2024 at 06:16:15AM +0900, Masami Hiramatsu wrote:
> > >> Hi Mike,
> > >>
> > >> On Thu, 11 Apr 2024 19:00:50 +0300
> > >> Mike Rapoport wrote:
> > >>
> > >>> From: "Mike Rapoport (IBM)"
> > >>>
> > >>> kprobes depended on CONFIG_MODULES because it has to allocate memory for
> > >>> code.
> > >>>
> > >>> Since code allocations are now implemented with execmem, kprobes can be
> > >>> enabled in non-modular kernels.
> > >>>
> > >>> Add #ifdef CONFIG_MODULE guards for the code dealing with kprobes inside
> > >>> modules, make CONFIG_KPROBES select CONFIG_EXECMEM and drop the
> > >>> dependency of CONFIG_KPROBES on CONFIG_MODULES.
> > >>
> > >> Thanks for this work, but this conflicts with the latest fix in v6.9-rc4.
> > >> Also, can you use IS_ENABLED(CONFIG_MODULES) instead of #ifdefs in
> > >> function body? We have enough dummy functions for that, so it should
> > >> not make a problem.
> > >
> > > The code in check_kprobe_address_safe() that gets the module and checks
> > > for
> > > __init functions does not compile with IS_ENABLED(CONFIG_MODULES).
> > > I can pull it out to a helper or leave #ifdef in the function body,
> > > whichever you prefer.
> >
> > As far as I can see, the only problem is MODULE_STATE_COMING.
> > Can we move 'enum module_state' out of #ifdef CONFIG_MODULES in module.h ?
>
> There's dereference of 'struct module' there:
>
> (*probed_mod)->state != MODULE_STATE_COMING) {
> ...
> }
>
> so moving out 'enum module_state' won't be enough.
Hmm, this part should be inline functions like;
#ifdef CONFIG_MODULES
static inline bool module_is_coming(struct module *mod)
{
return mod->state == MODULE_STATE_COMING;
}
#else
#define module_is_coming(mod) (false)
#endif
Then we don't need the enum.
Thank you,
>
> > >
> > >> --
> > >> Masami Hiramatsu
> > >
>
> --
> Sincerely yours,
> Mike.
>
--
Masami Hiramatsu (Google)
Re: [PATCH v4 14/15] kprobes: remove dependency on CONFIG_MODULES
On Sat, Apr 20, 2024 at 06:15:00PM +0900, Masami Hiramatsu wrote:
> On Sat, 20 Apr 2024 10:33:38 +0300
> Mike Rapoport wrote:
>
> > On Fri, Apr 19, 2024 at 03:59:40PM +, Christophe Leroy wrote:
> > >
> > >
> > > Le 19/04/2024 à 17:49, Mike Rapoport a écrit :
> > > > Hi Masami,
> > > >
> > > > On Thu, Apr 18, 2024 at 06:16:15AM +0900, Masami Hiramatsu wrote:
> > > >> Hi Mike,
> > > >>
> > > >> On Thu, 11 Apr 2024 19:00:50 +0300
> > > >> Mike Rapoport wrote:
> > > >>
> > > >>> From: "Mike Rapoport (IBM)"
> > > >>>
> > > >>> kprobes depended on CONFIG_MODULES because it has to allocate memory
> > > >>> for
> > > >>> code.
> > > >>>
> > > >>> Since code allocations are now implemented with execmem, kprobes can
> > > >>> be
> > > >>> enabled in non-modular kernels.
> > > >>>
> > > >>> Add #ifdef CONFIG_MODULE guards for the code dealing with kprobes
> > > >>> inside
> > > >>> modules, make CONFIG_KPROBES select CONFIG_EXECMEM and drop the
> > > >>> dependency of CONFIG_KPROBES on CONFIG_MODULES.
> > > >>
> > > >> Thanks for this work, but this conflicts with the latest fix in
> > > >> v6.9-rc4.
> > > >> Also, can you use IS_ENABLED(CONFIG_MODULES) instead of #ifdefs in
> > > >> function body? We have enough dummy functions for that, so it should
> > > >> not make a problem.
> > > >
> > > > The code in check_kprobe_address_safe() that gets the module and checks
> > > > for
> > > > __init functions does not compile with IS_ENABLED(CONFIG_MODULES).
> > > > I can pull it out to a helper or leave #ifdef in the function body,
> > > > whichever you prefer.
> > >
> > > As far as I can see, the only problem is MODULE_STATE_COMING.
> > > Can we move 'enum module_state' out of #ifdef CONFIG_MODULES in module.h
> > > ?
> >
> > There's dereference of 'struct module' there:
> >
> > (*probed_mod)->state != MODULE_STATE_COMING) {
> > ...
> > }
> >
> > so moving out 'enum module_state' won't be enough.
>
> Hmm, this part should be inline functions like;
>
> #ifdef CONFIG_MODULES
> static inline bool module_is_coming(struct module *mod)
> {
> return mod->state == MODULE_STATE_COMING;
> }
> #else
> #define module_is_coming(mod) (false)
I'd prefer
static inline module_is_coming(struct module *mod)
{
return false;
}
> #endif
>
> Then we don't need the enum.
> Thank you,
>
> --
> Masami Hiramatsu (Google)
--
Sincerely yours,
Mike.
[PATCH 0/6] Deduplicate string exposure in sysfs
Introduce a generic ->show() callback to expose a string as a device attribute in sysfs. Deduplicate various identical callbacks across the tree. Result: Minus 216 LoC, minus 1576 bytes vmlinux size (x86_64 allyesconfig). This is a byproduct of my upcoming PCI device authentication v2 patches. Lukas Wunner (6): driver core: Add device_show_string() helper for sysfs attributes hwmon: Use device_show_string() helper for sysfs attributes IB/qib: Use device_show_string() helper for sysfs attributes perf: Use device_show_string() helper for sysfs attributes platform/x86: Use device_show_string() helper for sysfs attributes scsi: Use device_show_string() helper for sysfs attributes arch/powerpc/perf/hv-24x7.c | 10 arch/x86/events/intel/core.c | 13 ++--- drivers/base/core.c | 9 drivers/hwmon/i5k_amb.c | 15 ++ drivers/hwmon/ibmpex.c | 14 ++ drivers/infiniband/hw/qib/qib.h | 1 - drivers/infiniband/hw/qib/qib_driver.c | 6 --- drivers/infiniband/hw/qib/qib_sysfs.c| 10 +--- drivers/perf/alibaba_uncore_drw_pmu.c| 12 + drivers/perf/arm-cci.c | 12 + drivers/perf/arm-ccn.c | 11 + drivers/perf/arm_cspmu/arm_cspmu.c | 10 drivers/perf/arm_cspmu/arm_cspmu.h | 7 +-- drivers/perf/arm_dsu_pmu.c | 11 + drivers/perf/cxl_pmu.c | 13 + drivers/perf/hisilicon/hisi_pcie_pmu.c | 13 + drivers/perf/hisilicon/hisi_uncore_pmu.c | 14 -- drivers/perf/hisilicon/hisi_uncore_pmu.h | 4 +- drivers/perf/hisilicon/hns3_pmu.c| 12 + drivers/perf/qcom_l3_pmu.c | 11 + drivers/perf/xgene_pmu.c | 11 + drivers/platform/x86/asus-wmi.c | 62 ++-- drivers/platform/x86/thinkpad_acpi.c | 10 +--- drivers/platform/x86/toshiba_acpi.c | 9 +--- drivers/scsi/bfa/bfad_attr.c | 28 +++ drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 11 + drivers/scsi/mvsas/mv_init.c | 10 +--- drivers/scsi/qla2xxx/qla_attr.c | 11 + drivers/scsi/smartpqi/smartpqi_init.c| 11 ++--- include/linux/device.h | 15 ++ 30 files changed, 85 insertions(+), 301 deletions(-) -- 2.43.0
[PATCH 1/6] driver core: Add device_show_string() helper for sysfs attributes
For drivers wishing to expose an unsigned long, int or bool at a static
memory location in sysfs, the driver core provides ready-made helpers
such as device_show_ulong() to be used as ->show() callback.
Some drivers need to expose a string and so far they all provide their
own ->show() implementation. arch/powerpc/perf/hv-24x7.c went so far
as to create a device_show_string() helper but kept it private.
Make it public for reuse by other drivers. The pattern seems to be
sufficiently frequent to merit a public helper.
Add a DEVICE_STRING_ATTR_RO() macro in line with the existing
DEVICE_ULONG_ATTR() and similar macros to ease declaration of string
attributes.
Signed-off-by: Lukas Wunner
---
arch/powerpc/perf/hv-24x7.c | 10 --
drivers/base/core.c | 9 +
include/linux/device.h | 15 +++
3 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/perf/hv-24x7.c b/arch/powerpc/perf/hv-24x7.c
index 057ec2e3451d..d400fa391c27 100644
--- a/arch/powerpc/perf/hv-24x7.c
+++ b/arch/powerpc/perf/hv-24x7.c
@@ -425,16 +425,6 @@ static char *memdup_to_str(char *maybe_str, int max_len,
gfp_t gfp)
return kasprintf(gfp, "%.*s", max_len, maybe_str);
}
-static ssize_t device_show_string(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct dev_ext_attribute *d;
-
- d = container_of(attr, struct dev_ext_attribute, attr);
-
- return sprintf(buf, "%s\n", (char *)d->var);
-}
-
static ssize_t cpumask_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 78dfa74ee18b..190d4a39c6a8 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2523,6 +2523,15 @@ ssize_t device_show_bool(struct device *dev, struct
device_attribute *attr,
}
EXPORT_SYMBOL_GPL(device_show_bool);
+ssize_t device_show_string(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct dev_ext_attribute *ea = to_ext_attr(attr);
+
+ return sysfs_emit(buf, "%s\n", (char *)ea->var);
+}
+EXPORT_SYMBOL_GPL(device_show_string);
+
/**
* device_release - free device structure.
* @kobj: device's kobject.
diff --git a/include/linux/device.h b/include/linux/device.h
index c515ba5756e4..63ac65db3ecb 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -132,6 +132,8 @@ ssize_t device_show_bool(struct device *dev, struct
device_attribute *attr,
char *buf);
ssize_t device_store_bool(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count);
+ssize_t device_show_string(struct device *dev, struct device_attribute *attr,
+ char *buf);
/**
* DEVICE_ATTR - Define a device attribute.
@@ -251,6 +253,19 @@ ssize_t device_store_bool(struct device *dev, struct
device_attribute *attr,
struct dev_ext_attribute dev_attr_##_name = \
{ __ATTR(_name, _mode, device_show_bool, device_store_bool),
&(_var) }
+/**
+ * DEVICE_STRING_ATTR_RO - Define a device attribute backed by a r/o string.
+ * @_name: Attribute name.
+ * @_mode: File mode.
+ * @_var: Identifier of string.
+ *
+ * Like DEVICE_ULONG_ATTR(), but @_var is a string. Because the length of the
+ * string allocation is unknown, the attribute must be read-only.
+ */
+#define DEVICE_STRING_ATTR_RO(_name, _mode, _var) \
+ struct dev_ext_attribute dev_attr_##_name = \
+ { __ATTR(_name, (_mode) & ~0222, device_show_string, NULL),
(_var) }
+
#define DEVICE_ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store) \
struct device_attribute dev_attr_##_name = \
__ATTR_IGNORE_LOCKDEP(_name, _mode, _show, _store)
--
2.43.0
Re: [GIT PULL] Please pull powerpc/linux.git powerpc-6.9-3 tag
The pull request you sent on Sat, 20 Apr 2024 13:24:34 +1000: > https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git > tags/powerpc-6.9-3 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/e43afae4a335ac0bf54c7a8f23ed65dd55449649 Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html
