Date:	Fri, 12 Mar 2010 18:05:41 -0500
From:	William Allen Simpson <william.allen.simpson@...il.com>
To:	Dan Carpenter <error27@...il.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Kernel Developers <linux-kernel@...r.kernel.org>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>,
	Michael Chan <mchan@...adcom.com>,
	Simon Horman <horms@...ge.net.au>
Subject: Re: [PATCH v4 2/7] net: remove old tcp_optlen function

All the drama is beside the point.  This patch merely removes a *rarely*
used function (2 drivers).  Not complicated....

There's a reason that this function isn't used much.  It doesn't work.


On 3/12/10 12:46 PM, Dan Carpenter wrote:
> So after you removed the checks this change includes:

I didn't remove any *existing* checks.  I had added *new* checks in my
earlier patch, then removed *my* checks from this patch as required by
David Miller.


> 1) Random slagging on the networking guys

I had to look up that "random slagging on" colloquialism.  Apparently,
the "random slagging" target would be *me* -- calling me "anal" and my
code "rediculious bloat" [sic] probably qualifies....

(Admittedly, I'm rather careful and may be overly cautious at times, after
some 30+ years of writing network drivers.  Once it's in half a billion
cell phones, it's hard/impossible to update.)

Since my first unpleasant interactions with David Miller on my very
earliest (October) netdev posts, I've conspicuously avoided contradicting
him.  I've merely *obeyed* his injunction here, and moved on....

The patch itself neutrally documents a coding requirement decision by that
networking maintainer by name.


> 2) u32 =>  int to ameliorate your static checker's complaints

Good idea.  Actually, I simply looked at the code and its history.


> 3) cleanups
>
Removing this function is really a *bug* fix (in several places), with
cleanups in the vicinity for obviously poor coding (variants in 3 places):

-	mss = 0;
-	if ((mss = skb_shinfo(skb)->gso_size) != 0) {
-		int tcp_opt_len, ip_tcp_len;

Cleaner as:

+	mss = skb_shinfo(skb)->gso_size;
+	if (mss != 0) {
+		struct tcphdr *th;

But I wouldn't have bothered had I not been changing that immediately
following line.  30+ years of experience with collaborative projects
informs me that it's best to make minor cleanups only where I'm already
improving the code nearby.  Otherwise, it creates patch conflicts.


> People have already explained that tcp_optlen() doesn't return
> negative values.

People?  The fact that the calculation itself can be negative appeared
the very first time I tested my own code using this bad function!


> negative values.  It would really help us if you could show how
> tcp_hdr(skb)->doff can be less than 5?
>
Oh, I've long since given up on lengthy explanations.  Both Eric and
Ilpo have repeatedly castigated me for being too wordy.

In this particular instance, I suggest that you take a look at all the
places that gso_size is set, and cross index with all the code paths that
place these TCP headers onto the txq without a check of doff -- as I did!

I'll specifically mention the tun and virtio_net devices, but I'm also
particularly concerned with af_packet.c and skbuff.c -- and the general
problem with inet_lro.c, too.

Amazingly enough, folks sometimes use Linux for routers....
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists