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

[Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>]

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



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