[ofw][patch] fixes client id insertion

[Anatoly Greenblatt]

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/20080709/8b0a3fa7/attachment.html>