This reverts commit ad6bcdad2b6724e113f191a12f859a9e8456b26d. I had nak'd it, and Greg said on the thread that it links that he wasn't going to take it either, especially since it's not his code or his tree, but then, seemingly accidentally, it got pushed up some months later, in what looks like a mistake, with no further discussion in the linked thread. So revert it, since it's clearly not intended.
Fixes: ad6bcdad2b67 ("vmgenid: emit uevent when VMGENID updates") Cc: stable@vger.kernel.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Link: https://lore.kernel.org/r/20230531095119.11202-2-bchalios@amazon.es Signed-off-by: Jason A. Donenfeld Jason@zx2c4.com --- drivers/virt/vmgenid.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c index b67a28da4702..a1c467a0e9f7 100644 --- a/drivers/virt/vmgenid.c +++ b/drivers/virt/vmgenid.c @@ -68,7 +68,6 @@ static int vmgenid_add(struct acpi_device *device) static void vmgenid_notify(struct acpi_device *device, u32 event) { struct vmgenid_state *state = acpi_driver_data(device); - char *envp[] = { "NEW_VMGENID=1", NULL }; u8 old_id[VMGENID_SIZE];
memcpy(old_id, state->this_id, sizeof(old_id)); @@ -76,7 +75,6 @@ static void vmgenid_notify(struct acpi_device *device, u32 event) if (!memcmp(old_id, state->this_id, sizeof(old_id))) return; add_vmfork_randomness(state->this_id, sizeof(state->this_id)); - kobject_uevent_env(&device->dev.kobj, KOBJ_CHANGE, envp); }
static const struct acpi_device_id vmgenid_ids[] = {
On Thu, Apr 18, 2024 at 01:48:08PM +0200, Jason A. Donenfeld wrote:
This reverts commit ad6bcdad2b6724e113f191a12f859a9e8456b26d. I had nak'd it, and Greg said on the thread that it links that he wasn't going to take it either, especially since it's not his code or his tree, but then, seemingly accidentally, it got pushed up some months later, in what looks like a mistake, with no further discussion in the linked thread. So revert it, since it's clearly not intended.
Fixes: ad6bcdad2b67 ("vmgenid: emit uevent when VMGENID updates") Cc: stable@vger.kernel.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Link: https://lore.kernel.org/r/20230531095119.11202-2-bchalios@amazon.es Signed-off-by: Jason A. Donenfeld Jason@zx2c4.com
drivers/virt/vmgenid.c | 2 -- 1 file changed, 2 deletions(-)
Sorry about that, I picked it up thinking I had missed it previously:
Acked-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
[Adding CC list of original patch plus regression tracker]
Hi Jason,
On 18.04.24 13:48, Jason A. Donenfeld wrote:
This reverts commit ad6bcdad2b6724e113f191a12f859a9e8456b26d. I had nak'd it, and Greg said on the thread that it links that he wasn't going to take it either, especially since it's not his code or his tree, but then, seemingly accidentally, it got pushed up some months later, in what looks like a mistake, with no further discussion in the linked thread. So revert it, since it's clearly not intended.
Reverting this patch creates a user space visible regression compared to v6.8. Please treat it as such.
I'm slightly confused to see you passionate about this patch after you ghosted the conversation you referenced:
https://lore.kernel.org/lkml/00d6172f-e291-4e96-9d3e-63ee8e60d556@amazon.com...
The purpose of this uevent is to notify systemd[1][2] (or similar) that a VM clone event happened, so it can for example regenerate MAC addresses if it generated them on boot, regenerate its unique machine id or simply force rerequest a new DHCP lease.
I don't understand how there's any correlation or dependency to vgetrandom() or anything RNG in this and why getting vgetrandom() merged upstream is even something to talk about in the same line as this patch [3].
We had a lengthy, constructive conversation with Ted at LPC last year about the "PRNG and clone" use case and concluded that it's best for everyone to simply assume the system could be cloned at any point, hence always force intermix of RDRAND or comparable to any PRNG output. We since no longer need an event for that case.
Alex
[1] https://github.com/systemd/systemd/issues/26380 [2] https://lore.kernel.org/lkml/ZJGNREN4tLzQXOJr@gardel-login/ [3] https://lore.kernel.org/lkml/CAHmME9pxc-nO_xa=4+1CnvbnuefbRTJHxM7n817c_TPeox...
#regzbot introduced: 3aadf100f93d8081
Fixes: ad6bcdad2b67 ("vmgenid: emit uevent when VMGENID updates") Cc: stable@vger.kernel.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Link: https://lore.kernel.org/r/20230531095119.11202-2-bchalios@amazon.es Signed-off-by: Jason A. Donenfeld Jason@zx2c4.com
drivers/virt/vmgenid.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c index b67a28da4702..a1c467a0e9f7 100644 --- a/drivers/virt/vmgenid.c +++ b/drivers/virt/vmgenid.c @@ -68,7 +68,6 @@ static int vmgenid_add(struct acpi_device *device) static void vmgenid_notify(struct acpi_device *device, u32 event) { struct vmgenid_state *state = acpi_driver_data(device);
- char *envp[] = { "NEW_VMGENID=1", NULL }; u8 old_id[VMGENID_SIZE];
memcpy(old_id, state->this_id, sizeof(old_id)); @@ -76,7 +75,6 @@ static void vmgenid_notify(struct acpi_device *device, u32 event) if (!memcmp(old_id, state->this_id, sizeof(old_id))) return; add_vmfork_randomness(state->this_id, sizeof(state->this_id));
- kobject_uevent_env(&device->dev.kobj, KOBJ_CHANGE, envp); }
static const struct acpi_device_id vmgenid_ids[] = {
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
Hi Alexander,
The process here seems weirdly aggressive and sneaky.
On 2023-06-19, I wrote that I didn't want to take this route for userspace notifications.
Then on 2023-06-28, you wrote to Greg asking him to take it instead of me. Nine minutes later, Greg said "yea sure." Then he caught up on the thread and some hours later wrote:
Wait, no, I'm not the maintainer of this, Jason is. And he already rejected it (and based on the changelog text, I would too), so why are you asking me a month later to take this?
Work with the maintainer please, don't try to route around them, you both know better than this.
Then on 2023-11-14 you wrote to me again asking me to take it, despite my earlier reservations not changing in the interim. I didn't have a chance to reply.
Then on 2023-11-30, Greg weirdly took it anyway, with zero discussion or evidence on the mailing list as to what had happened.
When I noticed what had happened (while working on his driver in the process of cleaning up/reworking patches that your Amazon employees sent me that needed work), suspicious that you tried to "route around" the proper way of getting this done and trick Greg again into taking a patch that's not his purview, I asked him wtf happened on IRC:
<gregkh> ugh, sorry, I don't remember that. I think Alexander talked to me at plumbers and said, "hey, please take this virt patch" <gregkh> but you are right, you NAKed it in that thread, I forgot that, sorry. Yes, revert it if that's needed.
Greg then ACK'd the revert commit which came with a stable@ marking and a Fixes: tag (for 6.8, which isn't very old).
So it looks to me like you twice tried to trick Greg into taking this, succeeded the second time, got caught, and now are trying to make a regression argument as a means of keeping your sneaky commit in there. All of this really _really_ rubs me the wrong way, I have to say.
I don't know what holds more weight here -- the predictable regression argument, or the fact that you snuck nack'd changes into a very very recent kernel that can still be removed while probably only affecting you. But I'm obviously not happy about this.
Jason
Hey Jason,
On 23.04.24 03:21, Jason A. Donenfeld wrote:
Hi Alexander,
The process here seems weirdly aggressive and sneaky.
On 2023-06-19, I wrote that I didn't want to take this route for userspace notifications.
Then on 2023-06-28, you wrote to Greg asking him to take it instead of me. Nine minutes later, Greg said "yea sure." Then he caught up on the thread and some hours later wrote:
Wait, no, I'm not the maintainer of this, Jason is. And he already rejected it (and based on the changelog text, I would too), so why are you asking me a month later to take this?
Work with the maintainer please, don't try to route around them, you both know better than this.
Then on 2023-11-14 you wrote to me again asking me to take it, despite my earlier reservations not changing in the interim. I didn't have a chance to reply.
Then on 2023-11-30, Greg weirdly took it anyway, with zero discussion or evidence on the mailing list as to what had happened.
When I noticed what had happened (while working on his driver in the process of cleaning up/reworking patches that your Amazon employees sent me that needed work), suspicious that you tried to "route around" the proper way of getting this done and trick Greg again into taking a patch that's not his purview, I asked him wtf happened on IRC:
<gregkh> ugh, sorry, I don't remember that. I think Alexander talked to me at plumbers and said, "hey, please take this virt patch" <gregkh> but you are right, you NAKed it in that thread, I forgot that, sorry. Yes, revert it if that's needed.
Greg then ACK'd the revert commit which came with a stable@ marking and a Fixes: tag (for 6.8, which isn't very old).
So it looks to me like you twice tried to trick Greg into taking this, succeeded the second time, got caught, and now are trying to make a regression argument as a means of keeping your sneaky commit in there. All of this really _really_ rubs me the wrong way, I have to say.
I don't know what holds more weight here -- the predictable regression argument, or the fact that you snuck nack'd changes into a very very recent kernel that can still be removed while probably only affecting you. But I'm obviously not happy about this.
I'm personally much more concerned about Linux' ability to deal with VM Clone events than "my personal use case". The group at Amazon you see working on this is working on AWS Lambda which owns the full host and guest stack, including Linux on both ends. They could happily patch their own Linux kernel. Instead, I have managed to get them to do "the right thing" and work with the Linux upstream community to build a viable solution that works for everyone.
However, every time they do that, all they get back is vgetrandom() arguments which are completely irrelevant to the conversation and deteriorate my efforts to get AWS to work *more* rather than less upstream. Can we please move this back to a technical discussion and based on technical grounds determine why sending a notification to user space when a VM was cloned via uevents is even remotely a bad idea?
Thanks,
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
On Di, 23.04.24 03:21, Jason A. Donenfeld (Jason@zx2c4.com) wrote:
Jason!
Can you please explain to me what the precise problem is with the uevent? It doesn't leak any information about the actual vmgenid, it just lets userspace know that the machine was cloned, basically. What's the problem with that? I'd really like to understand?
There are many usecases for this in the VM world, for example we'd like to hook things up so that various userspace managed concepts, such as DHCP leases, MAC addresses are automatically refreshed.
This has no relationship to RNGs or anything like this, it's just an event we can handle in userspace to trigger address refreshes like this.
Hence, why is the revert necessary? This was already in a released kernel, and we have started work on making use of this in systemd, and afaics this does not compromise the kernel RNG in even the remotest of ways, hence why is a revert necessary? From my usersace perspective it's just very very sad, that this simple, trivial interface we wanted to use, that was in a stable kernel is now gone again.
Can you explain what the problem with this single-line trivial interface is? I really would like to understand!
Lennart
(BTW: even if the uevent would leak the vmgenid somehow to userspace — which it does not —, at least on qemu — i.e. one of the most relevant VM platforms — the vmgenid can be read directly from userspace by cat'ing /sys/firmware/qemu_fw_cfg/by_name/etc/vmgenid_guid/raw, i.e. it's not that well protected anyway).
-- Lennart Poettering, Berlin
On 23.04.24 14:23, Lennart Poettering wrote:
On Di, 23.04.24 03:21, Jason A. Donenfeld (Jason@zx2c4.com) wrote:
Jason!
Can you please explain to me what the precise problem is with the uevent? It doesn't leak any information about the actual vmgenid, it just lets userspace know that the machine was cloned, basically. What's the problem with that? I'd really like to understand?
There are many usecases for this in the VM world, for example we'd like to hook things up so that various userspace managed concepts, such as DHCP leases, MAC addresses are automatically refreshed.
This has no relationship to RNGs or anything like this, it's just an event we can handle in userspace to trigger address refreshes like this.
Hence, why is the revert necessary? This was already in a released kernel, and we have started work on making use of this in systemd, and afaics this does not compromise the kernel RNG in even the remotest of ways, hence why is a revert necessary? From my usersace perspective it's just very very sad, that this simple, trivial interface we wanted to use, that was in a stable kernel is now gone again.
Can you explain what the problem with this single-line trivial interface is? I really would like to understand!
Jason, ping?
If I don't see technical reasoning from you here, I will assume that you agree with Lennart and my points of views and send a revert of your revert shortly to ensure systemd has its uevent still in 6.9.
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
I don't think adding UAPI to an individual device driver like this is a good approach especially considering that the virtio changes we discussed some time ago will likely augment this and create another means of a similar notification. And given that this intersects with other userspace-oriented work I hope to get back to pretty soon, I think introducing some adhoc mechanism like this adds clutter and isn't the ideal way forward.
On top of that, your extremely aggressive approach and apparent disregard for established processes and general sneakiness doesn't really earn you a lot of points here. There wasn't even an iota of recognition in your reply that you realize the way you've gone about this is not fine. That doesn't really sit too well, you know?
Hi Jason,
On 4/26/24 14:52, Jason A. Donenfeld wrote:
I don't think adding UAPI to an individual device driver like this is a good approach especially considering that the virtio changes we discussed some time ago will likely augment this and create another means of a similar notification. And given that this intersects with other userspace-oriented work I hope to get back to pretty soon, I think introducing some adhoc mechanism like this adds clutter and isn't the ideal way forward.
Correct me if I'm wrong, but the virtio changes were meant to mean "please reseed your PRNGs". That's why we wanted to route them via random.c. We designed them specifically so that virtio-rng would be only one of the potential systems that would emit such notifications, whereas other systems might have nothing to do with VM events.
With that in mind, could you describe how these events would be useful to the use case of Lennart? systemd does not need a notification every time the system believes PRNGs need to be reseeded. It explicitly needs a notification when a VM was cloned. This has nothing to do with PRNGs and I don't believe random.c, virtio-rng, or vgetrand() should be responsible for delivering this.
Cheers, Babis
On 26.04.24 15:43, Babis Chalios wrote:
Hi Jason,
On 4/26/24 14:52, Jason A. Donenfeld wrote:
I don't think adding UAPI to an individual device driver like this is a good approach especially considering that the virtio changes we discussed some time ago will likely augment this and create another means of a similar notification. And given that this intersects with other userspace-oriented work I hope to get back to pretty soon, I think introducing some adhoc mechanism like this adds clutter and isn't the ideal way forward.
Correct me if I'm wrong, but the virtio changes were meant to mean "please reseed your PRNGs". That's why we wanted to route them via random.c. We designed them specifically so that virtio-rng would be only one of the potential systems that would emit such notifications, whereas other systems might have nothing to do with VM events.
With that in mind, could you describe how these events would be useful to the use case of Lennart? systemd does not need a notification every time the system believes PRNGs need to be reseeded. It explicitly needs a notification when a VM was cloned. This has nothing to do with PRNGs and I don't believe random.c, virtio-rng, or vgetrand() should be responsible for delivering this.
I second this. A VM clone event may be one of multiple events that can cause an RNG reseed event. This is what Babis' patches to propagate such an event[1] implemented: A new type of multiplexed event that feeds from multiple sources; most notably *not* from VMGenID.
Due your reluctance to enable user space PRNGs to get any notion of reseed events [2], we have since abandoned that whole RNG reseed event approach: Going forward, for our environments, we'll simply mandate that PRNGs always mix in randomness that is guaranteed different post-clone (such as RDRAND). You want a clone safe system? Use one that does (fast and non-broken) RDRAND.
However, VM clone events are useful for other situations as all of us outlined multiple times in this and previous threads. While you can use VM clone events as a source for RNG reseed events, you can not use RNG reseed events (or a snapshot safe RNG source like /dev/random) as indicator for VM clones, because they will trigger more often and hence cause undesired side effects. You may want a reseed every 60s, but surely don't want a new MAC address every 60 seconds, right? :)
Now, theoretically I can see some merit for a single, abstracted event source for VM clones over a per-driver one. But practically, between VMGenID on ACPI and Device Tree systems, there are very for platforms that care about safe VM snapshots and wouldn't "just work". The only one I can think of atm is s390x. I don't know if an abstraction - like another driver that simply proxies notifications - would be worth it. Or if in that case we'd just expand the very same vmgenid driver to that other one-off platform that happens to run without DT or ACPI.
So, overall, I still don't see any better path forward to get a "VM cloned" event into systemd than the uevent.
Jason, could you please outline how your "other userspace-oriented work you hope to get back to soon" would help with the systemd use case?
Alex
[1] https://lore.kernel.org/lkml/20230823090107.65749-1-bchalios@amazon.es/ [2] https://lore.kernel.org/lkml/ZPXsuhXJhN9Q3hfH@zx2c4.com/
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
On Fr, 26.04.24 14:52, Jason A. Donenfeld (Jason@zx2c4.com) wrote:
I don't think adding UAPI to an individual device driver like this
Does vmgenid really qualify as "an individual device driver"? It's a pretty generic software interface, implemented by various different VMMs these days. It is also the only interface I am aware of that actually exists and would provide the concept right now?
if this was really hyperv specific, then I'd agree it's just an "individual device driver". But it's widely implemented, for example a trivial command line switch in qemu.
Hence, for something this generic, and widely deployed with multiple backend implementations I think we can say it's kinda more of a subsystem and less of an individual driver, no?
is a good approach especially considering that the virtio changes we discussed some time ago will likely augment this and create another means of a similar notification. And given that this intersects with other userspace-oriented work I hope to get back to pretty soon, I think introducing some adhoc mechanism like this adds clutter and isn't the ideal way forward.
If one day a virtio-based equivalent shows up, then I'd be entirely fine with supporting this in userspace directly too , because virtio too is a generic thing typically implemented by multiple VMM backends. From my userspace perspective I see little benefit in the kernel abstracting over vmgenid and virtio-genid (if that ever materializes), as a systemd person I am not asking for this kind of abstraction (in case anyone wonders). A generic ACPI device such as vmgenid is entirely enough of "generic" for me.
The way we would process the event in userspace in systemd (from a udev rule) is so generic that it's trivial to match against two generic interfaces, instead of just one.
And even if there's value in a generic abstraction provided by the kernel over both vmgenid and a future virtio-based thing: the kernel patch in question was a *single* line, and our hookup in userspace could easily be moved over when the day comes, because it's really not a rocket science level interface. It's a single parameterless event, how much easier could things get?
I understand that how this all happened wasn't to everyones wishes, but do we really have to make all of this so complex if it could just be so simple? Why delay this further, why go back again given the event, the interface itself is such an utter triviality? Do we really make such a threatre around a single line change, a single additional uevent, just because of politics?
Lennart
-- Lennart Poettering, Berlin
Hi Jason,
Friendly ping?
IMHO Lennart, Alex and myself have raised (what I think to be) valid technical points regarding your concerns about your belief that the uevent mechanism is an ad-hoc mechanism that you don't consider viable.
Just to summarize:
* Upon VM clone, user space needs to adjust various components (DHCP leases, MAC addresses, etc.) that have nothing to do with PRNGs. * The path of exposing the VM clone event via vgetrandom() (or any other interface of random.c) is simply wrong. The random subsystem is the natural component to inform about when cached entropy is stale. It should not be responsible for informing user space about VM clone events. IOW, "reseed your PRNGs" is not equivalent to "your VM has been cloned".
Given all this, it would help the discussion if you explained why you believe random.c should propagate VM clone events and how.
If you don't believe that, could you explain what is the problem with the proposed uevent mechanism? Personally, I agree with Lennart that VMGenID is a generic ACPI device built for exactly this purpose. VMGenID is not an "ad-hoc driver". It is a standard which is supported by most (all?) major VMMs out there today and its whole purpose is to deliver inside the VM a notification that it was cloned.
Cheers, Babis
Hey Jason,
On 29.04.24 11:04, Lennart Poettering wrote:
On Fr, 26.04.24 14:52, Jason A. Donenfeld (Jason@zx2c4.com) wrote:
I don't think adding UAPI to an individual device driver like this
Does vmgenid really qualify as "an individual device driver"? It's a pretty generic software interface, implemented by various different VMMs these days. It is also the only interface I am aware of that actually exists and would provide the concept right now?
if this was really hyperv specific, then I'd agree it's just an "individual device driver". But it's widely implemented, for example a trivial command line switch in qemu.
Hence, for something this generic, and widely deployed with multiple backend implementations I think we can say it's kinda more of a subsystem and less of an individual driver, no?
is a good approach especially considering that the virtio changes we discussed some time ago will likely augment this and create another means of a similar notification. And given that this intersects with other userspace-oriented work I hope to get back to pretty soon, I think introducing some adhoc mechanism like this adds clutter and isn't the ideal way forward.
If one day a virtio-based equivalent shows up, then I'd be entirely fine with supporting this in userspace directly too , because virtio too is a generic thing typically implemented by multiple VMM backends. From my userspace perspective I see little benefit in the kernel abstracting over vmgenid and virtio-genid (if that ever materializes), as a systemd person I am not asking for this kind of abstraction (in case anyone wonders). A generic ACPI device such as vmgenid is entirely enough of "generic" for me.
The way we would process the event in userspace in systemd (from a udev rule) is so generic that it's trivial to match against two generic interfaces, instead of just one.
And even if there's value in a generic abstraction provided by the kernel over both vmgenid and a future virtio-based thing: the kernel patch in question was a *single* line, and our hookup in userspace could easily be moved over when the day comes, because it's really not a rocket science level interface. It's a single parameterless event, how much easier could things get?
I understand that how this all happened wasn't to everyones wishes, but do we really have to make all of this so complex if it could just be so simple? Why delay this further, why go back again given the event, the interface itself is such an utter triviality? Do we really make such a threatre around a single line change, a single additional uevent, just because of politics?
Friendly ping again. We would really like to have a constructive technical conversation and collaboration on how to make forward progress with VM clone notifications for user space applications that hold unique data and hence need to learn about VM clone events, outside of any randomness semantics.
Thanks,
Alex
Amazon Web Services Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B Sitz: Berlin Ust-ID: DE 365 538 597
On Thu, Jun 13, 2024 at 6:37 PM Alexander Graf graf@amazon.com wrote:
Friendly ping again. We would really like to have a constructive technical conversation and collaboration on how to make forward progress with VM clone notifications for user space applications that hold unique data and hence need to learn about VM clone events, outside of any randomness semantics.
With the other work now mostly done, sure, let's pick this up again. I think next on the list was getting the virtio rng device delivering VM clone events and unique UUIDs. There was a spec change posted a while back and a patch for the kernel. Do you want to refresh those? I thought that was a promising direction -- and the one we all decided together in person as the most viable, race-free way, etc -- especially because it would make ways of exposing those IDs low cost. And, importantly for you, I think that might *also* cover the need that you have here, so we'll kill several birds with one stone.
Jason
On 17.09.24 20:04, Jason A. Donenfeld wrote:
On Thu, Jun 13, 2024 at 6:37 PM Alexander Graf graf@amazon.com wrote:
Friendly ping again. We would really like to have a constructive technical conversation and collaboration on how to make forward progress with VM clone notifications for user space applications that hold unique data and hence need to learn about VM clone events, outside of any randomness semantics.
With the other work now mostly done, sure, let's pick this up again. I think next on the list was getting the virtio rng device delivering VM clone events and unique UUIDs. There was a spec change posted a while back and a patch for the kernel. Do you want to refresh those? I thought that was a promising direction -- and the one we all decided together in person as the most viable, race-free way, etc -- especially because it would make ways of exposing those IDs low cost. And, importantly for you, I think that might *also* cover the need that you have here, so we'll kill several birds with one stone.
The virtio proposal only addressed consumers that require single atomic memory updates to learn about any event that is disruptive to their entropy sources. With vgetrandom and/or rdrand we solved that problem, so we can close the chapter of that class of use cases.
What is still open are user space applications that require event based notification on VM clone events - and *only* VM clone events. This mostly caters for tools like systemd which need to execute policy - such as generating randomly generated MAC addresses - in the event a VM was cloned.
That's the use case this patch "vmgenid: emit uevent when VMGENID updates" is about and I think the best path forward is to just revert the revert. A uevent from the device driver is a well established, well fitting Linux mechanism for that type of notification.
Alex
Amazon Web Services Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B Sitz: Berlin Ust-ID: DE 365 538 597
On 22.04.24 09:51, Alexander Graf wrote:
[Adding CC list of original patch plus regression tracker]
On 18.04.24 13:48, Jason A. Donenfeld wrote:
This reverts commit ad6bcdad2b6724e113f191a12f859a9e8456b26d. I had nak'd it, and Greg said on the thread that it links that he wasn't going to take it either, especially since it's not his code or his tree, but then, seemingly accidentally, it got pushed up some months later, in what looks like a mistake, with no further discussion in the linked thread. So revert it, since it's clearly not intended.
Reverting this patch creates a user space visible regression compared to v6.8.
A theoretical one? Sure! But did any machines actually used in production break? From my understanding of Linus approach to the "no regression" rule this is what matters most here.
And even if that was the case: It afaics also matters that the commit was just in proper releases for a short time frame. Linus thus might consider the whole situation along the lines of "we really did screw up here and to fix it we are bending the 'no regressions' rule slightly; sorry". Things like that iirc have happened in the past, but I might misremember here.
Linus, if I got you wrong there, please speak up. But right now I'm inclined to not handle this as a regression and drop it from the tracking.
Ciao, Thorsten
Please treat it as such.
I'm slightly confused to see you passionate about this patch after you ghosted the conversation you referenced:
https://lore.kernel.org/lkml/00d6172f-e291-4e96-9d3e-63ee8e60d556@amazon.com...
The purpose of this uevent is to notify systemd[1][2] (or similar) that a VM clone event happened, so it can for example regenerate MAC addresses if it generated them on boot, regenerate its unique machine id or simply force rerequest a new DHCP lease.
I don't understand how there's any correlation or dependency to vgetrandom() or anything RNG in this and why getting vgetrandom() merged upstream is even something to talk about in the same line as this patch [3].
We had a lengthy, constructive conversation with Ted at LPC last year about the "PRNG and clone" use case and concluded that it's best for everyone to simply assume the system could be cloned at any point, hence always force intermix of RDRAND or comparable to any PRNG output. We since no longer need an event for that case.
Alex
[1] https://github.com/systemd/systemd/issues/26380 [2] https://lore.kernel.org/lkml/ZJGNREN4tLzQXOJr@gardel-login/ [3] https://lore.kernel.org/lkml/CAHmME9pxc-nO_xa=4+1CnvbnuefbRTJHxM7n817c_TPeox...
#regzbot introduced: 3aadf100f93d8081
Fixes: ad6bcdad2b67 ("vmgenid: emit uevent when VMGENID updates") Cc: stable@vger.kernel.org Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Link: https://lore.kernel.org/r/20230531095119.11202-2-bchalios@amazon.es Signed-off-by: Jason A. Donenfeld Jason@zx2c4.com
drivers/virt/vmgenid.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c index b67a28da4702..a1c467a0e9f7 100644 --- a/drivers/virt/vmgenid.c +++ b/drivers/virt/vmgenid.c @@ -68,7 +68,6 @@ static int vmgenid_add(struct acpi_device *device) static void vmgenid_notify(struct acpi_device *device, u32 event) { struct vmgenid_state *state = acpi_driver_data(device); - char *envp[] = { "NEW_VMGENID=1", NULL }; u8 old_id[VMGENID_SIZE]; memcpy(old_id, state->this_id, sizeof(old_id)); @@ -76,7 +75,6 @@ static void vmgenid_notify(struct acpi_device *device, u32 event) if (!memcmp(old_id, state->this_id, sizeof(old_id))) return; add_vmfork_randomness(state->this_id, sizeof(state->this_id)); - kobject_uevent_env(&device->dev.kobj, KOBJ_CHANGE, envp); } static const struct acpi_device_id vmgenid_ids[] = {
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
linux-stable-mirror@lists.linaro.org
-
Alexander Graf -
Babis Chalios -
Greg Kroah-Hartman -
Jason A. Donenfeld -
Lennart Poettering -
Linux regression tracking (Thorsten Leemhuis)