[PATCH v4 1/3] kallsyms: move kallsyms_show_value() out of kallsyms.c

[PATCH v4 1/3] kallsyms: move kallsyms_show_value() out of kallsyms.c

[Maninder Singh]

function kallsyms_show_value() is used by other parts
like modules_open(), kprobes_read() etc. which can work in case of
!KALLSYMS also.

e.g. as of now lsmod do not show module address if KALLSYMS is disabled.
since kallsyms_show_value() defination is not present, it returns false
in !KALLSYMS.

/ # lsmod
test 12288 0 - Live 0x0000000000000000 (O)

So kallsyms_show_value() can be made generic
without dependency on KALLSYMS.

Thus moving out function to a new file ksyms_common.c.

With this patch code is just moved to new file
and no functional change.

Co-developed-by: Onkarnath <onkarnath.1@samsung.com>
Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
earlier conversations:(then it has dependancy on other change, but that
was stashed from linux-next, now it can be pushed)
https://lore.kernel.org/lkml/202205111525.92B1C597@keescook/T/
https://lkml.org/lkml/2022/4/13/47
v1 -> v2: separate out bpf and kallsyms change
v2 -> v3: make kallsym changes in2 patches, non functional and
functional change
v3 -> v4: patch order changed, file name changed form knosyms -> ksyms_common
and copyright header modified.

 kernel/Makefile       |  2 +-
 kernel/kallsyms.c     | 35 ---------------------------------
 kernel/ksyms_common.c | 45 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 36 deletions(-)
 create mode 100644 kernel/ksyms_common.c

diff --git a/kernel/Makefile b/kernel/Makefile
index f9e3fd9195d9..3947122d618b 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -10,7 +10,7 @@ obj-y     = fork.o exec_domain.o panic.o \
 	    extable.o params.o \
 	    kthread.o sys_ni.o nsproxy.o \
 	    notifier.o ksysfs.o cred.o reboot.o \
-	    async.o range.o smpboot.o ucount.o regset.o
+	    async.o range.o smpboot.o ucount.o regset.o ksyms_common.o
 
 obj-$(CONFIG_USERMODE_DRIVER) += usermode_driver.o
 obj-$(CONFIG_MULTIUSER) += groups.o
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 8193e947aa10..0f82c3d5a57d 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -907,41 +907,6 @@ late_initcall(bpf_ksym_iter_register);
 
 #endif /* CONFIG_BPF_SYSCALL */
 
