Re: [PATCH v4 2/7] net: remove old tcp_optlen function

[Dan Carpenter]

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@xxxxxxxxx
> CC: Michael Chan <mchan@xxxxxxxxxxxx>
> ---
>  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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/