OS-8280 Add /proc/sys/kernel/random/uuid to LX brands #361

pekdon · 2021-04-01T06:13:37Z

/proc/sys/kernel/random/uuid generates a random UUID on each read and is used by software such as xapian.

citrus-it

As an interested third party, I have a couple of comments on this, and thanks for doing it!

Since there are now two places in this file which generate a random UUID, and the other place doesn't generate a valid UUID, how about making a new function that generates a valid type 4 UUID, then use that function for both this and random/boot_id ? Real linux does generate valid type 4 UUIDs for boot_id so it would be nice if we did the same.

citrus-it · 2021-04-01T13:17:02Z

usr/src/uts/common/brand/lx/procfs/lx_prvnops.c

@@ -4995,6 +5000,34 @@ lxpr_read_sys_kernel_rand_entavl(lxpr_node_t *lxpnp, lxpr_uiobuf_t *uiobuf)
 	lxpr_uiobuf_printf(uiobuf, "%d\n", swrand_stats.ss_entEst);
 }

+/* ARGSUSED */


This line is no-longer necessary, it can be removed if you wish.

citrus-it · 2021-04-01T13:18:48Z

usr/src/uts/common/brand/lx/procfs/lx_prvnops.c

+	r[6] = 0x40 | (r[6] & 0x0f);
+	r[8] = 0x80 | (r[8] & 0x3f);


How about adding comments here to make it obvious what these lines do?

Suggested change

r[6] = 0x40 | (r[6] & 0x0f);

r[8] = 0x80 | (r[8] & 0x3f);

/* Set UUID version to 4 (random) */

r[6] = 0x40 | (r[6] & 0x0f);

/* Set UUID variant to 1 */

r[8] = 0x80 | (r[8] & 0x3f);

pekdon · 2021-04-01T20:01:28Z

Don't know why I didn't see these comments earlier, anyway, will address them tomorrow.

citrus-it · 2021-04-03T11:29:31Z

usr/src/uts/common/brand/lx/procfs/lx_prvnops.c

