| Message ID | 20190829111904.16042-3-parav@mellanox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce variable length mdev alias | expand |
On 2019/8/29 19:19, Parav Pandit wrote: > Mdev alias should be unique among all the mdevs, so that when such alias > is used by the mdev users to derive other objects, there is no > collision in a given system. > > Signed-off-by: Parav Pandit <parav@mellanox.com> > > --- > Changelog: > v1->v2: > - Moved alias NULL check at beginning > v0->v1: > - Fixed inclusiong of alias for NULL check > - Added ratelimited debug print for sha1 hash collision error > --- > drivers/vfio/mdev/mdev_core.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c > index 3bdff0469607..c9bf2ac362b9 100644 > --- a/drivers/vfio/mdev/mdev_core.c > +++ b/drivers/vfio/mdev/mdev_core.c > @@ -388,6 +388,13 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, > ret = -EEXIST; > goto mdev_fail; > } > + if (alias && tmp->alias && strcmp(alias, tmp->alias) == 0) { !strcmp(alias, tmp->alias) is a more common kernel pattern. Also if we limit max len of the alias in patch 1, maybe use that to limit the string compare too. > + mutex_unlock(&mdev_list_lock); > + ret = -EEXIST; > + dev_dbg_ratelimited(dev, "Hash collision in alias creation for UUID %pUl\n", > + uuid); > + goto mdev_fail; > + } > } > > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL); >
On Thu, 29 Aug 2019 06:19:00 -0500 Parav Pandit <parav@mellanox.com> wrote: > Mdev alias should be unique among all the mdevs, so that when such alias > is used by the mdev users to derive other objects, there is no > collision in a given system. > > Signed-off-by: Parav Pandit <parav@mellanox.com> > > --- > Changelog: > v1->v2: > - Moved alias NULL check at beginning > v0->v1: > - Fixed inclusiong of alias for NULL check > - Added ratelimited debug print for sha1 hash collision error > --- > drivers/vfio/mdev/mdev_core.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c > index 3bdff0469607..c9bf2ac362b9 100644 > --- a/drivers/vfio/mdev/mdev_core.c > +++ b/drivers/vfio/mdev/mdev_core.c > @@ -388,6 +388,13 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, > ret = -EEXIST; > goto mdev_fail; > } > + if (alias && tmp->alias && strcmp(alias, tmp->alias) == 0) { > + mutex_unlock(&mdev_list_lock); > + ret = -EEXIST; > + dev_dbg_ratelimited(dev, "Hash collision in alias creation for UUID %pUl\n", > + uuid); > + goto mdev_fail; > + } > } > > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL); Any reason not to merge this into the first patch?
> -----Original Message----- > From: Cornelia Huck <cohuck@redhat.com> > Sent: Friday, August 30, 2019 6:11 PM > To: Parav Pandit <parav@mellanox.com> > Cc: alex.williamson@redhat.com; Jiri Pirko <jiri@mellanox.com>; > kwankhede@nvidia.com; davem@davemloft.net; kvm@vger.kernel.org; linux- > kernel@vger.kernel.org; netdev@vger.kernel.org > Subject: Re: [PATCH v2 2/6] mdev: Make mdev alias unique among all mdevs > > On Thu, 29 Aug 2019 06:19:00 -0500 > Parav Pandit <parav@mellanox.com> wrote: > > > Mdev alias should be unique among all the mdevs, so that when such > > alias is used by the mdev users to derive other objects, there is no > > collision in a given system. > > > > Signed-off-by: Parav Pandit <parav@mellanox.com> > > > > --- > > Changelog: > > v1->v2: > > - Moved alias NULL check at beginning > > v0->v1: > > - Fixed inclusiong of alias for NULL check > > - Added ratelimited debug print for sha1 hash collision error > > --- > > drivers/vfio/mdev/mdev_core.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/vfio/mdev/mdev_core.c > > b/drivers/vfio/mdev/mdev_core.c index 3bdff0469607..c9bf2ac362b9 > > 100644 > > --- a/drivers/vfio/mdev/mdev_core.c > > +++ b/drivers/vfio/mdev/mdev_core.c > > @@ -388,6 +388,13 @@ int mdev_device_create(struct kobject *kobj, struct > device *dev, > > ret = -EEXIST; > > goto mdev_fail; > > } > > + if (alias && tmp->alias && strcmp(alias, tmp->alias) == 0) { > > + mutex_unlock(&mdev_list_lock); > > + ret = -EEXIST; > > + dev_dbg_ratelimited(dev, "Hash collision in alias > creation for UUID %pUl\n", > > + uuid); > > + goto mdev_fail; > > + } > > } > > > > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL); > > Any reason not to merge this into the first patch? No. It surely can be merged. Its easy to start with smaller patches instead of splitting. :-) Doing uniqueness comparison was easy to split as independent functionality, so did as 2nd patch. But either way is ok.
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c index 3bdff0469607..c9bf2ac362b9 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -388,6 +388,13 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, ret = -EEXIST; goto mdev_fail; } + if (alias && tmp->alias && strcmp(alias, tmp->alias) == 0) { + mutex_unlock(&mdev_list_lock); + ret = -EEXIST; + dev_dbg_ratelimited(dev, "Hash collision in alias creation for UUID %pUl\n", + uuid); + goto mdev_fail; + } } mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
Mdev alias should be unique among all the mdevs, so that when such alias is used by the mdev users to derive other objects, there is no collision in a given system. Signed-off-by: Parav Pandit <parav@mellanox.com> --- Changelog: v1->v2: - Moved alias NULL check at beginning v0->v1: - Fixed inclusiong of alias for NULL check - Added ratelimited debug print for sha1 hash collision error --- drivers/vfio/mdev/mdev_core.c | 7 +++++++ 1 file changed, 7 insertions(+)