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
>  - [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(_list_lock);
>  
>   dev_dbg(>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 *uuid)
>  {
>