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

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: <20211208221924.v4gqpkzzrbhgi2xe@kafai-mbp.dhcp.thefacebook.com>
Date:   Wed, 8 Dec 2021 14:19:25 -0800
From:   Martin KaFai Lau <kafai@...com>
To:     Daniel Borkmann <daniel@...earbox.net>
CC:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        <netdev@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>, <kernel-team@...com>
Subject: 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:
> > 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@...ress@...h@...t, no?
I think I was trying to say the CLOCK_REALTIME in __sk_buff->tstamp 
is not practically useful in bpf@...ress 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@...ress@...h@...t 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@...ress 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()