From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Carpenter Subject: Re: [PATCH v4 2/7] net: remove old tcp_optlen function Date: Fri, 12 Mar 2010 20:46:56 +0300 Message-ID: <20100312174656.GA12175@bicker> References: <4B98D592.6040301@gmail.com> <4B98D9A2.8060603@gmail.com> <4B9A40BC.4030301@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Linus Torvalds , Andrew Morton , Linux Kernel Developers , Linux Kernel Network Developers , David Miller , Michael Chan , Simon Horman To: William Allen Simpson Return-path: Received: from mail-fx0-f219.google.com ([209.85.220.219]:46470 "EHLO mail-fx0-f219.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752639Ab0CLRrR (ORCPT ); Fri, 12 Mar 2010 12:47:17 -0500 Content-Disposition: inline In-Reply-To: <4B9A40BC.4030301@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On Fri, Mar 12, 2010 at 08:25:16AM -0500, William Allen Simpson wrote: > 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, > although usage of int should ameliorate the issues. > So after you removed the checks this change includes: 1) Random slagging on the networking guys 2) u32 => int to ameliorate your static checker's complaints 3) cleanups People have already explained that tcp_optlen() doesn't return negative values. It would really help us if you could show how tcp_hdr(skb)->doff can be less than 5? regards, dan carpenter > Requires: > net: tcp_header_len_th and tcp_option_len_th > > Signed-off-by: William.Allen.Simpson@gmail.com > CC: Michael Chan > --- > drivers/net/bnx2.c | 29 +++++++++++++----------- > drivers/net/tg3.c | 60 +++++++++++++++++++++++--------------------------- > include/linux/tcp.h | 5 ---- > 3 files changed, 44 insertions(+), 50 deletions(-) > > No response from testers in 21+ weeks. > > [removed comment references to commit log] > > diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c > index 381887b..87607b8 100644 > --- a/drivers/net/bnx2.c > +++ b/drivers/net/bnx2.c > @@ -6335,6 +6335,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, as required by David Miller. > */ > static netdev_tx_t > bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev) > @@ -6378,19 +6380,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 */ > > vlan_tag_flags |= TX_BD_FLAGS_SW_LSO; > > - tcp_opt_len = tcp_optlen(skb); > - > 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; > @@ -6403,14 +6405,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; > } > - } else > - mss = 0; > + } > > mapping = pci_map_single(bp->pdev, skb->data, len, PCI_DMA_TODEVICE); > if (pci_dma_mapping_error(bp->pdev, mapping)) { > diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c > index 0fa7688..6ad8184 100644 > --- a/drivers/net/tg3.c > +++ b/drivers/net/tg3.c > @@ -5481,6 +5481,8 @@ static void tg3_set_txd(struct tg3_napi *tnapi, int entry, > > /* hard_start_xmit for devices that don't have any bugs and > * support TG3_FLG2_HW_TSO_2 and TG3_FLG2_HW_TSO_3 only. > + * > + * No TCP or IP length checking, as required by David Miller. > */ > static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, > struct net_device *dev) > @@ -5515,9 +5517,9 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, > > entry = tnapi->tx_prod; > base_flags = 0; > - mss = 0; > - if ((mss = skb_shinfo(skb)->gso_size) != 0) { > - int tcp_opt_len, ip_tcp_len; > + mss = skb_shinfo(skb)->gso_size; > + if (mss != 0) { > + struct tcphdr *th; > u32 hdrlen; > > if (skb_header_cloned(skb) && > @@ -5525,18 +5527,16 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, > dev_kfree_skb(skb); > goto out_unlock; > } > + th = tcp_hdr(skb); > > if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6) > hdrlen = skb_headlen(skb) - ETH_HLEN; > else { > struct iphdr *iph = ip_hdr(skb); > > - tcp_opt_len = tcp_optlen(skb); > - ip_tcp_len = ip_hdrlen(skb) + sizeof(struct tcphdr); > - > + hdrlen = ip_hdrlen(skb) + tcp_header_len_th(th); > + iph->tot_len = htons(mss + hdrlen); > iph->check = 0; > - iph->tot_len = htons(mss + ip_tcp_len + tcp_opt_len); > - hdrlen = ip_tcp_len + tcp_opt_len; > } > > if (tp->tg3_flags2 & TG3_FLG2_HW_TSO_3) { > @@ -5550,7 +5550,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, > base_flags |= (TXD_FLAG_CPU_PRE_DMA | > TXD_FLAG_CPU_POST_DMA); > > - tcp_hdr(skb)->check = 0; > + th->check = 0; > > } > else if (skb->ip_summed == CHECKSUM_PARTIAL) > @@ -5683,6 +5683,8 @@ tg3_tso_bug_end: > > /* hard_start_xmit for devices that have the 4G bug and/or 40-bit bug and > * support TG3_FLG2_HW_TSO_1 or firmware TSO only. > + * > + * No TCP or IP length checking, as required by David Miller. > */ > static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb, > struct net_device *dev) > @@ -5721,20 +5723,21 @@ static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb, > if (skb->ip_summed == CHECKSUM_PARTIAL) > base_flags |= TXD_FLAG_TCPUDP_CSUM; > > - if ((mss = skb_shinfo(skb)->gso_size) != 0) { > + mss = skb_shinfo(skb)->gso_size; > + if (mss != 0) { > struct iphdr *iph; > - u32 tcp_opt_len, ip_tcp_len, hdr_len; > + struct tcphdr *th; > + u32 hdr_len; > + int opt_bytes; > > if (skb_header_cloned(skb) && > pskb_expand_head(skb, 0, 0, GFP_ATOMIC)) { > dev_kfree_skb(skb); > goto out_unlock; > } > + th = tcp_hdr(skb); > + hdr_len = ip_hdrlen(skb) + tcp_header_len_th(th); > > - tcp_opt_len = tcp_optlen(skb); > - ip_tcp_len = ip_hdrlen(skb) + sizeof(struct tcphdr); > - > - hdr_len = ip_tcp_len + tcp_opt_len; > if (unlikely((ETH_HLEN + hdr_len) > 80) && > (tp->tg3_flags2 & TG3_FLG2_TSO_BUG)) > return (tg3_tso_bug(tp, skb)); > @@ -5746,13 +5749,14 @@ static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb, > iph->check = 0; > iph->tot_len = htons(mss + hdr_len); > if (tp->tg3_flags2 & TG3_FLG2_HW_TSO) { > - tcp_hdr(skb)->check = 0; > + th->check = 0; > base_flags &= ~TXD_FLAG_TCPUDP_CSUM; > } else > - tcp_hdr(skb)->check = ~csum_tcpudp_magic(iph->saddr, > - iph->daddr, 0, > - IPPROTO_TCP, > - 0); > + th->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr, > + 0, IPPROTO_TCP, 0); > + > + opt_bytes = hdr_len - sizeof(*iph) - sizeof(*th); > + /* assumes positive opt_bytes without checking */ > > if (tp->tg3_flags2 & TG3_FLG2_HW_TSO_3) { > mss |= (hdr_len & 0xc) << 12; > @@ -5763,19 +5767,11 @@ static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb, > mss |= hdr_len << 9; > else if ((tp->tg3_flags2 & TG3_FLG2_HW_TSO_1) || > GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5705) { > - if (tcp_opt_len || iph->ihl > 5) { > - int tsflags; > - > - tsflags = (iph->ihl - 5) + (tcp_opt_len >> 2); > - mss |= (tsflags << 11); > - } > + if (opt_bytes > 0) > + mss |= opt_bytes << (11 - 2); > } else { > - if (tcp_opt_len || iph->ihl > 5) { > - int tsflags; > - > - tsflags = (iph->ihl - 5) + (tcp_opt_len >> 2); > - base_flags |= tsflags << 12; > - } > + if (opt_bytes > 0) > + base_flags |= opt_bytes << (12 - 2); > } > } > #if TG3_VLAN_TAG_USED > diff --git a/include/linux/tcp.h b/include/linux/tcp.h > index 48ddeb8..854ad65 100644 > --- a/include/linux/tcp.h > +++ b/include/linux/tcp.h > @@ -220,11 +220,6 @@ static inline unsigned int tcp_hdrlen(const struct sk_buff *skb) > return tcp_hdr(skb)->doff * 4; > } > > -static inline unsigned int tcp_optlen(const struct sk_buff *skb) > -{ > - return (tcp_hdr(skb)->doff - 5) * 4; > -} > - > /* Length of fixed header plus standard options. */ > static inline unsigned int tcp_header_len_th(const struct tcphdr *th) > { > -- > 1.6.3.3 >