lkml.org 
[lkml]   [2010]   [Feb]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3 2/7] net: remove old tcp_optlen function
    From
    Date
    On Thu, 2010-02-25 at 15:39 -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.
    >
    > 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.Allen.Simpson@gmail.com
    > CC: Michael Chan <mchan@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.



    \
     
     \ /
      Last update: 2010-02-25 21:59    [W:0.029 / U:177.356 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site