lustre: why does cfs_get_random_bytes() exist?

lustre: why does cfs_get_random_bytes() exist?

[Theodore Ts'o]

I've been auditing uses of get_random_bytes() since there are places
where get_random_bytes() is getting used where something weaker, such
as prandom_u32() is quite sufficient.  Basically, if kernel code just
needs a random number which does not have any cryptographic
requirements (such as in ext[234]. which gets the new block group used
for inode allocations using get_random_bytes), then prandom_u32()
should be used instead of get_random_bytes() to save CPU overhead and
to reduce the drain on the /dev/urandom's entropy pool.

Typically, the reason for this is either for historical reasons, since
prandom_u32() hadn't existed when the code was written, or because
historical code was cut and pasted into newer code.

When I came across staging/lustre/lustre/libcfs/prng.c, I saw
something which is **really** weird.  It defines a cfs_rand() which is
functionally identical to prandom_u32().  More puzzlingly, it also
defines cfs_get_random_bytes() which calls get_random_bytes() and then
xor's the result with cfs_rand().  That last step has no cryptographic
effect, so I'm really wondering who thought this as a good idea and/or
necessary.

What I think should happen is that staging/lustre/lustre/libcfs/prng.c
should be removed, and calls to cfs_rand() should get replaced
prandom_u32(), and cfs_get_random_bytes() should get replaced with
get_random_bytes().

Does this sound reasonable?

Cheers,

						- Ted

Re: lustre: why does cfs_get_random_bytes() exist?

[Greg KH]

On Thu, Oct 03, 2013 at 12:39:08PM -0400, Theodore Ts'o wrote:
> I've been auditing uses of get_random_bytes() since there are places
> where get_random_bytes() is getting used where something weaker, such
> as prandom_u32() is quite sufficient.  Basically, if kernel code just
> needs a random number which does not have any cryptographic
> requirements (such as in ext[234]. which gets the new block group used
> for inode allocations using get_random_bytes), then prandom_u32()
> should be used instead of get_random_bytes() to save CPU overhead and
> to reduce the drain on the /dev/urandom's entropy pool.
> 
> Typically, the reason for this is either for historical reasons, since
> prandom_u32() hadn't existed when the code was written, or because
> historical code was cut and pasted into newer code.
> 
> When I came across staging/lustre/lustre/libcfs/prng.c, I saw
> something which is **really** weird.  It defines a cfs_rand() which is
> functionally identical to prandom_u32().  More puzzlingly, it also
> defines cfs_get_random_bytes() which calls get_random_bytes() and then
> xor's the result with cfs_rand().  That last step has no cryptographic
> effect, so I'm really wondering who thought this as a good idea and/or
> necessary.
> 
> What I think should happen is that staging/lustre/lustre/libcfs/prng.c
> should be removed, and calls to cfs_rand() should get replaced
> prandom_u32(), and cfs_get_random_bytes() should get replaced with
> get_random_bytes().
> 
> Does this sound reasonable?

Sounds reasonable to me, care to send a patch to do so?

thanks,

greg k-h

Re: lustre: why does cfs_get_random_bytes() exist?

[Theodore Ts'o]

On Thu, Oct 03, 2013 at 10:26:21AM -0700, Greg KH wrote:
> > Does this sound reasonable?
> 
> Sounds reasonable to me, care to send a patch to do so?
> 

I can do that, but I was waiting for Andras, Peng or Nikita to let me
now if there was something I was missing or not.  I'm pretty sure it's
something bogus, perhaps left over from a OS abstraction layer to
support Solaris or some such, but I am curious what was the historical
reason for the current code.

Cheers,

						- Ted

Re: lustre: why does cfs_get_random_bytes() exist?

[Dilger, Andreas]

On 2013/10/03 1:34 PM, "Theodore Ts'o" <tytso@mit.edu> wrote:
>On Thu, Oct 03, 2013 at 10:26:21AM -0700, Greg KH wrote:
>> > Does this sound reasonable?
>> 
>> Sounds reasonable to me, care to send a patch to do so?
>> 
>
>I can do that, but I was waiting for Andras, Peng or Nikita to let me
>now if there was something I was missing or not.  I'm pretty sure it's
>something bogus, perhaps left over from a OS abstraction layer to
>support Solaris or some such, but I am curious what was the historical
>reason for the current code.

The root of the problem with the existing get_random_bytes() is that when
the
Cray supercomputers with around 20k nodes reboot all at the same instant,
we
had problems with the Lustre client UUID (originally from
generate_random_uuid())
being identical between multiple nodes since they have virtually no
entropy at
boot time, and they and did not have hardware RNG.

The Lustre cfs_get_random_bytes() incorporates (via cfs_rand()) a seed
which
also hashes in the addresses from any network interfaces that are
configured.
Conversely, cfs_rand() also is seeded at startup from get_random_bytes() in
case a hardware RNG is available.  This ensures even with identical initial
conditions cfs_get_random_bytes() gets a different random stream on each
node.

It is also true that cfs_get_random_bytes() and cfs_rand() predate the
newer
APIs such as prandom_u32() (which, to be honest, I didn't even know existed
today).  This is work that was done for maybe 2.4.29? (RH 9?) kernels.

The number of users of cfs_get_random_bytes() is relatively small, mostly
just
for the client UUID and other unique identifiers at startup time.  The only
place it is used on an ongoing basis is for the capabilities that are
passed
from the metadata server to clients so the clients can validate they have
access
to the objects on the data server.  The rest of the code uses cfs_rand()
for
values that only need to be statistically uniform over time, so they don't
deplete the entropy pool.

I'm not against cleaning this up, if there is some mechanism for the
startup
code to add in the node interface addresses into the entropy pool, and this
is also used to perturb the prandom_u32() sequence after that point.
Besides
that initialization, it would be a simple search & replace for the users.

Cheers, Andreas
-- 
Andreas Dilger

Lustre Software Architect
Intel High Performance Data Division

Re: lustre: why does cfs_get_random_bytes() exist?

[Theodore Ts'o]

On Thu, Oct 03, 2013 at 11:06:58PM +0000, Dilger, Andreas wrote:
> 
> The Lustre cfs_get_random_bytes() incorporates (via cfs_rand()) a seed
> which
> also hashes in the addresses from any network interfaces that are
> configured.
> Conversely, cfs_rand() also is seeded at startup from get_random_bytes() in
> case a hardware RNG is available.  This ensures even with identical initial
> conditions cfs_get_random_bytes() gets a different random stream on each
> node.

With modern kernels, the /dev/random driver has the
add_device_randomness() interface which is used to mix in
personalization information, which includes the network MAC address.
So that particular concern should be covered without the hack of
mixing in cfs_rand().

> I'm not against cleaning this up, if there is some mechanism for the
> startup code to add in the node interface addresses into the entropy
> pool, and this is also used to perturb the prandom_u32() sequence
> after that point.

That's handled too, via the late initcall prandom_reseed().

Cheers,

					- Ted

Re: lustre: why does cfs_get_random_bytes() exist?

[Dilger, Andreas]

On 2013/10/03 5:45 PM, "Theodore Ts'o" <tytso@mit.edu> wrote:
>On Thu, Oct 03, 2013 at 11:06:58PM +0000, Dilger, Andreas wrote:
>> 
>> The Lustre cfs_get_random_bytes() incorporates (via cfs_rand()) a seed
>> which also hashes in the addresses from any network interfaces that are
>> configured.
>> Conversely, cfs_rand() also is seeded at startup from
>>get_random_bytes() in
>> case a hardware RNG is available.  This ensures even with identical
>>initial
>> conditions cfs_get_random_bytes() gets a different random stream on each
>> node.
>
>With modern kernels, the /dev/random driver has the
>add_device_randomness() interface which is used to mix in
>personalization information, which includes the network MAC address.
>So that particular concern should be covered without the hack of
>mixing in cfs_rand().

I think that depends on the network driver.  The Cray systems have some
very strange networking hardware that is beyond our control - definitely
not ethernet or Infiniband.

I'll have to ask the Cray folks if their network drivers do this today.

>> I'm not against cleaning this up, if there is some mechanism for the
>> startup code to add in the node interface addresses into the entropy
>> pool, and this is also used to perturb the prandom_u32() sequence
>> after that point.
>
>That's handled too, via the late initcall prandom_reseed().
>
>Cheers,
>
>					- Ted
>


Cheers, Andreas
-- 
Andreas Dilger

Lustre Software Architect
Intel High Performance Data Division

Re: lustre: why does cfs_get_random_bytes() exist?

[Theodore Ts'o]

On Sat, Oct 05, 2013 at 06:10:54AM +0000, Dilger, Andreas wrote:
> >With modern kernels, the /dev/random driver has the
> >add_device_randomness() interface which is used to mix in
> >personalization information, which includes the network MAC address.
> >So that particular concern should be covered without the hack of
> >mixing in cfs_rand().
> 
> I think that depends on the network driver.  The Cray systems have some
> very strange networking hardware that is beyond our control - definitely
> not ethernet or Infiniband.

add_device_randomness() is called from __dev_open() and
dev_set_mac_address() in net/core/dev.c.  This is above the ethernet
and infiniband level.  So as long as it looks like a Linux network
device, and they are setting the hardware media access address in the
standard place (dev->dev_addr), it should work fine.

If they don't then they should fix their drivers to call
add_device_randomness(); the answer shouldn't be to make every single
users of the Linux random number generation infrastructure work around
the problem at the subsystem or file system level!

						- Ted

Re: lustre: why does cfs_get_random_bytes() exist?

[Christoph Hellwig]

On Sat, Oct 05, 2013 at 10:21:21AM -0400, Theodore Ts'o wrote:
> add_device_randomness() is called from __dev_open() and
> dev_set_mac_address() in net/core/dev.c.  This is above the ethernet
> and infiniband level.  So as long as it looks like a Linux network
> device, and they are setting the hardware media access address in the
> standard place (dev->dev_addr), it should work fine.
> 
> If they don't then they should fix their drivers to call
> add_device_randomness(); the answer shouldn't be to make every single
> users of the Linux random number generation infrastructure work around
> the problem at the subsystem or file system level!

Besides these points a filesystem isn't the place to work around entropy
pool issues on some obscure platform.  Even if they can't fix it this
way for some reason the workaround still should be in the interaction
of the arch code and the Linux prng infrastructure and not in Lustre.