netdev - Re: [PATCH bpf-next v2 1/7] bpf, sockmap: add BPF_F

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]

Date: Sat, 19 Aug 2023 17:25:56 +0800
From: "liujian (CE)" <liujian56@...wei.com>
To: John Fastabend <john.fastabend@...il.com>, <jakub@...udflare.com>,
	<ast@...nel.org>, <daniel@...earbox.net>, <andrii@...nel.org>,
	<martin.lau@...ux.dev>, <song@...nel.org>, <yonghong.song@...ux.dev>,
	<kpsingh@...nel.org>, <sdf@...gle.com>, <haoluo@...gle.com>,
	<jolsa@...nel.org>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <dsahern@...nel.org>
CC: <netdev@...r.kernel.org>, <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v2 1/7] bpf, sockmap: add BPF_F_PERMANENTLY flag
 for skmsg redirect



On 2023/8/17 14:13, John Fastabend wrote:
> Liu Jian wrote:
>> If the sockmap msg redirection function is used only to forward packets
>> and no other operation, the execution result of the BPF_SK_MSG_VERDICT
>> program is the same each time. In this case, the BPF program only needs to
>> be run once. Add BPF_F_PERMANENTLY flag to bpf_msg_redirect_map() and
>> bpf_msg_redirect_hash() to implement this ability.
>>
> 
> I like the use case. Did you consider using
> 
>   long bpf_msg_apply_bytes(struct sk_msg_buff *msg, u32 bytes)
> 
> This could be set to UINT32_MAX and then the BPF prog would only be run
> every 0xfffffff bytes.
> 
I didn't realize that this feature could be used for this, and I thought 
it should have the same effect. Thanks John.
>> Then we can enable this function in the bpf program as follows:
>> bpf_msg_redirect_hash(xx, xx, xx, BPF_F_INGRESS | BPF_F_PERMANENTLY);
>>
>> Test results using netperf  TCP_STREAM mode:
>> for i in 1 64 128 512 1k 2k 32k 64k 100k 500k 1m;then
>> netperf -T 1,2 -t TCP_STREAM -H 127.0.0.1 -l 20 -- -m $i -s 100m,100m -S 100m,100m
>> done
>>
>> before:
>> 3.84 246.52 496.89 1885.03 3415.29 6375.03 40749.09 48764.40 51611.34 55678.26 55992.78
>> after:
>> 4.43 279.20 555.82 2080.79 3870.70 7105.44 41836.41 49709.75 51861.56 55211.00 54566.85
> 
> I suspect comparing against
> 
>    bpf_msg_redirect_hash(...)
>    bpf_msg_apply_bytes(msg, UINT32_MAX)
> 
> the diff will be rather small. I agree the API is nicer though to simply
Yes, it should have the same effect and looks good to me.
> set the flag. Its too bad we didn't think to add a forever to apply_bytes.
> I would prefer this API for example,
> 
>    bpf_msg_redirect_hash(...)
>    bpf_msg_apply_bytes(msg, 0, PERMANENT);
> 
What do you mean by this? Should I post another version for this?
> Given we have apply_bytes is it still useful to have a PERMANENT flag
> in your use case? Here we would just reset to UNINT32_MAX if we reached
> max bytes.
> 
If apply_bytes is set to UNINT32_MAX, the number of times that the bpf 
program runs should be small enough to meet my needs.
>>
>> Signed-off-by: Liu Jian <liujian56@...wei.com>
>> ---
>>   include/linux/skmsg.h          |  1 +
>>   include/uapi/linux/bpf.h       |  7 +++++--
>>   net/core/skmsg.c               |  1 +
>>   net/core/sock_map.c            |  4 ++--
>>   net/ipv4/tcp_bpf.c             | 21 +++++++++++++++------
>>   tools/include/uapi/linux/bpf.h |  7 +++++--
>>   6 files changed, 29 insertions(+), 12 deletions(-)
> 
> [...]
> 
>>   
>> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
>> index 70da85200695..cf622ea4f018 100644
>> --- a/tools/include/uapi/linux/bpf.h
>> +++ b/tools/include/uapi/linux/bpf.h
>> @@ -3004,7 +3004,8 @@ union bpf_attr {
>>    * 		egress interfaces can be used for redirection. The
>>    * 		**BPF_F_INGRESS** value in *flags* is used to make the
>>    * 		distinction (ingress path is selected if the flag is present,
>> - * 		egress path otherwise). This is the only flag supported for now.
>> + * 		egress path otherwise). The **BPF_F_PERMANENTLY** value in
>> + *		*flags* is used to indicates whether the eBPF result is permanent.
> 
> We at least need to document what happens if PERMANENTLY and apply_bytes are
> used together.
> 
>>    * 	Return
>>    * 		**SK_PASS** on success, or **SK_DROP** on error.
>>    *
>