Re: [PATCH 05/14] vfio/mdev: simplify mdev_type handling

[Eric Farman <farman@linux.ibm.com>]

On Tue, 2022-08-23 at 05:53 +0000, Zeng, Xin wrote:
> On Monday, August 22, 2022 2:22 PM, Christoph Hellwig <hch@lst.de>
> wrote:
> 
> >  /*
> >   * Used in mdev_type_attribute sysfs functions to return the
> > parent struct
> >   * device
> > @@ -85,6 +65,8 @@ static int mdev_device_remove_cb(struct device
> > *dev,
> > void *data)
> >   * @parent: parent structure registered
> >   * @dev: device structure representing parent device.
> >   * @mdev_driver: Device driver to bind to the newly created mdev
> > + * @types: Array of supported mdev types
> > + * @nr_types: Number of entries in @types
> >   *
> >   * Registers the @parent stucture as a parent for mdev types and
> > thus mdev
> >   * devices.  The caller needs to hold a reference on @dev that
> > must not be
> > @@ -93,20 +75,19 @@ static int mdev_device_remove_cb(struct device
> > *dev, void *data)
> >   * Returns a negative value on error, otherwise 0.
> >   */
> >  int mdev_register_parent(struct mdev_parent *parent, struct device
> > *dev,
> > -               struct mdev_driver *mdev_driver)
> > +               struct mdev_driver *mdev_driver, struct mdev_type
> > **types,
> > +               unsigned int nr_types)
> >  {
> >         char *env_string = "MDEV_STATE=registered";
> >         char *envp[] = { env_string, NULL };
> >         int ret;
> > 
> > -       /* check for mandatory ops */
> > -       if (!mdev_driver->supported_type_groups)
> > -               return -EINVAL;
> > -
> >         memset(parent, 0, sizeof(*parent));
> >         init_rwsem(&parent->unreg_sem);
> >         parent->dev = dev;
> >         parent->mdev_driver = mdev_driver;
> > +       parent->types = types;
> 
> This would potentially introduce a bug. Types is passed from the
> parent and memory reserved for it is
> managed by the parent driver, while if you are doing so, it will be
> freed when types->kobj is released in 
> mdev module, i.e. in mdev_type_release, types will be freed as a
> chunk of memory in heap. 
> This will lead to unpredictable behavior and require a fix,  either
> in here or in mdev_type_release.

I can confirm that this is still broken.

https://lore.kernel.org/r/65746aea193d4a814f895eca4b00b72cf29ac8f9.camel@linux.ibm.com/

Thanks,
Eric

> 
> Thanks,
> Xin
> 
> > +       parent->nr_types = nr_types;
> > 
> >         if (!mdev_bus_compat_class) {
> >                 mdev_bus_compat_class =
> > class_compat_register("mdev_bus");
> > +static int mdev_type_add(struct mdev_parent *parent, struct
> > mdev_type
> > *type)
> >  {
> > -       struct mdev_type *type;
> > -       struct attribute_group *group =
> > -               parent->mdev_driver-
> > > supported_type_groups[type_group_id];
> >         int ret;
> > 
> > -       if (!group->name) {
> > -               pr_err("%s: Type name empty!\n", __func__);
> > -               return ERR_PTR(-EINVAL);
> > -       }
> > -
> > -       type = kzalloc(sizeof(*type), GFP_KERNEL);
> > -       if (!type)
> > -               return ERR_PTR(-ENOMEM);
> > -
> >         type->kobj.kset = parent->mdev_types_kset;
> >         type->parent = parent;
> >         /* Pairs with the put in mdev_type_release() */
> >         get_device(parent->dev);
> > -       type->type_group_id = type_group_id;
> > 
> >         ret = kobject_init_and_add(&type->kobj, &mdev_type_ktype,
> > NULL,
> >                                    "%s-%s",
> > dev_driver_string(parent->dev),
> > -                                  group->name);
> > +                                  type->sysfs_name);
> >         if (ret) {
> >                 kobject_put(&type->kobj);
> > -               return ERR_PTR(ret);
> > +               return ret;
> >         }
> > 
> >         ret = sysfs_create_file(&type->kobj,
> > &mdev_type_attr_create.attr);