linux-kernel - RE: [PATCH 1/4] mdev: Introduce sha1 based mdev alias

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AM0PR05MB4866CC932630ADD9BDA51371D1A00@AM0PR05MB4866.eurprd05.prod.outlook.com>
Date:   Tue, 27 Aug 2019 11:33:54 +0000
From:   Parav Pandit <parav@...lanox.com>
To:     Cornelia Huck <cohuck@...hat.com>
CC:     "alex.williamson@...hat.com" <alex.williamson@...hat.com>,
        Jiri Pirko <jiri@...lanox.com>,
        "kwankhede@...dia.com" <kwankhede@...dia.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH 1/4] mdev: Introduce sha1 based mdev alias



> -----Original Message-----
> From: Cornelia Huck <cohuck@...hat.com>
> Sent: Tuesday, August 27, 2019 4:54 PM
> To: Parav Pandit <parav@...lanox.com>
> Cc: alex.williamson@...hat.com; Jiri Pirko <jiri@...lanox.com>;
> kwankhede@...dia.com; davem@...emloft.net; kvm@...r.kernel.org; linux-
> kernel@...r.kernel.org; netdev@...r.kernel.org
> Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> 
> On Tue, 27 Aug 2019 11:12:23 +0000
> Parav Pandit <parav@...lanox.com> wrote:
> 
> > > -----Original Message-----
> > > From: Cornelia Huck <cohuck@...hat.com>
> > > Sent: Tuesday, August 27, 2019 3:54 PM
> > > To: Parav Pandit <parav@...lanox.com>
> > > Cc: alex.williamson@...hat.com; Jiri Pirko <jiri@...lanox.com>;
> > > kwankhede@...dia.com; davem@...emloft.net; kvm@...r.kernel.org;
> > > linux- kernel@...r.kernel.org; netdev@...r.kernel.org
> > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> > >
> > > On Mon, 26 Aug 2019 15:41:16 -0500
> > > Parav Pandit <parav@...lanox.com> wrote:
> > >
> > > > Whenever a parent requests to generate mdev alias, generate a mdev
> > > > alias.
> > > > It is an optional attribute that parent can request to generate
> > > > for each of its child mdev.
> > > > mdev alias is generated using sha1 from the mdev name.
> > >
> > > Maybe add some motivation here as well?
> > >
> > > "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 (via sha1) for an mdev device. If
> > > generated, that alias is checked for collisions."
> > >
> > I did described the motivation in the cover letter with example and this
> design discussion thread.
> 
> Yes, but adding it to the patch description makes it available in the git history.
> 
Ok.

> > I will include above summary in v1.
> >
> > > What about:
> > >
> > > * @get_alias_length: optional callback to specify length of the alias to
> create
> > > *                    Returns unsigned integer: length of the alias to be created,
> > > *                                              0 to not create an alias
> > >
> > Ack.
> >
> > > I also think it might be beneficial to add a device parameter here
> > > now (rather than later); that seems to be something that makes sense.
> > >
> > Without showing the use, it shouldn't be added.
> 
> It just feels like an omission: Why should the vendor driver only be able to
> return one value here, without knowing which device it is for?
> If a driver supports different devices, it may have different requirements for
> them.
> 
Sure. Lets first have this requirement to add it.
I am against adding this length field itself without an actual vendor use case, which is adding some complexity in code today.
But it was ok to have length field instead of bool.

Lets not further add "no-requirement futuristic knobs" which hasn't shown its need yet.
When a vendor driver needs it, there is nothing prevents such addition.

> >
> > > >   * Parent device that support mediated device should be
> > > > registered with
> > > mdev
> > > >   * module with mdev_parent_ops structure.
> > > >   **/
> > > > @@ -92,6 +95,7 @@ struct mdev_parent_ops {
> > > >  	long	(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> > > >  			 unsigned long arg);
> > > >  	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct
> > > *vma);
> > > > +	unsigned int (*get_alias_length)(void);
> > > >  };
> > > >
> > > >  /* interface for exporting mdev supported type attributes */
> >