-static inline int kallsyms_for_perf(void)
-{
-#ifdef CONFIG_PERF_EVENTS
-	extern int sysctl_perf_event_paranoid;
-	if (sysctl_perf_event_paranoid <= 1)
-		return 1;
-#endif
-	return 0;
-}
-
-/*
- * We show kallsyms information even to normal users if we've enabled
- * kernel profiling and are explicitly not paranoid (so kptr_restrict
- * is clear, and sysctl_perf_event_paranoid isn't set).
- *
- * Otherwise, require CAP_SYSLOG (assuming kptr_restrict isn't set to
- * block even that).
- */
-bool kallsyms_show_value(const struct cred *cred)
-{
-	switch (kptr_restrict) {
-	case 0:
-		if (kallsyms_for_perf())
-			return true;
-		fallthrough;
-	case 1:
-		if (security_capable(cred, &init_user_ns, CAP_SYSLOG,
-				     CAP_OPT_NOAUDIT) == 0)
-			return true;
-		fallthrough;
-	default:
-		return false;
-	}
-}
-
 static int kallsyms_open(struct inode *inode, struct file *file)
 {
 	/*
diff --git a/kernel/ksyms_common.c b/kernel/ksyms_common.c
new file mode 100644
index 000000000000..e776f12f0f5a
--- /dev/null
+++ b/kernel/ksyms_common.c
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ksyms_common.c: A split of kernel/kallsyms.c
+ * Contains a few generic function definations independent of config KALLSYMS.
+ */
+#include <linux/kallsyms.h>
+#include <linux/security.h>
+
+#ifdef CONFIG_KALLSYMS
+static inline int kallsyms_for_perf(void)
+{
+#ifdef CONFIG_PERF_EVENTS
+	extern int sysctl_perf_event_paranoid;
+
+	if (sysctl_perf_event_paranoid <= 1)
+		return 1;
+#endif
+	return 0;
+}
+
+/*
+ * We show kallsyms information even to normal users if we've enabled
+ * kernel profiling and are explicitly not paranoid (so kptr_restrict
+ * is clear, and sysctl_perf_event_paranoid isn't set).
+ *
+ * Otherwise, require CAP_SYSLOG (assuming kptr_restrict isn't set to
+ * block even that).
+ */
+bool kallsyms_show_value(const struct cred *cred)
+{
+	switch (kptr_restrict) {
+	case 0:
+		if (kallsyms_for_perf())
+			return true;
+		fallthrough;
+	case 1:
+		if (security_capable(cred, &init_user_ns, CAP_SYSLOG,
+				     CAP_OPT_NOAUDIT) == 0)
+			return true;
+		fallthrough;
+	default:
+		return false;
+	}
+}
+#endif
-- 
2.17.1

[PATCH v4 2/3] bpf: make bpf_dump_raw_ok() based on CONFIG_KALLSYMS

[Maninder Singh]

bpf_dump_raw_ok() depends on kallsyms_show_value() and we already
have a false definition for the !CONFIG_KALLSYMS case. But we'll
soon expand on kallsyms_show_value() and so to make the code
easier to follow just provide a direct !CONFIG_KALLSYMS definition
for bpf_dump_raw_ok() as well.

Co-developed-by: Onkarnath <onkarnath.1@samsung.com>
Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>
---
 include/linux/filter.h | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index bbce89937fde..1f237a3bb11a 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -923,13 +923,21 @@ bool bpf_jit_supports_kfunc_call(void);
 bool bpf_jit_supports_far_kfunc_call(void);
 bool bpf_helper_changes_pkt_data(void *func);
 
+/*
+ * Reconstruction of call-sites is dependent on kallsyms,
+ * thus make dump the same restriction.
+ */
+#ifdef CONFIG_KALLSYMS
 static inline bool bpf_dump_raw_ok(const struct cred *cred)
 {
-	/* Reconstruction of call-sites is dependent on kallsyms,
-	 * thus make dump the same restriction.
-	 */
 	return kallsyms_show_value(cred);
 }
+#else
+static inline bool bpf_dump_raw_ok(const struct cred *cred)
+{
+	return false;
+}
+#endif
 
 struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
 				       const struct bpf_insn *patch, u32 len);
-- 
2.17.1

[PATCH v4 3/3] kallsyms: make kallsyms_show_value() as generic function

[Maninder Singh]

This change makes function kallsyms_show_value() as
generic function without dependency on CONFIG_KALLSYMS.

Now module address will be displayed with lsmod and /proc/modules.

Earlier:
=======
/ # insmod  test.ko
/ # lsmod
test 12288 0 - Live 0x0000000000000000 (O)  // No Module Load address
/ #

With change:
==========
/ # insmod test.ko
/ # lsmod
test 12288 0 - Live 0xffff800000fc0000 (O)  // Module address
/ # cat /proc/modules
test 12288 0 - Live 0xffff800000fc0000 (O)

Co-developed-by: Onkarnath <onkarnath.1@samsung.com>
Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 include/linux/kallsyms.h | 11 +++--------
 kernel/ksyms_common.c    |  2 --
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index 1037f4957caa..c3f075e8f60c 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -65,6 +65,9 @@ static inline void *dereference_symbol_descriptor(void *ptr)
 	return ptr;
 }
 
+/* How and when do we show kallsyms values? */
+extern bool kallsyms_show_value(const struct cred *cred);
+
 #ifdef CONFIG_KALLSYMS
 unsigned long kallsyms_sym_address(int idx);
 int kallsyms_on_each_symbol(int (*fn)(void *, const char *, unsigned long),
@@ -94,9 +97,6 @@ extern int sprint_backtrace_build_id(char *buffer, unsigned long address);
 
 int lookup_symbol_name(unsigned long addr, char *symname);
 
-/* How and when do we show kallsyms values? */
-extern bool kallsyms_show_value(const struct cred *cred);
-
 #else /* !CONFIG_KALLSYMS */
 
 static inline unsigned long kallsyms_lookup_name(const char *name)
@@ -154,11 +154,6 @@ static inline int lookup_symbol_name(unsigned long addr, char *symname)
 	return -ERANGE;
 }
 
