netdev - Re: [PATCH v2 net-next 01/14] net: Clear skb->tstamp only on the forwarding path

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]

Date:   Mon, 16 Jul 2018 16:15:28 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Jesus Sanchez-Palencia <jesus.sanchez-palencia@...el.com>,
        Eric Dumazet <eric.dumazet@...il.com>, netdev@...r.kernel.org
Cc:     tglx@...utronix.de, jan.altenberg@...utronix.de,
        vinicius.gomes@...el.com, kurt.kanzenbach@...utronix.de,
        henrik@...tad.us, richardcochran@...il.com,
        ilias.apalodimas@...aro.org, ivan.khoronzhuk@...aro.org,
        mlichvar@...hat.com, willemb@...gle.com, jhs@...atatu.com,
        xiyou.wangcong@...il.com, jiri@...nulli.us,
        jeffrey.t.kirsher@...el.com
Subject: Re: [PATCH v2 net-next 01/14] net: Clear skb->tstamp only on the
 forwarding path



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 <jesus.sanchez-palencia@...el.com>
>>> ---
>>>  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.
>>