[ofw][patch] fixes client id insertion

[Anatoly Greenblatt]

Hi Tzahi,

 

Usually udp checksum is not calculated.

The existing code checks that the dhcp packet is not greater than
sizeof(dhcp_pkt_t), but you're right about the IP header size, we need
to fix it because this is a common assumption, see existing code few
lines below: p_desc->local_ds[1].length = ...

 

Thanks,

Anatoly.

 

-----Original Message-----
From: Tzachi Dar [mailto:tzachid at mellanox.co.il] 
Sent: Wednesday, July 09, 2008 17:26
To: Anatoly Greenblatt; ofw at lists.openfabrics.org
Subject: RE: [ofw][patch] fixes client id insertion

 

Hi Anatoly,

 

I have looked at this patch, and there are some issues that I don't

quite understand:

 

* what about UDP checksum? It doesn't seem as you are calculating it.

 

* In this patch you are doing an assumption that the packet size will

always be the size of ip + the size of UDP + the size of dhcp 

"p_desc->p_buf->ip.hdr.length = cl_ntoh16( sizeof(ip_hdr_t) +

sizeof(udp_hdr_t) + sizeof(dhcp_pkt_t) );"

 

This assumes that the size is always constant. This brings two questions

as far as I understand:

1) If the size is always constant, why do we have to touch this in any

case?

2) at least the size of the ip header is not constant. So you should

probably replace sizeof(ip_hdr_t) with IP_HEADER_LENGTH(IpHdr);

 

Where #define IP_HEADER_LENGTH(pIpHdr)   \

        ( (ULONG)((pIpHdr->iph_verlen & 0x0F) << 2) )

 

 

Thanks

Tzachi

 

> -----Original Message-----

> From: ofw-bounces at lists.openfabrics.org 

> [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of 

> Anatoly Greenblatt

> Sent: Wednesday, June 25, 2008 5:13 PM

> To: ofw at lists.openfabrics.org

> Subject: [ofw][patch] fixes client id insertion

> 

>  <<fix_client_id_insertion.patch>> Hi,

> 

> The client id inserted in dhcp packets w/o updating lengths 

> in udp and ip headers. As a result in some cases the dhcp 

> packet was not accepted by dhcp server ("parameter is larger 

> than buffer" error).

> 

> The fix updates length in udp and ip header and calculate ip 

> header checksum if needed. Also fixes incorrect return code.

> 

> Best regards,

> Anatoly

> 

> Index: ipoib_port.c

> ===================================================================

> --- ipoib_port.c      (revision 1295)

> +++ ipoib_port.c      (working copy)

> @@ -3216,6 +3216,19 @@

>  }

>  

>  

> +unsigned short ipchksum(unsigned short *ip, int len) {

> +    unsigned long sum = 0;

> +

> +    len >>= 1;

> +    while (len--) {

> +        sum += *(ip++);

> +        if (sum > 0xFFFF)

> +            sum -= 0xFFFF;

> +    }

> +    return (unsigned short)((~sum) & 0x0000FFFF); }

> +

>  static NDIS_STATUS

>  __send_mgr_filter_dhcp(

>     IN                      ipoib_port_t* const

> p_port,

> @@ -3317,7 +3330,7 @@

>                       {

>                             IPOIB_PRINT_EXIT(

> TRACE_LEVEL_ERROR, IPOIB_DBG_ERROR,

>                                   ("Can't convert CID to

> IPoIB format.\n") );

> -                           return IB_INSUFFICIENT_MEMORY;

> +                           return NDIS_STATUS_RESOURCES;

>                       }

>                       /* Move the existing options 

> down, and add a new CID option */

>                       len = p_option - ( p_cid + 

> p_cid[1] + 2 ); @@ -3357,10 +3370,21 @@

>  

>           CL_ASSERT( p_cid[1] == 21 );

>           p_cid[23]= DHCP_OPT_END;

> -         ib_gid_set_default( &gid,

> p_port->p_adapter->guids.port_guid.guid );

> +         ib_gid_set_default( &gid,

> p_port->p_adapter->guids.port_guid );

>           cl_memcpy( &p_cid[7], &gid, sizeof(ib_gid_t) );

>           cl_memcpy( &p_cid[3], &p_port->ib_mgr.qpn,

> sizeof(p_port->ib_mgr.qpn) );           

>           p_ib_dhcp->htype = DHCP_HW_TYPE_IB;

> +

> +         /* update lengths to include any change we made */

> +         p_desc->p_buf->ip.hdr.length = cl_ntoh16(

> sizeof(ip_hdr_t) + sizeof(udp_hdr_t) + sizeof(dhcp_pkt_t) );

> +         p_desc->p_buf->ip.prot.udp.hdr.length = cl_ntoh16(

> sizeof(udp_hdr_t) + sizeof(dhcp_pkt_t) );

> +

> +         /* update crc in ip header */

> +         if( !p_port->p_adapter->params.send_chksum_offload )

> +         {

> +               p_desc->p_buf->ip.hdr.chksum = 0;

> +               p_desc->p_buf->ip.hdr.chksum =

> ipchksum((unsigned short*) &p_desc->p_buf->ip.hdr, sizeof(ip_hdr_t));

> +         }

>           break;

>  

>     /* Server messages. */

> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20080709/ae6f2d0f/attachment.html>