Re: Bug#708995: iptables firewall is dropping GRO'd packets

Re: Bug#708995: iptables firewall is dropping GRO'd packets

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]

I'm seeing packet loss when forwarding from a LAN to PPP, whenever GRO
kicks in on the LAN interface.

On Mon, 2013-05-20 at 05:48 +0100, Ben Hutchings wrote:
[...]
> The Windows system is connected to the LAN interface (int0).  Turning
> off GRO on this interface works around the problem.  But since GRO is
> on by default, it clearly ought to work properly with iptables.
> 
> I'll try to work out where the drops are occurring, but the
> perf net_dropmonitor script is also broken...
[...]

I've fixed that script and now I can see that it's not iptables but
tbf_enqueue() that is dropping the GRO'd packets.  I do traffic-shaping
on the PPP link like this:

tc qdisc replace dev ppp0 root tbf rate 420kbit latency 50ms burst 1540

The local TCP will never generate an skb larger than the burst size
because it knows the PPP interface can't do GSO or TSO.  And the wifi
network doesn't seem to be fast enough for GRO to have much of an
effect.  But a peer on the wired network can trigger GRO and this
produces an skb that exceeds the burst size.

Is this a bug in sch_tbf, or should I accept it as a limitation?  It
seems like it should do GSO on entry to the queue if necessary.

Ben.

-- 
Ben Hutchings
friends: People who know you well, but like you anyway.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Bug#708995: iptables firewall is dropping GRO'd packets

[Eric Dumazet]

On Tue, 2013-05-21 at 01:28 +0100, Ben Hutchings wrote:
> I'm seeing packet loss when forwarding from a LAN to PPP, whenever GRO
> kicks in on the LAN interface.
> 
> On Mon, 2013-05-20 at 05:48 +0100, Ben Hutchings wrote:
> [...]
> > The Windows system is connected to the LAN interface (int0).  Turning
> > off GRO on this interface works around the problem.  But since GRO is
> > on by default, it clearly ought to work properly with iptables.
> > 
> > I'll try to work out where the drops are occurring, but the
> > perf net_dropmonitor script is also broken...
> [...]
> 
> I've fixed that script and now I can see that it's not iptables but
> tbf_enqueue() that is dropping the GRO'd packets.  I do traffic-shaping
> on the PPP link like this:
> 
> tc qdisc replace dev ppp0 root tbf rate 420kbit latency 50ms burst 1540
> 
> The local TCP will never generate an skb larger than the burst size
> because it knows the PPP interface can't do GSO or TSO.  And the wifi
> network doesn't seem to be fast enough for GRO to have much of an
> effect.  But a peer on the wired network can trigger GRO and this
> produces an skb that exceeds the burst size.
> 
> Is this a bug in sch_tbf, or should I accept it as a limitation?  It
> seems like it should do GSO on entry to the queue if necessary.
> 

This has been discussed on netdev this year.

Jiri Pirko was working on this. 

(thread :  tbf: take into account gso skbs)

Re: Bug#708995: iptables firewall is dropping GRO'd packets

[Eric Dumazet]

On Mon, 2013-05-20 at 17:53 -0700, Eric Dumazet wrote:
> On Tue, 2013-05-21 at 01:28 +0100, Ben Hutchings wrote:
> > I'm seeing packet loss when forwarding from a LAN to PPP, whenever GRO
> > kicks in on the LAN interface.
> > 
> > On Mon, 2013-05-20 at 05:48 +0100, Ben Hutchings wrote:
> > [...]
> > > The Windows system is connected to the LAN interface (int0).  Turning
> > > off GRO on this interface works around the problem.  But since GRO is
> > > on by default, it clearly ought to work properly with iptables.
> > > 
> > > I'll try to work out where the drops are occurring, but the
> > > perf net_dropmonitor script is also broken...
> > [...]
> > 
> > I've fixed that script and now I can see that it's not iptables but
> > tbf_enqueue() that is dropping the GRO'd packets.  I do traffic-shaping
> > on the PPP link like this:
> > 
> > tc qdisc replace dev ppp0 root tbf rate 420kbit latency 50ms burst 1540
> > 
> > The local TCP will never generate an skb larger than the burst size
> > because it knows the PPP interface can't do GSO or TSO.  And the wifi
> > network doesn't seem to be fast enough for GRO to have much of an
> > effect.  But a peer on the wired network can trigger GRO and this
> > produces an skb that exceeds the burst size.
> > 
> > Is this a bug in sch_tbf, or should I accept it as a limitation?  It
> > seems like it should do GSO on entry to the queue if necessary.
> > 
> 
> This has been discussed on netdev this year.
> 
> Jiri Pirko was working on this. 
> 
> (thread :  tbf: take into account gso skbs)

