Re: [PATCH v8] drivers/misc: sysgenid: add system generation id driver
On Tue, Mar 23, 2021 at 05:35:14PM +0100, Greg KH wrote: > On Tue, Mar 23, 2021 at 04:10:27PM +, Catangiu, Adrian Costin wrote: > > Hi Greg, > > > > After your previous reply on this thread we started considering to provide > > this interface and framework/functionality through a userspace service > > instead of a kernel interface. > > The latest iteration on this evolving patch-set doesn’t have strong reasons > > for living in the kernel anymore - the only objectively strong advantage > > would be easier driving of ecosystem integration; but I am not sure that's > > a good enough reason to create a new kernel interface. > > > > I am now looking into adding this through Systemd. Either as a pluggable > > service or maybe even a systemd builtin offering. > > > > What are your thoughts on it? > > I'll gladly drop this patch if it's not needed in the kernel, thanks for > letting me know. > > greg k-h Systemd sounds good to me too. -- MST
Re: [PATCH v8] drivers/misc: sysgenid: add system generation id driver
On Tue, Mar 23, 2021 at 04:10:27PM +, Catangiu, Adrian Costin wrote: > Hi Greg, > > After your previous reply on this thread we started considering to provide > this interface and framework/functionality through a userspace service > instead of a kernel interface. > The latest iteration on this evolving patch-set doesn’t have strong reasons > for living in the kernel anymore - the only objectively strong advantage > would be easier driving of ecosystem integration; but I am not sure that's a > good enough reason to create a new kernel interface. > > I am now looking into adding this through Systemd. Either as a pluggable > service or maybe even a systemd builtin offering. > > What are your thoughts on it? Now dropped from my char-misc-testing branch. If you all decide you want this as a kernel driver, please resubmit it. Also next time, you might give me a heads-up that you don't want a patch merged... thanks, greg k-h
Re: [PATCH v8] drivers/misc: sysgenid: add system generation id driver
On Tue, Mar 23, 2021 at 04:10:27PM +, Catangiu, Adrian Costin wrote: > Hi Greg, > > After your previous reply on this thread we started considering to provide > this interface and framework/functionality through a userspace service > instead of a kernel interface. > The latest iteration on this evolving patch-set doesn’t have strong reasons > for living in the kernel anymore - the only objectively strong advantage > would be easier driving of ecosystem integration; but I am not sure that's a > good enough reason to create a new kernel interface. > > I am now looking into adding this through Systemd. Either as a pluggable > service or maybe even a systemd builtin offering. > > What are your thoughts on it? I'll gladly drop this patch if it's not needed in the kernel, thanks for letting me know. greg k-h
Re: [PATCH v8] drivers/misc: sysgenid: add system generation id driver
Hi Greg,
After your previous reply on this thread we started considering to provide this
interface and framework/functionality through a userspace service instead of a
kernel interface.
The latest iteration on this evolving patch-set doesn’t have strong reasons for
living in the kernel anymore - the only objectively strong advantage would be
easier driving of ecosystem integration; but I am not sure that's a good enough
reason to create a new kernel interface.
I am now looking into adding this through Systemd. Either as a pluggable
service or maybe even a systemd builtin offering.
What are your thoughts on it?
Thanks,
Adrian.
On 23/03/2021, 14:57, "Greg KH" wrote:
CAUTION: This email originated from outside of the organization. Do not
click links or open attachments unless you can confirm the sender and know the
content is safe.
On Mon, Mar 08, 2021 at 05:03:58PM +0100, Alexander Graf wrote:
>
>
> On 08.03.21 15:36, Greg KH wrote:
> >
> > On Mon, Mar 08, 2021 at 04:18:03PM +0200, Adrian Catangiu wrote:
> > > +static struct miscdevice sysgenid_misc = {
> > > + .minor = MISC_DYNAMIC_MINOR,
> > > + .name = "sysgenid",
> > > + .fops = ,
> > > +};
> >
> > Much cleaner, but:
> >
> > > +static int __init sysgenid_init(void)
> > > +{
> > > + int ret;
> > > +
> > > + sysgenid_data.map_buf = get_zeroed_page(GFP_KERNEL);
> > > + if (!sysgenid_data.map_buf)
> > > + return -ENOMEM;
> > > +
> > > + atomic_set(_data.generation_counter, 0);
> > > + atomic_set(_data.outdated_watchers, 0);
> > > + init_waitqueue_head(_data.read_waitq);
> > > + init_waitqueue_head(_data.outdated_waitq);
> > > + spin_lock_init(_data.lock);
> > > +
> > > + ret = misc_register(_misc);
> > > + if (ret < 0) {
> > > + pr_err("misc_register() failed for sysgenid\n");
> > > + goto err;
> > > + }
> > > +
> > > + return 0;
> > > +
> > > +err:
> > > + free_pages(sysgenid_data.map_buf, 0);
> > > + sysgenid_data.map_buf = 0;
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void __exit sysgenid_exit(void)
> > > +{
> > > + misc_deregister(_misc);
> > > + free_pages(sysgenid_data.map_buf, 0);
> > > + sysgenid_data.map_buf = 0;
> > > +}
> > > +
> > > +module_init(sysgenid_init);
> > > +module_exit(sysgenid_exit);
> >
> > So you do this for any bit of hardware that happens to be out there?
> > Will that really work? You do not have any hwid to trigger off of to
> > know that this is a valid device you can handle?
>
> The interface is already useful in a pure container context where the
> generation change request is triggered by software.
>
> And yes, there are hardware triggers, but Michael was quite unhappy about
> potential races between VMGenID change and SysGenID change and thus wanted
> to ideally separate the interfaces. So we went ahead and isolated the
> SysGenID one, as it's already useful as is.
>
> Hardware drivers to inject change events into SysGenID can then follow
> later, for all different hardware platforms. But SysGenID as in this patch
> is a completely hardware agnostic concept.
Ok, this is going to play havoc with fuzzers and other "automated
testers", should be fun to watch! :)
Let's queue this up and see what happens...
thanks,
greg k-h
Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar
Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in
Romania. Registration number J22/2621/2005.
Re: [PATCH v8] drivers/misc: sysgenid: add system generation id driver
On Mon, Mar 08, 2021 at 05:03:58PM +0100, Alexander Graf wrote:
>
>
> On 08.03.21 15:36, Greg KH wrote:
> >
> > On Mon, Mar 08, 2021 at 04:18:03PM +0200, Adrian Catangiu wrote:
> > > +static struct miscdevice sysgenid_misc = {
> > > + .minor = MISC_DYNAMIC_MINOR,
> > > + .name = "sysgenid",
> > > + .fops = ,
> > > +};
> >
> > Much cleaner, but:
> >
> > > +static int __init sysgenid_init(void)
> > > +{
> > > + int ret;
> > > +
> > > + sysgenid_data.map_buf = get_zeroed_page(GFP_KERNEL);
> > > + if (!sysgenid_data.map_buf)
> > > + return -ENOMEM;
> > > +
> > > + atomic_set(_data.generation_counter, 0);
> > > + atomic_set(_data.outdated_watchers, 0);
> > > + init_waitqueue_head(_data.read_waitq);
> > > + init_waitqueue_head(_data.outdated_waitq);
> > > + spin_lock_init(_data.lock);
> > > +
> > > + ret = misc_register(_misc);
> > > + if (ret < 0) {
> > > + pr_err("misc_register() failed for sysgenid\n");
> > > + goto err;
> > > + }
> > > +
> > > + return 0;
> > > +
> > > +err:
> > > + free_pages(sysgenid_data.map_buf, 0);
> > > + sysgenid_data.map_buf = 0;
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void __exit sysgenid_exit(void)
> > > +{
> > > + misc_deregister(_misc);
> > > + free_pages(sysgenid_data.map_buf, 0);
> > > + sysgenid_data.map_buf = 0;
> > > +}
> > > +
> > > +module_init(sysgenid_init);
> > > +module_exit(sysgenid_exit);
> >
> > So you do this for any bit of hardware that happens to be out there?
> > Will that really work? You do not have any hwid to trigger off of to
> > know that this is a valid device you can handle?
>
> The interface is already useful in a pure container context where the
> generation change request is triggered by software.
>
> And yes, there are hardware triggers, but Michael was quite unhappy about
> potential races between VMGenID change and SysGenID change and thus wanted
> to ideally separate the interfaces. So we went ahead and isolated the
> SysGenID one, as it's already useful as is.
>
> Hardware drivers to inject change events into SysGenID can then follow
> later, for all different hardware platforms. But SysGenID as in this patch
> is a completely hardware agnostic concept.
Ok, this is going to play havoc with fuzzers and other "automated
testers", should be fun to watch! :)
Let's queue this up and see what happens...
thanks,
greg k-h
Re: [PATCH v8] drivers/misc: sysgenid: add system generation id driver
On Mon, Mar 08, 2021 at 05:03:58PM +0100, Alexander Graf wrote:
>
>
> On 08.03.21 15:36, Greg KH wrote:
> >
> > On Mon, Mar 08, 2021 at 04:18:03PM +0200, Adrian Catangiu wrote:
> > > +static struct miscdevice sysgenid_misc = {
> > > + .minor = MISC_DYNAMIC_MINOR,
> > > + .name = "sysgenid",
> > > + .fops = ,
> > > +};
> >
> > Much cleaner, but:
> >
> > > +static int __init sysgenid_init(void)
> > > +{
> > > + int ret;
> > > +
> > > + sysgenid_data.map_buf = get_zeroed_page(GFP_KERNEL);
> > > + if (!sysgenid_data.map_buf)
> > > + return -ENOMEM;
> > > +
> > > + atomic_set(_data.generation_counter, 0);
> > > + atomic_set(_data.outdated_watchers, 0);
> > > + init_waitqueue_head(_data.read_waitq);
> > > + init_waitqueue_head(_data.outdated_waitq);
> > > + spin_lock_init(_data.lock);
> > > +
> > > + ret = misc_register(_misc);
> > > + if (ret < 0) {
> > > + pr_err("misc_register() failed for sysgenid\n");
> > > + goto err;
> > > + }
> > > +
> > > + return 0;
> > > +
> > > +err:
> > > + free_pages(sysgenid_data.map_buf, 0);
> > > + sysgenid_data.map_buf = 0;
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void __exit sysgenid_exit(void)
> > > +{
> > > + misc_deregister(_misc);
> > > + free_pages(sysgenid_data.map_buf, 0);
> > > + sysgenid_data.map_buf = 0;
> > > +}
> > > +
> > > +module_init(sysgenid_init);
> > > +module_exit(sysgenid_exit);
> >
> > So you do this for any bit of hardware that happens to be out there?
> > Will that really work? You do not have any hwid to trigger off of to
> > know that this is a valid device you can handle?
>
> The interface is already useful in a pure container context where the
> generation change request is triggered by software.
>
> And yes, there are hardware triggers, but Michael was quite unhappy about
> potential races between VMGenID change and SysGenID change and thus wanted
> to ideally separate the interfaces. So we went ahead and isolated the
> SysGenID one, as it's already useful as is.
>
> Hardware drivers to inject change events into SysGenID can then follow
> later, for all different hardware platforms. But SysGenID as in this patch
> is a completely hardware agnostic concept.
Ok, so what is going to cause this driver to be automatically loaded?
How will userspace "know" they want this and know to load it?
This really is just a shared memory "driver", it's gotten so small now,
so why can't this just be a userspace program/server now? :)
thanks,
greg k-h
Re: [PATCH v8] drivers/misc: sysgenid: add system generation id driver
On 08.03.21 15:36, Greg KH wrote:
On Mon, Mar 08, 2021 at 04:18:03PM +0200, Adrian Catangiu wrote:
+static struct miscdevice sysgenid_misc = {
+ .minor = MISC_DYNAMIC_MINOR,
+ .name = "sysgenid",
+ .fops = ,
+};
Much cleaner, but:
+static int __init sysgenid_init(void)
+{
+ int ret;
+
+ sysgenid_data.map_buf = get_zeroed_page(GFP_KERNEL);
+ if (!sysgenid_data.map_buf)
+ return -ENOMEM;
+
+ atomic_set(_data.generation_counter, 0);
+ atomic_set(_data.outdated_watchers, 0);
+ init_waitqueue_head(_data.read_waitq);
+ init_waitqueue_head(_data.outdated_waitq);
+ spin_lock_init(_data.lock);
+
+ ret = misc_register(_misc);
+ if (ret < 0) {
+ pr_err("misc_register() failed for sysgenid\n");
+ goto err;
+ }
+
+ return 0;
+
+err:
+ free_pages(sysgenid_data.map_buf, 0);
+ sysgenid_data.map_buf = 0;
+
+ return ret;
+}
+
+static void __exit sysgenid_exit(void)
+{
+ misc_deregister(_misc);
+ free_pages(sysgenid_data.map_buf, 0);
+ sysgenid_data.map_buf = 0;
+}
+
+module_init(sysgenid_init);
+module_exit(sysgenid_exit);
So you do this for any bit of hardware that happens to be out there?
Will that really work? You do not have any hwid to trigger off of to
know that this is a valid device you can handle?
The interface is already useful in a pure container context where the
generation change request is triggered by software.
And yes, there are hardware triggers, but Michael was quite unhappy
about potential races between VMGenID change and SysGenID change and
thus wanted to ideally separate the interfaces. So we went ahead and
isolated the SysGenID one, as it's already useful as is.
Hardware drivers to inject change events into SysGenID can then follow
later, for all different hardware platforms. But SysGenID as in this
patch is a completely hardware agnostic concept.
Alex
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
Re: [PATCH v8] drivers/misc: sysgenid: add system generation id driver
On Mon, Mar 08, 2021 at 04:18:03PM +0200, Adrian Catangiu wrote:
> +static struct miscdevice sysgenid_misc = {
> + .minor = MISC_DYNAMIC_MINOR,
> + .name = "sysgenid",
> + .fops = ,
> +};
Much cleaner, but:
> +static int __init sysgenid_init(void)
> +{
> + int ret;
> +
> + sysgenid_data.map_buf = get_zeroed_page(GFP_KERNEL);
> + if (!sysgenid_data.map_buf)
> + return -ENOMEM;
> +
> + atomic_set(_data.generation_counter, 0);
> + atomic_set(_data.outdated_watchers, 0);
> + init_waitqueue_head(_data.read_waitq);
> + init_waitqueue_head(_data.outdated_waitq);
> + spin_lock_init(_data.lock);
> +
> + ret = misc_register(_misc);
> + if (ret < 0) {
> + pr_err("misc_register() failed for sysgenid\n");
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + free_pages(sysgenid_data.map_buf, 0);
> + sysgenid_data.map_buf = 0;
> +
> + return ret;
> +}
> +
> +static void __exit sysgenid_exit(void)
> +{
> + misc_deregister(_misc);
> + free_pages(sysgenid_data.map_buf, 0);
> + sysgenid_data.map_buf = 0;
> +}
> +
> +module_init(sysgenid_init);
> +module_exit(sysgenid_exit);
So you do this for any bit of hardware that happens to be out there?
Will that really work? You do not have any hwid to trigger off of to
know that this is a valid device you can handle?
> +
> +MODULE_AUTHOR("Adrian Catangiu");
> +MODULE_DESCRIPTION("System Generation ID");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION("0.1");
MODULE_VERSION() isn't a thing, just drop it please :)
thnaks,
greg k-h
