| Message ID | 20210728081320.20394-6-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | sections: Unify kernel sections range check and use | expand |
On Wed, 28 Jul 2021 16:13:18 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > The is_kernel[_text]() function check the address whether or not > in kernel[_text] ranges, also they will check the address whether > or not in gate area, so use better name. Do you know what a gate area is? Because I believe gate area is kernel text, so the rename just makes it redundant and more confusing. -- Steve
On 2021/7/28 23:28, Steven Rostedt wrote: > On Wed, 28 Jul 2021 16:13:18 +0800 > Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >> The is_kernel[_text]() function check the address whether or not >> in kernel[_text] ranges, also they will check the address whether >> or not in gate area, so use better name. > Do you know what a gate area is? > > Because I believe gate area is kernel text, so the rename just makes it > redundant and more confusing. Yes, the gate area(eg, vectors part on ARM32, similar on x86/ia64) is kernel text. I want to keep the 'basic' section boundaries check, which only check the start/end of sections, all in section.h, could we use 'generic' or 'basic' or 'core' in the naming? * is_kernel_generic_data() --- come from core_kernel_data() in kernel.h * is_kernel_generic_text() The old helper could remain unchanged, any suggestion, thanks. > > -- Steve > . >
On Thu, 29 Jul 2021 10:00:51 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > On 2021/7/28 23:28, Steven Rostedt wrote: > > On Wed, 28 Jul 2021 16:13:18 +0800 > > Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > > >> The is_kernel[_text]() function check the address whether or not > >> in kernel[_text] ranges, also they will check the address whether > >> or not in gate area, so use better name. > > Do you know what a gate area is? > > > > Because I believe gate area is kernel text, so the rename just makes it > > redundant and more confusing. > > Yes, the gate area(eg, vectors part on ARM32, similar on x86/ia64) is > kernel text. > > I want to keep the 'basic' section boundaries check, which only check > the start/end > > of sections, all in section.h, could we use 'generic' or 'basic' or > 'core' in the naming? > > * is_kernel_generic_data() --- come from core_kernel_data() in kernel.h > * is_kernel_generic_text() > > The old helper could remain unchanged, any suggestion, thanks. Because it looks like the check of just being in the range of "_stext" to "_end" is just an internal helper, why not do what we do all over the kernel, and just prefix the function with a couple of underscores, that denote that it's internal? __is_kernel_text() Then you have: static inline int is_kernel_text(unsigned long addr) { if (__is_kernel_text(addr)) return 1; return in_gate_area_no_mm(addr); } -- Steve
On 2021/7/29 12:05, Steven Rostedt wrote: > On Thu, 29 Jul 2021 10:00:51 +0800 > Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >> On 2021/7/28 23:28, Steven Rostedt wrote: >>> On Wed, 28 Jul 2021 16:13:18 +0800 >>> Kefeng Wang <wangkefeng.wang@huawei.com> wrote: >>> >>>> The is_kernel[_text]() function check the address whether or not >>>> in kernel[_text] ranges, also they will check the address whether >>>> or not in gate area, so use better name. >>> Do you know what a gate area is? >>> >>> Because I believe gate area is kernel text, so the rename just makes it >>> redundant and more confusing. >> Yes, the gate area(eg, vectors part on ARM32, similar on x86/ia64) is >> kernel text. >> >> I want to keep the 'basic' section boundaries check, which only check >> the start/end >> >> of sections, all in section.h, could we use 'generic' or 'basic' or >> 'core' in the naming? >> >> * is_kernel_generic_data() --- come from core_kernel_data() in kernel.h >> * is_kernel_generic_text() >> >> The old helper could remain unchanged, any suggestion, thanks. > Because it looks like the check of just being in the range of "_stext" > to "_end" is just an internal helper, why not do what we do all over > the kernel, and just prefix the function with a couple of underscores, > that denote that it's internal? > > __is_kernel_text() OK， thanks for your advise, there's already a __is_kernel_text() in arch/x86/mm/init_32.c, I will change it to is_x32_kernel_text() to avoid conflict on x86_32. > > Then you have: > > static inline int is_kernel_text(unsigned long addr) > { > if (__is_kernel_text(addr)) > return 1; > return in_gate_area_no_mm(addr); > } > > -- Steve > . >
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c index 333650b9372a..c87d0dd4370d 100644 --- a/arch/x86/net/bpf_jit_comp.c +++ b/arch/x86/net/bpf_jit_comp.c @@ -372,7 +372,7 @@ static int __bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t, int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t, void *old_addr, void *new_addr) { - if (!is_kernel_text((long)ip) && + if (!is_kernel_text_or_gate_area((long)ip) && !is_bpf_text_address((long)ip)) /* BPF poking in modules is not supported */ return -EINVAL; diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h index 8a9d329c927c..4f501ac9c2c2 100644 --- a/include/linux/kallsyms.h +++ b/include/linux/kallsyms.h @@ -24,14 +24,14 @@ struct cred; struct module; -static inline int is_kernel_text(unsigned long addr) +static inline int is_kernel_text_or_gate_area(unsigned long addr) { if ((addr >= (unsigned long)_stext && addr < (unsigned long)_etext)) return 1; return in_gate_area_no_mm(addr); } -static inline int is_kernel(unsigned long addr) +static inline int is_kernel_or_gate_area(unsigned long addr) { if (addr >= (unsigned long)_stext && addr < (unsigned long)_end) return 1; @@ -41,9 +41,9 @@ static inline int is_kernel(unsigned long addr) static inline int is_ksym_addr(unsigned long addr) { if (IS_ENABLED(CONFIG_KALLSYMS_ALL)) - return is_kernel(addr); + return is_kernel_or_gate_area(addr); - return is_kernel_text(addr) || is_kernel_inittext(addr); + return is_kernel_text_or_gate_area(addr) || is_kernel_inittext(addr); } static inline void *dereference_symbol_descriptor(void *ptr) diff --git a/kernel/cfi.c b/kernel/cfi.c index e17a56639766..e7d90eff4382 100644 --- a/kernel/cfi.c +++ b/kernel/cfi.c @@ -282,7 +282,7 @@ static inline cfi_check_fn find_check_fn(unsigned long ptr) { cfi_check_fn fn = NULL; - if (is_kernel_text(ptr)) + if (is_kernel_text_or_gate_area(ptr)) return __cfi_check; /*
The is_kernel[_text]() function check the address whether or not in kernel[_text] ranges, also they will check the address whether or not in gate area, so use better name. Cc: Alexei Starovoitov <ast@kernel.org> Cc: Daniel Borkmann <daniel@iogearbox.net> Cc: Sami Tolvanen <samitolvanen@google.com> Cc: Nathan Chancellor <nathan@kernel.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: bpf@vger.kernel.org Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- arch/x86/net/bpf_jit_comp.c | 2 +- include/linux/kallsyms.h | 8 ++++---- kernel/cfi.c | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-)