-static inline bool kallsyms_show_value(const struct cred *cred)
-{
-	return false;
-}
-
 static inline int kallsyms_on_each_symbol(int (*fn)(void *, const char *, unsigned long),
 					  void *data)
 {
diff --git a/kernel/ksyms_common.c b/kernel/ksyms_common.c
index e776f12f0f5a..9603bbef095c 100644
--- a/kernel/ksyms_common.c
+++ b/kernel/ksyms_common.c
@@ -6,7 +6,6 @@
 #include <linux/kallsyms.h>
 #include <linux/security.h>
 
-#ifdef CONFIG_KALLSYMS
 static inline int kallsyms_for_perf(void)
 {
 #ifdef CONFIG_PERF_EVENTS
@@ -42,4 +41,3 @@ bool kallsyms_show_value(const struct cred *cred)
 		return false;
 	}
 }
-#endif
-- 
2.17.1

Re: [PATCH v4 1/3] kallsyms: move kallsyms_show_value() out of kallsyms.c

[Leizhen (ThunderTown)]

On 2023/6/6 12:28, Maninder Singh wrote:
> function kallsyms_show_value() is used by other parts
> like modules_open(), kprobes_read() etc. which can work in case of
> !KALLSYMS also.
> 
> e.g. as of now lsmod do not show module address if KALLSYMS is disabled.
> since kallsyms_show_value() defination is not present, it returns false
> in !KALLSYMS.
> 
> / # lsmod
> test 12288 0 - Live 0x0000000000000000 (O)
> 
> So kallsyms_show_value() can be made generic
> without dependency on KALLSYMS.
> 
> Thus moving out function to a new file ksyms_common.c.
> 
> With this patch code is just moved to new file
> and no functional change.
> 
> Co-developed-by: Onkarnath <onkarnath.1@samsung.com>
> Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> ---
> earlier conversations:(then it has dependancy on other change, but that
> was stashed from linux-next, now it can be pushed)
> https://lore.kernel.org/lkml/202205111525.92B1C597@keescook/T/
> https://lkml.org/lkml/2022/4/13/47
> v1 -> v2: separate out bpf and kallsyms change
> v2 -> v3: make kallsym changes in2 patches, non functional and
> functional change
> v3 -> v4: patch order changed, file name changed form knosyms -> ksyms_common
> and copyright header modified.
> 
>  kernel/Makefile       |  2 +-
>  kernel/kallsyms.c     | 35 ---------------------------------
>  kernel/ksyms_common.c | 45 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+), 36 deletions(-)
>  create mode 100644 kernel/ksyms_common.c
> 
> diff --git a/kernel/Makefile b/kernel/Makefile
> index f9e3fd9195d9..3947122d618b 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -10,7 +10,7 @@ obj-y     = fork.o exec_domain.o panic.o \
>  	    extable.o params.o \
>  	    kthread.o sys_ni.o nsproxy.o \
>  	    notifier.o ksysfs.o cred.o reboot.o \
> -	    async.o range.o smpboot.o ucount.o regset.o
> +	    async.o range.o smpboot.o ucount.o regset.o ksyms_common.o
>  
>  obj-$(CONFIG_USERMODE_DRIVER) += usermode_driver.o
>  obj-$(CONFIG_MULTIUSER) += groups.o
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 8193e947aa10..0f82c3d5a57d 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -907,41 +907,6 @@ late_initcall(bpf_ksym_iter_register);
>  
>  #endif /* CONFIG_BPF_SYSCALL */
>  
> -static inline int kallsyms_for_perf(void)
> -{
> -#ifdef CONFIG_PERF_EVENTS
> -	extern int sysctl_perf_event_paranoid;
> -	if (sysctl_perf_event_paranoid <= 1)
> -		return 1;
> -#endif
> -	return 0;
> -}
> -
> -/*
> - * We show kallsyms information even to normal users if we've enabled
> - * kernel profiling and are explicitly not paranoid (so kptr_restrict
> - * is clear, and sysctl_perf_event_paranoid isn't set).
> - *
> - * Otherwise, require CAP_SYSLOG (assuming kptr_restrict isn't set to
> - * block even that).
> - */
> -bool kallsyms_show_value(const struct cred *cred)
> -{
> -	switch (kptr_restrict) {
> -	case 0:
> -		if (kallsyms_for_perf())
> -			return true;
> -		fallthrough;
> -	case 1:
> -		if (security_capable(cred, &init_user_ns, CAP_SYSLOG,
> -				     CAP_OPT_NOAUDIT) == 0)
> -			return true;
> -		fallthrough;
> -	default:
> -		return false;
> -	}
> -}
> -
>  static int kallsyms_open(struct inode *inode, struct file *file)
>  {
>  	/*
> diff --git a/kernel/ksyms_common.c b/kernel/ksyms_common.c
> new file mode 100644
> index 000000000000..e776f12f0f5a
> --- /dev/null
> +++ b/kernel/ksyms_common.c
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0

Keep it the same as kernel/kallsyms.c. GPL-2.0-only

Sorry, I didn't think of that last time.
Otherwise,

Reviewed-by: Zhen Lei <thunder.leizhen@huawei.com>

> +/*
> + * ksyms_common.c: A split of kernel/kallsyms.c
> + * Contains a few generic function definations independent of config KALLSYMS.
> + */
> +#include <linux/kallsyms.h>
> +#include <linux/security.h>
> +
> +#ifdef CONFIG_KALLSYMS
> +static inline int kallsyms_for_perf(void)
> +{
> +#ifdef CONFIG_PERF_EVENTS
> +	extern int sysctl_perf_event_paranoid;
> +
> +	if (sysctl_perf_event_paranoid <= 1)
> +		return 1;
> +#endif
> +	return 0;
> +}
> +
> +/*
> + * We show kallsyms information even to normal users if we've enabled
> + * kernel profiling and are explicitly not paranoid (so kptr_restrict
> + * is clear, and sysctl_perf_event_paranoid isn't set).
> + *
> + * Otherwise, require CAP_SYSLOG (assuming kptr_restrict isn't set to
> + * block even that).
> + */
> +bool kallsyms_show_value(const struct cred *cred)
> +{
> +	switch (kptr_restrict) {
> +	case 0:
> +		if (kallsyms_for_perf())
> +			return true;
> +		fallthrough;
> +	case 1:
> +		if (security_capable(cred, &init_user_ns, CAP_SYSLOG,
> +				     CAP_OPT_NOAUDIT) == 0)
> +			return true;
> +		fallthrough;
> +	default:
> +		return false;
> +	}
> +}
> +#endif
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v4 2/3] bpf: make bpf_dump_raw_ok() based on CONFIG_KALLSYMS

