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

[Martin KaFai Lau <kafai@fb.com>]

On Wed, Dec 08, 2021 at 08:27:45PM +0100, Daniel Borkmann wrote:
> On 12/8/21 9:30 AM, Martin KaFai Lau wrote:
> > On Wed, Dec 08, 2021 at 12:18:46AM -0800, Martin KaFai Lau wrote:
> > > On Tue, Dec 07, 2021 at 10:48:53PM +0100, Daniel Borkmann wrote:
> [...]
> > > > 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?

> 
> > > 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.
> 
> > > 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()