Date: Sun, 2 Nov 2014 22:13:31 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: d@delphij.net Cc: "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, Mark R V Murray <mark@grondar.org> Subject: Re: svn commit: r273958 - head/sys/dev/random Message-ID: <20141102201331.GE53947@kib.kiev.ua> In-Reply-To: <54568E41.8030305@delphij.net> References: <201411020201.sA221unt091493@svn.freebsd.org> <720EB74E-094A-43F3-8B1C-47BC7F6FECC3@grondar.org> <1414934579.17308.248.camel@revolution.hippie.lan> <6FB65828-6A79-4BDE-A9F7-BC472BA538CE@grondar.org> <CAJ-VmomeOwE3LOpehhJ__G=FCoBDRXrrn%2BSfjwPFODts6YYHNQ@mail.gmail.com> <20141102192057.GB53947@kib.kiev.ua> <29A795E1-19E2-49E4-9653-143D3F6F3F12@grondar.org> <20141102194625.GC53947@kib.kiev.ua> <751CD860-95B9-4F68-AE69-976B42823AD0@grondar.org> <54568E41.8030305@delphij.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Nov 02, 2014 at 12:04:17PM -0800, Xin Li wrote:
> -----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@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.
Essentially same, but not quite. I am saying that ra_read() methods
must block by sleep when they cannot provide randomness immediately.
They should do this with some sleeping primitive, and the same
issue as with tsleep() above, but in random adaptor code, appears.
>
> >> 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.
Assume that there is no random adaptors which are able to provide
any randomness loaded at all. Some thread is blocked in read.
Now, you cannot load any module which would unblock the thread.
>
> I think that we should probably release the lock briefly and reacquire
> it after the sleep to allow a different thread to acquire it.
No. Lock must be dropped around the sleep. State should be checked
and loop restarted from the beginning if state changed. Some form
of the generation counter seems to be useful there. But this
is definitely of low priority.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141102201331.GE53947>