RE: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
> -Original Message- > From: Alex Williamson > Sent: Tuesday, August 27, 2019 9:55 PM > To: Parav Pandit > Cc: Cornelia Huck ; Jiri Pirko ; > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; > linux-kernel@vger.kernel.org; net...@vger.kernel.org > Subject: Re: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs > > On Tue, 27 Aug 2019 16:13:27 + > Parav Pandit wrote: > > > > -Original Message- > > > From: Alex Williamson > > > Sent: Tuesday, August 27, 2019 8:59 PM > > > To: Cornelia Huck > > > Cc: Parav Pandit ; Jiri Pirko > > > ; kwankh...@nvidia.com; da...@davemloft.net; > > > k...@vger.kernel.org; linux- ker...@vger.kernel.org; > > > net...@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 wrote: > > > > > > > On Tue, 27 Aug 2019 11:08:59 + Parav Pandit > > > > wrote: > > > > > > > > > > -Original Message- > > > > > > From: Cornelia Huck > > > > > > Sent: Tuesday, August 27, 2019 3:59 PM > > > > > > To: Parav Pandit > > > > > > Cc: alex.william...@redhat.com; Jiri Pirko > > > > > > ; kwankh...@nvidia.com; > > > > > > da...@davemloft.net; k...@vger.kernel.org; > > > > > > linux- ker...@vger.kernel.org; net...@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 > > > > > > 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 > > > > > > > --- > > > > > > > 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.
Re: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
On Tue, 27 Aug 2019 16:13:27 + Parav Pandit wrote: > > -Original Message- > > From: Alex Williamson > > Sent: Tuesday, August 27, 2019 8:59 PM > > To: Cornelia Huck > > Cc: Parav Pandit ; Jiri Pirko ; > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux- > > ker...@vger.kernel.org; net...@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 wrote: > > > > > On Tue, 27 Aug 2019 11:08:59 + > > > Parav Pandit wrote: > > > > > > > > -Original Message- > > > > > From: Cornelia Huck > > > > > Sent: Tuesday, August 27, 2019 3:59 PM > > > > > To: Parav Pandit > > > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; > > > > > linux- ker...@vger.kernel.org; net...@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 > > > > > 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 > > > > > > --- > > > > > > 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
RE: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
> -Original Message- > From: Alex Williamson > Sent: Tuesday, August 27, 2019 8:54 PM > To: Parav Pandit > Cc: Mark Bloch ; Jiri Pirko ; > kwankh...@nvidia.com; coh...@redhat.com; da...@davemloft.net; > k...@vger.kernel.org; linux-kernel@vger.kernel.org; net...@vger.kernel.org > Subject: Re: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs > > On Tue, 27 Aug 2019 04:28:37 + > Parav Pandit wrote: > > > Hi Mark, > > > > > -Original Message- > > > From: Mark Bloch > > > Sent: Tuesday, August 27, 2019 4:32 AM > > > To: Parav Pandit ; alex.william...@redhat.com; > > > Jiri Pirko ; kwankh...@nvidia.com; > > > coh...@redhat.com; da...@davemloft.net > > > Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org; > > > net...@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 > > > > --- > > > > 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
RE: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
> -Original Message- > From: Alex Williamson > Sent: Tuesday, August 27, 2019 8:59 PM > To: Cornelia Huck > Cc: Parav Pandit ; Jiri Pirko ; > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; net...@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 wrote: > > > On Tue, 27 Aug 2019 11:08:59 + > > Parav Pandit wrote: > > > > > > -Original Message- > > > > From: Cornelia Huck > > > > Sent: Tuesday, August 27, 2019 3:59 PM > > > > To: Parav Pandit > > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; > > > > linux- ker...@vger.kernel.org; net...@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 > > > > 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 > > > > > --- > > > > > 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); > > > > >
Re: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
On Tue, 27 Aug 2019 09:28:55 -0600 Alex Williamson wrote: > On Tue, 27 Aug 2019 13:29:46 +0200 > Cornelia Huck wrote: > > > On Tue, 27 Aug 2019 11:08:59 + > > Parav Pandit wrote: > > > > > > -Original Message- > > > > From: Cornelia Huck > > > > Sent: Tuesday, August 27, 2019 3:59 PM > > > > To: Parav Pandit > > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux- > > > > ker...@vger.kernel.org; net...@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 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 > > > > > --- > > > > > 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.
Re: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
On Tue, 27 Aug 2019 13:29:46 +0200 Cornelia Huck wrote: > On Tue, 27 Aug 2019 11:08:59 + > Parav Pandit wrote: > > > > -Original Message- > > > From: Cornelia Huck > > > Sent: Tuesday, August 27, 2019 3:59 PM > > > To: Parav Pandit > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux- > > > ker...@vger.kernel.org; net...@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 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 > > > > --- > > > > 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); > > >
Re: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
On Tue, 27 Aug 2019 04:28:37 + Parav Pandit wrote: > Hi Mark, > > > -Original Message- > > From: Mark Bloch > > Sent: Tuesday, August 27, 2019 4:32 AM > > To: Parav Pandit ; alex.william...@redhat.com; Jiri > > Pirko ; kwankh...@nvidia.com; coh...@redhat.com; > > da...@davemloft.net > > Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org; > > net...@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 > > > --- > > > 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
Re: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
On Tue, 27 Aug 2019 11:08:59 + Parav Pandit wrote: > > -Original Message- > > From: Cornelia Huck > > Sent: Tuesday, August 27, 2019 3:59 PM > > To: Parav Pandit > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux- > > ker...@vger.kernel.org; net...@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 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 > > > --- > > > 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); >
RE: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
> -Original Message- > From: Cornelia Huck > Sent: Tuesday, August 27, 2019 3:59 PM > To: Parav Pandit > Cc: alex.william...@redhat.com; Jiri Pirko ; > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; net...@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 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 > > --- > > 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);
Re: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
On Mon, 26 Aug 2019 15:41:17 -0500 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 > --- > 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);
RE: [PATCH 2/4] mdev: Make mdev alias unique among all mdevs
Hi Mark, > -Original Message- > From: Mark Bloch > Sent: Tuesday, August 27, 2019 4:32 AM > To: Parav Pandit ; alex.william...@redhat.com; Jiri > Pirko ; kwankh...@nvidia.com; coh...@redhat.com; > da...@davemloft.net > Cc: k...@vger.kernel.org; linux-kernel@vger.kernel.org; > net...@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 > > --- > > 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
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 > --- > 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