netdev - Re: [PATCH net-next-2.6] net: force a fresh timestamp for ingress packets

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:	Fri, 17 Dec 2010 07:30:15 +0000
From:	Jarek Poplawski <jarkao2@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	David Miller <davem@...emloft.net>,
	Changli Gao <xiaosuo@...il.com>,
	netdev <netdev@...r.kernel.org>,
	Patrick McHardy <kaber@...sh.net>
Subject: Re: [PATCH net-next-2.6] net: force a fresh timestamp for ingress
	packets

On Fri, Dec 17, 2010 at 12:31:45AM +0100, Eric Dumazet wrote:
> Le jeudi 16 décembre 2010 ?? 23:42 +0100, Jarek Poplawski a écrit :
> > On Thu, Dec 16, 2010 at 11:26:03PM +0100, Eric Dumazet wrote:
> > > Le jeudi 16 décembre 2010 ?? 23:08 +0100, Jarek Poplawski a écrit :
> > > 
> > > > Hmm... Do you expect more people start debugging SFQ or I missed your
> > > > point? ;-) Maybe such a change would be reasonable on a cloned skb?
> > > 
> > > My point was to permit an admin to check his ingress shaping works or
> > > not. In this respect, netem was a specialization of a general problem.
> > > 
> > > We had a prior discussion on timestamping skb in RX path, when RPS came
> > > in : Should we take timestamp before RPS or after. At that time we added
> > > a sysctl. Not sure it was the right choice :-(
> > > 
> > > I feel tcpdump (on TX side) should really display time of packet right
> > > before being given to device, but this is probably a matter of taste.
> > 
> > It might be reasonable unless it changes data for later users. That's
> > why I mentioned cloning.
> > 
> > > 
> > > Before commit 8caf153974f2 (net: sch_netem: Fix an inconsistency in
> > > ingress netem timestamps.), this is what was done.
> > 
> > Then why don't you try to let turn it off in netem, where it only
> > matters?

Forget this advice - I lost the point how you tested that.

> Because, if you read again my patch, you'll see :
> 
> #ifdef CONFIG_NET_CLS_ACT
>        if (!skb->tstamp.tv64 || (G_TC_FROM(skb->tc_verd) & AT_INGRESS))
>                 net_timestamp_set(skb);
> #else
> 
> So : 
> 
> If we are handling an INGRESS packet, we force a timestamp renew.

It is wrong because it brings back the inconsistency with ping etc.
described by Alex Sidorenko in the changelog of netem patch, but
use normal (not netem) ingress scheduling (ping results shouldn't
depend on sniffers).

> 
> Therefore :
> 
> We dont need in netem_dequeue() to force :
> 
> -#ifdef CONFIG_NET_CLS_ACT
> -                       /*
> -                        * If it's at ingress let's pretend the delay is
> -                        * from the network (tstamp will be updated).
> -                        */
> -                       if (G_TC_FROM(skb->tc_verd) & AT_INGRESS)
> -                               skb->tstamp.tv64 = 0;
> -#endif
> 
> Since :
> 
> We are going to renew timestamp anyway.
> 
> Conclusion :
> 
> I am eliminating dead code.
> 
> Is that OK ?

Not to me.

Thanks,
Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html