netdev - Re: [PATCH net-next v3 02/14] net-timestamp: allow two features to work parallelly

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: <CAL+tcoBfuFL7-EOBY4RLMdDZJcUSyq20pJW13OqzNazUP7=gaw@mail.gmail.com>
Date: Thu, 31 Oct 2024 15:04:40 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>, willemb@...gle.com, 
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	dsahern@...nel.org, 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@...ichev.me, 
	haoluo@...gle.com, jolsa@...nel.org, shuah@...nel.org, ykolal@...com, 
	bpf@...r.kernel.org, netdev@...r.kernel.org, 
	Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v3 02/14] net-timestamp: allow two features to
 work parallelly

On Thu, Oct 31, 2024 at 2:27 PM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 10/30/24 5:13 PM, Jason Xing wrote:
> > I realized that we will have some new sock_opt flags like
> > TS_SCHED_OPT_CB in patch 4, so we can control whether to print or
> > not... For each sock_opt point, they will be called without caring if
> > related flags in skb are set. Well, it's meaningless to add more
> > control of skb tsflags at each TS_xx_OPT_CB point.
> >
> > Am I understanding in a correct way? Now, I'm totally fine with this great idea!
> Yes, I think so.
>
> The sockops prog can choose to ignore any BPF_SOCK_OPS_TS_*_CB. The are only 3:
> SCHED, SND, and ACK. If the hwtstamp is available from a NIC, I think it would
> be quite wasteful to throw it away. ACK can be controlled by the
> TCP_SKB_CB(skb)->bpf_txstamp_ack.

Right, let me try this:)

> Going back to my earlier bpf_setsockopt(SOL_SOCKET, BPF_TX_TIMESTAMPING)
> comment. I think it may as well go back to use the "u8
> bpf_sock_ops_cb_flags;" and use the bpf_sock_ops_cb_flags_set() helper to
> enable/disable the timestamp related callback hook. May be add one
> BPF_SOCK_OPS_TX_TIMESTAMPING_CB_FLAG.

bpf_sock_ops_cb_flags this flag is only used in TCP condition, right?
If that is so, it cannot be suitable for UDP.

I'm thinking of this solution:
1) adding a new flag in SOF_TIMESTAMPING_OPT_BPF flag (in
include/uapi/linux/net_tstamp.h) which can be used by sk->sk_tsflags
2) flags =   SOF_TIMESTAMPING_OPT_BPF;    bpf_setsockopt(skops,
SOL_SOCKET, SO_TIMESTAMPING, &flags, sizeof(flags));
3) test if sk->sk_tsflags has this new flag in tcp_tx_timestamp() or
in udp_sendmsg()
...

>
> For tx, one new hook should be at the sendmsg and should be around
> tcp_tx_timestamp (?) for tcp. Another hook is __skb_tstamp_tx() which should be

I think there are two points we're supposed to record:
1) the moment tcp/udp_sendmsg() is triggered. It represents the syscall time.
2) another point in tcp_tx_timestamp(). It represents the timestamp of
the last skb in this sendmsg() call.
Users may happen to send a big packet.

> similar to your patch. Add a new kfunc to set shinfo->tx_flags |= SKBTX_BPF
> and/or TCP_SKB_CB(skb)->bpf_txstamp_ack during sendmsg.

Got it.

>
>
> For rx, add one BPF_SOCK_OPS_RX_TIMESTAMPING_CB_FLAG. bpf_sock_ops_cb_flags
> needs to move from the tcp_sock to the sock because it will be used by UDP also.
> When enabling or disabling this flag, it needs to take care of the
> net_{enable,disable}_timestamp. The same for the __sk_destruct() also.
>

I think if the solution I proposed as above is feasible, then we don't
have to move the tcp_sock which brings more extra work :)

Thanks,
Jason