RE: [PATCH v1 1/5] mdev: Introduce sha1 based mdev alias

2019-08-29 Thread Parav Pandit 



> -Original Message-
> From: Alex Williamson 
> Sent: Thursday, August 29, 2019 3:05 AM
> To: Parav Pandit 
> Cc: Jiri Pirko ; kwankh...@nvidia.com;
> coh...@redhat.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH v1 1/5] mdev: Introduce sha1 based mdev alias
> 
> On Wed, 28 Aug 2019 15:25:44 -0600
> Alex Williamson  wrote:
> 
> > On Tue, 27 Aug 2019 14:16:50 -0500
> > Parav Pandit  wrote:
> > >  module_init(mdev_init)
> > > diff --git a/drivers/vfio/mdev/mdev_private.h
> b/drivers/vfio/mdev/mdev_private.h
> > > index 7d922950caaf..cf1c0d9842c6 100644
> > > --- a/drivers/vfio/mdev/mdev_private.h
> > > +++ b/drivers/vfio/mdev/mdev_private.h
> > > @@ -33,6 +33,7 @@ struct mdev_device {
> > >   struct kobject *type_kobj;
> > >   struct device *iommu_device;
> > >   bool active;
> > > + const char *alias;
> 
> Nit, put this above active to avoid creating a hole in the structure.
> Thanks,
> 
Ack.

> Alex

RE: [PATCH v1 1/5] mdev: Introduce sha1 based mdev alias

2019-08-29 Thread Parav Pandit 



> -Original Message-
> From: Alex Williamson 
> Sent: Thursday, August 29, 2019 2:56 AM
> To: Parav Pandit 
> Cc: Jiri Pirko ; kwankh...@nvidia.com;
> coh...@redhat.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH v1 1/5] mdev: Introduce sha1 based mdev alias
> 

> > +   /* Allocate and init descriptor */
> > +   hash_desc = kvzalloc(sizeof(*hash_desc) +
> > +crypto_shash_descsize(alias_hash),
> > +GFP_KERNEL);
> > +   if (!hash_desc)
> > +   goto desc_err;
> > +
> > +   hash_desc->tfm = alias_hash;
> > +
> > +   digest_size = crypto_shash_digestsize(alias_hash);
> > +
> > +   digest = kzalloc(digest_size, GFP_KERNEL);
> > +   if (!digest) {
> > +   ret = -ENOMEM;
> > +   goto digest_err;
> > +   }
> > +   crypto_shash_init(hash_desc);
> > +   crypto_shash_update(hash_desc, uuid, UUID_STRING_LEN);
> > +   crypto_shash_final(hash_desc, digest);
> 
> All of these can fail and many, if not most, of the callers appear that they 
> might
> test the return value.  Thanks,
Right. Changing the signature and honoring return value in v2.

> 
> Alex

Re: [PATCH v1 1/5] mdev: Introduce sha1 based mdev alias

2019-08-28 Thread Alex Williamson 
On Wed, 28 Aug 2019 15:25:44 -0600
Alex Williamson  wrote:

