Re: [PATCH for-next 09/14] IB/mlx5: Add support for resize CQ

[Eli Cohen <eli-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>]

On Tue, Jan 14, 2014 at 05:36:50PM +0100, Yann Droneaud wrote:
> > +	if (ib_copy_from_udata(&ucmd, udata, sizeof(ucmd)))
> > +		return -EFAULT;
> > +
> 
> You might also write
> 
>          err = ib_copy_from_udata(&ucmd, udata, sizeof(ucmd));
>          if (err)
>                  return err;
> 
> Then you should check reserved fields being set to the default value:
> As noted by Daniel Vetter in its article "Botching up ioctls"[1]
>   "Check *all* unused fields and flags and all the padding for whether 
>    it's 0, and reject the ioctl if that's not the case. Otherwise your 
>    nice plan for future extensions is going right down the gutters 
>    since someone *will* submit an ioctl struct with random stack 
>    garbage in the yet unused parts. Which then bakes in the ABI that 
>    those fields can never be used for anything else but garbage."
> It's  important to ensure that reserved fields are set to known value,
> so that it will be possible to use them latter to extend the ABI.
> 
> [1] http://blog.ffwll.ch/2013/11/botching-up-ioctls.html
> 
>          if (ucmd.reserved0 || ucmd.reserved1)
>                  return -EINVAL;
> 
It is not likely that someone will pass non-zero values here since
libmlx5 clears and most apps will use it. But I agree with your
comment - thanks for pointing this out. Probably there are other
places that need to be checked.


> > +	}
> > +	mutex_unlock(&cq->resize_mutex);
>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Is everything in this section really critical.
> For example, allocating and setting 'in' structure or releasing the
> ressources could probably move outside the mutex protected section ?
> 

Well, you could move things around to shorten the overall time the
lock is held but that might require structural changes in the code
that will not necessairily fit nice. Resizing a CQ is not a frequent
operation and this lock is used to avoid concurrent attempts of
resizing of the same CQ so I would not invest more effort here.

> >  
> > 
> >  int mlx5_core_modify_cq(struct mlx5_core_dev *dev, struct mlx5_core_cq *cq,
> > -			struct mlx5_modify_cq_mbox_in *in)
> > +			struct mlx5_modify_cq_mbox_in *in, int in_sz)
>                                                             ^^^^^^^^^^
> 
> Should probably be 'unsigned' ? size_t ?
> 
> same here.
> 

The resized value is defined int at the ib core layer so I chose to
follow the same type to avoid need for casting. Maybe a future patch
could change the type all over.

> diff --git a/include/linux/mlx5/device.h b/include/linux/mlx5/device.h
> > index dbb03ca..87e2371 100644
> > --- a/include/linux/mlx5/device.h
> > +++ b/include/linux/mlx5/device.h
> > @@ -710,6 +711,7 @@ struct mlx5_modify_cq_mbox_in {
> >  
> >  struct mlx5_modify_cq_mbox_out {
> >  	struct mlx5_outbox_hdr	hdr;
> > +	u8			rsvd[8];
> >  };
> >  
> >  struct mlx5_enable_hca_mbox_in {
> > 
> 
> It not clear why 8 bytes are needed here ?
> 
This is a requirement of the driver/firmware interface.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html