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]
Message-ID: <20090416214846.GA9375@ami.dom.local>
Date:	Thu, 16 Apr 2009 23:48:46 +0200
From:	Jarek Poplawski <jarkao2@...il.com>
To:	David Miller <davem@...emloft.net>
Cc:	alexandre.sidorenko@...com, netdev@...r.kernel.org,
	Stephen Hemminger <shemminger@...tta.com>
Subject: Re: An inconsistency/bug in ingress netem timestamps

David Miller wrote, On 04/16/2009 12:10 PM:

> From: Alex Sidorenko <alexandre.sidorenko@...com>
> Date: Wed, 15 Apr 2009 16:10:43 -0400
> 
>> On April 15, 2009 03:50:22 pm Jarek Poplawski wrote:
>>
>>> Isn't it when act_mirred calls dev_queue_xmit with dev_queue_xmit_nit?
>>> But, as above mentioned, I doubt it's "updated properly" in this case.
>> I can see that dev_queue_xmit_nit calls net_timestamp(skb) unconditionally. I 
>> agree that to fix this properly we need to update tstamp in another place 
>> explicitly (in ifb or netem?).
> 
> Since IFB completely bypasses netif_rx() and netif_receive_skb() I
> think it should unconditionally set skb->tstamp.tv64 to zero and
> invoke net_timestamp()

IFB calls netif_rx() and I don't understand why do we need to update
tstamp again except for this netem case.

> This would match the behavior of loopback and tunnels, and in my
> opinion this is reasonable.  There will be virtually no overhead
> added unless timestamping is enabled via ping or similar, and in
> return we get what I think is correctness :-)

I think we need some consistency in counting or not counting packet
scheduling delays into timestamps. Anyway we should avoid unnecessary
updates like now, so I'm proposing something different (for testing).

Jarek P.
---

 net/core/dev.c        |    5 +++++
 net/sched/sch_netem.c |    8 ++++++++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 91d792d..ca740c0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1336,7 +1336,12 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct packet_type *ptype;
 
+#ifdef CONFIG_NET_CLS_ACT
+	if (!(skb->tstamp.tv64 && (G_TC_FROM(skb->tc_verd) & AT_INGRESS)))
+		net_timestamp(skb);
+#else
 	net_timestamp(skb);
+#endif
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(ptype, &ptype_all, list) {
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index d876b87..2b88295 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -280,6 +280,14 @@ static struct sk_buff *netem_dequeue(struct Qdisc *sch)
 			if (unlikely(!skb))
 				return NULL;
 
+#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
 			pr_debug("netem_dequeue: return skb=%p\n", skb);
 			sch->q.qlen--;
 			return skb;
--
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