Re: [NET]: TSO requires SG, enforce this at device registry.

Re: [NET]: TSO requires SG, enforce this at device registry.

[Jeff Garzik]

Linux Kernel Mailing List wrote:
> diff -Nru a/net/core/dev.c b/net/core/dev.c
> --- a/net/core/dev.c	2004-10-22 10:15:57 -07:00
> +++ b/net/core/dev.c	2004-10-22 10:15:57 -07:00
> @@ -2871,6 +2871,14 @@
>  		dev->features &= ~NETIF_F_SG;
>  	}
>  
> +	/* TSO requires that SG is present as well. */
> +	if ((dev->features & NETIF_F_TSO) &&
> +	    !(dev->features & NETIF_F_SG)) {
> +		printk("%s: Dropping NETIF_F_TSO since no SG feature.\n",
> +		       dev->name);
> +		dev->features &= ~NETIF_F_TSO;
> +	}


Although this patch is correct, I am pondering whether this fully covers 
the problems in the field.

There are currently two classes of problems I am seeing, that generate 
real-life bug reports:

1) Given current driver implementations of ethtool ioctls, sysadmin is 
free to create a combination of bits that are IMHO a bug.  One can argue 
that this is an extension of "root can shoot himself in the foot", so 
who knows.

2) Programmers writing drivers do not appear to be clear that SG is 
required to tx-csum/tso, and also, should not be present without one or 
both of those bits set.

	Jeff

Re: [NET]: TSO requires SG, enforce this at device registry.

[David S. Miller]

On Mon, 25 Oct 2004 01:50:41 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> 1) Given current driver implementations of ethtool ioctls, sysadmin is 
> free to create a combination of bits that are IMHO a bug.  One can argue 
> that this is an extension of "root can shoot himself in the foot", so 
> who knows.

I made a followon posting proposing ethtool changes which
would enforce the rules there too, did you see it?

Re: [NET]: TSO requires SG, enforce this at device registry.

[Jeff Garzik]

David S. Miller wrote:
> On Mon, 25 Oct 2004 01:50:41 -0400
> Jeff Garzik <jgarzik@pobox.com> wrote:
> 
> 
>>1) Given current driver implementations of ethtool ioctls, sysadmin is 
>>free to create a combination of bits that are IMHO a bug.  One can argue 
>>that this is an extension of "root can shoot himself in the foot", so 
>>who knows.
> 
> 
> I made a followon posting proposing ethtool changes which
> would enforce the rules there too, did you see it?


Sorry, I didn't see it.  URL or grep string?

	Jeff

Re: [NET]: TSO requires SG, enforce this at device registry.

[David S. Miller]

On Mon, 25 Oct 2004 02:28:24 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> David S. Miller wrote:
> > I made a followon posting proposing ethtool changes which
> > would enforce the rules there too, did you see it?
> 
> 
> Sorry, I didn't see it.  URL or grep string?

It was to linux-net, here is a replay.

Date: Wed, 20 Oct 2004 16:35:10 -0700
From: "David S. Miller" <davem@davemloft.net>
To: Herbert Xu <herbert@gondor.apana.org.au>
Cc: thomas.spatzier@de.ibm.com, linux-net@vger.kernel.org
Subject: Re: [PATCH] select appropriate skb size in tcp_sendmsg when TSO is
 used
Message-Id: <20041020163510.6d13e9c7.davem@davemloft.net>
In-Reply-To: <E1CKE5P-0005SP-00@gondolin.me.apana.org.au>
References: <OF96546AB5.ACE12043-ONC1256F33.0027BC6B-C1256F33.002D4A6E@de.ibm.com>
	<E1CKE5P-0005SP-00@gondolin.me.apana.org.au>