> On Tue, 27 Aug 2019 14:16:50 -0500
> Parav Pandit  wrote:
> >  module_init(mdev_init)
> > diff --git a/drivers/vfio/mdev/mdev_private.h 
> > b/drivers/vfio/mdev/mdev_private.h
> > index 7d922950caaf..cf1c0d9842c6 100644
> > --- a/drivers/vfio/mdev/mdev_private.h
> > +++ b/drivers/vfio/mdev/mdev_private.h
> > @@ -33,6 +33,7 @@ struct mdev_device {
> > struct kobject *type_kobj;
> > struct device *iommu_device;
> > bool active;
> > +   const char *alias;

Nit, put this above active to avoid creating a hole in the structure.
Thanks,

Alex

Re: [PATCH v1 1/5] mdev: Introduce sha1 based mdev alias

2019-08-28 Thread Alex Williamson 
On Tue, 27 Aug 2019 14:16:50 -0500
Parav Pandit  wrote:

> Some vendor drivers want an identifier for an mdev device that is
> shorter than the UUID, due to length restrictions in the consumers of
> that identifier.
> 
> Add a callback that allows a vendor driver to request an alias of a
> specified length to be generated for an mdev device. If generated,
> that alias is checked for collisions.
> 
> It is an optional attribute.
> mdev alias is generated using sha1 from the mdev name.
> 
> Signed-off-by: Parav Pandit 
> 
> ---
> Changelog:
> 
> v0->v1:
>  - Moved alias length check outside of the parent lock
>  - Moved alias and digest allocation from kvzalloc to kzalloc
>  - &alias[0] changed to alias
>  - alias_length check is nested under get_alias_length callback check
>  - Changed comments to start with an empty line
>  - Fixed cleaunup of hash if mdev_bus_register() fails
>  - Added comment where alias memory ownership is handed over to mdev device
>  - Updated commit log to indicate motivation for this feature
> ---
>  drivers/vfio/mdev/mdev_core.c| 110 ++-
>  drivers/vfio/mdev/mdev_private.h |   5 +-
>  drivers/vfio/mdev/mdev_sysfs.c   |  13 ++--
>  include/linux/mdev.h |   4 ++
>  4 files changed, 122 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index b558d4cfd082..62d29f57fe0c 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -10,9 +10,11 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "mdev_private.h"
>  
> @@ -27,6 +29,8 @@ static struct class_compat *mdev_bus_compat_class;
>  static LIST_HEAD(mdev_list);
>  static DEFINE_MUTEX(mdev_list_lock);
>  
> +static struct crypto_shash *alias_hash;
> +
>  struct device *mdev_parent_dev(struct mdev_device *mdev)
>  {
>   return mdev->parent->dev;
> @@ -150,6 +154,16 @@ int mdev_register_device(struct device *dev, const 
> struct mdev_parent_ops *ops)
>   if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
>   return -EINVAL;
>  
> + if (ops->get_alias_length) {
> + unsigned int digest_size;
> + unsigned int aligned_len;
> +
> + aligned_len = roundup(ops->get_alias_length(), 2);
> + digest_size = crypto_shash_digestsize(alias_hash);
> + if (aligned_len / 2 > digest_size)
> + return -EINVAL;
> + }
> +
>   dev = get_device(dev);
>   if (!dev)
>   return -EINVAL;
> @@ -259,6 +273,7 @@ static void mdev_device_free(struct mdev_device *mdev)
>   mutex_unlock(&mdev_list_lock);
>  
>   dev_dbg(&mdev->dev, "MDEV: destroying\n");
> + kfree(mdev->alias);
>   kfree(mdev);
>  }
>  
> @@ -269,18 +284,88 @@ static void mdev_device_release(struct device *dev)
>   mdev_device_free(mdev);
>  }
>  
> -int mdev_device_create(struct kobject *kobj,
> -struct device *dev, const guid_t *uuid)
> +static const char *
> +generate_alias(const char *uuid, unsigned int max_alias_len)
> +{
> + struct shash_desc *hash_desc;
> + unsigned int digest_size;
> + unsigned char *digest;
> + unsigned int alias_len;
> + char *alias;
> + int ret = 0;
> +
> + /*
> +  * Align to multiple of 2 as bin2hex will generate
> +  * even number of bytes.
> +  */
> + alias_len = roundup(max_alias_len, 2);
> + alias = kzalloc(alias_len + 1, GFP_KERNEL);
> + if (!alias)
> + return NULL;
> +
> + /* Allocate and init descriptor */
> + hash_desc = kvzalloc(sizeof(*hash_desc) +
> +  crypto_shash_descsize(alias_hash),
> +  GFP_KERNEL);
> + if (!hash_desc)
> + goto desc_err;
> +
> + hash_desc->tfm = alias_hash;
> +
> + digest_size = crypto_shash_digestsize(alias_hash);
> +
> + digest = kzalloc(digest_size, GFP_KERNEL);
> + if (!digest) {
> + ret = -ENOMEM;
> + goto digest_err;
> + }
> + crypto_shash_init(hash_desc);
> + crypto_shash_update(hash_desc, uuid, UUID_STRING_LEN);
> + crypto_shash_final(hash_desc, digest);

All of these can fail and many, if not most, of the callers appear
that they might test the return value.  Thanks,

Alex

> + bin2hex(alias, digest, min_t(unsigned int, digest_size, alias_len / 2));
> + /*
> +  * When alias length is odd, zero out and additional last byte
> +  * that bin2hex has copied.
> +  */
> + if (max_alias_len % 2)
> + alias[max_alias_len] = 0;
> +
> + kfree(digest);
> + kvfree(hash_desc);
> + return alias;
> +
> +digest_err:
> + kvfree(hash_desc);
> +desc_err:
> + kfree(alias);
> + return NULL;
> +}
> +
> +int mdev_device_create(struct kobject *kobj, struct device *dev,
> +const char *uuid_str, const guid_t

[PATCH v1 1/5] mdev: Introduce sha1 based mdev alias

2019-08-27 Thread Parav Pandit 
Some vendor drivers want an identifier for an mdev device that is
shorter than the UUID, due to length restrictions in the consumers of
that identifier.

Add a callback that allows a vendor driver to request an alias of a
specified length to be generated for an mdev device. If generated,
that alias is checked for collisions.

It is an optional attribute.
mdev alias is generated using sha1 from the mdev name.

Signed-off-by: Parav Pandit 

---
Changelog:

v0->v1:
 - Moved alias length check outside of the parent lock
 - Moved alias and digest allocation from kvzalloc to kzalloc
 - &alias[0] changed to alias
 - alias_length check is nested under get_alias_length callback check
 - Changed comments to start with an empty line
 - Fixed cleaunup of hash if mdev_bus_register() fails
 - Added comment where alias memory ownership is handed over to mdev device
 - Updated commit log to indicate motivation for this feature
---
 drivers/vfio/mdev/mdev_core.c| 110 ++-
 drivers/vfio/mdev/mdev_private.h |   5 +-
 drivers/vfio/mdev/mdev_sysfs.c   |  13 ++--
 include/linux/mdev.h |   4 ++
 4 files changed, 122 insertions(+), 10 deletions(-)

diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index b558d4cfd082..62d29f57fe0c 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -10,9 +10,11 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
+#include 
 
 #include "mdev_private.h"
 
@@ -27,6 +29,8 @@ static struct class_compat *mdev_bus_compat_class;
 static LIST_HEAD(mdev_list);
 static DEFINE_MUTEX(mdev_list_lock);
 
+static struct crypto_shash *alias_hash;
+
 struct device *mdev_parent_dev(struct mdev_device *mdev)
 {
return mdev->parent->dev;
@@ -150,6 +154,16 @@ int mdev_register_device(struct device *dev, const struct 
mdev_parent_ops *ops)
if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
return -EINVAL;
 
+   if (ops->get_alias_length) {
+   unsigned int digest_size;
+   unsigned int aligned_len;
+
+   aligned_len = roundup(ops->get_alias_length(), 2);
+   digest_size = crypto_shash_digestsize(alias_hash);
+   if (aligned_len / 2 > digest_size)
+   return -EINVAL;
+   }
+
dev = get_device(dev);
if (!dev)
return -EINVAL;
@@ -259,6 +273,7 @@ static void mdev_device_free(struct mdev_device *mdev)
mutex_unlock(&mdev_list_lock);
 
dev_dbg(&mdev->dev, "MDEV: destroying\n");
+   kfree(mdev->alias);
kfree(mdev);
 }
 
@@ -269,18 +284,88 @@ static void mdev_device_release(struct device *dev)
mdev_device_free(mdev);
 }
 