I have tested the following (net-next) patch

diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index c8388f3..a132620 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -116,14 +116,50 @@ struct tbf_sched_data {
 	struct qdisc_watchdog watchdog;	/* Watchdog timer */
 };
 
+
+static int tbf_segment(struct sk_buff *skb, struct Qdisc *sch, struct Qdisc *child)
+{
+	struct sk_buff *segs, *nskb;
+	netdev_features_t features = netif_skb_features(skb);
+	int ret, nb;
+
+	segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
+
+	if (IS_ERR_OR_NULL(segs))
+		return qdisc_reshape_fail(skb, sch);
+
+	nb = 0;
+	while (segs) {
+		nskb = segs->next;
+		segs->next = NULL;
+		qdisc_skb_cb(segs)->pkt_len = segs->len;
+
+		ret = qdisc_enqueue(segs, child);
+		if (ret != NET_XMIT_SUCCESS) {
+			if (net_xmit_drop_count(ret))
+				sch->qstats.drops++;
+		} else {
+			nb++;
+		}
+		segs = nskb;
+	}
+	sch->q.qlen += nb;
+	if (nb > 1)
+		qdisc_tree_decrease_qlen(sch, 1 - nb);
+	consume_skb(skb);
+	return nb > 0 ? NET_XMIT_SUCCESS : NET_XMIT_DROP;
+}
+
 static int tbf_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 {
 	struct tbf_sched_data *q = qdisc_priv(sch);
 	int ret;
 
-	if (qdisc_pkt_len(skb) > q->max_size)
+	if (qdisc_pkt_len(skb) > q->max_size) {
+		if (skb_is_gso(skb))
+			return tbf_segment(skb, sch, q->qdisc);
 		return qdisc_reshape_fail(skb, sch);
-
+	}
 	ret = qdisc_enqueue(skb, q->qdisc);
 	if (ret != NET_XMIT_SUCCESS) {
 		if (net_xmit_drop_count(ret))

Re: Bug#708995: iptables firewall is dropping GRO'd packets

[Jiri Pirko]

Tue, May 21, 2013 at 04:03:10AM CEST, eric.dumazet@gmail.com wrote:
>On Mon, 2013-05-20 at 17:53 -0700, Eric Dumazet wrote:
>> On Tue, 2013-05-21 at 01:28 +0100, Ben Hutchings wrote:
>> > I'm seeing packet loss when forwarding from a LAN to PPP, whenever GRO
>> > kicks in on the LAN interface.
>> > 
>> > On Mon, 2013-05-20 at 05:48 +0100, Ben Hutchings wrote:
>> > [...]
>> > > The Windows system is connected to the LAN interface (int0).  Turning
>> > > off GRO on this interface works around the problem.  But since GRO is
>> > > on by default, it clearly ought to work properly with iptables.
>> > > 
>> > > I'll try to work out where the drops are occurring, but the
>> > > perf net_dropmonitor script is also broken...
>> > [...]
>> > 
>> > I've fixed that script and now I can see that it's not iptables but
>> > tbf_enqueue() that is dropping the GRO'd packets.  I do traffic-shaping
>> > on the PPP link like this:
>> > 
>> > tc qdisc replace dev ppp0 root tbf rate 420kbit latency 50ms burst 1540
>> > 
>> > The local TCP will never generate an skb larger than the burst size
>> > because it knows the PPP interface can't do GSO or TSO.  And the wifi
>> > network doesn't seem to be fast enough for GRO to have much of an
>> > effect.  But a peer on the wired network can trigger GRO and this
>> > produces an skb that exceeds the burst size.
>> > 
>> > Is this a bug in sch_tbf, or should I accept it as a limitation?  It
>> > seems like it should do GSO on entry to the queue if necessary.
>> > 
>> 
>> This has been discussed on netdev this year.
>> 
>> Jiri Pirko was working on this. 
>> 
>> (thread :  tbf: take into account gso skbs)
>
>I have tested the following (net-next) patch

This is looking good to me. On my test machine it makes tbf work correctly
with gso packets.
I worked on similar patch (enqueue path) myself some time ago but I got
distracted by other tasks.

Do you plan to submit this patch?


>
>diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
>index c8388f3..a132620 100644
>--- a/net/sched/sch_tbf.c
>+++ b/net/sched/sch_tbf.c
>@@ -116,14 +116,50 @@ struct tbf_sched_data {
> 	struct qdisc_watchdog watchdog;	/* Watchdog timer */
> };
> 
>+
>+static int tbf_segment(struct sk_buff *skb, struct Qdisc *sch, struct Qdisc *child)
>+{
>+	struct sk_buff *segs, *nskb;
>+	netdev_features_t features = netif_skb_features(skb);
>+	int ret, nb;
>+
>+	segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
>+
>+	if (IS_ERR_OR_NULL(segs))
>+		return qdisc_reshape_fail(skb, sch);
>+
>+	nb = 0;
>+	while (segs) {
>+		nskb = segs->next;
>+		segs->next = NULL;
>+		qdisc_skb_cb(segs)->pkt_len = segs->len;
>+
>+		ret = qdisc_enqueue(segs, child);
>+		if (ret != NET_XMIT_SUCCESS) {
>+			if (net_xmit_drop_count(ret))
>+				sch->qstats.drops++;
>+		} else {
>+			nb++;
>+		}
>+		segs = nskb;
>+	}
>+	sch->q.qlen += nb;
>+	if (nb > 1)
>+		qdisc_tree_decrease_qlen(sch, 1 - nb);
>+	consume_skb(skb);
>+	return nb > 0 ? NET_XMIT_SUCCESS : NET_XMIT_DROP;
>+}
>+
> static int tbf_enqueue(struct sk_buff *skb, struct Qdisc *sch)
> {
> 	struct tbf_sched_data *q = qdisc_priv(sch);
> 	int ret;
> 
>-	if (qdisc_pkt_len(skb) > q->max_size)
>+	if (qdisc_pkt_len(skb) > q->max_size) {
>+		if (skb_is_gso(skb))
>+			return tbf_segment(skb, sch, q->qdisc);
> 		return qdisc_reshape_fail(skb, sch);
>-
>+	}
> 	ret = qdisc_enqueue(skb, q->qdisc);
> 	if (ret != NET_XMIT_SUCCESS) {
> 		if (net_xmit_drop_count(ret))
>
>
>

Re: Bug#708995: iptables firewall is dropping GRO'd packets

[Eric Dumazet]

On Tue, 2013-05-21 at 14:55 +0200, Jiri Pirko wrote:

> This is looking good to me. On my test machine it makes tbf work correctly
> with gso packets.
> I worked on similar patch (enqueue path) myself some time ago but I got
> distracted by other tasks.

Yes, I understood this.

> 
> Do you plan to submit this patch?
> 

Of course I will, when correctly tested.

[PATCH net-next] sch_tbf: segment too big GSO packets

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

If a GSO packet has a length above tbf burst limit, the packet
is currently silently dropped.

Current way to handle this is to set the device in non GSO/TSO mode, or
setting high bursts, and its sub optimal.

We can actually segment too big GSO packets, and send individual
segments as tbf parameters allow, allowing for better interoperability.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_tbf.c |   47 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index c8388f3..38008b0 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -116,14 +116,57 @@ struct tbf_sched_data {
 	struct qdisc_watchdog watchdog;	/* Watchdog timer */
 };
 
+
+/* GSO packet is too big, segment it so that tbf can transmit
+ * each segment in time
+ */
+static int tbf_segment(struct sk_buff *skb, struct Qdisc *sch)
+{
+	struct tbf_sched_data *q = qdisc_priv(sch);
+	struct sk_buff *segs, *nskb;
+	netdev_features_t features = netif_skb_features(skb);
+	int ret, nb;
+
+	segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
+
+	if (IS_ERR_OR_NULL(segs))
+		return qdisc_reshape_fail(skb, sch);
+
+	nb = 0;
+	while (segs) {
+		nskb = segs->next;
+		segs->next = NULL;
+		if (likely(segs->len <= q->max_size)) {
+			qdisc_skb_cb(segs)->pkt_len = segs->len;
+			ret = qdisc_enqueue(segs, q->qdisc);
+		} else {
+			ret = qdisc_reshape_fail(skb, sch);
+		}
+		if (ret != NET_XMIT_SUCCESS) {
+			if (net_xmit_drop_count(ret))
+				sch->qstats.drops++;
+		} else {
+			nb++;
+		}
+		segs = nskb;
+	}
+	sch->q.qlen += nb;
+	if (nb > 1)
+		qdisc_tree_decrease_qlen(sch, 1 - nb);
+	consume_skb(skb);
+	return nb > 0 ? NET_XMIT_SUCCESS : NET_XMIT_DROP;
+}
+
 static int tbf_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 {
 	struct tbf_sched_data *q = qdisc_priv(sch);
 	int ret;
 
-	if (qdisc_pkt_len(skb) > q->max_size)
+	if (qdisc_pkt_len(skb) > q->max_size) {
+		if (skb_is_gso(skb))
+			return tbf_segment(skb, sch);
 		return qdisc_reshape_fail(skb, sch);
-
+	}
 	ret = qdisc_enqueue(skb, q->qdisc);
 	if (ret != NET_XMIT_SUCCESS) {
 		if (net_xmit_drop_count(ret))

Re: [PATCH net-next] sch_tbf: segment too big GSO packets

[Jiri Pirko]

Tue, May 21, 2013 at 08:16:46PM CEST, eric.dumazet@gmail.com wrote:
>From: Eric Dumazet <edumazet@google.com>
>
>If a GSO packet has a length above tbf burst limit, the packet
>is currently silently dropped.
>
>Current way to handle this is to set the device in non GSO/TSO mode, or
>setting high bursts, and its sub optimal.
>
>We can actually segment too big GSO packets, and send individual
>segments as tbf parameters allow, allowing for better interoperability.
>
>Signed-off-by: Eric Dumazet <edumazet@google.com>
>Cc: Ben Hutchings <ben@decadent.org.uk>
>Cc: Jiri Pirko <jiri@resnulli.us>
>Cc: Jamal Hadi Salim <jhs@mojatatu.com>

Reviewed-by: Jiri Pirko <jiri@resnulli.us>


>---
> net/sched/sch_tbf.c |   47 ++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 45 insertions(+), 2 deletions(-)
>
>diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
>index c8388f3..38008b0 100644
>--- a/net/sched/sch_tbf.c
>+++ b/net/sched/sch_tbf.c
>@@ -116,14 +116,57 @@ struct tbf_sched_data {
> 	struct qdisc_watchdog watchdog;	/* Watchdog timer */
> };
> 
>+
>+/* GSO packet is too big, segment it so that tbf can transmit
>+ * each segment in time
>+ */
>+static int tbf_segment(struct sk_buff *skb, struct Qdisc *sch)
>+{
>+	struct tbf_sched_data *q = qdisc_priv(sch);
>+	struct sk_buff *segs, *nskb;
>+	netdev_features_t features = netif_skb_features(skb);
>+	int ret, nb;
>+
>+	segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
>+
>+	if (IS_ERR_OR_NULL(segs))
>+		return qdisc_reshape_fail(skb, sch);
>+
>+	nb = 0;
>+	while (segs) {
>+		nskb = segs->next;
>+		segs->next = NULL;
>+		if (likely(segs->len <= q->max_size)) {
>+			qdisc_skb_cb(segs)->pkt_len = segs->len;
>+			ret = qdisc_enqueue(segs, q->qdisc);
>+		} else {
>+			ret = qdisc_reshape_fail(skb, sch);
>+		}
>+		if (ret != NET_XMIT_SUCCESS) {
>+			if (net_xmit_drop_count(ret))
>+				sch->qstats.drops++;
>+		} else {
>+			nb++;
>+		}
>+		segs = nskb;
>+	}
>+	sch->q.qlen += nb;
>+	if (nb > 1)
>+		qdisc_tree_decrease_qlen(sch, 1 - nb);
>+	consume_skb(skb);
>+	return nb > 0 ? NET_XMIT_SUCCESS : NET_XMIT_DROP;
>+}
>+
> static int tbf_enqueue(struct sk_buff *skb, struct Qdisc *sch)
> {
> 	struct tbf_sched_data *q = qdisc_priv(sch);
> 	int ret;
> 
>-	if (qdisc_pkt_len(skb) > q->max_size)
>+	if (qdisc_pkt_len(skb) > q->max_size) {
>+		if (skb_is_gso(skb))
>+			return tbf_segment(skb, sch);
> 		return qdisc_reshape_fail(skb, sch);
>-
>+	}
> 	ret = qdisc_enqueue(skb, q->qdisc);
> 	if (ret != NET_XMIT_SUCCESS) {
> 		if (net_xmit_drop_count(ret))
>
>

Re: [PATCH net-next] sch_tbf: segment too big GSO packets

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Tue, 21 May 2013 22:39:45 +0200

> Tue, May 21, 2013 at 08:16:46PM CEST, eric.dumazet@gmail.com wrote:
>>From: Eric Dumazet <edumazet@google.com>
>>
>>If a GSO packet has a length above tbf burst limit, the packet
>>is currently silently dropped.
>>
>>Current way to handle this is to set the device in non GSO/TSO mode, or
>>setting high bursts, and its sub optimal.
>>
>>We can actually segment too big GSO packets, and send individual
>>segments as tbf parameters allow, allowing for better interoperability.
>>
>>Signed-off-by: Eric Dumazet <edumazet@google.com>
>>Cc: Ben Hutchings <ben@decadent.org.uk>
>>Cc: Jiri Pirko <jiri@resnulli.us>
>>Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> 
> Reviewed-by: Jiri Pirko <jiri@resnulli.us>

Applied, thanks everyone.