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:   Wed, 18 Jul 2018 11:19:16 -0700
From:   Jesus Sanchez-Palencia <jesus.sanchez-palencia@...el.com>
To:     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

Hi Eric,


On 07/16/2018 04:15 PM, Eric Dumazet wrote:
> 
> 
> 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.


Thanks for the clarification, I wasn't following you before.

I believe we're fine with what we have *today*, because the qdisc will drop skbs
that don't have a valid skb->sk, or if the SO_TXTIME flags is not set.

This is a problem, however, if another qdisc is developed tomorrow and uses the
same information but do not perform the same checks. Or, if the packet somehow
gets to the controller and the HW has the "launch time" feature enabled and the
driver uses the tstamp information the same way we do.

If we want to protect from that now, I think we should go with the most
immediate solution and clear skb->tstamp somewhere in ip_forward(), as you've
pointed before. It seems to me this wouldn't break any other feature that might
be re-using the tstamp information.

This is also a more correct fix for the problem you raised before:

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


IMHO this Rx tstamp should never be forwarded to the Tx path as a valid txtime,
regardless if the time base used was UTC or TAI.


What do you think?

Jesus






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