-int mdev_device_create(struct kobject *kobj,
-  struct device *dev, const guid_t *uuid)
+static const char *
+generate_alias(const char *uuid, unsigned int max_alias_len)
+{
+   struct shash_desc *hash_desc;
+   unsigned int digest_size;
+   unsigned char *digest;
+   unsigned int alias_len;
+   char *alias;
+   int ret = 0;
+
+   /*
+* Align to multiple of 2 as bin2hex will generate
+* even number of bytes.
+*/
+   alias_len = roundup(max_alias_len, 2);
+   alias = kzalloc(alias_len + 1, GFP_KERNEL);
+   if (!alias)
+   return NULL;
+
+   /* Allocate and init descriptor */
+   hash_desc = kvzalloc(sizeof(*hash_desc) +
+crypto_shash_descsize(alias_hash),
+GFP_KERNEL);
+   if (!hash_desc)
+   goto desc_err;
+
+   hash_desc->tfm = alias_hash;
+
+   digest_size = crypto_shash_digestsize(alias_hash);
+
+   digest = kzalloc(digest_size, GFP_KERNEL);
+   if (!digest) {
+   ret = -ENOMEM;
+   goto digest_err;
+   }
+   crypto_shash_init(hash_desc);
+   crypto_shash_update(hash_desc, uuid, UUID_STRING_LEN);
+   crypto_shash_final(hash_desc, digest);
+   bin2hex(alias, digest, min_t(unsigned int, digest_size, alias_len / 2));
+   /*
+* When alias length is odd, zero out and additional last byte
+* that bin2hex has copied.
+*/
+   if (max_alias_len % 2)
+   alias[max_alias_len] = 0;
+
+   kfree(digest);
+   kvfree(hash_desc);
+   return alias;
+
+digest_err:
+   kvfree(hash_desc);
+desc_err:
+   kfree(alias);
+   return NULL;
+}
+
+int mdev_device_create(struct kobject *kobj, struct device *dev,
+  const char *uuid_str, const guid_t *uuid)
 {
int ret;
struct mdev_device *mdev, *tmp;
struct mdev_parent *parent;
struct mdev_type *type = to_mdev_type(kobj);
+   const char *alias = NULL;
 
parent = mdev_get_parent(type->parent);
if (!parent)
return -EINVAL;
 
+   if (parent->ops->get_alias_length) {
+