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  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:   Fri, 4 Sep 2020 18:28:38 -0700
From:   Xie He <xie.he.0141@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Krzysztof Halasa <khc@...waw.pl>,
        "David S. Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Martin Schiller <ms@....tdt.de>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>
Subject: Re: [PATCH net v2] drivers/net/wan/hdlc_fr: Add needed_headroom for
 PVC devices

Thank you for your email, Jakub!

On Fri, Sep 4, 2020 at 3:14 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> Since this is a tunnel protocol on top of HDLC interfaces, and
> hdlc_setup_dev() sets dev->hard_header_len = 16; should we actually
> set the needed_headroom to 10 + 16 = 26? I'm not clear on where/if
> hdlc devices actually prepend 16 bytes of header, though.

The HDLC device is not actually prepending any header when it is used
with this driver. When the PVC device has prepended its header and
handed over the skb to the HDLC device, the HDLC device just hands it
over to the hardware driver for transmission without prepending any
header.

If we grep "header_ops" and "skb_push" in "hdlc.c" and "hdlc_fr.c", we
can see there is no "header_ops" implemented in these two files and
all "skb_push" happen in the PVC device in hdlc_fr.c.

For this reason, I have previously submitted a patch to change the
value of hard_header_len of the HDLC device from 16 to 0, because it
is not actually used.

See:
2b7bcd967a0f (drivers/net/wan/hdlc: Change the default of hard_header_len to 0)

> > diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
> > index 9acad651ea1f..12b35404cd8e 100644
> > --- a/drivers/net/wan/hdlc_fr.c
> > +++ b/drivers/net/wan/hdlc_fr.c
> > @@ -1041,7 +1041,7 @@ static void pvc_setup(struct net_device *dev)
> >  {
> >       dev->type = ARPHRD_DLCI;
> >       dev->flags = IFF_POINTOPOINT;
> > -     dev->hard_header_len = 10;
> > +     dev->hard_header_len = 0;
>
> Is there a need to set this to 0? Will it not be zero after allocation?

Oh. I understand your point. Theoretically we don't need to set it to
0 because it already has the default value of 0. I'm setting it to 0
only because I want to tell future developers that this value is
intentionally set to 0, and it is not carelessly missed out.

Powered by blists - more mailing lists