* [PATCH net] net: netem: fix error path for corrupted GSO frames
@ 2019-10-16 16:00 Jakub Kicinski
2019-10-16 22:42 ` Cong Wang
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2019-10-16 16:00 UTC (permalink / raw)
To: davem
Cc: netdev, oss-drivers, stephen, Jakub Kicinski, kbuild test robot,
Dan Carpenter, Ben Hutchings, Simon Horman
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.
Note that if segmentation ever produced a single-skb list
the backlog calculation will not take place (segs == NULL)
but that should hopefully never happen..
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 | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 0e44039e729c..31a6afd035b2 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;
}
@@ -595,7 +596,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
unsigned int len, last_len;
int nb = 0;
- len = skb->len;
+ len = skb ? skb->len : 0;
while (segs) {
skb2 = segs->next;
@@ -612,7 +613,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
}
segs = skb2;
}
- qdisc_tree_reduce_backlog(sch, -nb, prev_len - len);
+ qdisc_tree_reduce_backlog(sch, !skb - nb, prev_len - len);
}
return NET_XMIT_SUCCESS;
}
--
2.23.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net] net: netem: fix error path for corrupted GSO frames 2019-10-16 16:00 [PATCH net] net: netem: fix error path for corrupted GSO frames Jakub Kicinski @ 2019-10-16 22:42 ` Cong Wang 2019-10-16 23:22 ` Jakub Kicinski 0 siblings, 1 reply; 6+ messages in thread From: Cong Wang @ 2019-10-16 22:42 UTC (permalink / raw) To: Jakub Kicinski Cc: David Miller, Linux Kernel Network Developers, oss-drivers, Stephen Hemminger, kbuild test robot, Dan Carpenter, Ben Hutchings, Simon Horman On Wed, Oct 16, 2019 at 3:23 PM Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > 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. > Note that if segmentation ever produced a single-skb list > the backlog calculation will not take place (segs == NULL) > but that should hopefully never happen.. > > 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 | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c > index 0e44039e729c..31a6afd035b2 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; > } > > @@ -595,7 +596,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch, > unsigned int len, last_len; > int nb = 0; > > - len = skb->len; > + len = skb ? skb->len : 0; > > while (segs) { > skb2 = segs->next; > @@ -612,7 +613,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch, > } > segs = skb2; > } > - qdisc_tree_reduce_backlog(sch, -nb, prev_len - len); > + qdisc_tree_reduce_backlog(sch, !skb - nb, prev_len - len); Am I the only one has trouble to understand the expression "!skb - nb"? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: netem: fix error path for corrupted GSO frames 2019-10-16 22:42 ` Cong Wang @ 2019-10-16 23:22 ` Jakub Kicinski 2019-10-17 18:10 ` Cong Wang 0 siblings, 1 reply; 6+ messages in thread From: Jakub Kicinski @ 2019-10-16 23:22 UTC (permalink / raw) To: Cong Wang Cc: David Miller, Linux Kernel Network Developers, oss-drivers, Stephen Hemminger, kbuild test robot, Dan Carpenter, Ben Hutchings, Simon Horman On Wed, 16 Oct 2019 15:42:28 -0700, Cong Wang wrote: > > @@ -612,7 +613,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch, > > } > > segs = skb2; > > } > > - qdisc_tree_reduce_backlog(sch, -nb, prev_len - len); > > + qdisc_tree_reduce_backlog(sch, !skb - nb, prev_len - len); > > Am I the only one has trouble to understand the expression > "!skb - nb"? The backward logic of qdisc_tree_reduce_backlog() always gives me a pause :S Is -nb + !skb any better? The point is we have a "credit" for the "head" skb we dropped. If we didn't manage to queue any of the segs then the expression becomes ...reduce_backlog(sch, 1, prev_len) basically cleaning up after the head. My knee jerk reaction was -> we should return DROP if head got dropped, but that just makes things more nasty because we requeue the segs directly into netem so if we say DROP we have to special case all the segs which succeeded, that gets even more hairy. I'm open to suggestions.. :( ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: netem: fix error path for corrupted GSO frames 2019-10-16 23:22 ` Jakub Kicinski @ 2019-10-17 18:10 ` Cong Wang 2019-10-17 18:44 ` Jakub Kicinski 0 siblings, 1 reply; 6+ messages in thread From: Cong Wang @ 2019-10-17 18:10 UTC (permalink / raw) To: Jakub Kicinski Cc: David Miller, Linux Kernel Network Developers, oss-drivers, Stephen Hemminger, kbuild test robot, Dan Carpenter, Ben Hutchings, Simon Horman On Wed, Oct 16, 2019 at 4:22 PM Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Wed, 16 Oct 2019 15:42:28 -0700, Cong Wang wrote: > > > @@ -612,7 +613,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch, > > > } > > > segs = skb2; > > > } > > > - qdisc_tree_reduce_backlog(sch, -nb, prev_len - len); > > > + qdisc_tree_reduce_backlog(sch, !skb - nb, prev_len - len); > > > > Am I the only one has trouble to understand the expression > > "!skb - nb"? > > The backward logic of qdisc_tree_reduce_backlog() always gives me a > pause :S Yeah, reducing with a negative value is actually an add. Feel free to add a wrapper for this if you think it is better. > > Is > -nb + !skb > any better? I don't see how they are different. :-/ > > The point is we have a "credit" for the "head" skb we dropped. If we > didn't manage to queue any of the segs then the expression becomes > ...reduce_backlog(sch, 1, prev_len) basically cleaning up after the > head. > > My knee jerk reaction was -> we should return DROP if head got dropped, > but that just makes things more nasty because we requeue the segs > directly into netem so if we say DROP we have to special case all the > segs which succeeded, that gets even more hairy. Hmm? My understanding is that !skb is either 0 or 1, so you end up with either "-nb" or "1 - nb". The formal is easy to understand, while the later is harder as I don't see why you need to plus 1. > > I'm open to suggestions.. :( Why not write the code in a more readable way, for instance with the :? operator? And, adding a comment in the code? Thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: netem: fix error path for corrupted GSO frames 2019-10-17 18:10 ` Cong Wang @ 2019-10-17 18:44 ` Jakub Kicinski 2019-10-17 19:28 ` Cong Wang 0 siblings, 1 reply; 6+ messages in thread From: Jakub Kicinski @ 2019-10-17 18:44 UTC (permalink / raw) To: Cong Wang Cc: David Miller, Linux Kernel Network Developers, oss-drivers, Stephen Hemminger, kbuild test robot, Dan Carpenter, Ben Hutchings, Simon Horman On Thu, 17 Oct 2019 11:10:06 -0700, Cong Wang wrote: > On Wed, Oct 16, 2019 at 4:22 PM Jakub Kicinski wrote: > > On Wed, 16 Oct 2019 15:42:28 -0700, Cong Wang wrote: > > > > @@ -612,7 +613,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch, > > > > } > > > > segs = skb2; > > > > } > > > > - qdisc_tree_reduce_backlog(sch, -nb, prev_len - len); > > > > + qdisc_tree_reduce_backlog(sch, !skb - nb, prev_len - len); > > > > > > Am I the only one has trouble to understand the expression > > > "!skb - nb"? > > > > The backward logic of qdisc_tree_reduce_backlog() always gives me a > > pause :S > > Yeah, reducing with a negative value is actually an add. Feel free > to add a wrapper for this if you think it is better. I was avoiding adding the wrapper due to stable, but perhaps it should be okay. How does this look? --->8-------------- diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index 637548d54b3e..66c2dc6a4742 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -630,6 +630,13 @@ void qdisc_reset(struct Qdisc *qdisc); void qdisc_put(struct Qdisc *qdisc); void qdisc_put_unlocked(struct Qdisc *qdisc); void qdisc_tree_reduce_backlog(struct Qdisc *qdisc, int n, int len); + +static inline void +qdisc_tree_increase_backlog(struct Qdisc *qdisc, int n, int len) +{ + qdisc_tree_reduce_backlog(qdisc, -n, -len); +} + #ifdef CONFIG_NET_SCHED int qdisc_offload_dump_helper(struct Qdisc *q, enum tc_setup_type type, void *type_data); diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c index 0e44039e729c..e22c13b56bfc 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; } @@ -592,10 +593,13 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch, finish_segs: if (segs) { - unsigned int len, last_len; + unsigned int last_len, len = 0; int nb = 0; - len = skb->len; + if (skb) { + len += skb->len; + nb++; + } while (segs) { skb2 = segs->next; @@ -612,7 +616,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_increase_backlog(sch, nb - 1, len - prev_len); } return NET_XMIT_SUCCESS; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: netem: fix error path for corrupted GSO frames 2019-10-17 18:44 ` Jakub Kicinski @ 2019-10-17 19:28 ` Cong Wang 0 siblings, 0 replies; 6+ messages in thread From: Cong Wang @ 2019-10-17 19:28 UTC (permalink / raw) To: Jakub Kicinski Cc: David Miller, Linux Kernel Network Developers, oss-drivers, Stephen Hemminger, kbuild test robot, Dan Carpenter, Ben Hutchings, Simon Horman On Thu, Oct 17, 2019 at 11:44 AM Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Thu, 17 Oct 2019 11:10:06 -0700, Cong Wang wrote: > > On Wed, Oct 16, 2019 at 4:22 PM Jakub Kicinski wrote: > > > On Wed, 16 Oct 2019 15:42:28 -0700, Cong Wang wrote: > > > > > @@ -612,7 +613,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch, > > > > > } > > > > > segs = skb2; > > > > > } > > > > > - qdisc_tree_reduce_backlog(sch, -nb, prev_len - len); > > > > > + qdisc_tree_reduce_backlog(sch, !skb - nb, prev_len - len); > > > > > > > > Am I the only one has trouble to understand the expression > > > > "!skb - nb"? > > > > > > The backward logic of qdisc_tree_reduce_backlog() always gives me a > > > pause :S > > > > Yeah, reducing with a negative value is actually an add. Feel free > > to add a wrapper for this if you think it is better. > > I was avoiding adding the wrapper due to stable, but perhaps it should > be okay. Up to you, you can defer the wrapper to net-next. > > How does this look? "nb - 1" with a comment is much better than "!skb - nb". Thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-17 19:28 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-16 16:00 [PATCH net] net: netem: fix error path for corrupted GSO frames Jakub Kicinski 2019-10-16 22:42 ` Cong Wang 2019-10-16 23:22 ` Jakub Kicinski 2019-10-17 18:10 ` Cong Wang 2019-10-17 18:44 ` Jakub Kicinski 2019-10-17 19:28 ` Cong Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).