Re: [RFC PATCH net-next 2/2] net: Reset forwarded skb->tstamp before delivering to user space

On Wed, Dec 08, 2021 at 08:27:45PM +0100, Daniel Borkmann wrote:

On 12/8/21 9:30 AM, Martin KaFai Lau wrote:

quoted

On Wed, Dec 08, 2021 at 12:18:46AM -0800, Martin KaFai Lau wrote:

quoted

On Tue, Dec 07, 2021 at 10:48:53PM +0100, Daniel Borkmann wrote:

[...]

quoted

quoted

quoted

One other thing I wonder, BPF progs at host-facing veth's tc ingress which
are not aware of skb->tstamp will then see a tstamp from future given we
intentionally bypass the net_timestamp_check() and might get confused (or
would confuse higher-layer application logic)? Not quite sure yet if they
would be the only affected user.

Considering the variety of clock used in skb->tstamp (real/mono, and also
tai in SO_TXTIME),  in general I am not sure if the tc-bpf can assume anything
in the skb->tstamp now.

But today that's either only 0 or real via __net_timestamp() if skb->tstamp is
read at bpf@ingress@veth@host, no?

I think I was trying to say the CLOCK_REALTIME in __sk_buff->tstamp 
is not practically useful in bpf@ingress other than an increasing number.
No easy way to get the 'now' in CLOCK_REALTIME to compare with
in order to learn if it is future or current time.  Setting
it based on bpf_ktime_get_ns() in MONO is likely broken currently
regardless of the skb is forwarded or delivered locally.

We have a use case that wants to change the forwarded EDT
in bpf@ingress@veth@host before forwarding.  If it needs to
keep __sk_buff->tstamp as the 'now' CLOCK_REALTIME in ingress,
it needs to provide a separate way for the bpf to check/change
the forwarded EDT.

Daniel, do you have suggestion on where to temporarily store
the forwarded EDT so that the bpf@ingress can access?

quoted

quoted

Also, there is only mono clock bpf_ktime_get helper, the most reasonable usage
now for tc-bpf is to set the EDT which is in mono.  This seems to be the
intention when the __sk_buff->tstamp was added.

Yep, fwiw, that's also how we only use it in our code base today.

quoted

quoted

For ingress, it is real clock now.  Other than simply printing it out,
it is hard to think of a good way to use the value.  Also, although
it is unlikely, net_timestamp_check() does not always stamp the skb.

For non bpf ingress, hmmm.... yeah, not sure if it is indeed an issue :/
may be save the tx tstamp first and then temporarily restamp with __net_timestamp()