netdev - Re: [RFC PATCH 02/13] net: dsa: implement a central TX reallocation procedure

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: <20201017220104.wejlxn2a4seefkfv@skbuf>
Date:   Sat, 17 Oct 2020 22:01:05 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        Christian Eggers <ceggers@...i.de>,
        Kurt Kanzenbach <kurt@...utronix.de>
Subject: Re: [RFC PATCH 02/13] net: dsa: implement a central TX reallocation
 procedure

On Sun, Oct 18, 2020 at 12:36:00AM +0300, Vladimir Oltean wrote:
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index d4326940233c..790f5c8deb13 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -548,6 +548,36 @@ netdev_tx_t dsa_enqueue_skb(struct sk_buff *skb, struct net_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(dsa_enqueue_skb);
>  
> +static int dsa_realloc_skb(struct sk_buff *skb, struct net_device *dev)

I forgot to actually pad the skb here, if it's a tail tagger, silly me.
The following changes should do the trick.

> +{
> +	struct net_device *master = dsa_slave_to_master(dev);

The addition of master->needed_headroom and master->needed_tailroom used
to be here, that's why this unused variable is here.

> +	struct dsa_slave_priv *p = netdev_priv(dev);
> +	struct dsa_slave_stats *e;
> +	int headroom, tailroom;
	int padlen = 0, err;
> +
> +	headroom = dev->needed_headroom;
> +	tailroom = dev->needed_tailroom;
> +	/* For tail taggers, we need to pad short frames ourselves, to ensure
> +	 * that the tail tag does not fail at its role of being at the end of
> +	 * the packet, once the master interface pads the frame.
> +	 */
> +	if (unlikely(tailroom && skb->len < ETH_ZLEN))
> +		tailroom += ETH_ZLEN - skb->len;
		~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
		padlen = ETH_ZLEN - skb->len;
	tailroom += padlen;
> +
> +	if (likely(skb_headroom(skb) >= headroom &&
> +		   skb_tailroom(skb) >= tailroom) &&
> +		   !skb_cloned(skb))
> +		/* No reallocation needed, yay! */
> +		return 0;
> +
> +	e = this_cpu_ptr(p->extra_stats);
> +	u64_stats_update_begin(&e->syncp);
> +	e->tx_reallocs++;
> +	u64_stats_update_end(&e->syncp);
> +
> +	return pskb_expand_head(skb, headroom, tailroom, GFP_ATOMIC);
	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
	err = pskb_expand_head(skb, headroom, tailroom, GFP_ATOMIC);
	if (err < 0 || !padlen)
		return err;

	return __skb_put_padto(skb, padlen, false);
> +}
> +
>  static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
>  {
>  	struct dsa_slave_priv *p = netdev_priv(dev);
> @@ -567,6 +597,11 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
>  	 */
>  	dsa_skb_tx_timestamp(p, skb);
>  
> +	if (dsa_realloc_skb(skb, dev)) {
> +		kfree_skb(skb);
> +		return NETDEV_TX_OK;
> +	}
> +
>  	/* Transmit function may have to reallocate the original SKB,
>  	 * in which case it must have freed it. Only free it here on error.
>  	 */
> @@ -1802,6 +1837,14 @@ int dsa_slave_create(struct dsa_port *port)
>  	slave_dev->netdev_ops = &dsa_slave_netdev_ops;
>  	if (ds->ops->port_max_mtu)
>  		slave_dev->max_mtu = ds->ops->port_max_mtu(ds, port->index);
> +	/* Try to save one extra realloc later in the TX path (in the master)
> +	 * by also inheriting the master's needed headroom and tailroom.
> +	 * The 8021q driver also does this.
> +	 */

Also, this comment is bogus given the current code. It should be removed
from here, and...

> +	if (cpu_dp->tag_ops->tail_tag)
> +		slave_dev->needed_tailroom = cpu_dp->tag_ops->overhead;
> +	else
> +		slave_dev->needed_headroom = cpu_dp->tag_ops->overhead;
...put here, along with:
	slave_dev->needed_headroom += master->needed_headroom;
	slave_dev->needed_tailroom += master->needed_tailroom;
>  	SET_NETDEV_DEVTYPE(slave_dev, &dsa_type);
>  
>  	netdev_for_each_tx_queue(slave_dev, dsa_slave_set_lockdep_class_one,
> -- 
> 2.25.1
>