[PATCH v2 1/2] bpf: make defination of bpf_dump_raw_ok based on CONFIG_KALLSYMS

[PATCH v2 1/2] bpf: make defination of bpf_dump_raw_ok based on CONFIG_KALLSYMS

[Maninder Singh]

No functional change with this commit.

As of now bpf_dump_raw_ok() is dependent on kallsyms_show_value().
Rearranging the code to return false directly in defination of
bpf_dump_raw_ok() based on CONFIG_KALLSYMS.

@reason: next patch will make kallsyms_show_value() defination
generic and bpf_dump_raw_ok() will return true otherwise.

Thus make decision based on CONFIG rather than kallsyms_show_value(),
as bpf functionality is heavily dependent on KALLSYMS.

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>
---
v1 -> v2 : made separate patches for kallsyms and bpf

 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 v2 2/2] kallsyms: add kallsyms_show_value defination in all cases

[Maninder Singh]

kallsyms_show_value return false if KALLSYMS is disabled,
but it is getting used in module.c also for fetching module address.

Thus when KALLSYMS is disabled, system will not print module
load address:

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)

New file knosyms.c is added (name suggested by Kees Cook),
which includes common defination irrespective of CONFIG_KALLSYMS.

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://lkml.org/lkml/2022/5/11/212
https://lkml.org/lkml/2022/4/13/47

 include/linux/kallsyms.h | 10 +++-----
 kernel/Makefile          |  2 +-
 kernel/kallsyms.c        | 35 ---------------------------
 kernel/knosyms.c         | 52 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 56 insertions(+), 43 deletions(-)
 create mode 100644 kernel/knosyms.c

diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index fe3c9993b5bf..3f3559ba0c56 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -24,6 +24,9 @@
 struct cred;
 struct module;
 
+/* How and when do we show kallsyms values? */
+extern bool kallsyms_show_value(const struct cred *cred);
+
 static inline int is_kernel_text(unsigned long addr)
 {
 	if (__is_kernel_text(addr))
@@ -95,8 +98,6 @@ extern int sprint_backtrace_build_id(char *buffer, unsigned long address);
 int lookup_symbol_name(unsigned long addr, char *symname);
 int lookup_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name);
 
-/* How and when do we show kallsyms values? */
-extern bool kallsyms_show_value(const struct cred *cred);
 
 #else /* !CONFIG_KALLSYMS */
 
@@ -160,11 +161,6 @@ static inline int lookup_symbol_attrs(unsigned long addr, unsigned long *size, u
 	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/Makefile b/kernel/Makefile
index b69c95315480..76efc634b1a4 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 knosyms.o
 
 obj-$(CONFIG_USERMODE_DRIVER) += usermode_driver.o
 obj-$(CONFIG_MULTIUSER) += groups.o
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 77747391f49b..1dcb7051f852 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -961,41 +961,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/knosyms.c b/kernel/knosyms.c
new file mode 100644
index 000000000000..f2c8755e22e7
--- /dev/null
+++ b/kernel/knosyms.c
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 Samsung Electronics Co., Ltd
+ *
+ * Author:
+ *	Maninder singh <maninder1.s@samsung.com>
+ *	Onkarnath <onkarnath.1@samsung.com>
+ *
+ * A split of kernel/kallsyms.c
+ * to contain few generic function definations independent of config KALLSYMS
+ * or defined under KALLSYMS disabled.
+ */
+
+#include <linux/kallsyms.h>
+#include <linux/security.h>
+
+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;
+	}
+}
+
-- 
2.17.1

Re: [PATCH v2 2/2] kallsyms: add kallsyms_show_value defination in all cases

[Luis Chamberlain]

On Wed, May 31, 2023 at 02:17:45PM +0530, Maninder Singh wrote:
>  include/linux/kallsyms.h | 10 +++-----
>  kernel/Makefile          |  2 +-
>  kernel/kallsyms.c        | 35 ---------------------------
>  kernel/knosyms.c         | 52 ++++++++++++++++++++++++++++++++++++++++

You missed my point of the value of doing a move to a new file and
making it easier for folks to review. For instance I am giving up on
reviewing this patch alone because you made all these changes to a new
file *and* also included a functional change in it. Think about it from
a reviewer perspective, you want to make their life easier, not harder.

So, to do that you first move all the stuff into a new file with 0
functional changes. Then, you make a functional change as a separate
commit. So this becomes 3 commits then.

Sit back and then think after you have done this: does it make sense
then afterwards to re-arrange the order of the patches so to make it
easier for folks to review this patchset? If so what order should
I put those changes in? I don't know the answer to this question but
just think about it once you have done that.

For instance, confirming there was 0 functional changes to your first
patch actually took me about 3 minutes or so, how can you reduce the
time to review to a few seconds for a non-functional change? Work on
your commit logs and your changes in light of this so to make patch
review easier.

  Luis

Re: [PATCH v2 1/2] bpf: make defination of bpf_dump_raw_ok based on CONFIG_KALLSYMS

[Luis Chamberlain]

On Wed, May 31, 2023 at 02:17:44PM +0530, Maninder Singh wrote:
> No functional change with this commit.
> 
> As of now bpf_dump_raw_ok() is dependent on kallsyms_show_value().
> Rearranging the code to return false directly in defination of
> bpf_dump_raw_ok() based on CONFIG_KALLSYMS.
> 
> @reason: next patch will make kallsyms_show_value() defination
> generic and bpf_dump_raw_ok() will return true otherwise.

This reads a bit funny and the split here doesn't help the patch
reviewier, so you can help with that a bit with the commit log
and be a bit clearer to throw in the few pieces of nuggets the
reader should be aware of when reviewing your patch right away.
So how about instaed:

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.

This makes it easier to read and makes it clear bpf_dump_raw_ok()
will immediately return false when !CONFIG_KALLSYMS, making it
easier to understand what happens for heavy users such as bfp.

With this you can add:

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis