netdev - Re: [PATCH net-next 1/2] net: move the hsize check to the else block in skb

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:   Mon, 11 Jan 2021 08:26:09 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>,
        "linux-sctp @ vger . kernel . org" <linux-sctp@...r.kernel.org>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Neil Horman <nhorman@...driver.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next 1/2] net: move the hsize check to the else block
 in skb_segment

On Mon, Jan 11, 2021 at 4:45 AM Xin Long <lucien.xin@...il.com> wrote:
>
> After commit 89319d3801d1 ("net: Add frag_list support to skb_segment"),
> it goes to process frag_list when !hsize in skb_segment(). However, when
> using skb frag_list, sg normally should not be set. In this case, hsize
> will be set with len right before !hsize check, then it won't go to
> frag_list processing code.
>
> So the right thing to do is move the hsize check to the else block, so
> that it won't affect the !hsize check for frag_list processing.
>
> Signed-off-by: Xin Long <lucien.xin@...il.com>
> ---
>  net/core/skbuff.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 7626a33..ea79359 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3855,8 +3855,6 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>                 hsize = skb_headlen(head_skb) - offset;
>                 if (hsize < 0)
>                         hsize = 0;
> -               if (hsize > len || !sg)
> -                       hsize = len;
>
>                 if (!hsize && i >= nfrags && skb_headlen(list_skb) &&
>                     (skb_headlen(list_skb) == len || sg)) {

So looking at the function it seems like the only spot where the
standard path actually reads the hsize value is right here, and it is
overwritten before we exit the non-error portion of the if statement.
I wonder if we couldn't save ourselves a few cycles and avoid an
unnecessary assignment by replacing the "!hsize" with a check for
"hsize <= 0" and just move the entire set of checks above down into
the lower block.

> @@ -3901,6 +3899,9 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>                         skb_release_head_state(nskb);
>                         __skb_push(nskb, doffset);
>                 } else {
> +                       if (hsize > len || !sg)
> +                               hsize = len;
> +

Then you could essentially just add the "if (hsize < 0)" piece here as
an "else if" check and avoid the check if it isn't needed.

>                         nskb = __alloc_skb(hsize + doffset + headroom,
>                                            GFP_ATOMIC, skb_alloc_rx_flag(head_skb),
>                                            NUMA_NO_NODE);
> --
> 2.1.0
>