| 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
| ||
|
Message-ID: <20211216222332.fltkclu4x3udpomr@kafai-mbp.dhcp.thefacebook.com>
Date: Thu, 16 Dec 2021 14:23:32 -0800
From: Martin KaFai Lau <kafai@...com>
To: Willem de Bruijn <willemb@...gle.com>
CC: <netdev@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
David Miller <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, <kernel-team@...com>
Subject: Re: [RFC PATCH v2 net-next] net: Preserve skb delivery time during
forward
On Thu, Dec 16, 2021 at 10:32:23AM -0500, Willem de Bruijn wrote:
[ ... ]
> > (c: skb->tstamp = 0)
> > vv
> > tcp-sender => veth@...ns => veth@...tns(b: rx: skb->tstamp = real_clock) => fq@...0
> > ^^
> > (a: skb->tstamp = 0)
> >
> > (a) veth@...ns TX to veth@...tns:
> > skb->tstamp (mono clock) is a EDT and it is in future time.
> > Reset to 0 so that it won't skip the net_timestamp_check at the
> > RX side in (b).
> > (b) RX (netif_rx) in veth@...tns:
> > net_timestamp_check puts a current time (real clock) in skb->tstamp.
> > (c) veth@...tns forward to fq@...0:
> > skb->tstamp is reset back to 0 again because fq is using
> > mono clock.
> >
> > This leads to an unstable TCP throughput issue described by Daniel in [0].
> >
> > We also have a use case that a bpf runs at ingress@...h@...tns
> > to set EDT in skb->tstamp to limit the bandwidth usage
> > of a particular netns. This EDT currently also gets
> > reset in step (c) as described above.
[ ... ]
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 6535294f6a48..9bf0a1e2a1bd 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -435,9 +435,17 @@ enum {
> > /* device driver is going to provide hardware time stamp */
> > SKBTX_IN_PROGRESS = 1 << 2,
> >
> > + /* shinfo stores a future tx_delivery_tstamp instead of hwtstamps */
> > + SKBTX_DELIVERY_TSTAMP = 1 << 3,
> > +
> > /* generate wifi status information (where possible) */
> > SKBTX_WIFI_STATUS = 1 << 4,
> >
> > + /* skb->tstamp stored a future delivery time which
> > + * was set by a local sk and it can be fowarded.
> > + */
> > + SKBTX_DELIVERY_TSTAMP_ALLOW_FWD = 1 << 5,
> > +
> > /* generate software time stamp when entering packet scheduling */
> > SKBTX_SCHED_TSTAMP = 1 << 6,
> > };
> > @@ -530,7 +538,14 @@ struct skb_shared_info {
> > /* Warning: this field is not always filled in (UFO)! */
> > unsigned short gso_segs;
> > struct sk_buff *frag_list;
> > - struct skb_shared_hwtstamps hwtstamps;
> > + union {
> > + /* If SKBTX_DELIVERY_TSTAMP is set in tx_flags,
> > + * tx_delivery_tstamp is stored instead of
> > + * hwtstamps.
> > + */
>
> Should we just encode the timebase and/or type { timestamp,
> delivery_time } in th lower bits of the timestamp field? Its
> resolution is higher than actual clock precision.
In skb->tstamp ?
> > + struct skb_shared_hwtstamps hwtstamps;
> > + u64 tx_delivery_tstamp;
> > + };
> > unsigned int gso_type;
> > u32 tskey;
> >
> > @@ -1463,9 +1478,44 @@ static inline unsigned int skb_end_offset(const struct sk_buff *skb)
> >
> > static inline struct skb_shared_hwtstamps *skb_hwtstamps(struct sk_buff *skb)
> > {
> > + if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_DELIVERY_TSTAMP)) {
> > + skb_shinfo(skb)->tx_flags &= ~SKBTX_DELIVERY_TSTAMP;
> > + skb_shinfo(skb)->tx_delivery_tstamp = 0;
> > + }
> > return &skb_shinfo(skb)->hwtstamps;
> > }
> >
> > +/* Caller only needs to read the hwtstamps as ktime.
> > + * To update hwtstamps, HW device driver should call the writable
> > + * version skb_hwtstamps() that returns a pointer.
> > + */
> > +static inline ktime_t skb_hwtstamps_ktime(const struct sk_buff *skb)
> > +{
> > + if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_DELIVERY_TSTAMP))
> > + return 0;
> > + return skb_shinfo(skb)->hwtstamps.hwtstamp;
> > +}
> > +
> > +static inline void skb_scrub_tstamp(struct sk_buff *skb)
>
> skb_save_delivery_time?
yep. ok.
>
> is non-zero skb->tstamp test not sufficient, instead of
> SKBTX_DELIVERY_TSTAMP_ALLOW_FWD.
>
> It is if only called on the egress path. Is bpf on ingress the only
> reason for this?
Ah. ic. meaning testing non-zero skb->tstamp and then call
skb_save_delivery_time() only during the veth-egress-path:
somewhere in veth_xmit() => veth_forward_skb() but before
skb->tstamp was reset to 0 in __dev_forward_skb().
Keep *_forward() and bpf_out_*() unchanged (i.e. keep skb->tstamp = 0)
because the skb->tstamp could be stamped by net_timestamp_check().
Then SKBTX_DELIVERY_TSTAMP_ALLOW_FWD is not needed.
Did I understand your suggestion correctly?
However, we still need a bit to distinguish tx_delivery_tstamp
from hwtstamps.
>
> > +{
> > + if (skb_shinfo(skb)->tx_flags & SKBTX_DELIVERY_TSTAMP_ALLOW_FWD) {
> > + skb_shinfo(skb)->tx_delivery_tstamp = skb->tstamp;
> > + skb_shinfo(skb)->tx_flags |= SKBTX_DELIVERY_TSTAMP;
> > + skb_shinfo(skb)->tx_flags &= ~SKBTX_DELIVERY_TSTAMP_ALLOW_FWD;
> > + }
>
> Is this only called when there are no clones/shares?
No, I don't think so. TCP clone it. I also started thinking about
this after noticing a mistake in the change in __tcp_transmit_skb().
There are other places that change tx_flags, e.g. tcp_offload.c.
It is not shared at those places or there is some specific points
in the stack that is safe to change ?
>
> > + skb->tstamp = 0;
> > +}
> > +
> > +static inline void skb_restore_delivery_time(struct sk_buff *skb)
> > +{
> > + if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_DELIVERY_TSTAMP)) {
> > + skb->tstamp = skb_shinfo(skb)->tx_delivery_tstamp;
> > + skb_shinfo(skb)->tx_delivery_tstamp = 0;
> > + skb_shinfo(skb)->tx_flags &= ~SKBTX_DELIVERY_TSTAMP;
> > + skb_shinfo(skb)->tx_flags |= SKBTX_DELIVERY_TSTAMP_ALLOW_FWD;
> > + }
> > +}
> > +
> > static inline struct ubuf_info *skb_zcopy(struct sk_buff *skb)
> > {
> > bool is_zcopy = skb && skb_shinfo(skb)->flags & SKBFL_ZEROCOPY_ENABLE;
> > diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> > index ec646656dbf1..a3ba6195f2e3 100644
> > --- a/net/bridge/br_forward.c
> > +++ b/net/bridge/br_forward.c
> > @@ -62,7 +62,7 @@ EXPORT_SYMBOL_GPL(br_dev_queue_push_xmit);
> >
> > int br_forward_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
> > {
> > - skb->tstamp = 0;
> > + skb_scrub_tstamp(skb);
> > return NF_HOOK(NFPROTO_BRIDGE, NF_BR_POST_ROUTING,
> > net, sk, skb, NULL, skb->dev,
> > br_dev_queue_push_xmit);
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index a855e41bbe39..e9e7de758cba 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
Powered by blists - more mailing lists