X-Mailer: Sylpheed version 0.9.12 (GTK+ 1.2.10; sparc-unknown-linux-gnu)
X-Face: "_;p5u5aPsO,_Vsx"^v-pEq09'CU4&Dc1$fQExov$62l60cgCc%FnIwD=.UF^a>?5'9Kn[;433QFVV9M..2eN.@4ZWPGbdi<=?[:T>y?SD(R*-3It"Vj:)"dP
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Wed, 20 Oct 2004 20:52:55 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> This is bogus.  The subsequent if clause is what allows the size to
> exceed mss_cache_std.
> 
> TSO without NETIF_F_SG is not worth it.

Indeed.  I'm going to enforce this at device registration time
as in the patch at the end of this email.

BTW, we allow mucking of all of these SG, TSO, CSUM settings
via ethtool yet the "X needs Y" rules are not enforced.  I
can't think of an easy way to do this without touching a lot
of drivers.  Perhaps something like:

int ethtool_feature_change(struct net_device *dev, u32 flag_bit, int on_off);

So drivers/net/tg3.c:tg3_set_tx() might then look like this:

static int tg3_set_tx_csum(struct net_device *dev, u32 data)
{
	struct tg3 *tp = netdev_priv(dev);
	int err;

	if (tp->tg3_flags & TG3_FLAG_BROKEN_CHECKSUMS) {
		if (data != 0)
			return -EINVAL;
		return 0;
	}

	else
		dev->features &= ~NETIF_F_IP_CSUM;

	return ethtool_feature_change(dev, NETIF_F_IP_CSUM, data);
}

And ethtool_feature_change() might be implemented like so:

int ethtool_feature_change(struct net_device *dev, u32 flag_bit, int on_off)
{
	/* If checksumming is being disabled, both SG and
	 * TSO must be disabled as well.
	 */
	if (!on_off &&
	    (flag_bit & (NETIF_F_IP_CSUM |
			 NETIF_F_HW_CSUM |
			 NETIF_F_NO_CSUM))) {
		dev->features &= ~(NETIF_F_SG | NETIF_F_TSO);
	}

	/* If SG is being disabled, TSO must be disabled
	 * as well.
	 */
	if (!on_off && (flag_bit & NETIF_F_SG))
		dev->features &= ~NETIF_F_TSO;

	/* TSO requires SG */
	if (on_off &&
	    (flag_bit & NETIF_F_TSO) &&
	    !(dev->features & NETIF_F_SG))
		return -EINVAL;

	/* SG requires CSUM */
	if (on_off &&
	    (flag_bit & NETIF_F_SG) &&
	    !(dev->features & (NETIF_F_IP_CSUM |
			       NETIF_F_HW_CSUM |
			       NETIF_F_NO_CSUM)))
		return -EINVAL;

	if (on_off)
		dev->features |= flag_bit;
	else
		dev->features &= ~flag_bit;

	return 0;
}

And then we add usage of this thing to the various drivers
and the generic implementation of the appropriate ethtool
ops in net/core/ethtool.c

It just occured to me that instead of manually clearing
dev->feature bits we should invoke the appropriate ethtool
op to accomplish that just in case the device needs to do
something implementation specific when disabling said features.
This implies that ethtool_feature_change() should be invoked
without any device locks set to prevent deadlocks.

Comments?

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/10/20 16:16:33-07:00 davem@nuts.davemloft.net 
#   [NET]: TSO requires SG, enforce this at device registry.
#   
#   Signed-off-by: David S. Miller <davem@davemloft.net>
# 
# net/core/dev.c
#   2004/10/20 16:16:03-07:00 davem@nuts.davemloft.net +8 -0
#   [NET]: TSO requires SG, enforce this at device registry.
# 
diff -Nru a/net/core/dev.c b/net/core/dev.c
--- a/net/core/dev.c	2004-10-20 16:16:47 -07:00
+++ b/net/core/dev.c	2004-10-20 16:16:47 -07:00
@@ -2871,6 +2871,14 @@
 		dev->features &= ~NETIF_F_SG;
 	}
 
+	/* TSO requires that SG is present as well. */
+	if ((dev->features & NETIF_F_TSO) &&
+	    !(dev->features & NETIF_F_SG)) {
+		printk("%s: Dropping NETIF_F_TSO since no SG feature.\n",
+		       dev->name);
+		dev->features &= ~NETIF_F_TSO;
+	}
+
 	/*
 	 *	nil rebuild_header routine,
 	 *	that should be never called and used as just bug trap.