linux-kernel - Re: [PATCH net-next RFC v4] net: hdlc

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: <YC4sB9OCl5mm3JAw@unreal>
Date:   Thu, 18 Feb 2021 10:57:43 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Xie He <xie.he.0141@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, linux-x25@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Martin Schiller <ms@....tdt.de>,
        Krzysztof Halasa <khc@...waw.pl>,
        Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org
Subject: Re: [PATCH net-next RFC v4] net: hdlc_x25: Queue outgoing LAPB frames

On Tue, Feb 16, 2021 at 12:18:13PM -0800, Xie He wrote:
> When sending packets, we will first hand over the (L3) packets to the
> LAPB module. The LAPB module will then hand over the corresponding LAPB
> (L2) frames back to us for us to transmit.
>
> The LAPB module can also emit LAPB (L2) frames at any time, even without
> an (L3) packet currently being sent on the device. This happens when the
> LAPB module tries to send (L3) packets queued up in its internal queue,
> or when the LAPB module decides to send some (L2) control frame.
>
> This means we need to have a queue for these outgoing LAPB (L2) frames,
> otherwise frames can be dropped if sent when the hardware driver is
> already busy in transmitting. The queue needs to be controlled by
> the hardware driver's netif_stop_queue and netif_wake_queue calls.
> Therefore, we need to use the device's qdisc TX queue for this purpose.
> However, currently outgoing LAPB (L2) frames are not queued.
>
> On the other hand, outgoing (L3) packets (before they are handed over
> to the LAPB module) don't need to be queued, because the LAPB module
> already has an internal queue for them, and is able to queue new outgoing
> (L3) packets at any time. However, currently outgoing (L3) packets are
> being queued in the device's qdisc TX queue, which is controlled by
> the hardware driver's netif_stop_queue and netif_wake_queue calls.
> This is unnecessary and meaningless.
>
> To fix these issues, we can split the HDLC device into two devices -
> a virtual X.25 device and the actual HDLC device, use the virtual X.25
> device to send (L3) packets and then use the actual HDLC device to
> queue LAPB (L2) frames. The outgoing (L2) LAPB queue will be controlled
> by the hardware driver's netif_stop_queue and netif_wake_queue calls,
> while outgoing (L3) packets will not be affected by these calls.
>
> Cc: Martin Schiller <ms@....tdt.de>
> Signed-off-by: Xie He <xie.he.0141@...il.com>
> ---

<...>

> +static void x25_setup_virtual_dev(struct net_device *dev)
> +{
> +	dev->netdev_ops	     = &hdlc_x25_netdev_ops;
> +	dev->type            = ARPHRD_X25;
> +	dev->addr_len        = 0;
> +	dev->hard_header_len = 0;
> +	dev->mtu             = HDLC_MAX_MTU;
> +
> +	/* When transmitting data:
> +	 * first we'll remove a pseudo header of 1 byte,
> +	 * then the LAPB module will prepend an LAPB header of at most 3 bytes.
> +	 */
> +	dev->needed_headroom = 3 - 1;

It is nice that you are resending your patch without the resolution.
However it will be awesome if you don't ignore review comments and fix this "3 - 1"
by writing solid comment above.

Thanks and good luck.