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
> > --- a/drivers/infiniband/core/uverbs_ioctl.c
> > +++ 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 ?
diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
index cfbef25b3a73..03feffeab942 100644
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -558,7 +558,7 @@ static int ib_uverbs_cmd_verbs(struct ib_uverbs_file *ufile,
struct uverbs_api *uapi = ufile->device->uapi;
struct radix_tree_iter attrs_iter;
struct bundle_priv *pbundle;
- struct bundle_priv onstack;
+ struct bundle_priv onstack = {0};
void __rcu **slot;
int destroy_ret;
int ret;
@@ -575,7 +575,8 @@ static int ib_uverbs_cmd_verbs(struct ib_uverbs_file *ufile,
method_elm = rcu_dereference_protected(*slot, true);
if (!method_elm->use_stack) {
- pbundle = kmalloc(method_elm->bundle_size, GFP_KERNEL);
+ pbundle = kmalloc(method_elm->bundle_size,
+ GFP_KERNEL | __GFP_ZERO);
if (!pbundle)
return -ENOMEM;
pbundle->internal_avail =
Shamir
