netdev - Re: [PATCH] drivers/net/wan/lapbether: Use needed_headroom instead of hard_header

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]

Message-ID: <CAM_iQpV8GDu2U_+4LwSy=uHc6_0FvCx_7ZPCOQ15=hccpaOCig@mail.gmail.com>
Date:   Mon, 27 Jul 2020 22:24:45 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Xie He <xie.he.0141@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux X25 <linux-x25@...r.kernel.org>
Subject: Re: [PATCH] drivers/net/wan/lapbether: Use needed_headroom instead of hard_header_len

Hello,

On Mon, Jul 27, 2020 at 12:41 PM Xie He <xie.he.0141@...il.com> wrote:
>
> Hi Cong Wang,
>
> I'm wishing to change a driver from using "hard_header_len" to using
> "needed_headroom" to declare its needed headroom. I submitted a patch
> and it is decided it needs to be reviewed. I see you participated in
> "hard_header_len vs needed_headroom" discussions in the past. Can you
> help me review this patch? Thanks!
>
> The patch is at:
> http://patchwork.ozlabs.org/project/netdev/patch/20200726110524.151957-1-xie.he.0141@gmail.com/
>
> In my understanding, hard_header_len should be the length of the header
> created by dev_hard_header. Any additional headroom needed should be
> declared in needed_headroom instead of hard_header_len. I came to this
> conclusion by examining the logic of net/packet/af_packet.c:packet_snd.

I am not familiar with this WAN driver, but I suggest you to look at
the following commit, which provides a lot of useful information:

commit 9454f7a895b822dd8fb4588fc55fda7c96728869
Author: Brian Norris <briannorris@...omium.org>
Date:   Wed Feb 26 16:05:11 2020 -0800

    mwifiex: set needed_headroom, not hard_header_len

    hard_header_len provides limitations for things like AF_PACKET, such
    that we don't allow transmitting packets smaller than this.

    needed_headroom provides a suggested minimum headroom for SKBs, so that
    we can trivally add our headers to the front.

    The latter is the correct field to use in this case, while the former
    mostly just prevents sending small AF_PACKET frames.

    In any case, mwifiex already does its own bounce buffering [1] if we
    don't have enough headroom, so hints (not hard limits) are all that are
    needed.

    This is the essentially the same bug (and fix) that brcmfmac had, fixed
    in commit cb39288fd6bb ("brcmfmac: use ndev->needed_headroom to reserve
    additional header space").

    [1] mwifiex_hard_start_xmit():
            if (skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN) {
            [...]
                    /* Insufficient skb headroom - allocate a new skb */

Hope this helps.

Thanks.