On Tue, May 21, 2019 at 10:05:44PM +0300, Gal Pressman wrote:
> 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?
Because very little was ever doing anything with the return codes. In
nearly all cases it just caused a memory leak.
So I want to say not being able to destroy is a catastrophic error
and the driver must cope with it, however it deems fit. WARN_ON & leak
or fatal error the device, or whatever it wants.
But the ULPs will not be exposed to this, and don't contain any code
to handle it.
> > 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.
It isn't necessary now, and probably never will be as everyone should
use the ioctl flow which already has this split validation approach.
Jason
