diff mbox series

[v2,sysctl-next] bpf: move the bpf syscall sysctl table to bpf module

Message ID 20220223102808.80846-1-zhuyan34@huawei.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [v2,sysctl-next] bpf: move the bpf syscall sysctl table to bpf module | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next fail VM_Test
netdev/tree_selection success Not a local patch

Commit Message

Yan Zhu Feb. 23, 2022, 10:28 a.m. UTC 
Aggregating the code of the feature in the code file of the feature
itself can improve readability and reduce merge conflicts. So move
the bpf syscall sysctl table to kernel/bpf/syscall.c

Signed-off-by: Yan Zhu <zhuyan34@huawei.com>

---
v1->v2:
  1.Added patch branch identifier sysctl-next.
  2.Re-describe the reason for the patch submission.
---
 kernel/bpf/syscall.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 kernel/sysctl.c      | 71 ----------------------------------------------
 2 files changed, 80 insertions(+), 71 deletions(-)

Comments

Daniel Borkmann Feb. 28, 2022, 3:53 p.m. UTC | #1 
Hi Yan,

On 2/23/22 11:28 AM, Yan Zhu wrote:
> Aggregating the code of the feature in the code file of the feature
> itself can improve readability and reduce merge conflicts. So move
> the bpf syscall sysctl table to kernel/bpf/syscall.c
> 
> Signed-off-by: Yan Zhu <zhuyan34@huawei.com>
> 
> ---
> v1->v2:
>    1.Added patch branch identifier sysctl-next.
>    2.Re-describe the reason for the patch submission.

I'm not applying it given there is very little value in this change, see also what
has been said earlier:

https://lore.kernel.org/bpf/CAADnVQKmBoQEG1+nmrCg2ePVncn9rZJX9R4eucP9ULiY=xVGjQ@mail.gmail.com/

Thanks,
Daniel
Luis Chamberlain Feb. 28, 2022, 11:41 p.m. UTC | #2 
On Mon, Feb 28, 2022 at 04:53:44PM +0100, Daniel Borkmann wrote:
> Hi Yan,
> 
> On 2/23/22 11:28 AM, Yan Zhu wrote:
> > Aggregating the code of the feature in the code file of the feature
> > itself can improve readability and reduce merge conflicts. So move
> > the bpf syscall sysctl table to kernel/bpf/syscall.c
> > 
> > Signed-off-by: Yan Zhu <zhuyan34@huawei.com>
> > 
> > ---
> > v1->v2:
> >    1.Added patch branch identifier sysctl-next.
> >    2.Re-describe the reason for the patch submission.
> 
> I'm not applying it given there is very little value in this change, see also what
> has been said earlier:
> 
> https://lore.kernel.org/bpf/CAADnVQKmBoQEG1+nmrCg2ePVncn9rZJX9R4eucP9ULiY=xVGjQ@mail.gmail.com/

Daniel,

sorry folk are seing you patches with crap commit logs. The
justification should be made clearer: we're moving sysctls out of
kernel/sysctl.c as its a mess. I already moved all filesystem sysctls
out. And with time the goal is to move all sysctls out to their own
susbsystem/actual user.

kernel/sysctl.c has grown to an insane mess and its easy to run
into conflicts with it. The effort to move them out is part of this.

The commit logs should not suck though...

  Luis
Yan Zhu March 2, 2022, 1:58 a.m. UTC | #3 
Mon, 28 Feb 2022 15:41:40 -0800, Luis Chamberlain wrote:
> On Mon, Feb 28, 2022 at 04:53:44PM +0100, Daniel Borkmann wrote:
> > Hi Yan,
> > 
> > On 2/23/22 11:28 AM, Yan Zhu wrote:
> > > Aggregating the code of the feature in the code file of the feature
> > > itself can improve readability and reduce merge conflicts. So move
> > > the bpf syscall sysctl table to kernel/bpf/syscall.c
> > > 
> > > Signed-off-by: Yan Zhu <zhuyan34@huawei.com>
> > > 
> > > ---
> > > v1->v2:
> > >    1.Added patch branch identifier sysctl-next.
> > >    2.Re-describe the reason for the patch submission.
> > 
> > I'm not applying it given there is very little value in this change, see also what
> > has been said earlier:
> > 
> > https://lore.kernel.org/bpf/CAADnVQKmBoQEG1+nmrCg2ePVncn9rZJX9R4eucP9ULiY=xVGjQ@mail.gmail.com/
> 
> Daniel,
> 
> sorry folk are seing you patches with crap commit logs. The
> justification should be made clearer: we're moving sysctls out of
> kernel/sysctl.c as its a mess. I already moved all filesystem sysctls
> out. And with time the goal is to move all sysctls out to their own
> susbsystem/actual user.
> kernel/sysctl.c has grown to an insane mess and its easy to run
> into conflicts with it. The effort to move them out is part of this.
Luis,

Thanks for the suggestion, I will use it as my patch from the commit
message to be able to clearly describe the purpose of the patch.

> The commit logs should not suck though...
diff mbox series

