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