[Leizhen (ThunderTown)]

On 2023/6/6 12:28, Maninder Singh wrote:
> bpf_dump_raw_ok() depends on kallsyms_show_value() and we already
> have a false definition for the !CONFIG_KALLSYMS case. But we'll
> soon expand on kallsyms_show_value() and so to make the code
> easier to follow just provide a direct !CONFIG_KALLSYMS definition
> for bpf_dump_raw_ok() as well.

Reviewed-by: Zhen Lei <thunder.leizhen@huawei.com>

> 
> Co-developed-by: Onkarnath <onkarnath.1@samsung.com>
> Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  include/linux/filter.h | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index bbce89937fde..1f237a3bb11a 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -923,13 +923,21 @@ bool bpf_jit_supports_kfunc_call(void);
>  bool bpf_jit_supports_far_kfunc_call(void);
>  bool bpf_helper_changes_pkt_data(void *func);
>  
> +/*
> + * Reconstruction of call-sites is dependent on kallsyms,
> + * thus make dump the same restriction.
> + */
> +#ifdef CONFIG_KALLSYMS
>  static inline bool bpf_dump_raw_ok(const struct cred *cred)
>  {
> -	/* Reconstruction of call-sites is dependent on kallsyms,
> -	 * thus make dump the same restriction.
> -	 */
>  	return kallsyms_show_value(cred);
>  }
> +#else
> +static inline bool bpf_dump_raw_ok(const struct cred *cred)
> +{
> +	return false;
> +}
> +#endif
>  
>  struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
>  				       const struct bpf_insn *patch, u32 len);
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v4 3/3] kallsyms: make kallsyms_show_value() as generic function

[Leizhen (ThunderTown)]

