From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [PATCH v2 net-next 01/14] net: Clear skb->tstamp only on the forwarding path Date: Mon, 16 Jul 2018 16:15:28 -0700 Message-ID: <463024c6-e085-ea88-688b-b42df679876a@gmail.com> References: <20180703224300.25300-1-jesus.sanchez-palencia@intel.com> <20180703224300.25300-2-jesus.sanchez-palencia@intel.com> <1e52c128-59f4-43ae-3487-059a84ae61c3@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: tglx@linutronix.de, jan.altenberg@linutronix.de, vinicius.gomes@intel.com, kurt.kanzenbach@linutronix.de, henrik@austad.us, richardcochran@gmail.com, ilias.apalodimas@linaro.org, ivan.khoronzhuk@linaro.org, mlichvar@redhat.com, willemb@google.com, jhs@mojatatu.com, xiyou.wangcong@gmail.com, jiri@resnulli.us, jeffrey.t.kirsher@intel.com To: Jesus Sanchez-Palencia , Eric Dumazet , netdev@vger.kernel.org Return-path: Received: from mail-pl0-f67.google.com ([209.85.160.67]:39025 "EHLO mail-pl0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728947AbeGPXpH (ORCPT ); Mon, 16 Jul 2018 19:45:07 -0400 Received: by mail-pl0-f67.google.com with SMTP id p23-v6so4661062plo.6 for ; Mon, 16 Jul 2018 16:15:30 -0700 (PDT) In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 07/16/2018 02:52 PM, Jesus Sanchez-Palencia wrote: > Hi Eric, > > > > On 07/13/2018 10:35 AM, Eric Dumazet wrote: >> >> >> On 07/03/2018 03:42 PM, Jesus Sanchez-Palencia wrote: >>> This is done in preparation for the upcoming time based transmission >>> patchset. Now that skb->tstamp will be used to hold packet's txtime, >>> we must ensure that it is being cleared when traversing namespaces. >>> Also, doing that from skb_scrub_packet() before the early return would >>> break our feature when tunnels are used. >>> >>> Signed-off-by: Jesus Sanchez-Palencia >>> --- >>> net/core/skbuff.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >>> index 1357f36c8a5e..c4e24ac27464 100644 >>> --- a/net/core/skbuff.c >>> +++ b/net/core/skbuff.c >>> @@ -4898,7 +4898,6 @@ EXPORT_SYMBOL(skb_try_coalesce); >>> */ >>> void skb_scrub_packet(struct sk_buff *skb, bool xnet) >>> { >>> - skb->tstamp = 0; >>> skb->pkt_type = PACKET_HOST; >>> skb->skb_iif = 0; >>> skb->ignore_df = 0; >>> @@ -4912,6 +4911,7 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet) >>> >>> ipvs_reset(skb); >>> skb->mark = 0; >>> + skb->tstamp = 0; >>> } >>> EXPORT_SYMBOL_GPL(skb_scrub_packet); >>> >>> >> >> >> >> I believe we had some misunderstanding here. >> >> What I meant by forwarding is the following case : >> >> - We receive a packet. >> - netstamp_wanted is >0 (because at least one packet capture is active) >> - __net_timestamp() is called and does : >> skb->tstamp = ktime_get_real(); >> >> Then this skb is forwarded into an interface where EDT is taken into >> consideration by either a qdisc or a device. >> >> Since CLOCK_TAI is a different base than CLOCK_REALTIME, we might have a problem. > > > I'm not sure we have a problem here. For the Tx path I only see > net_timestamp_set() being called from dev_queue_xmit_nit(). And even there, it's > a clone of the skb that gets timestamped. > > I believe the original skb, which had the valid txtime copied into skb->tstamp, > is not modified anywhere along that path. > > What am I missing, please? > > Thanks, > Jesus > I am simply stating that a linux router, receiving packet on ethX and forwarding them on ethY, could have a problem if ethY has a qdisc looking at skb->tstamp assuming a timestamp in CLOCK_TAI base. In this case, skb->tstamp would have been set at ingress (not using CLOCK_TAI but CLOCK_REALTIME), and would be read at egress (assuming CLOCK_TAI) Normal IPV4 routing path would be in net/ipv4/ip_forward.c, no scrubbing ever happens, and no cloning either. Your patch (Clear skb->tstamp only on the forwarding path) is not handling the typical forward path, only the cases where 'scrubbing' is used. > > >> >> >> Solutions for this problem : >> >> 1) Convert all our skb->tstamp usages to CLOCK_TAI base. >> >> or >> >> 2) clear skb->tstamp in forwarding paths, including the ones not scrubbing the packet. >> >> My preference is 1), even if it is a bit more work. >>