| Message ID | 20191016160050.27703-1-jakub.kicinski@netronome.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: netem: fix error path for corrupted GSO frames | expand |
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"?
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.. :(
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.
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; }
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.
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; }
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(-)