On 2023/6/6 12:28, Maninder Singh wrote:
> This change makes function kallsyms_show_value() as
> generic function without dependency on CONFIG_KALLSYMS.
> 
> Now module address will be displayed with lsmod and /proc/modules.
> 
> Earlier:
> =======
> / # insmod  test.ko
> / # lsmod
> test 12288 0 - Live 0x0000000000000000 (O)  // No Module Load address
> / #
> 
> With change:
> ==========
> / # insmod test.ko
> / # lsmod
> test 12288 0 - Live 0xffff800000fc0000 (O)  // Module address
> / # cat /proc/modules
> test 12288 0 - Live 0xffff800000fc0000 (O)

Reviewed-by: Zhen Lei <thunder.leizhen@huawei.com>

> 
> Co-developed-by: Onkarnath <onkarnath.1@samsung.com>
> Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> ---
>  include/linux/kallsyms.h | 11 +++--------
>  kernel/ksyms_common.c    |  2 --
>  2 files changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
> index 1037f4957caa..c3f075e8f60c 100644
> --- a/include/linux/kallsyms.h
> +++ b/include/linux/kallsyms.h
> @@ -65,6 +65,9 @@ static inline void *dereference_symbol_descriptor(void *ptr)
>  	return ptr;
>  }
>  
> +/* How and when do we show kallsyms values? */
> +extern bool kallsyms_show_value(const struct cred *cred);
> +
>  #ifdef CONFIG_KALLSYMS
>  unsigned long kallsyms_sym_address(int idx);
>  int kallsyms_on_each_symbol(int (*fn)(void *, const char *, unsigned long),
> @@ -94,9 +97,6 @@ extern int sprint_backtrace_build_id(char *buffer, unsigned long address);
>  
>  int lookup_symbol_name(unsigned long addr, char *symname);
>  
> -/* How and when do we show kallsyms values? */
> -extern bool kallsyms_show_value(const struct cred *cred);
> -
>  #else /* !CONFIG_KALLSYMS */
>  
>  static inline unsigned long kallsyms_lookup_name(const char *name)
> @@ -154,11 +154,6 @@ static inline int lookup_symbol_name(unsigned long addr, char *symname)
>  	return -ERANGE;
>  }
>  
> -static inline bool kallsyms_show_value(const struct cred *cred)
> -{
> -	return false;
> -}
> -
>  static inline int kallsyms_on_each_symbol(int (*fn)(void *, const char *, unsigned long),
>  					  void *data)
>  {
> diff --git a/kernel/ksyms_common.c b/kernel/ksyms_common.c
> index e776f12f0f5a..9603bbef095c 100644
> --- a/kernel/ksyms_common.c
> +++ b/kernel/ksyms_common.c
> @@ -6,7 +6,6 @@
>  #include <linux/kallsyms.h>
>  #include <linux/security.h>
>  
> -#ifdef CONFIG_KALLSYMS
>  static inline int kallsyms_for_perf(void)
>  {
>  #ifdef CONFIG_PERF_EVENTS
> @@ -42,4 +41,3 @@ bool kallsyms_show_value(const struct cred *cred)
>  		return false;
>  	}
>  }
> -#endif
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v4 2/3] bpf: make bpf_dump_raw_ok() based on CONFIG_KALLSYMS

[Andrii Nakryiko]

On Mon, Jun 5, 2023 at 9:28 PM Maninder Singh <maninder1.s@samsung.com> wrote:
>
> bpf_dump_raw_ok() depends on kallsyms_show_value() and we already
> have a false definition for the !CONFIG_KALLSYMS case. But we'll
> soon expand on kallsyms_show_value() and so to make the code
> easier to follow just provide a direct !CONFIG_KALLSYMS definition
> for bpf_dump_raw_ok() as well.

I'm sorry, I'm failing to follow the exact reasoning about
simplification. It seems simpler to have

static inline bool kallsyms_show_value(const struct cred *cred)
{
    return false;
}

and control it from kallsyms-related internal header, rather than
adding CONFIG_KALLSYMS ifdef-ery to include/linux/filter.h and
redefining that `return false` decision. What if in the future we
decide that if !CONFIG_KALLSYMS it's ok to show raw addresses, now
we'll have to remember to update it in two places.

Unless I'm missing some other complications?

