[PATCH net v2 0/2] net: netem: fix further issues with packet corruption

[PATCH net v2 0/2] net: netem: fix further issues with packet corruption

[Jakub Kicinski]

Hi!

This set is fixing two more issues with the netem packet corruption.

First patch (which was previously posted) avoids NULL pointer dereference
if the first frame gets freed due to allocation or checksum failure.
v2 improves the clarity of the code a little as requested by Cong.

Second patch ensures we don't return SUCCESS if the frame was in fact
dropped. Thanks to this commit message for patch 1 no longer needs the
"this will still break with a single-frame failure" disclaimer.

Jakub Kicinski (2):
  net: netem: fix error path for corrupted GSO frames
  net: netem: correct the parent's backlog when corrupted packet was
    dropped

 net/sched/sch_netem.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

-- 
2.23.0

[PATCH net v2 1/2] net: netem: fix error path for corrupted GSO frames

[Jakub Kicinski]

To corrupt a GSO frame we first perform segmentation.  We then
proceed using the first segment instead of the full GSO skb and
requeue the rest of the segments as separate packets.

If there are any issues with processing the first segment we
still want to process the rest, therefore we jump to the
finish_segs label.

Commit 177b8007463c ("net: netem: fix backlog accounting for
corrupted GSO frames") started using the pointer to the first
segment in the "rest of segments processing", but as mentioned
above the first segment may had already been freed at this point.

Backlog corrections for parent qdiscs have to be adjusted.

Fixes: 177b8007463c ("net: netem: fix backlog accounting for corrupted GSO frames")
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Reported-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 net/sched/sch_netem.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 0e44039e729c..942eb17f413c 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -509,6 +509,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		if (skb->ip_summed == CHECKSUM_PARTIAL &&
 		    skb_checksum_help(skb)) {
 			qdisc_drop(skb, sch, to_free);
+			skb = NULL;
 			goto finish_segs;
 		}
 
@@ -593,9 +594,10 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 finish_segs:
 	if (segs) {
 		unsigned int len, last_len;
-		int nb = 0;
+		int nb;
 
-		len = skb->len;
+		len = skb ? skb->len : 0;
+		nb = skb ? 1 : 0;
 
 		while (segs) {
 			skb2 = segs->next;
@@ -612,7 +614,8 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			}
 			segs = skb2;
 		}
-		qdisc_tree_reduce_backlog(sch, -nb, prev_len - len);
+		/* Parent qdiscs accounted for 1 skb of size @prev_len */
+		qdisc_tree_reduce_backlog(sch, -(nb - 1), -(len - prev_len));
 	}
 	return NET_XMIT_SUCCESS;
 }
-- 
2.23.0

[PATCH net v2 2/2] net: netem: correct the parent's backlog when corrupted packet was dropped

[Jakub Kicinski]

If packet corruption failed we jump to finish_segs and return
NET_XMIT_SUCCESS. Seeing success will make the parent qdisc
increment its backlog, that's incorrect - we need to return
NET_XMIT_DROP.

Fixes: 6071bd1aa13e ("netem: Segment GSO packets on enqueue")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 net/sched/sch_netem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 942eb17f413c..42e557d48e4e 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -616,6 +616,8 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		}
 		/* Parent qdiscs accounted for 1 skb of size @prev_len */
 		qdisc_tree_reduce_backlog(sch, -(nb - 1), -(len - prev_len));
+	} else if (!skb) {
+		return NET_XMIT_DROP;
 	}
 	return NET_XMIT_SUCCESS;
 }
-- 
2.23.0

Re: [PATCH net v2 0/2] net: netem: fix further issues with packet corruption

[Stephen Hemminger]

On Fri, 18 Oct 2019 09:16:56 -0700
Jakub Kicinski <jakub.kicinski@netronome.com> wrote:

> Hi!
> 
> This set is fixing two more issues with the netem packet corruption.
> 
> First patch (which was previously posted) avoids NULL pointer dereference
> if the first frame gets freed due to allocation or checksum failure.
> v2 improves the clarity of the code a little as requested by Cong.
> 
> Second patch ensures we don't return SUCCESS if the frame was in fact
> dropped. Thanks to this commit message for patch 1 no longer needs the
> "this will still break with a single-frame failure" disclaimer.
> 
> Jakub Kicinski (2):
>   net: netem: fix error path for corrupted GSO frames
>   net: netem: correct the parent's backlog when corrupted packet was
>     dropped
> 
>  net/sched/sch_netem.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 


LGTM

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Re: [PATCH net v2 0/2] net: netem: fix further issues with packet corruption

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Fri, 18 Oct 2019 09:16:56 -0700

> This set is fixing two more issues with the netem packet corruption.
> 
> First patch (which was previously posted) avoids NULL pointer dereference
> if the first frame gets freed due to allocation or checksum failure.
> v2 improves the clarity of the code a little as requested by Cong.
> 
> Second patch ensures we don't return SUCCESS if the frame was in fact
> dropped. Thanks to this commit message for patch 1 no longer needs the
> "this will still break with a single-frame failure" disclaimer.

Series applied and queued up for -stable, thanks.