> On Apr 29, 2020, at 4:33 PM, Daniel Borkmann <dan...@iogearbox.net> wrote:
>
>>
[...]
>> +
>> + fd = anon_inode_getfd("bpf-stats", &bpf_stats_fops, NULL, 0);
>
> Missing O_CLOEXEC or intentional (if latter, I'd have expected a comment
> here though)?
Yeah, we should have O_CLOEXEC here. Will fix (unless you want fix it at
commit time).
>
>> + if (fd >= 0)
>> + static_key_slow_inc(&bpf_stats_enabled_key.key);
>> +
>> + mutex_unlock(&bpf_stats_enabled_mutex);
>> + return fd;
>> +}
>> +
>> +#define BPF_ENABLE_STATS_LAST_FIELD enable_stats.type
>> +
> [...]
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index e961286d0e14..af08ef0690cb 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -201,6 +201,40 @@ static int max_extfrag_threshold = 1000;
>> #endif /* CONFIG_SYSCTL */
>> +#ifdef CONFIG_BPF_SYSCALL
>> +static int bpf_stats_handler(struct ctl_table *table, int write,
>> + void __user *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;
>> +}
>
> nit: I wonder if most of the logic could have been shared with
> proc_do_static_key() here and only the mutex passed as an arg to
> the common helper?
We have static saved_val here, so it is not so easy to share it.
I think it is cleaner with separate functions.
Thanks,
Song