>
> Co-developed-by: Onkarnath <onkarnath.1@samsung.com>
> Signed-off-by: Onkarnath <onkarnath.1@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  include/linux/filter.h | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index bbce89937fde..1f237a3bb11a 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -923,13 +923,21 @@ bool bpf_jit_supports_kfunc_call(void);
>  bool bpf_jit_supports_far_kfunc_call(void);
>  bool bpf_helper_changes_pkt_data(void *func);
>
> +/*
> + * Reconstruction of call-sites is dependent on kallsyms,
> + * thus make dump the same restriction.
> + */
> +#ifdef CONFIG_KALLSYMS
>  static inline bool bpf_dump_raw_ok(const struct cred *cred)
>  {
> -       /* Reconstruction of call-sites is dependent on kallsyms,
> -        * thus make dump the same restriction.
> -        */
>         return kallsyms_show_value(cred);
>  }
> +#else
> +static inline bool bpf_dump_raw_ok(const struct cred *cred)
> +{
> +       return false;
> +}
> +#endif
>
>  struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
>                                        const struct bpf_insn *patch, u32 len);
> --
> 2.17.1
>

RE: [PATCH v4 2/3] bpf: make bpf_dump_raw_ok() based on CONFIG_KALLSYMS

[Maninder Singh]

Hi Andrii Nakryiko,

>>
>> bpf_dump_raw_ok() depends on kallsyms_show_value() and we already
>> have a false definition for the !CONFIG_KALLSYMS case. But we'll
>> soon expand on kallsyms_show_value() and so to make the code
>> easier to follow just provide a direct !CONFIG_KALLSYMS definition
>> for bpf_dump_raw_ok() as well.
>
> I'm sorry, I'm failing to follow the exact reasoning about
> simplification. It seems simpler to have
> 
> static inline bool kallsyms_show_value(const struct cred *cred)
> {
>     return false;
> }
> 
> and control it from kallsyms-related internal header, rather than
> adding CONFIG_KALLSYMS ifdef-ery to include/linux/filter.h and
> redefining that `return false` decision. What if in the future we
> decide that if !CONFIG_KALLSYMS it's ok to show raw addresses, now
> we'll have to remember to update it in two places.
> 
> Unless I'm missing some other complications?
> 

Patch 3/3 does the same, it extends functionality of kallsyms_show_value()
in case of  !CONFIG_KALLSYMS.

All other users likes modules code, kprobe codes are using this API
for sanity/permission, and then prints the address like below:

static int kprobe_blacklist_seq_show(struct seq_file *m, void *v)
{
...
        if (!kallsyms_show_value(m->file->f_cred))
                seq_printf(m, "0x%px-0x%px\t%ps\n", NULL, NULL,
                           (void *)ent->start_addr);
        else
                seq_printf(m, "0x%px-0x%px\t%ps\n", (void *)ent->start_addr,
                           (void *)ent->end_addr, (void *)ent->start_addr);
..
}

so there will be no issues if we move kallsyms_show_value() out of KALLSYMS dependency.
and these codes will work in case of !KALLSYMS also.

but BPF code logic was complex and seems this API was used as checking for whether KALLSYMS is
enabled or not as per comment in bpf_dump_raw_ok():

/*
 * Reconstruction of call-sites is dependent on kallsyms,
 * thus make dump the same restriction.
 */

