On 21/05/2019 20:35, Jason Gunthorpe wrote:
> On Mon, May 20, 2019 at 05:24:43PM +0300, Gal Pressman wrote:
>
>>>>> Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
>>>>> drivers/infiniband/hw/efa/efa_verbs.c | 24 ------------------------
>>>>> 1 file changed, 24 deletions(-)
>>>>>
>>>>> diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c
>>>>> index 6d6886c9009f..4999a74cee24 100644
>>>>> +++ b/drivers/infiniband/hw/efa/efa_verbs.c
>>>>> @@ -436,12 +436,6 @@ void efa_dealloc_pd(struct ib_pd *ibpd, struct ib_udata *udata)
>>>>> struct efa_dev *dev = to_edev(ibpd->device);
>>>>> struct efa_pd *pd = to_epd(ibpd);
>>>>>
>>>>> - if (udata->inlen &&
>>>>> - !ib_is_udata_cleared(udata, 0, udata->inlen)) {
>>>>> - ibdev_dbg(&dev->ibdev, "Incompatible ABI params\n");
>>>>> - return;
>>>>> - }
>
> Regardless of the issue of udata validity, these checks still cannot
> be here.
>
> We are moving the whole core to not return error codes from driver
> object destroy - because destroy is not allowed to fail in many flows.
What is the reason for that?
>
> So, drivers do not have the option to validate the udata and fail
> destroy at this point. If it ever comes up, then we will need to split
> validation into another step on the uapi path that is done before
> invoking the actual destroy function.
Is it really necessary? The udata in these flows is new, so there's no reason
any driver won't be able to work with a cleared udata and fail the validations,
even if it expands it in the future.
>
> Generally speaking this means a driver should never use a classical
> udata for destroy.
>
> Instead its provider should call destroy via the new-style ioctl API
> and the kernel should define a proper schema that is checked before we
> even reach the driver.
Sounds good, agree that the checks should be removed for now.
Thanks Leon,
Acked-by: Gal Pressman <galpress@xxxxxxxxxx>
