Re: [PATCH] [RFC] Time-based RFC 4122 UUID generator

[Helge Deller <deller@gmx.de>]

Hello Valdis,

On Sunday 07 October 2007, Valdis.Kletnieks@vt.edu wrote:
> On Sat, 06 Oct 2007 15:53:37 +0200, Helge Deller said:
> > --- a/drivers/char/random.c
> > +++ b/drivers/char/random.c
> > @@ -1157,6 +1158,8 @@ void generate_random_uuid(unsigned char uuid_out[16])
> >  	uuid_out[6] = (uuid_out[6] & 0x0F) | 0x40;
> >  	/* Set the UUID variant to DCE */
> >  	uuid_out[8] = (uuid_out[8] & 0x3F) | 0x80;
> > +	/* Set multicast bit to avoid conflicts with NIC MAC addresses */
> > +	uuid_out[10] |= 0x80;
> >  }
> 
> 
> Erm, was it *intended* that you also changed the behavior of generate_random_uuid()?

Thanks that you ask! 
I really should have mentioned it in my initial posting.

Yes, this change was intentional, as it in fact fixes a possible bug in the original code.
Section 4.1.6 in RFC 4122 states regarding the "NodeID":
:  For systems with no IEEE address, a randomly or pseudo-randomly
:  generated value may be used; see Section 4.5.  The multicast bit must
:  be set in such addresses, in order that they will never conflict with
:  addresses obtained from network cards. 

So up to now it was just pure ("random") luck if this bit was set or not.

While at it...
My patch addressed another small non-critical glitch in the current implementation as well.
An "strace cat /proc/sys/kernel/random/uuid" shows:
: ....
: open("/proc/sys/kernel/random/uuid", O_RDONLY|O_LARGEFILE) = 3
: fstat64(3, {st_mode=S_IFREG|0444, st_size=0, ...}) = 0
: read(3, "378244d0-9b27-4f92-8b3a-386ed526"..., 1024) = 37
: write(1, "378244d0-9b27-4f92-8b3a-386ed526"..., 37378244d0-9b27-4f92-8b3a-386ed52692a8) = 37
: read(3, "", 1024)                       = 0
: close(3)                                = 0
: ...
The first read() returns the newly generated random uuid.
The second read() just returns zero, but the current implementation then already created a new uuid which wasn't used at all.
Although this is not critical, it was nevertheless leaking unnecessarily entropy from the pool.
For the case of the new time-based UUIDs it was worse. It unnecessary incremented the clock_seq number.
This issue is addressed by this part of my patch (esp.: "*ppos > 0"):
@@ -1191,13 +1317,13 @@ static int proc_do_uuid(ctl_table *table, int write, struct file *filp,
        ctl_table fake_table;
        unsigned char buf[64], tmp_uuid[16], *uuid;
 
-       uuid = table->data;
-       if (!uuid) {
-               uuid = tmp_uuid;
-               uuid[8] = 0;
+       /* random/time UUIDs need to be read completely at once */
+       if (table->ctl_name != RANDOM_BOOT_ID && *ppos > 0) {
+               *lenp = 0;
+               return 0;
        }

Helge