On 24-Apr-19 15:52, Jason Gunthorpe wrote:
> On Wed, Apr 24, 2019 at 03:37:42PM +0300, Shamir Rabinovitch wrote:
>> On Mon, Apr 22, 2019 at 09:00:36PM +0300, Gal Pressman wrote:
>>> On 22-Apr-19 20:22, Jason Gunthorpe wrote:
>>>> On Wed, Apr 17, 2019 at 11:02:38AM +0300, Gal Pressman wrote:
>>>>> On 31-Mar-19 19:10, Shamir Rabinovitch wrote:
>>>>>> the uverbs_attr_bundle with the ucontext is sent down to the drivers
>>>>>> ib_x destroy path as ib_udata. next patch will use the ib_udata to
>>>>>> free the drivers destroy path from the dependency in 'uobject->context'
>>>>>> as we already did for the create path.
>>>>>>
>>>>>> Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@xxxxxxxxxx>
>>>>>
>>>>> Hi Shamir,
>>>>> When running over for-next branch (with this patch), it looks like the driver
>>>>> udata passed to dereg_mr callback is corrupted (would expect it to
>>>>> be cleared),
>>>>
>>>> Cleared as in equal to NULL or cleared as in the values of the
>>>> structure don't make sense?
>>>
>>> I expect a valid pointer (not NULL) to a zeroed struct, instead I get a pointer
>>> to a struct with random values (invalid pointer/uninitialized struct).
>>
>> Gal, I think that you have valid udata but with uninitialized content.
>> Please look in ib_uverbs_cmd_verbs and see how the bundle_priv is
>> allocated for the ioctl command from stack / kmalloc. In both cases
>> the memory is not zeroed but still valid.
>>
>>> This does happen in other destroy callbacks as well apparently, in contrary to
>>> what my previous mail stated.
>>>
>>>>
>>>> Hum. There is this weird thing where methods that don't have a udata
>>>> defined don't always get a filled in udata pointer.
>>>>
>>>> Does this fix it?
>>>>
>>>> diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
>>>> index cfbef25b3a73af..829b0c6944d842 100644
>>>> +++ b/drivers/infiniband/core/uverbs_ioctl.c
>>>> @@ -453,6 +453,8 @@ static int ib_uverbs_run_method(struct bundle_priv *pbundle,
>>>> uverbs_fill_udata(&pbundle->bundle,
>>>> &pbundle->bundle.driver_udata,
>>>> UVERBS_ATTR_UHW_IN, UVERBS_ATTR_UHW_OUT);
>>>> + else
>>>> + pbundle->bundle.driver_udata = (struct ib_udata){};
>>>>
>>>> if (destroy_bkey != UVERBS_API_ATTR_BKEY_LEN) {
>>>> struct uverbs_obj_attr *destroy_attr =
>>>>
>>>
>>> I'll test, thanks.
>>
>> Maybe this can zero the udata as you requested ?
>
> We don't need to zero the whole thing no the system call fast path..
Jason, are you going to send a patch with your proposed fix?
