netdev - Re: [PATCH v8 bpf-next 1/3] bpf: sharing bpf runtime stats with BPF_ENABLE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5BF895D9-6761-45FB-BE78-034BB2C73C6F@fb.com>
Date:   Thu, 30 Apr 2020 00:28:55 +0000
From:   Song Liu <songliubraving@...com>
To:     Daniel Borkmann <daniel@...earbox.net>
CC:     bpf <bpf@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ast@...nel.org" <ast@...nel.org>, Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH v8 bpf-next 1/3] bpf: sharing bpf runtime stats with
 BPF_ENABLE_STATS



> On Apr 29, 2020, at 4:33 PM, Daniel Borkmann <daniel@...earbox.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