[ewg] OFED-1.3.2-20080728-0355.tgz issues

[Doug Ledford]

On Mon, 2008-09-08 at 13:21 +0300, Eli Cohen wrote:
> > Next, I was watching the compiler warnings as things compiled, and I got
> > an array subscript out of bounds warning.  Turns out it was a legitimate
> > error.  There is a thinko in ipoib_transport_dev_init (well, at least in
> > the file I had here, I assume the final version of the files you have
> > would end up the same or close to it once quilt has done what I did
> > manually).  The snippet to fix the bug looks like this:
> > 
> > @@ -228,8 +233,9 @@ int ipoib_transport_dev_init(struct net_device *dev, struct ib_device *ca)
> >                 priv->rx_wr_draft[i].sg_list = &priv->sglist_draft[i][0];
> >                 if (i < UD_POST_RCV_COUNT - 1)
> >                         priv->rx_wr_draft[i].next = &priv->rx_wr_draft[i + 1];
> > +               else
> > +                       priv->rx_wr_draft[i].next = NULL;
> >         }
> > -       priv->rx_wr_draft[i].next = NULL;
> > 
> >         if (ipoib_ud_need_sg(priv->max_ib_mtu)) {
> >                 for (i = 0; i < UD_POST_RCV_COUNT; ++i) {
> > 
> 
> Yes, that's a bug, and I think the reason it did not occur as a real
> problem is because the whole ipoib_dev_priv struct is cleared at
> allocation so the last entry was NULL due to that.

Whether it showed up as a problem or not, the compiler warning was an
array out of bounds warning.  That's not the type of compiler warning
that should be ignored as it almost always points to a bug.  I guess I
was a little surprised that even though the compile tests on the kernel
pass, that you guys allow that type of warning to go unchecked.  I make
a habit out of reviewing kernel compile warnings on the code I maintain
and, when possible, I fix all the warnings just so things like this get
caught.

>  Anyway, I think I
> prefer this fix to the same problem:
> 
> Index: ofed_kernel-fixes/drivers/infiniband/ulp/ipoib/ipoib_verbs.c
> ===================================================================
> --- ofed_kernel-fixes.orig/drivers/infiniband/ulp/ipoib/ipoib_verbs.c	2008-09-08 13:07:02.000000000 +0300
> +++ ofed_kernel-fixes/drivers/infiniband/ulp/ipoib/ipoib_verbs.c	2008-09-08 13:08:41.000000000 +0300
> @@ -234,7 +234,7 @@ int ipoib_transport_dev_init(struct net_
>  		if (i < UD_POST_RCV_COUNT - 1)
>  			priv->rx_wr_draft[i].next = &priv->rx_wr_draft[i + 1];
>  	}
> -	priv->rx_wr_draft[i].next = NULL;
> +	priv->rx_wr_draft[UD_POST_RCV_COUNT - 1].next = NULL;
>  
>  	if (ipoib_ud_need_sg(priv->max_ib_mtu)) {
>  		for (i = 0; i < UD_POST_RCV_COUNT; ++i) {
> 
> What do you think?

If you're going to keep the setting of the last item to NULL outside the
loop, then you can also remove the if inside the loop as you'll just
overwrite the last entry when you exit the loop.

-- 
Doug Ledford <dledford at redhat.com>
              GPG KeyID: CFBFF194
              http://people.redhat.com/dledford

Infiniband specific RPMs available at
              http://people.redhat.com/dledford/Infiniband

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
URL: <http://lists.openfabrics.org/pipermail/ewg/attachments/20080911/72e1e919/attachment.sig>