[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Minios-devel] [UNIKRAFT PATCH v3 1/1] lib/ukswrand: Add /dev/random and /dev/urandom



Hey,

while you work on a patch that applies to upstream, I still have some comments about this patch.

On 15.06.19 09:03, Costin Lupu wrote:
Actually the v2 was already pushed upstream. The new changes should be
in a different patch.

On 6/15/19 3:55 AM, Vlad-Andrei BĂDOIU (78692) wrote:
This patch adds the /dev/random and /dev/urandom devices. Since we do not have
any entropy generating function both devices share the same handlers. The
registration of the devices is guarded by the config option CONFIG_DEV_RANDOM.

Signed-off-by: Vlad-Andrei Badoiu <vlad_andrei.badoiu@xxxxxxxxxxxxxxx>
Reviewed-by: Costin Lupu <costin.lupu@xxxxxxxxx>
---
  lib/ukswrand/Config.uk   |   8 +++
  lib/ukswrand/Makefile.uk |   3 +
  lib/ukswrand/mwc_dev.c   | 120 +++++++++++++++++++++++++++++++++++++++
  3 files changed, 131 insertions(+)
  create mode 100644 lib/ukswrand/mwc_dev.c

diff --git a/lib/ukswrand/Config.uk b/lib/ukswrand/Config.uk
index 44f6ee50..5b0adb85 100644
--- a/lib/ukswrand/Config.uk
+++ b/lib/ukswrand/Config.uk
@@ -17,4 +17,12 @@ endchoice
  config LIBUKSWRAND_INITIALSEED
        int "Initial random seed"
        default 23
+
+config LIBUKSWRAND_DEV_RANDOM
+       bool "random and urandom devices"
+       select LIBDEVFS
+       default n
+       help
+               Registers the random and urandom devices

Please mention that this registers to libdevfs.

+
  endif
diff --git a/lib/ukswrand/Makefile.uk b/lib/ukswrand/Makefile.uk
index 8d4482c4..8c6b3637 100644
--- a/lib/ukswrand/Makefile.uk
+++ b/lib/ukswrand/Makefile.uk
@@ -4,3 +4,6 @@ CINCLUDES-$(CONFIG_LIBUKSWRAND) += -I$(LIBUKSWRAND_BASE)/include
  CXXINCLUDES-$(CONFIG_LIBUKSWRAND) += -I$(LIBUKSWRAND_BASE)/include
LIBUKSWRAND_SRCS-$(CONFIG_LIBUKSWRAND_MWC) += $(LIBUKSWRAND_BASE)/mwc.c
+ifdef CONFIG_LIBUKSWRAND_DEV_RANDOM
+LIBUKSWRAND_SRCS-$(CONFIG_LIBUKSWRAND_MWC) += $(LIBUKSWRAND_BASE)/mwc_dev.c
+endif
diff --git a/lib/ukswrand/mwc_dev.c b/lib/ukswrand/mwc_dev.c
new file mode 100644
index 00000000..09a9c817
--- /dev/null
+++ b/lib/ukswrand/mwc_dev.c
@@ -0,0 +1,120 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Authors: Vlad-Andrei Badoiu <vlad_andrei.badoiu@xxxxxxxxxxxxxxx>
+ *
+ * Copyright (c) 2019, University Politehnica of Bucharest. All rights 
reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of the copyright holder nor the names of its
+ *    contributors may be used to endorse or promote products derived from
+ *    this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ *
+ * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
+ */
+
+#include <stdlib.h>
+#include <string.h>
+#include <uk/ctors.h>
+#include <uk/print.h>
+#include <uk/swrand.h>
+#include <uk/assert.h>
+#include <uk/config.h>
+#include <uk/essentials.h>
+#include <vfscore/uio.h>
+#include <devfs/device.h>
+
+#define DEV_RANDOM_NAME "random"
+#define DEV_URANDOM_NAME "urandom"
+
+int dev_random_read(struct device *dev __unused, struct uio *uio,
+                       int flags __unused)
+{
+       size_t count, step, chunk_size, i;
+       char *buf;
+       __u32 rd;
+
+       buf = uio->uio_iov->iov_base;
+       count = uio->uio_iov->iov_len;
+
+       step = sizeof(__u32);
+       chunk_size = count % step;
+
+       for (i = 0; i < count - chunk_size; i += step)
+               *((__u32 *)buf + i) = uk_swrand_randr();
+
+       /* fill the remaining bytes of the buffer */
+       if (chunk_size > 0) {
+               rd = uk_swrand_randr();
+               memcpy(buf + i, &rd, chunk_size);
+       }
+
+       uio->uio_resid = 0;
+       return 0;
+}

You should add a "/* TODO */" comment that mentions that this function is not thread-safe nor SMP-safe since uk_swrand_randr() isn't either.

+
+int dev_random_open(struct device *device __unused, int mode __unused)
+{
+       return 0;
+}
+
+
+int dev_random_close(struct device *device __unused)
+{
+       return 0;
+}
+
+static struct devops random_devops = {
+       .read = dev_random_read,
+       .open = dev_random_open,
+       .close = dev_random_close,
+};
+
+static struct driver drv_random = {
+       .devops = &random_devops,
+       .devsz = 0,
+       .name = DEV_RANDOM_NAME
+};
+
+static struct driver drv_urandom = {
+       .devops = &random_devops,
+       .devsz = 0,
+       .name = DEV_URANDOM_NAME
+};
+
+__constructor_prio(102) static void _uk_dev_swrand_ctor(void)
+{
+       struct device *dev;
+
+       uk_pr_info("Register %s and %s\n",
+               DEV_RANDOM_NAME, DEV_URANDOM_NAME);

I would still prefer a bit more verbose message:
  "Register '%s' and '%s' to devfs\n"

+
+       /* register /dev/urandom */
+       dev = device_create(&drv_urandom, DEV_URANDOM_NAME, D_CHR);
+       if (dev == NULL)
+               uk_pr_err("Failed to register %s\n", DEV_URANDOM_NAME);
+
+       /* register /dev/random */
+       dev = device_create(&drv_random, DEV_RANDOM_NAME, D_CHR);
+       if (dev == NULL)
+               uk_pr_err("fAiled to register %s\n", DEV_RANDOM_NAME);

You have a typo here. ;-) I also still prefer that you mention that the operation failed to register to devfs with both messages:

 uk_pr_err("Failed to register '%s' to devfs\n", [...]);

+}


Thanks a lot,

Simon

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.