Re: [PATCH net-next 07/19] vfio/mdev: Introduce sha1 based mdev alias

From: Jiri Pirko <jiri@resnulli.us>
To: Parav Pandit <parav@mellanox.com>
Cc: "alex.williamson@redhat.com" <alex.williamson@redhat.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Saeed Mahameed <saeedm@mellanox.com>,
	"kwankhede@nvidia.com" <kwankhede@nvidia.com>,
	"leon@kernel.org" <leon@kernel.org>,
	"cohuck@redhat.com" <cohuck@redhat.com>,
	Jiri Pirko <jiri@mellanox.com>,
	"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>
Subject: Re: [PATCH net-next 07/19] vfio/mdev: Introduce sha1 based mdev alias
Date: Fri, 8 Nov 2019 17:28:03 +0100	[thread overview]
Message-ID: <20191108162803.GO6990@nanopsycho> (raw)
In-Reply-To: <AM0PR05MB48667AF9F6EACF0CE1688262D17B0@AM0PR05MB4866.eurprd05.prod.outlook.com>

Fri, Nov 08, 2019 at 04:59:53PM CET, parav@mellanox.com wrote:

[...]

>> >+	if (parent->ops->get_alias_length) {
>> >+		unsigned int alias_len;
>> >+
>> >+		alias_len = parent->ops->get_alias_length();
>> >+		if (alias_len) {
>> 
>> I think this should be with WARN_ON. Driver should not never return such
>> 0 and if it does, it's a bug.
>>
>Ok. will add it.
> 
>> Also I think this check should be extended by checking value is multiple of 2.
>Do you mean driver must set alias length as always multiple of 2? Why?

Why not? Why would driver want to have even len? If say 11 is too long,
it should return 10. The last byte for even is set by your code
to '0' anyway...

>
>> Then you can avoid the roundup() above. No need to allow even len.
>Did you mean "no need to allow odd"? or? 

Yes, odd.

> 
>> 
>> [...]
>> 
>> >diff --git a/drivers/vfio/mdev/mdev_sysfs.c
>> >b/drivers/vfio/mdev/mdev_sysfs.c index 7570c7602ab4..43afe0e80b76
>> >100644
>> >--- a/drivers/vfio/mdev/mdev_sysfs.c
>> >+++ b/drivers/vfio/mdev/mdev_sysfs.c
>> >@@ -63,15 +63,18 @@ static ssize_t create_store(struct kobject *kobj,
>> struct device *dev,
>> > 		return -ENOMEM;
>> >
>> > 	ret = guid_parse(str, &uuid);
>> >-	kfree(str);
>> > 	if (ret)
>> >-		return ret;
>> >+		goto err;
>> >
>> >-	ret = mdev_device_create(kobj, dev, &uuid);
>> >+	ret = mdev_device_create(kobj, dev, str, &uuid);
>> 
>> Why to pass the same thing twice? Move the guid_parse() call to the
>> beginning of mdev_device_create() function.
>>
>Because alias should be unique and need to hold the lock while searching for duplicate.
>So it is not done twice, and moving guid_parse() won't help due to need of lock.

I'm not saying anything about a lock. Not sure why do you think so.
I'm saying that you pass the same value in 2 args. That's it.
Better to pass it as char* only and process it inside.
If by guid_parse() or otherwise, does not matter. That is my point.

> 
>> 
>> > 	if (ret)
>> >-		return ret;
>> >+		goto err;
>> >
>> >-	return count;
>> >+	ret = count;
>> >+
>> >+err:
>> >+	kfree(str);
>> >+	return ret;
>> > }
>> >
>> > MDEV_TYPE_ATTR_WO(create);
>> 
>> [...]