Date: Sun, 02 Nov 2014 13:39:10 -0800 From: Xin Li <delphij@delphij.net> To: Ian Lepore <ian@FreeBSD.org>, d@delphij.net Cc: Konstantin Belousov <kostikbel@gmail.com>, "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: <5456A47E.2030405@delphij.net> In-Reply-To: <1414961583.1200.27.camel@revolution.hippie.lan> 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> <20141102201331.GE53947@kib.kiev.ua> <545693B4.8030602@delphij.net> <1414961583.1200.27.camel@revolution.hippie.lan>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------090504050004050003030801
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
On 11/02/14 12:53, Ian Lepore wrote:
> On Sun, 2014-11-02 at 12:27 -0800, Xin Li wrote:
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512
>>
>> Hi, Mark,
>>
>> I'd like to propose the attached patch for review. It replaces
>> tsleep's with sx_sleep's, then checks the return value and quit
>> the loop.
>>
>> Cheers, - --
>
> It still doesn't handle the partial read/write case Kostik
> mentioned, but there are plenty of other drivers that don't get
> that right. Given that the ra_read/ra_write functions can't return
> error, it would only be errors from uiomove() in play. I guess it
> would be something like this:
>
> nbytes = uio->uio_resid; while (uio->uio_resid && !error) { c =
> MIN(uio->uio_resid, PAGE_SIZE);
> (random_adaptor->ra_read)(random_buf, c); error =
> uiomove(random_buf, c, uio); } if (uio->uio_resid != nbytes) error
> = 0; /* Return partial read, not error. */
>
> Also, there's now a mix of if (error == 0) and if (!error) near
> each other (I tend to prefer using ! only on boolean_t, but I even
> more prefer local consistancy. :)
Thanks for pointing this out.
I've added some checks at:
http://pastebin.com/ZGsqpUHR
Also attached.
Cheers,
- --
Xin LI <delphij@delphij.net> https://www.delphij.net/
FreeBSD - The Power to Serve! Live free or die
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0
iQIcBAEBCgAGBQJUVqR+AAoJEJW2GBstM+nsLl8QAIFQQZCGwwkeTlVkG/i9YzAl
pK4uox2dXouN1Zfwi9Q3ymB0DsUbnXt9IEP9JCD4Z8ZeFKUIC2mNI/V+t3etV5Kr
kVdvayizwbJ0jLBYu6rUIoeZeqWFnLB25PitJFCPOAqJ+H1h/hV56Zx9SvkAWEIe
spGpvq+eeHRYl/AzVMQFHjax0V7bo3yd+3klDlU3DT77WFvSRngNKUxHgVPMsoBY
xm4YRPy6MLAHXlEoHtoERc5Qyhj00Km4ZRHtXn3tFh6LSAdtXmbBiVXlHWZ0eVEn
5vH0d3MKRCnKtQg2ydEA5SWoCTMHIVCfoB/yQ4B0N0FupgdEUXMlag0iBV8DZcmb
nv1sd68V3GgDjiiAwevhbSLQ0cHr5FABtTdXwExClR2sOh3rN1PONsfbzfs3FgkO
TWts6znUcblmpCFyQyz49/r+SQ35S1YXCKIZB5Z43yysPgR20uuSKbD0XKAne28u
acntrXXgrfERSue1T5qz2xwH+dBWGuJtGDq1wSeP1kw8aWoX+ApAbUColCY2pEhm
OawGlJ6qnr4VgEIl4vQ/S9gaxMX33K/KCHJERaCA/8h1WQQklmRTky4URZ6bAaHJ
NMEd6GYD25GmfIY7kHB/tuPfjiWDwGnNh8mvSSAZBJM81gW+/9xqKwKn3OZ3kMFY
eM+SpnlDm4skN3geKGse
=yQhY
-----END PGP SIGNATURE-----
--------------090504050004050003030801
Content-Type: text/plain; charset=UTF-8;
name="random-tsleep.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="random-tsleep.diff"
Index: sys/dev/random/random_adaptors.c
===================================================================
--- sys/dev/random/random_adaptors.c (revision 273982)
+++ sys/dev/random/random_adaptors.c (working copy)
@@ -171,9 +171,8 @@ random_adaptor_register(const char *name, struct r
sx_xlock(&random_adaptors_lock);
LIST_INSERT_HEAD(&random_adaptors_list, rra, rra_entries);
random_adaptor_choose();
+ KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__));
sx_xunlock(&random_adaptors_lock);
-
- KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__));
}
void
@@ -182,9 +181,9 @@ random_adaptor_deregister(const char *name)
struct random_adaptors *rra;
KASSERT(name != NULL, ("invalid input to %s", __func__));
- KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__));
sx_xlock(&random_adaptors_lock);
+ KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__));
LIST_FOREACH(rra, &random_adaptors_list, rra_entries)
if (strcmp(rra->rra_name, name) == 0) {
LIST_REMOVE(rra, rra_entries);
@@ -208,16 +207,18 @@ random_adaptor_read(struct cdev *dev __unused, str
printf("random: %s %ld\n", __func__, uio->uio_resid);
#endif
- KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__));
+ random_buf = malloc(PAGE_SIZE, M_ENTROPY, M_WAITOK);
sx_slock(&random_adaptors_lock);
+ KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__));
+
/* Let the entropy source do any pre-read setup. */
(random_adaptor->ra_read)(NULL, 0);
/* (Un)Blocking logic */
error = 0;
- while (!random_adaptor->ra_seeded()) {
+ while (!random_adaptor->ra_seeded() && error == 0) {
if (flags & O_NONBLOCK) {
error = EWOULDBLOCK;
break;
@@ -224,7 +225,10 @@ random_adaptor_read(struct cdev *dev __unused, str
}
/* Sleep instead of going into a spin-frenzy */
- tsleep(&random_adaptor, PUSER | PCATCH, "block", hz/10);
+ error = sx_sleep(&random_adaptor, &random_adaptors_lock,
+ PUSER | PCATCH, "block", hz/10);
+ KASSERT(random_adaptor != NULL, ("No active random adaptor in %s",
+ __func__));
/* keep tapping away at the pre-read until we seed/unblock. */
(random_adaptor->ra_read)(NULL, 0);
@@ -241,12 +245,10 @@ random_adaptor_read(struct cdev *dev __unused, str
mtx_unlock(&random_read_rate_mtx);
- if (!error) {
+ if (error == 0) {
+ nbytes = uio->uio_resid;
/* The actual read */
-
- random_buf = malloc(PAGE_SIZE, M_ENTROPY, M_WAITOK);
-
while (uio->uio_resid && !error) {
c = MIN(uio->uio_resid, PAGE_SIZE);
(random_adaptor->ra_read)(random_buf, c);
@@ -256,11 +258,15 @@ random_adaptor_read(struct cdev *dev __unused, str
/* Let the entropy source do any post-read cleanup. */
(random_adaptor->ra_read)(NULL, 1);
- free(random_buf, M_ENTROPY);
+ if (nbytes != uio->uio_resid && (error == ERESTART ||
+ error == EINTR) )
+ error = 0; /* Return partial read, not error. */
+
}
-
sx_sunlock(&random_adaptors_lock);
+ free(random_buf, M_ENTROPY);
+
return (error);
}
@@ -269,6 +275,8 @@ random_adaptor_read_rate(void)
{
int ret;
+ sx_assert(&random_adaptors_lock, SA_LOCKED);
+
KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__));
mtx_lock(&random_read_rate_mtx);
@@ -287,18 +295,20 @@ random_adaptor_write(struct cdev *dev __unused, st
{
int c, error = 0;
void *random_buf;
+ ssize_t nbytes;
#ifdef RANDOM_DEBUG
printf("random: %s %zd\n", __func__, uio->uio_resid);
#endif
- KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__));
+ random_buf = malloc(PAGE_SIZE, M_ENTROPY, M_WAITOK);
sx_slock(&random_adaptors_lock);
- random_buf = malloc(PAGE_SIZE, M_ENTROPY, M_WAITOK);
+ KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__));
- while (uio->uio_resid > 0) {
+ nbytes = uio->uio_resid;
+ while (uio->uio_resid > 0 && error == 0) {
c = MIN(uio->uio_resid, PAGE_SIZE);
error = uiomove(random_buf, c, uio);
if (error)
@@ -306,13 +316,20 @@ random_adaptor_write(struct cdev *dev __unused, st
(random_adaptor->ra_write)(random_buf, c);
/* Introduce an annoying delay to stop swamping */
- tsleep(&random_adaptor, PUSER | PCATCH, "block", hz/10);
+ error = sx_sleep(&random_adaptor, &random_adaptors_lock,
+ PUSER | PCATCH, "block", hz/10);
+ KASSERT(random_adaptor != NULL, ("No active random adaptor in %s",
+ __func__));
}
+ sx_sunlock(&random_adaptors_lock);
+
+ if (nbytes != uio->uio_resid && (error == ERESTART ||
+ error == EINTR) )
+ error = 0; /* Partial write, not error. */
+
free(random_buf, M_ENTROPY);
- sx_sunlock(&random_adaptors_lock);
-
return (error);
}
@@ -325,10 +342,10 @@ random_adaptor_poll(struct cdev *dev __unused, int
printf("random: %s\n", __func__);
#endif
+ sx_slock(&random_adaptors_lock);
+
KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__));
- sx_slock(&random_adaptors_lock);
-
if (events & (POLLIN | POLLRDNORM)) {
if (random_adaptor->ra_seeded())
events &= (POLLIN | POLLRDNORM);
@@ -382,9 +399,9 @@ random_sysctl_active_adaptor_handler(SYSCTL_HANDLE
struct sbuf sbuf;
int error;
+ sx_slock(&random_adaptors_lock);
KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__));
- sx_slock(&random_adaptors_lock);
sbuf_new_for_sysctl(&sbuf, NULL, 16, req);
LIST_FOREACH(rra, &random_adaptors_list, rra_entries)
if (rra->rra_ra == random_adaptor) {
@@ -454,9 +471,9 @@ static void
random_adaptors_seed(void *unused __unused)
{
+ sx_slock(&random_adaptors_lock);
KASSERT(random_adaptor != NULL, ("No active random adaptor in %s", __func__));
- sx_slock(&random_adaptors_lock);
random_adaptor->ra_reseed();
sx_sunlock(&random_adaptors_lock);
--------------090504050004050003030801
Content-Type: application/octet-stream;
name="random-tsleep.diff.sig"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
filename="random-tsleep.diff.sig"
iQIcBAABCgAGBQJUVqR+AAoJEJW2GBstM+ns37wP/iTTXi8vOQwX3a/Rbs74kml917+RlNMI
sTzCEiymLTw1eCF458oGAbKx/F03Te964FvaAaL5gvyme39fSon35AnBPwjbgv8gABWjoElQ
1KshqtjLsvrSGyF7ev15dXTugw/J1zRW6d9E6qxE2qU6kLXh8KO3Al8AyRSmOEArB/tvhkyW
yu96o9f8pDQjFsrv4uaMg4bMxkbvBg5F0Yo5jNFmihWZDngWykcybTs4+mxP4fKBX8fCfVRJ
0MNXDP6BO/nbzTagMMQaIh3ADwIeccsQDsAbGsfYtuH36cY7FzMIsVske0VvXkAPTUPHi8sc
WwlzwsI1jn/zZZHHn8IMR5QqahGmE/wt0lLUNqMRD0le2pZ24xoR6W2DxCBKrwsJOq0p9gkI
rjXOyUj1fb5v43+s/+TnrShP0ja+B2MSvYGZigOnO9ykMmxWME4VYBnA5NYPtNlcioJDSAU+
9JhXvptxW1BGhASsHgaSYk6KbnINHjd3G0z6E3qrSfBsA0bm7j928gQnnlYKTMch4gq3PIYi
lGAYnTY/7V6b49ArZHlihPaywxTGhg1b9SLnzUgozhsiYf2WvMta+ORFs1fcqd1hrESqzoJ0
rQjQcbcfZqe/R8kcv7u4cSOFq0SD0r/a+/QG2XVhWiFz1Bg+UT4drX3TbQJe6BJe7ht2CJ5Q
VOAP
--------------090504050004050003030801--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5456A47E.2030405>