Patch

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 35646db3d950..50f85b47d478 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -4888,3 +4888,83 @@  const struct bpf_verifier_ops bpf_syscall_verifier_ops = {
 const struct bpf_prog_ops bpf_syscall_prog_ops = {
 	.test_run = bpf_prog_test_run_syscall,
 };
+
+#ifdef CONFIG_SYSCTL
+static int bpf_stats_handler(struct ctl_table *table, int write,
+			     void *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct static_key *key = (struct static_key *)table->data;
+	static int saved_val;
+	int val, ret;
+	struct ctl_table tmp = {
+		.data   = &val,
+		.maxlen = sizeof(val),
+		.mode   = table->mode,
+		.extra1 = SYSCTL_ZERO,
+		.extra2 = SYSCTL_ONE,
+	};
+
+	if (write && !capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	mutex_lock(&bpf_stats_enabled_mutex);
+	val = saved_val;
+	ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
+	if (write && !ret && val != saved_val) {
+		if (val)
+			static_key_slow_inc(key);
+		else
+			static_key_slow_dec(key);
+		saved_val = val;
+	}
+	mutex_unlock(&bpf_stats_enabled_mutex);
+	return ret;
+}
+
+static int bpf_unpriv_handler(struct ctl_table *table, int write,
+			      void *buffer, size_t *lenp, loff_t *ppos)
+{
+	int ret, unpriv_enable = *(int *)table->data;
+	bool locked_state = unpriv_enable == 1;
+	struct ctl_table tmp = *table;
+
+	if (write && !capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	tmp.data = &unpriv_enable;
+	ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
+	if (write && !ret) {
+		if (locked_state && unpriv_enable != 1)
+			return -EPERM;
+		*(int *)table->data = unpriv_enable;
+	}
+	return ret;
+}
+
+static struct ctl_table bpf_syscall_table[] = {
+	{
+		.procname	= "unprivileged_bpf_disabled",
+		.data		= &sysctl_unprivileged_bpf_disabled,
+		.maxlen		= sizeof(sysctl_unprivileged_bpf_disabled),
+		.mode		= 0644,
+		.proc_handler	= bpf_unpriv_handler,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_TWO,
+	},
+	{
+		.procname	= "bpf_stats_enabled",
+		.data		= &bpf_stats_enabled_key.key,
+		.maxlen		= sizeof(bpf_stats_enabled_key),
+		.mode		= 0644,
+		.proc_handler	= bpf_stats_handler,
+	},
+	{ }
+};
+
+static int __init bpf_syscall_sysctl_init(void)
+{
+	register_sysctl_init("kernel", bpf_syscall_table);
+	return 0;
+}
+late_initcall(bpf_syscall_sysctl_init);
+#endif /* CONFIG_SYSCTL */
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index ae5e59396b5d..c64db3755d9c 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -146,59 +146,6 @@  static const int max_extfrag_threshold = 1000;
 
 #endif /* CONFIG_SYSCTL */
 
-#if defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_SYSCTL)
-static int bpf_stats_handler(struct ctl_table *table, int write,
-			     void *buffer, size_t *lenp, loff_t *ppos)
-{
-	struct static_key *key = (struct static_key *)table->data;
-	static int saved_val;
-	int val, ret;
-	struct ctl_table tmp = {
-		.data   = &val,
-		.maxlen = sizeof(val),
-		.mode   = table->mode,
-		.extra1 = SYSCTL_ZERO,
-		.extra2 = SYSCTL_ONE,
-	};
-
-	if (write && !capable(CAP_SYS_ADMIN))
-		return -EPERM;
-
-	mutex_lock(&bpf_stats_enabled_mutex);
-	val = saved_val;
-	ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
-	if (write && !ret && val != saved_val) {
-		if (val)
-			static_key_slow_inc(key);
-		else
-			static_key_slow_dec(key);
-		saved_val = val;
-	}
-	mutex_unlock(&bpf_stats_enabled_mutex);
-	return ret;
-}
-
-static int bpf_unpriv_handler(struct ctl_table *table, int write,
-			      void *buffer, size_t *lenp, loff_t *ppos)
-{
-	int ret, unpriv_enable = *(int *)table->data;
-	bool locked_state = unpriv_enable == 1;
-	struct ctl_table tmp = *table;
-
-	if (write && !capable(CAP_SYS_ADMIN))
-		return -EPERM;
-
-	tmp.data = &unpriv_enable;
-	ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
-	if (write && !ret) {
-		if (locked_state && unpriv_enable != 1)
-			return -EPERM;
-		*(int *)table->data = unpriv_enable;
-	}
-	return ret;
-}
-#endif /* CONFIG_BPF_SYSCALL && CONFIG_SYSCTL */
-
 /*
  * /proc/sys support
  */
@@ -2188,24 +2135,6 @@  static struct ctl_table kern_table[] = {
 		.extra2		= SYSCTL_ONE,
 	},
 #endif
-#ifdef CONFIG_BPF_SYSCALL
-	{
-		.procname	= "unprivileged_bpf_disabled",
-		.data		= &sysctl_unprivileged_bpf_disabled,
-		.maxlen		= sizeof(sysctl_unprivileged_bpf_disabled),
-		.mode		= 0644,
-		.proc_handler	= bpf_unpriv_handler,
-		.extra1		= SYSCTL_ZERO,
-		.extra2		= SYSCTL_TWO,
-	},
-	{
-		.procname	= "bpf_stats_enabled",
-		.data		= &bpf_stats_enabled_key.key,
-		.maxlen		= sizeof(bpf_stats_enabled_key),
-		.mode		= 0644,
-		.proc_handler	= bpf_stats_handler,
-	},
-#endif
 #if defined(CONFIG_TREE_RCU)
 	{
 		.procname	= "panic_on_rcu_stall",