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

["Zeng, Xin" <xin.zeng@intel.com>]

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.

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);