| Message ID | 20241125231825.2586179-1-martin.ottens@fau.de (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/sched: netem: account for backlog updates from child qdisc | expand |
On Tue, 26 Nov 2024 00:18:25 +0100 Martin Ottens <martin.ottens@fau.de> wrote: > When netem is used with a child qdisc, the child qdisc can use > 'qdisc_tree_reduce_backlog' to inform its parent, netem, about created or > dropped SKBs. This function updates 'qlen' and the backlog statistics of > netem, but netem does not account for changes made by a child qdisc. If a > child qdisc creates new SKBs during enqueue and informs its parent about > this, netem's 'qlen' value is increased. When netem dequeues the newly > created SKBs from the child, the 'qlen' in netem is not updated. If 'qlen' > reaches the configured limit, the enqueue function stops working, even > though the tfifo is not full. > > Reproduce the bug: > Ensure that the sender machine has GSO enabled. Configure netem as root > qdisc and tbf as its child on the outgoing interface of the machine > as follows: > $ tc qdisc add dev <oif> root handle 1: netem delay 100ms limit 100 > $ tc qdisc add dev <oif> parent 1:0 tbf rate 50Mbit burst 1542 latency 50ms > > Send bulk TCP traffic out via this interface, e.g., by running an iPerf3 > client on the machine. Check the qdisc statistics: > $ tc -s qdisc show dev <oif> > > tbf segments the GSO SKBs (tbf_segment) and updates the netem's 'qlen'. > The interface fully stops transferring packets and "locks". In this case, > the child qdisc and tfifo are empty, but 'qlen' indicates the tfifo is at > its limit and no more packets are accepted. > > This patch adds a counter for the entries in the tfifo. Netem's 'qlen' is > only decreased when a packet is returned by its dequeue function, and not > during enqueuing into the child qdisc. External updates to 'qlen' are thus > accounted for and only the behavior of the backlog statistics changes. > This statistics now show the total number/length of SKBs held in the tfifo > and in the child qdisc. (Note: the 'backlog' byte-statistic of netem is > incorrect in the example above even after the patch is applied due to a > bug in tbf. See my previous patch ([PATCH] net/sched: tbf: correct backlog > statistic for GSO packets)). > > Signed-off-by: Martin Ottens <martin.ottens@fau.de> Does this also address this issue with running non-work conserving inner qdisc? See: https://marc.info/?l=linux-netdev&m=172779429511319&w=2
On 26.11.24 01:12, Stephen Hemminger wrote: > Does this also address this issue with running non-work conserving > inner qdisc? > > See: https://marc.info/?l=linux-netdev&m=172779429511319&w=2 The bug that my patch fixes only occurs when netem interacts with an inner qdisc like tbf, the behavior when netem interacts with its parent qdiscs is not changed by the patch. Therefore the patch does not address the UaF bug.
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c index fe6fed291a7b..71ec9986ed37 100644 --- a/net/sched/sch_netem.c +++ b/net/sched/sch_netem.c @@ -79,6 +79,8 @@ struct netem_sched_data { struct sk_buff *t_head; struct sk_buff *t_tail; + u32 t_len; + /* optional qdisc for classful handling (NULL at netem init) */ struct Qdisc *qdisc; @@ -383,6 +385,7 @@ static void tfifo_reset(struct Qdisc *sch) rtnl_kfree_skbs(q->t_head, q->t_tail); q->t_head = NULL; q->t_tail = NULL; + q->t_len = 0; } static void tfifo_enqueue(struct sk_buff *nskb, struct Qdisc *sch) @@ -412,6 +415,7 @@ static void tfifo_enqueue(struct sk_buff *nskb, struct Qdisc *sch) rb_link_node(&nskb->rbnode, parent, p); rb_insert_color(&nskb->rbnode, &q->t_root); } + q->t_len++; sch->q.qlen++; } @@ -518,7 +522,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch, 1<<get_random_u32_below(8); } - if (unlikely(sch->q.qlen >= sch->limit)) { + if (unlikely(q->t_len >= sch->limit)) { /* re-link segs, so that qdisc_drop_all() frees them all */ skb->next = segs; qdisc_drop_all(skb, sch, to_free); @@ -702,8 +706,8 @@ static struct sk_buff *netem_dequeue(struct Qdisc *sch) tfifo_dequeue: skb = __qdisc_dequeue_head(&sch->q); if (skb) { - qdisc_qstats_backlog_dec(sch, skb); deliver: + qdisc_qstats_backlog_dec(sch, skb); qdisc_bstats_update(sch, skb); return skb; } @@ -719,8 +723,7 @@ static struct sk_buff *netem_dequeue(struct Qdisc *sch) if (time_to_send <= now && q->slot.slot_next <= now) { netem_erase_head(q, skb); - sch->q.qlen--; - qdisc_qstats_backlog_dec(sch, skb); + q->t_len--; skb->next = NULL; skb->prev = NULL; /* skb->dev shares skb->rbnode area, @@ -747,16 +750,21 @@ static struct sk_buff *netem_dequeue(struct Qdisc *sch) if (net_xmit_drop_count(err)) qdisc_qstats_drop(sch); qdisc_tree_reduce_backlog(sch, 1, pkt_len); + sch->qstats.backlog -= pkt_len; + sch->q.qlen--; } goto tfifo_dequeue; } + sch->q.qlen--; goto deliver; } if (q->qdisc) { skb = q->qdisc->ops->dequeue(q->qdisc); - if (skb) + if (skb) { + sch->q.qlen--; goto deliver; + } } qdisc_watchdog_schedule_ns(&q->watchdog, @@ -766,8 +774,10 @@ static struct sk_buff *netem_dequeue(struct Qdisc *sch) if (q->qdisc) { skb = q->qdisc->ops->dequeue(q->qdisc); - if (skb) + if (skb) { + sch->q.qlen--; goto deliver; + } } return NULL; }
When netem is used with a child qdisc, the child qdisc can use 'qdisc_tree_reduce_backlog' to inform its parent, netem, about created or dropped SKBs. This function updates 'qlen' and the backlog statistics of netem, but netem does not account for changes made by a child qdisc. If a child qdisc creates new SKBs during enqueue and informs its parent about this, netem's 'qlen' value is increased. When netem dequeues the newly created SKBs from the child, the 'qlen' in netem is not updated. If 'qlen' reaches the configured limit, the enqueue function stops working, even though the tfifo is not full. Reproduce the bug: Ensure that the sender machine has GSO enabled. Configure netem as root qdisc and tbf as its child on the outgoing interface of the machine as follows: $ tc qdisc add dev <oif> root handle 1: netem delay 100ms limit 100 $ tc qdisc add dev <oif> parent 1:0 tbf rate 50Mbit burst 1542 latency 50ms Send bulk TCP traffic out via this interface, e.g., by running an iPerf3 client on the machine. Check the qdisc statistics: $ tc -s qdisc show dev <oif> tbf segments the GSO SKBs (tbf_segment) and updates the netem's 'qlen'. The interface fully stops transferring packets and "locks". In this case, the child qdisc and tfifo are empty, but 'qlen' indicates the tfifo is at its limit and no more packets are accepted. This patch adds a counter for the entries in the tfifo. Netem's 'qlen' is only decreased when a packet is returned by its dequeue function, and not during enqueuing into the child qdisc. External updates to 'qlen' are thus accounted for and only the behavior of the backlog statistics changes. This statistics now show the total number/length of SKBs held in the tfifo and in the child qdisc. (Note: the 'backlog' byte-statistic of netem is incorrect in the example above even after the patch is applied due to a bug in tbf. See my previous patch ([PATCH] net/sched: tbf: correct backlog statistic for GSO packets)). Signed-off-by: Martin Ottens <martin.ottens@fau.de> --- net/sched/sch_netem.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)