netdev - Re: [PATCH bpf-next] bpf: add sacked flag in BPF_SOCK_OPS_RETRANS

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: <152f00e2-8f60-4f39-8ab4-fdab1b0bc01a@linux.dev>
Date: Wed, 17 Apr 2024 15:48:55 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Eric Dumazet <edumazet@...gle.com>, Philo Lu <lulie@...ux.alibaba.com>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, davem@...emloft.net,
 kuba@...nel.org, pabeni@...hat.com, ast@...nel.org, daniel@...earbox.net,
 andrii@...nel.org, eddyz87@...il.com, song@...nel.org,
 yonghong.song@...ux.dev, john.fastabend@...il.com, kpsingh@...nel.org,
 sdf@...gle.com, haoluo@...gle.com, jolsa@...nel.org, dsahern@...nel.org,
 laoar.shao@...il.com, xuanzhuo@...ux.alibaba.com, fred.cc@...baba-inc.com
Subject: Re: [PATCH bpf-next] bpf: add sacked flag in BPF_SOCK_OPS_RETRANS_CB

On 4/17/24 6:11 AM, Eric Dumazet wrote:
> On Wed, Apr 17, 2024 at 2:46 PM Philo Lu <lulie@...ux.alibaba.com> wrote:
>>
>> Add TCP_SKB_CB(skb)->sacked as the 4th arg of sockops passed to bpf
>> program. Then we can get the retransmission efficiency by counting skbs
>> w/ and w/o TCPCB_EVER_RETRANS mark. And for this purpose, sacked
>> updating is moved after the BPF_SOCK_OPS_RETRANS_CB hook.
>>
>> Signed-off-by: Philo Lu <lulie@...ux.alibaba.com>
> 
> This might be a naive question, but how the bpf program know what is the meaning
> of each bit ?
> 
> Are they exposed already, and how future changes in TCP stack could
> break old bpf programs ?
> 
> #define TCPCB_SACKED_ACKED 0x01 /* SKB ACK'd by a SACK block */
> #define TCPCB_SACKED_RETRANS 0x02 /* SKB retransmitted */
> #define TCPCB_LOST 0x04 /* SKB is lost */
> #define TCPCB_TAGBITS 0x07 /* All tag bits */
> #define TCPCB_REPAIRED 0x10 /* SKB repaired (no skb_mstamp_ns) */
> #define TCPCB_EVER_RETRANS 0x80 /* Ever retransmitted frame */
> #define TCPCB_RETRANS (TCPCB_SACKED_RETRANS|TCPCB_EVER_RETRANS| \
> TCPCB_REPAIRED)

I think it is the best to use the trace_tcp_retransmit_skb() tracepoint instead.

iiuc the use case, moving the "TCP_SKB_CB(skb)->sacked |= TCPCB_EVER_RETRANS;" 
after the tracepoint should have similar effect.

If the TCPCB_* is moved to a enum, it will be included in the "vmlinux.h" that 
the bpf prog can use and no need to expose them in uapi.