| 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
| ||
|
Message-ID: <f701aad45e35579c8b79836ffeb86ea9@dev.tdt.de>
Date: Mon, 15 Feb 2021 07:27:35 +0100
From: Martin Schiller <ms@....tdt.de>
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,
Krzysztof Halasa <khc@...waw.pl>
Subject: Re: [PATCH net-next RFC v2] net: hdlc_x25: Queue outgoing LAPB frames
On 2021-02-10 18:35, Xie He wrote:
> When sending packets, we will first hand over the (L3) packets to the
> LAPB module, then the LAPB module will 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 without an
> (L3) packet currently being sent, when it is trying to send (L3)
> packets
> queued up in its internal queue, or when it decides to send some (L2)
> control frame.
>
> This means we need have a queue for these outgoing LAPB (L2) frames to
> avoid congestion. This queue needs to be controlled by the hardware
> drivers' netif_stop_queue and netif_wake_queue calls. So we need to use
> a qdisc TX queue for this purpose.
>
> On the other hand, the outgoing (L3) packets don't need to be queued,
> because the LAPB module already has an internal queue for them.
>
> However, currently the outgoing LAPB (L2) frames are not queued. This
> can cause frames to be dropped by hardware drivers when they are busy.
> At the same time the (L3) packets are being queued and controlled by
> hardware drivers' netif_stop_queue and netif_wake_queue calls. This is
> unnecessary and meaningless.
>
> To solve this problem, we can split the HDLC device into two devices:
> a virtual X.25 device and an actual HDLC device, using the virtual
> X.25 device to send (L3) packets and using the actual HDLC device to
> queue LAPB (L2) frames. The outgoing LAPB queue will be controlled by
> hardware drivers' netif_stop_queue and netif_wake_queue calls, while
> outgoing (L3) packets will not be affected by these calls.
At first glance, the patch looks quite reasonable. The only thing I
noticed right away is that you also included the changes of your patch
"Return meaningful error code in x25_open".
I hope to get back to the office this week and test it.
>
> Cc: Martin Schiller <ms@....tdt.de>
> Signed-off-by: Xie He <xie.he.0141@...il.com>
> ---
>
> Change from RFC v1:
> Properly initialize state(hdlc)->x25_dev and state(hdlc)->x25_dev_lock.
>
> ---
> drivers/net/wan/hdlc_x25.c | 158 ++++++++++++++++++++++++++++++-------
> 1 file changed, 129 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
> index bb164805804e..2a7b3c3d0c05 100644
> --- a/drivers/net/wan/hdlc_x25.c
> +++ b/drivers/net/wan/hdlc_x25.c
> @@ -23,6 +23,13 @@
>
> struct x25_state {
> x25_hdlc_proto settings;
> + struct net_device *x25_dev;
> + spinlock_t x25_dev_lock; /* Protects the x25_dev pointer */
> +};
> +
> +/* Pointed to by netdev_priv(x25_dev) */
> +struct x25_device {
> + struct net_device *hdlc_dev;
> };
>
> static int x25_ioctl(struct net_device *dev, struct ifreq *ifr);
> @@ -32,6 +39,11 @@ static struct x25_state *state(hdlc_device *hdlc)
> return hdlc->state;
> }
>
> +static struct x25_device *dev_to_x25(struct net_device *dev)
> +{
> + return netdev_priv(dev);
> +}
> +
> /* These functions are callbacks called by LAPB layer */
>
> static void x25_connect_disconnect(struct net_device *dev, int
> reason, int code)
> @@ -89,15 +101,10 @@ static int x25_data_indication(struct net_device
> *dev, struct sk_buff *skb)
>
> static void x25_data_transmit(struct net_device *dev, struct sk_buff
> *skb)
> {
> - hdlc_device *hdlc = dev_to_hdlc(dev);
> -
> + skb->dev = dev_to_x25(dev)->hdlc_dev;
> + skb->protocol = htons(ETH_P_HDLC);
> skb_reset_network_header(skb);
> - skb->protocol = hdlc_type_trans(skb, dev);
> -
> - if (dev_nit_active(dev))
> - dev_queue_xmit_nit(skb, dev);
> -
> - hdlc->xmit(skb, dev); /* Ignore return value :-( */
> + dev_queue_xmit(skb);
> }
>
>
> @@ -163,17 +170,18 @@ static int x25_open(struct net_device *dev)
> .data_indication = x25_data_indication,
> .data_transmit = x25_data_transmit,
> };
> - hdlc_device *hdlc = dev_to_hdlc(dev);
> + struct net_device *hdlc_dev = dev_to_x25(dev)->hdlc_dev;
> + hdlc_device *hdlc = dev_to_hdlc(hdlc_dev);
> struct lapb_parms_struct params;
> int result;
>
> result = lapb_register(dev, &cb);
> if (result != LAPB_OK)
> - return result;
> + return -ENOMEM;
>
> result = lapb_getparms(dev, ¶ms);
> if (result != LAPB_OK)
> - return result;
> + return -EINVAL;
>
> if (state(hdlc)->settings.dce)
> params.mode = params.mode | LAPB_DCE;
> @@ -188,16 +196,104 @@ static int x25_open(struct net_device *dev)
>
> result = lapb_setparms(dev, ¶ms);
> if (result != LAPB_OK)
> - return result;
> + return -EINVAL;
>
> return 0;
> }
>
>
>
> -static void x25_close(struct net_device *dev)
> +static int x25_close(struct net_device *dev)
> {
> lapb_unregister(dev);
> + return 0;
> +}
> +
> +static const struct net_device_ops hdlc_x25_netdev_ops = {
> + .ndo_open = x25_open,
> + .ndo_stop = x25_close,
> + .ndo_start_xmit = x25_xmit,
> +};
> +
> +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;
> +}
> +
> +static int x25_hdlc_open(struct net_device *dev)
> +{
> + struct hdlc_device *hdlc = dev_to_hdlc(dev);
> + struct net_device *x25_dev;
> + char x25_dev_name[sizeof(x25_dev->name)];
> + int result;
> +
> + if (strlen(dev->name) + 4 >= sizeof(x25_dev_name))
> + return -EINVAL;
> +
> + strcpy(x25_dev_name, dev->name);
> + strcat(x25_dev_name, "_x25");
> +
> + x25_dev = alloc_netdev(sizeof(struct x25_device), x25_dev_name,
> + NET_NAME_PREDICTABLE, x25_setup_virtual_dev);
> + if (!x25_dev)
> + return -ENOMEM;
> +
> + /* 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.
> + */
> + x25_dev->needed_headroom = 3 - 1;
> + x25_dev->mtu = dev->mtu - (3 - 1);
> + dev_to_x25(x25_dev)->hdlc_dev = dev;
> +
> + result = register_netdevice(x25_dev);
> + if (result) {
> + free_netdev(x25_dev);
> + return result;
> + }
> +
> + spin_lock_bh(&state(hdlc)->x25_dev_lock);
> + state(hdlc)->x25_dev = x25_dev;
> + spin_unlock_bh(&state(hdlc)->x25_dev_lock);
> +
> + return 0;
> +}
> +
> +static void x25_hdlc_close(struct net_device *dev)
> +{
> + struct hdlc_device *hdlc = dev_to_hdlc(dev);
> + struct net_device *x25_dev = state(hdlc)->x25_dev;
> +
> + if (x25_dev->flags & IFF_UP)
> + dev_close(x25_dev);
> +
> + spin_lock_bh(&state(hdlc)->x25_dev_lock);
> + state(hdlc)->x25_dev = NULL;
> + spin_unlock_bh(&state(hdlc)->x25_dev_lock);
> +
> + unregister_netdevice(x25_dev);
> + free_netdev(x25_dev);
> +}
> +
> +static void x25_hdlc_start(struct net_device *dev)
> +{
> + struct hdlc_device *hdlc = dev_to_hdlc(dev);
> + struct net_device *x25_dev = state(hdlc)->x25_dev;
> +
> + /* hdlc.c guarantees no racing so we're sure x25_dev is valid */
> + netif_carrier_on(x25_dev);
> +}
> +
> +static void x25_hdlc_stop(struct net_device *dev)
> +{
> + struct hdlc_device *hdlc = dev_to_hdlc(dev);
> + struct net_device *x25_dev = state(hdlc)->x25_dev;
> +
> + /* hdlc.c guarantees no racing so we're sure x25_dev is valid */
> + netif_carrier_off(x25_dev);
> }
>
>
> @@ -205,27 +301,38 @@ static void x25_close(struct net_device *dev)
> static int x25_rx(struct sk_buff *skb)
> {
> struct net_device *dev = skb->dev;
> + struct hdlc_device *hdlc = dev_to_hdlc(dev);
> + struct net_device *x25_dev;
>
> if ((skb = skb_share_check(skb, GFP_ATOMIC)) == NULL) {
> dev->stats.rx_dropped++;
> return NET_RX_DROP;
> }
>
> - if (lapb_data_received(dev, skb) == LAPB_OK)
> - return NET_RX_SUCCESS;
> -
> - dev->stats.rx_errors++;
> + spin_lock_bh(&state(hdlc)->x25_dev_lock);
> + x25_dev = state(hdlc)->x25_dev;
> + if (!x25_dev)
> + goto drop;
> + if (lapb_data_received(x25_dev, skb) != LAPB_OK)
> + goto drop;
> + spin_unlock_bh(&state(hdlc)->x25_dev_lock);
> + return NET_RX_SUCCESS;
> +
> +drop:
> + spin_unlock_bh(&state(hdlc)->x25_dev_lock);
> + dev->stats.rx_dropped++;
> dev_kfree_skb_any(skb);
> return NET_RX_DROP;
> }
>
>
> static struct hdlc_proto proto = {
> - .open = x25_open,
> - .close = x25_close,
> + .open = x25_hdlc_open,
> + .close = x25_hdlc_close,
> + .start = x25_hdlc_start,
> + .stop = x25_hdlc_stop,
> .ioctl = x25_ioctl,
> .netif_rx = x25_rx,
> - .xmit = x25_xmit,
> .module = THIS_MODULE,
> };
>
> @@ -298,16 +405,9 @@ static int x25_ioctl(struct net_device *dev,
> struct ifreq *ifr)
> return result;
>
> memcpy(&state(hdlc)->settings, &new_settings, size);
> + state(hdlc)->x25_dev = NULL;
> + spin_lock_init(&state(hdlc)->x25_dev_lock);
>
> - /* There's no header_ops so hard_header_len should be 0. */
> - dev->hard_header_len = 0;
> - /* When transmitting data:
> - * first we'll remove a pseudo header of 1 byte,
> - * then we'll prepend an LAPB header of at most 3 bytes.
> - */
> - dev->needed_headroom = 3 - 1;
> -
> - dev->type = ARPHRD_X25;
> call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, dev);
> netif_dormant_off(dev);
> return 0;
Powered by blists - more mailing lists