Re: [PATCH 0/7] tcp: bugs and cleanup for 2.6.33
William Allen Simpson
When subtracting, return signed int to allow error checking.
These functions will be used in subsequent patches that implement
additional features.
Signed-off-by: William.Al...@gmail.com
---
include/linux/tcp.h | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
William Allen Simpson
crashing bugs, signed versus unsigned (underflow) bugs, TCP DoS bugs,
TCP data corruption, and TCP performance problems....
Combination of patches reported in October, November, December, January,
and February, for 2.6.32 and now 2.6.33.
This code has had previous review and several months of limited testing.
Some portions were removed during the various TCPCT part 1 patch splits,
then were cut off by the sudden unexpected end of that merge window.
[03 Dec 2009] I've restarted the sub-numbering (again).
Of particular interest are the TCPCT header extensions that already
appear in the next phase of testing with other platforms. These patches
allow correct reception without data corruption.
The remainder of the original TCPCT part 2 will be merged with part 3.
[Updated to 2010 Feb 24 2.6.33 release.]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Ben Hutchings
> Redefine two TCP header functions to accept TCP header pointer.
> When subtracting, return signed int to allow error checking.
This patch only adds functions, so why do you say 'redefine'?
> These functions will be used in subsequent patches that implement
> additional features.
[...]
> +/* Length of standard options only. This could be negative. */
> +static inline int tcp_option_len_th(const struct tcphdr *th)
> +{
> + return (int)(th->doff * 4) - sizeof(*th);
> +}
I don't see the point of this cast; the left operand of the subtraction
will in any case be promoted to size_t to match the right operand.
Did you mean
return (int)(th->doff * 4 - sizeof(*th));
?
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
Ben Hutchings
> The tcp_optlen() function returns a potential *negative* unsigned.
>
> In the only two existing files using the old tcp_optlen() function,
> clean up confusing and inconsistent mixing of both byte and word
> offsets, and other coding style issues. Document assumptions.
>
> Quoth David Miller:
> This is transmit, and the packets can only come from the Linux
> TCP stack, not some external entity.
>
> You're being way too anal here, and adding these checks to
> drivers would be just a lot of rediculious bloat. [sic]
>
> Therefore, there are *no* checks for bad TCP and IP header sizes, nor
> any semantic changes. The drivers should function exactly as existing.
>
> No response from testers in 19+ weeks.
There's no need for these passive-aggressive comments and they certainly
won't get your patches merged any faster.
> Requires:
> net: tcp_header_len_th and tcp_option_len_th
>
> Signed-off-by: William.Al...@gmail.com
> CC: Michael Chan <mc...@broadcom.com>
> ---
> drivers/net/bnx2.c | 29 +++++++++++++-----------
> drivers/net/tg3.c | 60 +++++++++++++++++++++++---------------------------
> include/linux/tcp.h | 5 ----
> 3 files changed, 44 insertions(+), 50 deletions(-)
> diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> index 65df1de..45452c5 100644
> --- a/drivers/net/bnx2.c
> +++ b/drivers/net/bnx2.c
> @@ -6352,6 +6352,8 @@ bnx2_vlan_rx_register(struct net_device *dev, struct vlan_group *vlgrp)
> /* Called with netif_tx_lock.
> * bnx2_tx_int() runs without netif_tx_lock unless it needs to call
> * netif_wake_queue().
> + *
> + * No TCP or IP length checking, per David Miller (see commit log).
> */
> static netdev_tx_t
> bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
> @@ -6396,19 +6398,19 @@ bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
> (TX_BD_FLAGS_VLAN_TAG | (vlan_tx_tag_get(skb) << 16));
> }
> #endif
> - if ((mss = skb_shinfo(skb)->gso_size)) {
> - u32 tcp_opt_len;
> - struct iphdr *iph;
> + mss = skb_shinfo(skb)->gso_size;
> + if (mss != 0) {
> + struct tcphdr *th = tcp_hdr(skb);
> + int tcp_opt_words = th->doff - (sizeof(*th) >> 2);
> + /* assumes positive tcp_opt_words without checking */
It would be far more helpful to add a comment to the top of this block
that notes that skbs with gso_size set are known to have valid headers.
> vlan_tag_flags |= TX_BD_FLAGS_SW_LSO;
>
> - tcp_opt_len = tcp_optlen(skb);
> -
How is this an improvement?
> if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6) {
> u32 tcp_off = skb_transport_offset(skb) -
> sizeof(struct ipv6hdr) - ETH_HLEN;
>
> - vlan_tag_flags |= ((tcp_opt_len >> 2) << 8) |
> + vlan_tag_flags |= (tcp_opt_words << 8) |
> TX_BD_FLAGS_SW_FLAGS;
> if (likely(tcp_off == 0))
> vlan_tag_flags &= ~TX_BD_FLAGS_TCP6_OFF0_MSK;
> @@ -6421,14 +6423,15 @@ bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
> mss |= (tcp_off & 0xc) << TX_BD_TCP6_OFF2_SHL;
> }
> } else {
> - iph = ip_hdr(skb);
> - if (tcp_opt_len || (iph->ihl > 5)) {
> - vlan_tag_flags |= ((iph->ihl - 5) +
> - (tcp_opt_len >> 2)) << 8;
> - }
> + struct iphdr *iph = ip_hdr(skb);
> + int ip_opt_words = iph->ihl - (sizeof(*iph) >> 2);
> + /* assumes positive ip_opt_words without checking */
> + int opt_words = ip_opt_words + tcp_opt_words;
> +
> + if (opt_words > 0)
> + vlan_tag_flags |= opt_words << 8;
[...]
That *is* clearer.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
--
William Allen Simpson
SYNACK transaction data.
This is a straightforward re-implementation of an earlier (year-old)
patch that no longer applies cleanly, with permission of the original
author (Adam Langley). The patch was previously reviewed:
http://thread.gmane.org/gmane.linux.network/102586
This function will also be used in subsequent patches that implement
additional features.
Requires:
TCPCT part 1g: Responder Cookie => Initiator
net: tcp_header_len_th and tcp_option_len_th
Signed-off-by: William.Al...@gmail.com
---
net/ipv4/tcp_input.c | 26 +++++++++++++++++++++++++-
1 files changed, 25 insertions(+), 1 deletions(-)
William Allen Simpson
patch is in the next message with the proper subject.
David Miller
Date: Thu, 25 Feb 2010 15:30:03 -0500
> I'd have thought that there would be greater interest about patching
> crashing bugs, signed versus unsigned (underflow) bugs, TCP DoS bugs,
> TCP data corruption, and TCP performance problems....
Your patches add as many bugs and problems as they claim to solve.
You also attack, in your commit messages and code coments, the
very people you want to look at your changes and integrate them.
How you hope to make forward progress in these circumstances is
beyond me.
Just remember William: Whilst people have a right to say whatever
they want, they must earn the privilege to being listened to.
And currently many people have you set strictly to ignore.