netdev - Re: [PATCH net] net: netem: fix error path for corrupted GSO frames

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 16 Oct 2019 15:42:28 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     David Miller <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        oss-drivers@...ronome.com,
        Stephen Hemminger <stephen@...workplumber.org>,
        kbuild test robot <lkp@...el.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Ben Hutchings <ben@...adent.org.uk>,
        Simon Horman <simon.horman@...ronome.com>
Subject: Re: [PATCH net] net: netem: fix error path for corrupted GSO frames

On Wed, Oct 16, 2019 at 3:23 PM Jakub Kicinski
<jakub.kicinski@...ronome.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@...el.com>
> Reported-by: Dan Carpenter <dan.carpenter@...cle.com>
> Reported-by: Ben Hutchings <ben@...adent.org.uk>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Reviewed-by: Simon Horman <simon.horman@...ronome.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"?