svn commit: r273958 - head/sys/dev/random

[Xin Li]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 11/02/14 11:56, Mark R V Murray wrote:
> 
>> On 2 Nov 2014, at 19:46, Konstantin Belousov
>> <kostikbel at gmail.com> wrote:
>> 
>>> I don???t quite follow what you mean, but it sounds like you
>>> understand the problem. Could you please explain with a bit
>>> more detail?
>> 
>> Which problem ? There are two.
>> 
>> One is the Adrian' complain. tsleep(9) catches signals, and
>> return EINTR/ERESTART when catched.  Typical driver code checks
>> for the errors from {t,m}sleep(9) and aborts the operation if
>> error is returned.  I.e. you should do error = tsleep(...); if
>> (error != 0) { abort the loop; return to caller; } The fine
>> detail is that for the case when read has already partially 
>> progressed, i.e. something was copied out to uio, the error must 
>> not be returned, but short read performed instead.
> 
> OK, I think I follow this.
> 
> In another mail you say:
>> Yes, this is because error from tsleep() in
>> random_adaptor_read() does not abort the loop.  But next loop
>> iteration calls tsleep() which returns immediately since there is
>> still pending signal. The process continues indefinitely.
> 
> .. which supports this what you say further above. Thanks.
> 
>> This leads to another question about the code in
>> random_adapter_read(): if ra_read method sleeps, it must handle
>> the signals as well, return error, and the second loop which
>> perorms ra_read/uiomove should be aborted as well.  Again, error
>> from either ra_read or uiomove(9) must result in short read if
>> something was already copied to uio. Currently, there is no error
>> returned by ra_read (or it is ignored), and error from uiomove
>> always returned, even if something was already copied.
> 
> Are you saying the same thing again, or something else? If you are
> saying something else, then I am struggling to follow you.

These are essentially the same issue.  Since we do tsleep() with
PCATCH, the result should be checked and handled.

>> Second problem is that random_adaptor_lock is owned while
>> tsleep() is called (or whatever sleep primitive is used inside
>> ra_read).  If platform could only provide randomness through some
>> hw, and module is loaded while thread is blocked, module cannot
>> register, while reading thread cannot make progress.
> 
> I’m sorry, I don’t understand this.

There is a race condition here (practically I don't think it's a real
world problem right now, but we'd better fix it before it become one).
 Consider this:

dummy adapter registered

		A process requests a read(), now with
		&random_adaptors_lock held (shared)

				Another thread calls in and attempted
				to register yarrow.

Now, the third thread can not succeed until the second thread hits an
Ctrl+C.

I think that we should probably release the lock briefly and reacquire
it after the sleep to allow a different thread to acquire it.

Cheers,
- -- 
Xin LI <delphij at delphij.net>    https://www.delphij.net/
FreeBSD - The Power to Serve!           Live free or die
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0

iQIcBAEBCgAGBQJUVo5BAAoJEJW2GBstM+ns6SIP/2hhmB4//+Dc6drDF54+Yh7d
8pI16CcC58WPUrf4a3xWQFTErpwLBdaRJ0c/G6r2FNv5ZWMomlNnMEvdsj88Bn1y
8cj6NxZTfAEK4YvxvYqSFEPLIlD4hnDwLyQ8ZN78uS9gOJLmZ0uaOwAulU09zTyg
OC7rIsirUG70PKe2tC4/qYjWMWg89Pdj2XYs7+aWoKDJWNNLUetllA+NTwUI2/I1
RBgRM2rTPkNVh9jd5ZZb87BtK+r4i+kaykKTYssZFXZpn6ii7NiNN1C9s6yE8eBd
GBus2b4vCi/5g4Pdlm7geKuoi1l/UgjRiAhVPiDcdb4tuE5qmcuGuohFXM8ibrwn
AT7RVV5jcZhxOvSJlsk1yXVbrDThIfnm/rg9pbsq6+/IF96CfWTFKSSeugBKodiK
QFx9gdqi6U3AA2aQ0ydQm5zG0TUR6pQVcdiDL8FKUj/Tq2zHlKilHhYFskFL1XX4
KL83e1m8CXGjkuLSX1MpG27l30mwbESfFApaNppQjFXJHF/jHh9aS0JHKIgvCvRh
BVJFr4D8Iiioj7gtduVtX362YUIFdQxPfv2ekHmlPwN9+NaEVCAAd4PvLcuxmr9R
4aRN+JHYOlPZibmwDnlldlM2DIj5wBSB2h61+Sb6hq6USEo/IaveOnVDN8unXNnE
AXYRn8M1aNAmMG8Kb/Y3
=D/my
-----END PGP SIGNATURE-----