[ofw][patch] fixes client id insertion

[Tzachi Dar]

Applied at 1364.
 
It seems that we will have to deal with ip options at a different time.
 
Thanks
Tzachi


________________________________

	From: Anatoly Greenblatt [mailto:anatolyg at voltaire.com] 
	Sent: Wednesday, July 09, 2008 6:09 PM
	To: Tzachi Dar; ofw at lists.openfabrics.org
	Subject: RE: [ofw][patch] fixes client id insertion
	
	

	On the second thought, this is correct assumption because of the
way how send_buf_t is defined. So let me answer two questions again

	 

	1.	The code checks that the content is no greater than dhcp
header + 312 bytes (options). That is the space reserved for dhcp
request in send_buf_t. The actual size is less and may vary, so if we
change the content of dhcp options we must update lengths. Instead of
calculating the actual size I fill the maximum size. This is ok because
the last option is indicated by 0xff, which lets the receiver to ignore
extra bytes. 
	2.	The size of ip header in this case is constant, see
send_buf_t definition. 

	 

	Retgards,

	Anatoly.

	 

	
________________________________


	From: ofw-bounces at lists.openfabrics.org
[mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Anatoly
Greenblatt
	Sent: Wednesday, July 09, 2008 17:50
	To: Tzachi Dar; ofw at lists.openfabrics.org
	Subject: RE: [ofw][patch] fixes client id insertion

	 

	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/20080710/5e8b922a/attachment.html>