| Message ID | 20190826204119.54386-3-parav@mellanox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce variable length mdev alias | expand |
On 8/26/19 1:41 PM, 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> > --- > drivers/vfio/mdev/mdev_core.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c > index e825ff38b037..6eb37f0c6369 100644 > --- a/drivers/vfio/mdev/mdev_core.c > +++ b/drivers/vfio/mdev/mdev_core.c > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, > ret = -EEXIST; > goto mdev_fail; > } > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) { alias can be NULL here no? > + mutex_unlock(&mdev_list_lock); > + ret = -EEXIST; > + goto mdev_fail; > + } > } > > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL); > Mark
Hi Mark, > -----Original Message----- > From: Mark Bloch <markb@mellanox.com> > Sent: Tuesday, August 27, 2019 4:32 AM > To: Parav Pandit <parav@mellanox.com>; alex.williamson@redhat.com; Jiri > Pirko <jiri@mellanox.com>; kwankhede@nvidia.com; cohuck@redhat.com; > davem@davemloft.net > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; > netdev@vger.kernel.org > Subject: Re: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs > > > > On 8/26/19 1:41 PM, 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> > > --- > > drivers/vfio/mdev/mdev_core.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/vfio/mdev/mdev_core.c > > b/drivers/vfio/mdev/mdev_core.c index e825ff38b037..6eb37f0c6369 > > 100644 > > --- a/drivers/vfio/mdev/mdev_core.c > > +++ b/drivers/vfio/mdev/mdev_core.c > > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj, > struct device *dev, > > ret = -EEXIST; > > goto mdev_fail; > > } > > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) { > > alias can be NULL here no? > If alias is NULL, tmp->alias would also be null because for given parent either we have alias or we don’t. So its not possible to have tmp->alias as null and alias as non null. But it may be good/defensive to add check for both. > > + mutex_unlock(&mdev_list_lock); > > + ret = -EEXIST; > > + goto mdev_fail; > > + } > > } > > > > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL); > > > > Mark
On Mon, 26 Aug 2019 15:41:17 -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> > --- > drivers/vfio/mdev/mdev_core.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c > index e825ff38b037..6eb37f0c6369 100644 > --- a/drivers/vfio/mdev/mdev_core.c > +++ b/drivers/vfio/mdev/mdev_core.c > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, > ret = -EEXIST; > goto mdev_fail; > } > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) { Any way we can relay to the caller that the uuid was fine, but that we had a hash collision? Duplicate uuids are much more obvious than a collision here. > + mutex_unlock(&mdev_list_lock); > + ret = -EEXIST; > + goto mdev_fail; > + } > } > > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> -----Original Message----- > From: Cornelia Huck <cohuck@redhat.com> > Sent: Tuesday, August 27, 2019 3:59 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 2/4] mdev: Make mdev alias unique among all mdevs > > On Mon, 26 Aug 2019 15:41:17 -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> > > --- > > drivers/vfio/mdev/mdev_core.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/vfio/mdev/mdev_core.c > > b/drivers/vfio/mdev/mdev_core.c index e825ff38b037..6eb37f0c6369 > > 100644 > > --- a/drivers/vfio/mdev/mdev_core.c > > +++ b/drivers/vfio/mdev/mdev_core.c > > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj, struct > device *dev, > > ret = -EEXIST; > > goto mdev_fail; > > } > > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) { > > Any way we can relay to the caller that the uuid was fine, but that we had a > hash collision? Duplicate uuids are much more obvious than a collision here. > How do you want to relay this rare event? Netlink interface has way to return the error message back, but sysfs is limited due to its error code based interface. > > + mutex_unlock(&mdev_list_lock); > > + ret = -EEXIST; > > + goto mdev_fail; > > + } > > } > > > > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
On Tue, 27 Aug 2019 11:08:59 +0000 Parav Pandit <parav@mellanox.com> wrote: > > -----Original Message----- > > From: Cornelia Huck <cohuck@redhat.com> > > Sent: Tuesday, August 27, 2019 3:59 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 2/4] mdev: Make mdev alias unique among all mdevs > > > > On Mon, 26 Aug 2019 15:41:17 -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> > > > --- > > > drivers/vfio/mdev/mdev_core.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/vfio/mdev/mdev_core.c > > > b/drivers/vfio/mdev/mdev_core.c index e825ff38b037..6eb37f0c6369 > > > 100644 > > > --- a/drivers/vfio/mdev/mdev_core.c > > > +++ b/drivers/vfio/mdev/mdev_core.c > > > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj, struct > > device *dev, > > > ret = -EEXIST; > > > goto mdev_fail; > > > } > > > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) { > > > > Any way we can relay to the caller that the uuid was fine, but that we had a > > hash collision? Duplicate uuids are much more obvious than a collision here. > > > How do you want to relay this rare event? > Netlink interface has way to return the error message back, but sysfs is limited due to its error code based interface. I don't know, that's why I asked :) The problem is that "uuid already used" and "hash collision" are indistinguishable. While "use a different uuid" will probably work in both cases, "increase alias length" might be a good alternative in some cases. But if there is no good way to relay the problem, we can live with it. > > > > + mutex_unlock(&mdev_list_lock); > > > + ret = -EEXIST; > > > + goto mdev_fail; > > > + } > > > } > > > > > > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL); >
On Tue, 27 Aug 2019 04:28:37 +0000 Parav Pandit <parav@mellanox.com> wrote: > Hi Mark, > > > -----Original Message----- > > From: Mark Bloch <markb@mellanox.com> > > Sent: Tuesday, August 27, 2019 4:32 AM > > To: Parav Pandit <parav@mellanox.com>; alex.williamson@redhat.com; Jiri > > Pirko <jiri@mellanox.com>; kwankhede@nvidia.com; cohuck@redhat.com; > > davem@davemloft.net > > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; > > netdev@vger.kernel.org > > Subject: Re: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs > > > > > > > > On 8/26/19 1:41 PM, 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> > > > --- > > > drivers/vfio/mdev/mdev_core.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/vfio/mdev/mdev_core.c > > > b/drivers/vfio/mdev/mdev_core.c index e825ff38b037..6eb37f0c6369 > > > 100644 > > > --- a/drivers/vfio/mdev/mdev_core.c > > > +++ b/drivers/vfio/mdev/mdev_core.c > > > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj, > > struct device *dev, > > > ret = -EEXIST; > > > goto mdev_fail; > > > } > > > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) { > > > > alias can be NULL here no? > > > If alias is NULL, tmp->alias would also be null because for given parent either we have alias or we don’t. > So its not possible to have tmp->alias as null and alias as non null. > But it may be good/defensive to add check for both. mdev_list is a global list of all mdev devices, how can we make any assumptions that an element has the same parent? Thanks, Alex > > > + mutex_unlock(&mdev_list_lock); > > > + ret = -EEXIST; > > > + goto mdev_fail; > > > + } > > > } > > > > > > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL); > > > > > > > Mark
On Tue, 27 Aug 2019 13:29:46 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Tue, 27 Aug 2019 11:08:59 +0000 > Parav Pandit <parav@mellanox.com> wrote: > > > > -----Original Message----- > > > From: Cornelia Huck <cohuck@redhat.com> > > > Sent: Tuesday, August 27, 2019 3:59 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 2/4] mdev: Make mdev alias unique among all mdevs > > > > > > On Mon, 26 Aug 2019 15:41:17 -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> > > > > --- > > > > drivers/vfio/mdev/mdev_core.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/drivers/vfio/mdev/mdev_core.c > > > > b/drivers/vfio/mdev/mdev_core.c index e825ff38b037..6eb37f0c6369 > > > > 100644 > > > > --- a/drivers/vfio/mdev/mdev_core.c > > > > +++ b/drivers/vfio/mdev/mdev_core.c > > > > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj, struct > > > device *dev, > > > > ret = -EEXIST; > > > > goto mdev_fail; > > > > } > > > > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) { > > > > > > Any way we can relay to the caller that the uuid was fine, but that we had a > > > hash collision? Duplicate uuids are much more obvious than a collision here. > > > > > How do you want to relay this rare event? > > Netlink interface has way to return the error message back, but sysfs is limited due to its error code based interface. > > I don't know, that's why I asked :) > > The problem is that "uuid already used" and "hash collision" are > indistinguishable. While "use a different uuid" will probably work in > both cases, "increase alias length" might be a good alternative in some > cases. > > But if there is no good way to relay the problem, we can live with it. It's a rare event, maybe just dev_dbg(dev, "Hash collision creating alias \"%s\" for mdev device %pUl\n",... Thanks, Alex > > > > + mutex_unlock(&mdev_list_lock); > > > > + ret = -EEXIST; > > > > + goto mdev_fail; > > > > + } > > > > } > > > > > > > > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL); > > >
On Tue, 27 Aug 2019 09:28:55 -0600 Alex Williamson <alex.williamson@redhat.com> wrote: > On Tue, 27 Aug 2019 13:29:46 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > > > On Tue, 27 Aug 2019 11:08:59 +0000 > > Parav Pandit <parav@mellanox.com> wrote: > > > > > > -----Original Message----- > > > > From: Cornelia Huck <cohuck@redhat.com> > > > > Sent: Tuesday, August 27, 2019 3:59 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 2/4] mdev: Make mdev alias unique among all mdevs > > > > > > > > On Mon, 26 Aug 2019 15:41:17 -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> > > > > > --- > > > > > drivers/vfio/mdev/mdev_core.c | 5 +++++ > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > diff --git a/drivers/vfio/mdev/mdev_core.c > > > > > b/drivers/vfio/mdev/mdev_core.c index e825ff38b037..6eb37f0c6369 > > > > > 100644 > > > > > --- a/drivers/vfio/mdev/mdev_core.c > > > > > +++ b/drivers/vfio/mdev/mdev_core.c > > > > > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj, struct > > > > device *dev, > > > > > ret = -EEXIST; > > > > > goto mdev_fail; > > > > > } > > > > > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) { > > > > > > > > Any way we can relay to the caller that the uuid was fine, but that we had a > > > > hash collision? Duplicate uuids are much more obvious than a collision here. > > > > > > > How do you want to relay this rare event? > > > Netlink interface has way to return the error message back, but sysfs is limited due to its error code based interface. > > > > I don't know, that's why I asked :) > > > > The problem is that "uuid already used" and "hash collision" are > > indistinguishable. While "use a different uuid" will probably work in > > both cases, "increase alias length" might be a good alternative in some > > cases. > > > > But if there is no good way to relay the problem, we can live with it. > > It's a rare event, maybe just dev_dbg(dev, "Hash collision creating alias \"%s\" for mdev device %pUl\n",... Sounds good to me.
> -----Original Message----- > From: Alex Williamson <alex.williamson@redhat.com> > Sent: Tuesday, August 27, 2019 8:59 PM > To: Cornelia Huck <cohuck@redhat.com> > Cc: Parav Pandit <parav@mellanox.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 2/4] mdev: Make mdev alias unique among all mdevs > > On Tue, 27 Aug 2019 13:29:46 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > > > On Tue, 27 Aug 2019 11:08:59 +0000 > > Parav Pandit <parav@mellanox.com> wrote: > > > > > > -----Original Message----- > > > > From: Cornelia Huck <cohuck@redhat.com> > > > > Sent: Tuesday, August 27, 2019 3:59 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 2/4] mdev: Make mdev alias unique among all > > > > mdevs > > > > > > > > On Mon, 26 Aug 2019 15:41:17 -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> > > > > > --- > > > > > drivers/vfio/mdev/mdev_core.c | 5 +++++ > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > diff --git a/drivers/vfio/mdev/mdev_core.c > > > > > b/drivers/vfio/mdev/mdev_core.c index e825ff38b037..6eb37f0c6369 > > > > > 100644 > > > > > --- a/drivers/vfio/mdev/mdev_core.c > > > > > +++ b/drivers/vfio/mdev/mdev_core.c > > > > > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj, > struct > > > > device *dev, > > > > > ret = -EEXIST; > > > > > goto mdev_fail; > > > > > } > > > > > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) { > > > > > > > > Any way we can relay to the caller that the uuid was fine, but > > > > that we had a hash collision? Duplicate uuids are much more obvious than > a collision here. > > > > > > > How do you want to relay this rare event? > > > Netlink interface has way to return the error message back, but sysfs is > limited due to its error code based interface. > > > > I don't know, that's why I asked :) > > > > The problem is that "uuid already used" and "hash collision" are > > indistinguishable. While "use a different uuid" will probably work in > > both cases, "increase alias length" might be a good alternative in > > some cases. > > > > But if there is no good way to relay the problem, we can live with it. > > It's a rare event, maybe just dev_dbg(dev, "Hash collision creating alias \"%s\" > for mdev device %pUl\n",... > Ok. dev_dbg_once() to avoid message flood. > Thanks, > Alex > > > > > > + mutex_unlock(&mdev_list_lock); > > > > > + ret = -EEXIST; > > > > > + goto mdev_fail; > > > > > + } > > > > > } > > > > > > > > > > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL); > > > > >
> -----Original Message----- > From: Alex Williamson <alex.williamson@redhat.com> > Sent: Tuesday, August 27, 2019 8:54 PM > To: Parav Pandit <parav@mellanox.com> > Cc: Mark Bloch <markb@mellanox.com>; Jiri Pirko <jiri@mellanox.com>; > kwankhede@nvidia.com; cohuck@redhat.com; davem@davemloft.net; > kvm@vger.kernel.org; linux-kernel@vger.kernel.org; netdev@vger.kernel.org > Subject: Re: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs > > On Tue, 27 Aug 2019 04:28:37 +0000 > Parav Pandit <parav@mellanox.com> wrote: > > > Hi Mark, > > > > > -----Original Message----- > > > From: Mark Bloch <markb@mellanox.com> > > > Sent: Tuesday, August 27, 2019 4:32 AM > > > To: Parav Pandit <parav@mellanox.com>; alex.williamson@redhat.com; > > > Jiri Pirko <jiri@mellanox.com>; kwankhede@nvidia.com; > > > cohuck@redhat.com; davem@davemloft.net > > > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; > > > netdev@vger.kernel.org > > > Subject: Re: [PATCH 2/4] mdev: Make mdev alias unique among all > > > mdevs > > > > > > > > > > > > On 8/26/19 1:41 PM, 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> > > > > --- > > > > drivers/vfio/mdev/mdev_core.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/drivers/vfio/mdev/mdev_core.c > > > > b/drivers/vfio/mdev/mdev_core.c index e825ff38b037..6eb37f0c6369 > > > > 100644 > > > > --- a/drivers/vfio/mdev/mdev_core.c > > > > +++ b/drivers/vfio/mdev/mdev_core.c > > > > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj, > > > struct device *dev, > > > > ret = -EEXIST; > > > > goto mdev_fail; > > > > } > > > > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) { > > > > > > alias can be NULL here no? > > > > > If alias is NULL, tmp->alias would also be null because for given parent either > we have alias or we don’t. > > So its not possible to have tmp->alias as null and alias as non null. > > But it may be good/defensive to add check for both. > > mdev_list is a global list of all mdev devices, how can we make any > assumptions that an element has the same parent? Thanks, > Oh yes, right. If tmp->alias is not_null but alias can be NULL. I will fix the check. > Alex > > > > > + mutex_unlock(&mdev_list_lock); > > > > + ret = -EEXIST; > > > > + goto mdev_fail; > > > > + } > > > > } > > > > > > > > mdev = kzalloc(sizeof(*mdev), GFP_KERNEL); > > > > > > > > > > Mark
On Tue, 27 Aug 2019 16:13:27 +0000 Parav Pandit <parav@mellanox.com> wrote: > > -----Original Message----- > > From: Alex Williamson <alex.williamson@redhat.com> > > Sent: Tuesday, August 27, 2019 8:59 PM > > To: Cornelia Huck <cohuck@redhat.com> > > Cc: Parav Pandit <parav@mellanox.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 2/4] mdev: Make mdev alias unique among all mdevs > > > > On Tue, 27 Aug 2019 13:29:46 +0200 > > Cornelia Huck <cohuck@redhat.com> wrote: > > > > > On Tue, 27 Aug 2019 11:08:59 +0000 > > > Parav Pandit <parav@mellanox.com> wrote: > > > > > > > > -----Original Message----- > > > > > From: Cornelia Huck <cohuck@redhat.com> > > > > > Sent: Tuesday, August 27, 2019 3:59 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 2/4] mdev: Make mdev alias unique among all > > > > > mdevs > > > > > > > > > > On Mon, 26 Aug 2019 15:41:17 -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> > > > > > > --- > > > > > > drivers/vfio/mdev/mdev_core.c | 5 +++++ > > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > > > diff --git a/drivers/vfio/mdev/mdev_core.c > > > > > > b/drivers/vfio/mdev/mdev_core.c index e825ff38b037..6eb37f0c6369 > > > > > > 100644 > > > > > > --- a/drivers/vfio/mdev/mdev_core.c > > > > > > +++ b/drivers/vfio/mdev/mdev_core.c > > > > > > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj, > > struct > > > > > device *dev, > > > > > > ret = -EEXIST; > > > > > > goto mdev_fail; > > > > > > } > > > > > > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) { > > > > > > > > > > Any way we can relay to the caller that the uuid was fine, but > > > > > that we had a hash collision? Duplicate uuids are much more obvious than > > a collision here. > > > > > > > > > How do you want to relay this rare event? > > > > Netlink interface has way to return the error message back, but sysfs is > > limited due to its error code based interface. > > > > > > I don't know, that's why I asked :) > > > > > > The problem is that "uuid already used" and "hash collision" are > > > indistinguishable. While "use a different uuid" will probably work in > > > both cases, "increase alias length" might be a good alternative in > > > some cases. > > > > > > But if there is no good way to relay the problem, we can live with it. > > > > It's a rare event, maybe just dev_dbg(dev, "Hash collision creating alias \"%s\" > > for mdev device %pUl\n",... > > > Ok. > dev_dbg_once() to avoid message flood. I'd suggest a rate-limit rather than a once. The fact that the kernel may have experienced a collision at some time in the past does not help someone debug why they can't create a device now. The only way we're going to get a flood is if a user sufficiently privileged to create mdev devices stumbles onto a collision and continues to repeat the same operation. That falls into shoot-yourself-in-the-foot behavior imo. Thanks, Alex
> -----Original Message----- > From: Alex Williamson <alex.williamson@redhat.com> > Sent: Tuesday, August 27, 2019 9:55 PM > To: Parav Pandit <parav@mellanox.com> > Cc: Cornelia Huck <cohuck@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 2/4] mdev: Make mdev alias unique among all mdevs > > On Tue, 27 Aug 2019 16:13:27 +0000 > Parav Pandit <parav@mellanox.com> wrote: > > > > -----Original Message----- > > > From: Alex Williamson <alex.williamson@redhat.com> > > > Sent: Tuesday, August 27, 2019 8:59 PM > > > To: Cornelia Huck <cohuck@redhat.com> > > > Cc: Parav Pandit <parav@mellanox.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 2/4] mdev: Make mdev alias unique among all > > > mdevs > > > > > > On Tue, 27 Aug 2019 13:29:46 +0200 > > > Cornelia Huck <cohuck@redhat.com> wrote: > > > > > > > On Tue, 27 Aug 2019 11:08:59 +0000 Parav Pandit > > > > <parav@mellanox.com> wrote: > > > > > > > > > > -----Original Message----- > > > > > > From: Cornelia Huck <cohuck@redhat.com> > > > > > > Sent: Tuesday, August 27, 2019 3:59 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 2/4] mdev: Make mdev alias unique among > > > > > > all mdevs > > > > > > > > > > > > On Mon, 26 Aug 2019 15:41:17 -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> > > > > > > > --- > > > > > > > drivers/vfio/mdev/mdev_core.c | 5 +++++ > > > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/vfio/mdev/mdev_core.c > > > > > > > b/drivers/vfio/mdev/mdev_core.c index > > > > > > > e825ff38b037..6eb37f0c6369 > > > > > > > 100644 > > > > > > > --- a/drivers/vfio/mdev/mdev_core.c > > > > > > > +++ b/drivers/vfio/mdev/mdev_core.c > > > > > > > @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject > > > > > > > *kobj, > > > struct > > > > > > device *dev, > > > > > > > ret = -EEXIST; > > > > > > > goto mdev_fail; > > > > > > > } > > > > > > > + if (tmp->alias && strcmp(tmp->alias, alias) == 0) { > > > > > > > > > > > > Any way we can relay to the caller that the uuid was fine, but > > > > > > that we had a hash collision? Duplicate uuids are much more > > > > > > obvious than > > > a collision here. > > > > > > > > > > > How do you want to relay this rare event? > > > > > Netlink interface has way to return the error message back, but > > > > > sysfs is > > > limited due to its error code based interface. > > > > > > > > I don't know, that's why I asked :) > > > > > > > > The problem is that "uuid already used" and "hash collision" are > > > > indistinguishable. While "use a different uuid" will probably work > > > > in both cases, "increase alias length" might be a good alternative > > > > in some cases. > > > > > > > > But if there is no good way to relay the problem, we can live with it. > > > > > > It's a rare event, maybe just dev_dbg(dev, "Hash collision creating alias > \"%s\" > > > for mdev device %pUl\n",... > > > > > Ok. > > dev_dbg_once() to avoid message flood. > > I'd suggest a rate-limit rather than a once. The fact that the kernel may have > experienced a collision at some time in the past does not help someone > debug why they can't create a device now. The only way we're going to get a > flood is if a user sufficiently privileged to create mdev devices stumbles onto > a collision and continues to repeat the same operation. That falls into > shoot-yourself-in-the-foot behavior imo. > Thanks, > Ok. Will do.
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c index e825ff38b037..6eb37f0c6369 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -375,6 +375,11 @@ int mdev_device_create(struct kobject *kobj, struct device *dev, ret = -EEXIST; goto mdev_fail; } + if (tmp->alias && strcmp(tmp->alias, alias) == 0) { + mutex_unlock(&mdev_list_lock); + ret = -EEXIST; + 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> --- drivers/vfio/mdev/mdev_core.c | 5 +++++ 1 file changed, 5 insertions(+)