+lxpr_gen_uuid(char *uuid, size_t size)
+{
+	uint8_t r[16];
+	(void) random_get_bytes(r, sizeof (r));


random_get_bytes() can possibly fail if there is insufficient entropy, returning EAGAIN (I wonder if that's why the original implementation called random_get_bytes() several times for each component @jjelinek ?)
In that case, the UUID is going to be built from whatever was on the stack.
I think we should rather just fall back to random_get_pseudo_bytes() so it's at least a bit more random.

Suggested change

(void) random_get_bytes(r, sizeof (r));

if (random_get_bytes(r, sizeof (r)) != 0)

(void) random_get_pseudo_bytes(r, sizeof (r));

Thanks for the feedback, falsely assumed it wouldn't fail as the original code didn't check the return value either.

danmcd · 2021-04-05T05:11:00Z

Can you leave any notes about how you tested this? I'd like to pull this in, but I can't w/o knowing how you tested this (so we can repeat it internally). The code looks good and I'll mark it as a +1 for review, but we can't grant integration approval without testing notes.

danmcd

Needs test notes, but code seems decent enough.

pekdon · 2021-04-05T07:11:04Z

Can you leave any notes about how you tested this? I'd like to pull this in, but I can't w/o knowing how you tested this (so we can repeat it internally). The code looks good and I'll mark it as a +1 for review, but we can't grant integration approval without testing notes.

So far I've only done manual tests being all new to SmartOS and it's ecosystem. What I've done is to ensure that the output is valid version 4 variant 1 UUIDs using online validators, and verified that boot_id does not change. I've also been able to run sup using this, which is what started this in the first place.

The code is yet to be stress tested.

While reading up on testing in the SmartOS project I noticed the lx-tests repository so will create a PR which add simple functional tests there.

Any specific tests other than writing functional tests in lx-tests you would want me to do @danmcd?

danmcd · 2021-04-05T15:20:08Z

Honestly, for this, you probably don't need to worry about stress-tests. I was more concerned with was it any different from expectations (i.e. would a Linux user not be surprised by a difference) and it seems it hasn't. You also regression-tested boot_id, which is also good.

@citrus-it told me via chat he's done your tests on his own integration of this into OmniOS, so I'd like him to report here, and I'll turn this into an OS ticket as well.

danmcd · 2021-04-05T15:27:08Z

OS ticket filed: https://smartos.org/bugview/OS-8280
Once Andy's test notes are in (and I may need to cstyle this according to him?) either I or someone else can IA this.

pekdon · 2021-04-05T15:59:07Z

Have some simple tests prepared now, https://github.com/pekdon/lx-tests/tree/procfs-random-tests, but I guess those should wait until this is merged.

pekdon · 2021-04-05T16:01:11Z

Honestly, for this, you probably don't need to worry about stress-tests.

The only thing that would be nice to know that is that it gracefully falls back into using pseudorandom code on low entropy situations I guess, hard to device such a test however.

citrus-it · 2021-04-05T20:07:30Z

I integrated this into OmniOS and did some basic testing - booting an LX zone and reading from the new uuid file as fast as possible, then checked the returned UUIDs were valid type 4. Even when reading as fast as possible, I wasn't able to observe random_get_bytes() failing so that it needed to fall back to random_get_pseudo_bytes() though.

I also check that the boot_id file returned a valid type 4 UUID and it did not change over time, and that it changed on zone reboot.

Re the cstyle, it is just the indentation of the continuation lines to snprintf() which should be 4 spaces if SmartOS is aligned with the illumos style.

usr/src/uts/common/brand/lx/procfs/lx_prvnops.c

papertigers · 2021-04-06T15:22:41Z

usr/src/uts/common/brand/lx/procfs/lx_prvnops.c

+	char uuid[LX_BOOTID_LEN];
+
+	ASSERT(lxpnp->lxpr_type == LXPR_SYS_KERNEL_RAND_UUID);
+	ASSERT(zone->zone_brand == &lx_brand);


In smartos lx proc can be found at /system/lxproc in a native zone. I don't see the sys dir under that path, but I do see it in an lx zone. I just want to confirm someone in a native zone cant use this path to trigger this assert. I would like to verify that we don't mount sys in native.

https://github.com/joyent/illumos-joyent/blob/2a7c46f8d5561b2947bce41d61abe7aae999ba8e/usr/src/uts/common/brand/lx/procfs/lx_prvnops.c#L31

I guess this answers my question :)

usr/src/uts/common/brand/lx/procfs/lx_prvnops.c

danmcd · 2021-04-06T15:50:57Z

Unless @pekdon wants to fix that last UUID-length nit, I'm willing to IA this given the testing it's got.

Add /proc/sys/kernel/random/uuid that generates a random UUID on each read. It is used by software such as xapian. Updated /proc/sys/kernel/random/boot_id to use the same UUID code to return a valid UUID instead of a bogus one dropping the use of LX_BOOTID_LEN in favour of UUID_PRINTABLE_STRING_LENGTH.

citrus-it reviewed Apr 1, 2021

View reviewed changes

citrus-it reviewed Apr 3, 2021

View reviewed changes

danmcd previously approved these changes Apr 5, 2021

View reviewed changes

danmcd changed the title ~~Add /proc/sys/kernel/random/uuid to LX brands~~ OS-8280 Add /proc/sys/kernel/random/uuid to LX brands Apr 5, 2021

danmcd requested a review from papertigers April 5, 2021 17:54

danmcd reviewed Apr 5, 2021

View reviewed changes

usr/src/uts/common/brand/lx/procfs/lx_prvnops.c Outdated Show resolved Hide resolved

pekdon dismissed danmcd’s stale review via 2a7c46f April 6, 2021 06:20

papertigers reviewed Apr 6, 2021

View reviewed changes

papertigers previously approved these changes Apr 6, 2021

View reviewed changes

papertigers reviewed Apr 6, 2021

View reviewed changes

usr/src/uts/common/brand/lx/procfs/lx_prvnops.c Outdated Show resolved Hide resolved

pekdon dismissed papertigers’s stale review via eb67e95 April 6, 2021 19:33

papertigers approved these changes Apr 6, 2021

View reviewed changes

danmcd added the integration-approval label Apr 6, 2021

danmcd merged commit 307c10b into TritonDataCenter:master Apr 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OS-8280 Add /proc/sys/kernel/random/uuid to LX brands #361

OS-8280 Add /proc/sys/kernel/random/uuid to LX brands #361

pekdon commented Apr 1, 2021

citrus-it left a comment

citrus-it Apr 1, 2021

citrus-it Apr 1, 2021 •

edited

pekdon commented Apr 1, 2021

citrus-it Apr 3, 2021 •

edited

pekdon Apr 3, 2021

danmcd commented Apr 5, 2021

danmcd left a comment

pekdon commented Apr 5, 2021 •

edited

danmcd commented Apr 5, 2021

danmcd commented Apr 5, 2021

pekdon commented Apr 5, 2021

pekdon commented Apr 5, 2021

citrus-it commented Apr 5, 2021

papertigers Apr 6, 2021

papertigers Apr 6, 2021

danmcd commented Apr 6, 2021

	(void) random_get_bytes(r, sizeof (r));
	if (random_get_bytes(r, sizeof (r)) != 0)
	(void) random_get_pseudo_bytes(r, sizeof (r));

OS-8280 Add /proc/sys/kernel/random/uuid to LX brands #361

OS-8280 Add /proc/sys/kernel/random/uuid to LX brands #361

Conversation

pekdon commented Apr 1, 2021

citrus-it left a comment

Choose a reason for hiding this comment

citrus-it Apr 1, 2021

Choose a reason for hiding this comment

citrus-it Apr 1, 2021 • edited

Choose a reason for hiding this comment

pekdon commented Apr 1, 2021

citrus-it Apr 3, 2021 • edited

Choose a reason for hiding this comment

pekdon Apr 3, 2021

Choose a reason for hiding this comment

danmcd commented Apr 5, 2021

danmcd left a comment

Choose a reason for hiding this comment

pekdon commented Apr 5, 2021 • edited

danmcd commented Apr 5, 2021

danmcd commented Apr 5, 2021

pekdon commented Apr 5, 2021

pekdon commented Apr 5, 2021

citrus-it commented Apr 5, 2021

papertigers Apr 6, 2021

Choose a reason for hiding this comment

papertigers Apr 6, 2021

Choose a reason for hiding this comment

danmcd commented Apr 6, 2021

citrus-it Apr 1, 2021 •

edited

citrus-it Apr 3, 2021 •

edited

pekdon commented Apr 5, 2021 •

edited