On Tue, 2022-08-23 at 05:53 +0000, Zeng, Xin wrote:
> On Monday, August 22, 2022 2:22 PM, Christoph Hellwig <hch@xxxxxx>
> 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@xxxxxxxxxxxxx/
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);
