netdev - Re: [PATCH] net/sched: tbf: correct backlog statistic for GSO packets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CANn89iJ7uOuDCzErfeymGuyaP9ECqjFK5ZF9o3cuvR3+VLWfFg@mail.gmail.com>
Date: Mon, 25 Nov 2024 14:29:39 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Martin Ottens <martin.ottens@....de>
Cc: Jamal Hadi Salim <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>, 
	Jiri Pirko <jiri@...nulli.us>, "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net/sched: tbf: correct backlog statistic for GSO packets

On Mon, Nov 25, 2024 at 2:14 PM Martin Ottens <martin.ottens@....de> wrote:
>
> When the length of a GSO packet in the tbf qdisc is larger than the burst
> size configured the packet will be segmented by the tbf_segment function.
> Whenever this function is used to enqueue SKBs, the backlog statistic of
> the tbf is not increased correctly. This can lead to underflows of the
> 'backlog' byte-statistic value when these packets are dequeued from tbf.
>
> Reproduce the bug:
> Ensure that the sender machine has GSO enabled. Configured the tbf on
> the outgoing interface of the machine as follows (burstsize = 1 MTU):
> $ tc qdisc add dev <oif> root handle 1: tbf rate 50Mbit burst 1514 latency 50ms
>
> Send bulk TCP traffic out via this interface, e.g., by running an iPerf3
> client on this machine. Check the qdisc statistics:
> $ tc -s qdisc show dev <oif>
>
> The 'backlog' byte-statistic has incorrect values while traffic is
> transferred, e.g., high values due to u32 underflows. When the transfer
> is stopped, the value is != 0, which should never happen.
>
> This patch fixes this bug by updating the statistics correctly, even if
> single SKBs of a GSO SKB cannot be enqueued.
>
> Signed-off-by: Martin Ottens <martin.ottens@....de>

Please add a Fixe: tag. I think this would be

Fixes: e43ac79a4bc6 ("sch_tbf: segment too big GSO packets")

> ---
>  net/sched/sch_tbf.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
> index f1d09183ae63..ef7752f9d0d9 100644
> --- a/net/sched/sch_tbf.c
> +++ b/net/sched/sch_tbf.c
> @@ -220,17 +220,18 @@ static int tbf_segment(struct sk_buff *skb, struct Qdisc *sch,
>         skb_list_walk_safe(segs, segs, nskb) {
>                 skb_mark_not_on_list(segs);
>                 qdisc_skb_cb(segs)->pkt_len = segs->len;
> -               len += segs->len;
>                 ret = qdisc_enqueue(segs, q->qdisc, to_free);
>                 if (ret != NET_XMIT_SUCCESS) {
>                         if (net_xmit_drop_count(ret))
>                                 qdisc_qstats_drop(sch);
>                 } else {
>                         nb++;
> +                       len += segs->len;

I do not think it is safe to access segs->len after qdisc_enqueue() :
We lost ownership of segs already.

I would store the segs->len in a temporary variable before calling
qdisc_enqueue()

>                 }
>         }
>         sch->q.qlen += nb;
> -       if (nb > 1)
> +       sch->qstats.backlog += len;
> +       if (nb > 0)
>                 qdisc_tree_reduce_backlog(sch, 1 - nb, prev_len - len);
>         consume_skb(skb);

We might also call kfree_skb(skb) instead of consume_skb() if nb == 0

>         return nb > 0 ? NET_XMIT_SUCCESS : NET_XMIT_DROP;
> --
> 2.39.5
>