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
[PATCH v8] drivers/misc: sysgenid: add system generation id driver
- Background and problem The System Generation ID feature is required in virtualized or containerized environments by applications that work with local copies or caches of world-unique data such as random values, uuids, monotonically increasing counters, cryptographic nonces, etc. Such applications can be negatively affected by VM or container snapshotting when the VM or container is either cloned or returned to an earlier point in time. Solving the uniqueness problem strongly enough for cryptographic purposes requires a mechanism which can deterministically reseed userspace PRNGs with new entropy at restore time. This mechanism must also support the high-throughput and low-latency use-cases that led programmers to pick a userspace PRNG in the first place; be usable by both application code and libraries; allow transparent retrofitting behind existing popular PRNG interfaces without changing application code; it must be efficient, especially on snapshot restore; and be simple enough for wide adoption. - Solution This commit introduces a mechanism that standardizes an API for applications and libraries to be made aware of uniqueness breaking events such as VM or container snapshotting, and allow them to react and adapt to such events. The System Generation ID is meant to help in these scenarios by providing a monotonically increasing u32 counter that changes each time the VM or container is restored from a snapshot. The `sysgenid` driver exposes a monotonic incremental System Generation u32 counter via a char-dev filesystem interface accessible through `/dev/sysgenid`. It provides synchronous and asynchronous SysGen counter update notifications, as well as counter retrieval and confirmation mechanisms. The counter starts from zero when the driver is initialized and monotonically increments every time the system generation changes. Userspace applications or libraries can (a)synchronously consume the system generation counter through the provided filesystem interface, to make any necessary internal adjustments following a system generation update. The provided filesystem interface operations can be used to build a system level safe workflow that guest software can follow to protect itself from negative system snapshot effects. System generation changes are driven by userspace software through a dedicated driver ioctl. **Please note**, SysGenID alone does not guarantee complete snapshot safety to applications using it. A certain workflow needs to be followed at the system level, in order to make the system snapshot-resilient. Please see the "Snapshot Safety Prerequisites" section in the included documentation. Signed-off-by: Adrian Catangiu --- v7 -> v8: - remove vmgenid driver - sysgenid: remove support for HW backends, SysGenID is now driven by software through a safe, consistent interface - without HW backend there is no IRQ to race with mmap() interface - update documentation v6 -> v7: - remove sysgenid uevent v5 -> v6: - sysgenid: watcher tracking disabled by default - sysgenid: add SYSGENID_SET_WATCHER_TRACKING ioctl to allow each file descriptor to set whether they should be tracked as watchers - rename SYSGENID_FORCE_GEN_UPDATE -> SYSGENID_TRIGGER_GEN_UPDATE - rework all documentation to clearly capture all prerequisites for achieving snapshot safety when using the provided mechanism - sysgenid documentation: replace individual filesystem operations examples with a higher level example showcasing system-level snapshot-safe workflow v4 -> v5: - sysgenid: generation changes are also exported through uevents - remove SYSGENID_GET_OUTDATED_WATCHERS ioctl - document sysgenid ioctl major/minor numbers v3 -> v4: - split functionality in two separate kernel modules: 1. drivers/misc/sysgenid.c which provides the generic userspace interface and mechanisms 2. drivers/virt/vmgenid.c as VMGENID acpi device driver that seeds kernel entropy and acts as a driving backend for the generic sysgenid - rename /dev/vmgenid -> /dev/sysgenid - rename uapi header file vmgenid.h -> sysgenid.h - rename ioctls VMGENID_* -> SYSGENID_* - add ‘min_gen’ parameter to SYSGENID_FORCE_GEN_UPDATE ioctl - fix races in documentation examples v2 -> v3: - separate the core driver logic and interface, from the ACPI device. The ACPI vmgenid device is now one possible backend - fix issue when timeout=0 in VMGENID_WAIT_WATCHERS - add locking to avoid races between fs ops handlers and hw irq driven generation updates - change VMGENID_WAIT_WATCHERS ioctl so if the current caller is outdated or a generation change happens while waiting (thus making current caller outdated), the ioctl returns -EINTR to signal the user to handle event and retry. Fixes blocking on oneself - add VMGENID_FORCE_GEN_UPDATE ioctl conditioned by CAP_CHECKPOINT_RESTORE capability, through which software can force generation bump
