Hi Eli, Le mercredi 15 janvier 2014 à 09:33 +0200, Eli Cohen a écrit : > 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;
I'd like to have your opinion on this change. I'm going to patch every call to ib_copy_from_udata() to follow my proposed scheme (eg. returned error code from ib_copy_from_udata() instead of hard coded error value). > > > > 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. Is libmlx5/libibverbs the ABI ? Unfortunately, anybody is allowed to access the kernel uverbs API directly, so we must take care of the kernel ABI, just in case. > But I agree with your > comment - thanks for pointing this out. Probably there are other > places that need to be checked. > For code not yet part of a released kernel version, we can fix that. But for all other, it would require proper checking/thinking before rejecting reserved field not set to 0 since it might theoterically break existing userspace program: it will be a departure from previous ABI. > > > > + } > > > + 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. > I agree. I would found more readable to have the two locks held next each other. YMMV. > > > > > > > > > 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. > But it's the size of struct mlx5_modify_cq_mbox_in, not the number of 'cqe' to resize the cq to. > > 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. It's a bit of magic :( Regards. -- Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