also as per below code: 
(we were not sure whether BPF will work or not with patch 3/3 because of no expertise on BPF code,
so we keep the behaviour same)

       if (ulen) {
                if (bpf_dump_raw_ok(file->f_cred)) {
                        unsigned long ksym_addr;
                        u64 __user *user_ksyms;
                        u32 i;

                        /* copy the address of the kernel symbol
                         * corresponding to each function
                         */
                        ulen = min_t(u32, info.nr_jited_ksyms, ulen);
                        user_ksyms = u64_to_user_ptr(info.jited_ksyms);
                        if (prog->aux->func_cnt) {
                                for (i = 0; i < ulen; i++) {
   ...
   }
   
earlier conversation for this change:
https://lkml.org/lkml/2022/4/13/326

here Petr CC'ed BPF maintainers to know their opinion whether BPF code can work with patch 3/3,
if not then we need this patch.

Thanks,
Maninder Singh

Re: [PATCH v4 2/3] bpf: make bpf_dump_raw_ok() based on CONFIG_KALLSYMS

[Andrii Nakryiko]

On Tue, Jun 6, 2023 at 8:46 PM Maninder Singh <maninder1.s@samsung.com> wrote:
>
> Hi Andrii Nakryiko,
>
> >>
> >> bpf_dump_raw_ok() depends on kallsyms_show_value() and we already
> >> have a false definition for the !CONFIG_KALLSYMS case. But we'll
> >> soon expand on kallsyms_show_value() and so to make the code
> >> easier to follow just provide a direct !CONFIG_KALLSYMS definition
> >> for bpf_dump_raw_ok() as well.
> >
> > I'm sorry, I'm failing to follow the exact reasoning about
> > simplification. It seems simpler to have
> >
> > static inline bool kallsyms_show_value(const struct cred *cred)
> > {
> >     return false;
> > }
> >
> > and control it from kallsyms-related internal header, rather than
> > adding CONFIG_KALLSYMS ifdef-ery to include/linux/filter.h and
> > redefining that `return false` decision. What if in the future we
> > decide that if !CONFIG_KALLSYMS it's ok to show raw addresses, now
> > we'll have to remember to update it in two places.
> >
> > Unless I'm missing some other complications?
> >
>
> Patch 3/3 does the same, it extends functionality of kallsyms_show_value()
> in case of  !CONFIG_KALLSYMS.
>
> All other users likes modules code, kprobe codes are using this API
> for sanity/permission, and then prints the address like below:
>
> static int kprobe_blacklist_seq_show(struct seq_file *m, void *v)
> {
> ...
>         if (!kallsyms_show_value(m->file->f_cred))
>                 seq_printf(m, "0x%px-0x%px\t%ps\n", NULL, NULL,
>                            (void *)ent->start_addr);
>         else
>                 seq_printf(m, "0x%px-0x%px\t%ps\n", (void *)ent->start_addr,
>                            (void *)ent->end_addr, (void *)ent->start_addr);
> ..
> }
>
> so there will be no issues if we move kallsyms_show_value() out of KALLSYMS dependency.
> and these codes will work in case of !KALLSYMS also.
>
> but BPF code logic was complex and seems this API was used as checking for whether KALLSYMS is
> enabled or not as per comment in bpf_dump_raw_ok():
>
> /*
>  * Reconstruction of call-sites is dependent on kallsyms,
>  * thus make dump the same restriction.
>  */
>
> also as per below code:
> (we were not sure whether BPF will work or not with patch 3/3 because of no expertise on BPF code,
> so we keep the behaviour same)

I think bpf_dump_raw_ok() is purely about checking whether it's ok to
return unobfuscated kernel addresses to user/BPF program. So it feels
like it should be ok to just rely on kallsyms_show_value() and not
special case here. If some of the code relies on actually having
CONFIG_KALLSYMS and related functionality, it should be properly
guarded already (or should enforce `SELECT KALLSYMS` in Kconfig).

>
>        if (ulen) {
>                 if (bpf_dump_raw_ok(file->f_cred)) {
>                         unsigned long ksym_addr;
>                         u64 __user *user_ksyms;
>                         u32 i;
>
>                         /* copy the address of the kernel symbol
>                          * corresponding to each function
>                          */
>                         ulen = min_t(u32, info.nr_jited_ksyms, ulen);
>                         user_ksyms = u64_to_user_ptr(info.jited_ksyms);
>                         if (prog->aux->func_cnt) {
>                                 for (i = 0; i < ulen; i++) {
>    ...
>    }
>
> earlier conversation for this change:
> https://lkml.org/lkml/2022/4/13/326
>
> here Petr CC'ed BPF maintainers to know their opinion whether BPF code can work with patch 3/3,
> if not then we need this patch.
>
> Thanks,
> Maninder Singh

RE: [PATCH v4 2/3] bpf: make bpf_dump_raw_ok() based on CONFIG_KALLSYMS

[Maninder Singh]

Hi,

> > also as per below code:
> > (we were not sure whether BPF will work or not with patch 3/3 because of no expertise on BPF code,
> > so we keep the behaviour same)
> 
> I think bpf_dump_raw_ok() is purely about checking whether it's ok to
> return unobfuscated kernel addresses to user/BPF program. So it feels
> like it should be ok to just rely on kallsyms_show_value() and not
> special case here. If some of the code relies on actually having
> CONFIG_KALLSYMS and related functionality, it should be properly
> guarded already (or should enforce `SELECT KALLSYMS` in Kconfig).

Thanks for your confirmation, I will resend patches without bpf change.

Thanks,
Maninder Singh