Re: [PATCH] qcow2 disk format
On Sun, 12 Aug 2018 22:51:24 -0700, Ori Bernstein wrote: > This patch adds preliminary support for creating qcow2 disk images. It gives the 'vmctl create' command an option '-f', which can be given arguments of either 'raw' (defualt) or 'qcow2', and it creates a disk of the appropriate format. The qcow2 header is duplicated inline, since it seemed better than the other options (putting it into a header and messing around with include/source paths so that we could share it with vmd, or creating a libqcow2) That decision may get revisited when I find time to implement snapshotting, since creating a snapshot will involve relatively deep modifications to the disk (adjusting refcounts, rewriting L1/L2 tables, etc). --- usr.sbin/vmctl/main.c | 18 +-- usr.sbin/vmctl/vmctl.8 | 7 ++- usr.sbin/vmctl/vmctl.c | 124 - usr.sbin/vmctl/vmctl.h | 3 +- 4 files changed, 143 insertions(+), 9 deletions(-) diff --git usr.sbin/vmctl/main.c usr.sbin/vmctl/main.c index b7674d0c980..f39ccbdbc1f 100644 --- usr.sbin/vmctl/main.c +++ usr.sbin/vmctl/main.c @@ -63,7 +63,8 @@ intctl_receive(struct parse_result *, int, char *[]); struct ctl_command ctl_commands[] = { { "console",CMD_CONSOLE,ctl_console,"id" }, - { "create", CMD_CREATE, ctl_create, "\"path\" -s size", 1 }, + { "create", CMD_CREATE, ctl_create, + "\"path\" -s size [-f fmt]", 1 }, { "load", CMD_LOAD, ctl_load, "\"path\"" }, { "log",CMD_LOG,ctl_log,"(verbose|brief)" }, { "reload", CMD_RELOAD, ctl_reload, "" }, @@ -470,24 +471,28 @@ int ctl_create(struct parse_result *res, int argc, char *argv[]) { int ch, ret; - const char *paths[2]; + const char *paths[2], *format; if (argc < 2) ctl_usage(res->ctl); paths[0] = argv[1]; paths[1] = NULL; + format = "raw"; if (pledge("stdio rpath wpath cpath", NULL) == -1) err(1, "pledge"); argc--; argv++; - while ((ch = getopt(argc, argv, "s:")) != -1) { + while ((ch = getopt(argc, argv, "s:f:")) != -1) { switch (ch) { case 's': if (parse_size(res, optarg, 0) != 0) errx(1, "invalid size: %s", optarg); break; + case 'f': + format = optarg; + break; default: ctl_usage(res->ctl); /* NOTREACHED */ @@ -498,7 +503,12 @@ ctl_create(struct parse_result *res, int argc, char *argv[]) fprintf(stderr, "missing size argument\n"); ctl_usage(res->ctl); } - ret = create_imagefile(paths[0], res->size); + if (strcmp(format, "raw") == 0) + ret = create_raw_imagefile(paths[0], res->size); + else if (strcmp(format, "qcow2") == 0) + ret = create_qc2_imagefile(paths[0], res->size); + else + errx(1, "unknown image format %s", format); if (ret != 0) { errno = ret; err(1, "create imagefile operation failed"); diff --git usr.sbin/vmctl/vmctl.8 usr.sbin/vmctl/vmctl.8 index 81ecbeb6c1d..358928c7ff6 100644 --- usr.sbin/vmctl/vmctl.8 +++ usr.sbin/vmctl/vmctl.8 @@ -50,12 +50,15 @@ Using .Xr cu 1 connect to the console of the VM with the specified .Ar id . -.It Cm create Ar path Fl s Ar size +.It Cm create Ar path Fl s Ar size Op Fl f Ar format Creates a VM disk image file with the specified .Ar path and .Ar size , -rounded to megabytes. +rounded to megabytes. The disk +.Ar format +may be specified as either 'raw' or 'qcow2', defaulting to 'raw' +if left unspecified. .It Cm load Ar filename Load additional configuration from the specified file. .It Cm log brief diff --git usr.sbin/vmctl/vmctl.c usr.sbin/vmctl/vmctl.c index 7ab9a9bc60c..b3a4e6d7b14 100644 --- usr.sbin/vmctl/vmctl.c +++ usr.sbin/vmctl/vmctl.c @@ -735,7 +735,7 @@ vm_console(struct vmop_info_result *list, size_t ct) } /* - * create_imagefile + * create_raw_imagefile * * Create an empty imagefile with the specified path and size. * @@ -749,7 +749,7 @@ vm_console(struct vmop_info_result *list, size_t ct) * E : Various other E errno codes due to other I/O errors */ int -create_imagefile(const char *imgfile_path, long imgsize) +create_raw_imagefile(const char *imgfile_path, long imgsize) { int fd, ret; @@ -770,3 +770,123 @@ create_imagefile(const char *imgfile_path, long imgsize) ret = close(fd); return (ret); } + +/* + * create_imagefile + * + * Create an empty qcow2 imagefile with the specified path and size. + * + * Parameters: + * imgfile_path: path to the image file to create + * imgsize : si
Re: [PATCH] qcow2 disk format
On Sun, 12 Aug 2018 22:51:24 -0700, Ori Bernstein wrote: > On Sun, 5 Aug 2018 00:52:32 -0700, Ori Bernstein wrote: > > And, now something that actually appears to work. You can create a > disk on OpenBSD using qemu: > > qemu-img create foo.qc2 16G > > add it to your vm.conf: > > disk "/path/to/foo.qc2" > > boot and install OpenBSD on it as normal, and if you decide you don't like > hardware virtualization, you can point qemu at it and run using that: > > qemu-system-x86_64 -m 1024 -hda foo.qc2 > > Snapshots haven't been tested yet, and tools need to be added, incompatible > extensions are silently ignored, and there could stand to be a bit more sanity > checking. > > vioscribble.c should also probably be extracted into a regress test, rather > than just something that sits beside the I/O code. > > Patch below: One more update, with some significant differences: - External snapshots will work if you comment out the chroot and add rpath to the pledges. This is a bad idea, so external snapshots will return a clean error until I can figure out a good way to plumb the fds, shuffle around the pledges, or do something else to make it possible to open the backing files from the vm process. - Internal snapshots seem to work, but you will need qemu to manage them. qemu-img snapshot -c snapname disk.qc2 # create qemu-img snapshot -a snapname disk.qc2 # revert These have only been tested lightly, so I wouldn't trust them fully yet. - vioscribble has been turned into a regress test, and grew license information. - A somewhat embarrassing bug, where I malloced the wrong type, was fixed. --- regress/usr.sbin/vmd/diskfmt/Makefile | 28 ++ regress/usr.sbin/vmd/diskfmt/vioscribble.c | 165 + usr.sbin/vmd/Makefile | 2 +- usr.sbin/vmd/vioqcow2.c| 574 + usr.sbin/vmd/virtio.c | 10 +- usr.sbin/vmd/virtio.h | 1 + 6 files changed, 776 insertions(+), 4 deletions(-) create mode 100644 regress/usr.sbin/vmd/diskfmt/Makefile create mode 100644 regress/usr.sbin/vmd/diskfmt/vioscribble.c create mode 100644 usr.sbin/vmd/vioqcow2.c diff --git regress/usr.sbin/vmd/diskfmt/Makefile regress/usr.sbin/vmd/diskfmt/Makefile new file mode 100644 index 000..71bb2b8ce52 --- /dev/null +++ regress/usr.sbin/vmd/diskfmt/Makefile @@ -0,0 +1,28 @@ +# $OpenBSD: Makefile,v 1.5 2018/07/20 22:18:49 bluhm Exp $ + +# This regression test creates a raw disk image and a +# qcow disk image, and scribbles the same data to both +# of them. It verifies that they both have the same +# result. +# +# In order for this test to work, qemu must be installed +# in order to create the disk images. + +VMD_DIR=$(BSDSRCDIR)/usr.sbin/vmd/ + +PROG=vioscribble +SRCS=vioscribble.c $(VMD_DIR)/vioqcow2.c $(VMD_DIR)/vioraw.c +CFLAGS+=-I$(VMD_DIR) -pthread +LDFLAGS+=-pthread + +run-regress-vioscribble: scribble-images + +scribble-images: + rm -f scribble.raw scribble.qc2 + vmctl create scribble.raw -s 4G + qemu-img create -f qcow2 scribble.qc2 4G + + +.PHONY: ${REGRESS_TARGETS} scribble-images + +.include diff --git regress/usr.sbin/vmd/diskfmt/vioscribble.c regress/usr.sbin/vmd/diskfmt/vioscribble.c new file mode 100644 index 000..3821c3b277b --- /dev/null +++ regress/usr.sbin/vmd/diskfmt/vioscribble.c @@ -0,0 +1,165 @@ +/* $OpenBSD: $ */ + +/* + * Copyright (c) 2018 Ori Bernstein + * + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +/* + * Quick hack of a program to try to test vioqcow2.c against + * vioraw.c. + * + * Compile with: + * + * cc -pthread -o scribble vioscribble.c vioqcow2.c vioraw.c + */ +#include /* PAGE_SIZE */ +#include +#include + +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "pci.h" +#include "vmd.h" +#include "vmm.h" +#include "virtio.h" + +#define CLUSTERSZ 65536 + +int verbose; +struct virtio_backing qcowfile; +struct virtio_backing rawfile; + +/* We expect the scribble disks to be 4g in size */ +#define DISK
Re: [PATCH] Pluggable disk formats for vmd (qcow2 preparation)
One more minor update to this patch: - Remove unused enum - Remove unused function prototype - Move some qcow2-specific headers into the qcow2 patch. --- usr.sbin/vmd/Makefile | 2 +- usr.sbin/vmd/vioraw.c | 73 ++ usr.sbin/vmd/vioscsi.c | 7 +++-- usr.sbin/vmd/virtio.c | 55 - usr.sbin/vmd/virtio.h | 17 +--- 5 files changed, 124 insertions(+), 30 deletions(-) create mode 100644 usr.sbin/vmd/vioraw.c diff --git usr.sbin/vmd/Makefile usr.sbin/vmd/Makefile index 7ea090f8886..24c1d1b1d4a 100644 --- usr.sbin/vmd/Makefile +++ usr.sbin/vmd/Makefile @@ -6,7 +6,7 @@ PROG= vmd SRCS= vmd.c control.c log.c priv.c proc.c config.c vmm.c SRCS+= vm.c loadfile_elf.c pci.c virtio.c i8259.c mc146818.c SRCS+= ns8250.c i8253.c vmboot.c ufs.c disklabel.c dhcp.c packet.c -SRCS+= parse.y atomicio.c vioscsi.c +SRCS+= parse.y atomicio.c vioscsi.c vioraw.c CFLAGS+= -Wall -I${.CURDIR} CFLAGS+= -Wstrict-prototypes -Wmissing-prototypes diff --git usr.sbin/vmd/vioraw.c usr.sbin/vmd/vioraw.c new file mode 100644 index 000..d377546de67 --- /dev/null +++ usr.sbin/vmd/vioraw.c @@ -0,0 +1,73 @@ +/* $OpenBSD: $ */ +/* + * Copyright (c) 2018 Ori Bernstein + * + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +#include + +#include +#include + +#include +#include + +#include "vmd.h" +#include "vmm.h" +#include "virtio.h" + +static ssize_t +raw_pread(void *file, char *buf, size_t len, off_t off) +{ + return pread(*(int *)file, buf, len, off); +} + +static ssize_t +raw_pwrite(void *file, char *buf, size_t len, off_t off) +{ + return pwrite(*(int *)file, buf, len, off); +} + +static void +raw_close(void *file) +{ + close(*(int *)file); + free(file); +} + +/* + * Initializes a raw disk image backing file from an fd. + * Stores the number of 512 byte sectors in *szp, + * returning -1 for error, 0 for success. + */ +int +virtio_init_raw(struct virtio_backing *file, off_t *szp, int fd) +{ + off_t sz; + int *fdp; + + sz = lseek(fd, 0, SEEK_END); + if (sz == -1) + return -1; + + fdp = malloc(sizeof(int)); + *fdp = fd; + file->p = fdp; + file->pread = raw_pread; + file->pwrite = raw_pwrite; + file->close = raw_close; + *szp = sz / 512; + return 0; +} + diff --git usr.sbin/vmd/vioscsi.c usr.sbin/vmd/vioscsi.c index 93867887598..af504f0550d 100644 --- usr.sbin/vmd/vioscsi.c +++ usr.sbin/vmd/vioscsi.c @@ -197,7 +197,7 @@ vioscsi_start_read(struct vioscsi_dev *dev, off_t block, ssize_t n_blocks) goto nomem; info->len = n_blocks * VIOSCSI_BLOCK_SIZE_CDROM; info->offset = block * VIOSCSI_BLOCK_SIZE_CDROM; - info->fd = dev->fd; + info->file = &dev->file; return info; @@ -210,7 +210,10 @@ nomem: static const uint8_t * vioscsi_finish_read(struct ioinfo *info) { - if (pread(info->fd, info->buf, info->len, info->offset) != info->len) { + struct virtio_backing *f; + + f = info->file; + if (f->pread(f->p, info->buf, info->len, info->offset) != info->len) { info->error = errno; log_warn("vioscsi read error"); return NULL; diff --git usr.sbin/vmd/virtio.c usr.sbin/vmd/virtio.c index 4622ef4943f..099f1df6a7e 100644 --- usr.sbin/vmd/virtio.c +++ usr.sbin/vmd/virtio.c @@ -361,7 +361,7 @@ vioblk_start_read(struct vioblk_dev *dev, off_t sector, ssize_t sz) goto nomem; info->len = sz; info->offset = sector * VIRTIO_BLK_SECTOR_SIZE; - info->fd = dev->fd; + info->file = &dev->file; return info; @@ -375,7 +375,10 @@ nomem: static const uint8_t * vioblk_finish_read(struct ioinfo *info) { - if (pread(info->fd, info->buf, info->len, info->offset) != info->len) { + struct virtio_backing *file; + + file = info->file; + if (file->pread(file->p, info->buf, info->len, info->offset) != info->len) { info->error = errno; log_warn("vioblk read error"); return NULL; @@ -398,7 +401,7 @@ vioblk_start_write(struct vioblk_dev *dev
Re: radeondrm(4) fix
> Date: Mon, 20 Aug 2018 11:43:31 +1000 > From: Jonathan Gray > > On Sun, Aug 19, 2018 at 07:38:44PM +0200, Mark Kettenis wrote: > > If memory is supposed to be cached, we should make it so. Matches > > what the Linux version of this code does. > > > > ok? > > Sure, I had something similiar in a tree here. > Could you keep the comment in the original code as well? Can you just commit your version? > Index: ttm_bo_util.c > === > RCS file: /cvs/src/sys/dev/pci/drm/ttm/ttm_bo_util.c,v > retrieving revision 1.19 > diff -u -p -r1.19 ttm_bo_util.c > --- ttm_bo_util.c 25 Apr 2018 01:27:46 - 1.19 > +++ ttm_bo_util.c 20 Aug 2018 01:32:45 - > @@ -514,6 +514,9 @@ EXPORT_SYMBOL(ttm_io_prot); > > pgprot_t ttm_io_prot(uint32_t caching_flags, pgprot_t tmp) > { > + /* Cached mappings need no adjustment */ > + if (caching_flags & TTM_PL_FLAG_CACHED) > + return tmp; > #ifdef PMAP_WC > if (caching_flags & TTM_PL_FLAG_WC) > return PMAP_WC; >
Re: ac(8) can segfault in two different ways
Sorry, I made a mistake in review here: On Sun, Aug 19, 2018 at 11:26:22PM -0500, Scott Cheloha wrote: > On Sun, Aug 19, 2018 at 09:40:43AM +0100, Ricardo Mestre wrote: > > > > [...] > > @@ -446,7 +451,8 @@ ac(FILE *fp) > > */ > > if (*usr.ut_name) { > > if (strncmp(usr.ut_line, "tty", 3) != 0 || > > - strchr("pqrstuvwxyzPQRST", usr.ut_line[3]) > > != NULL || > > + memchr("pqrstuvwxyzPQRST", usr.ut_line[3], > > + sizeof("pqrstuvwxyzPQRST")) != NULL || > > The memchr length needs to be sizeof("p...") - 1, otherwise you'll include > the NUL terminator in your search. I am wrong about the strchr. It's correct and should not be changed, I misread it yesterday and then again today. > > > *usr.ut_host != '\0') > > head = log_in(head, &usr); > > } else > > @@ -457,7 +463,8 @@ ac(FILE *fp) > > (void)fclose(fp); > > if (!(Flags & AC_W)) > > usr.ut_time = time(NULL); > > - (void)strlcpy(usr.ut_line, "~", sizeof usr.ut_line); > > + (void)strncpy(usr.ut_line, "~", sizeof usr.ut_line); > > + usr.ut_line[sizeof(usr.ut_line) - 1] = '\0'; > > Same deal here, use sizeof(usr.ut_line) - 1 for the strncpy length. > > > > if (Flags & AC_D) { > > ltm = localtime(&usr.ut_time); > >
Re: ac(8) can segfault in two different ways
On Sun, Aug 19, 2018 at 09:40:43AM +0100, Ricardo Mestre wrote: > > With your suggestion now it doesn't segfault anymore, but it still outputs > crazy stuff, do we really want this? Also -p still shows contents of the file > as per below: > > 0 serial@skoll:/usr/src/usr.sbin/ac$ obj/ac -w ./ac.c > total762774013172614.25 > 0 serial@skoll:/usr/src/usr.sbin/ac$ obj/ac -w ./ac.c -p > opyright (c) 1994 Christopher G. 338687831669364.19 > etain the above copyright > * 258230964833358.44 > user_list *Users = NULL; > static 46353282047238.85 > -") == 0) > fp = stdin; > else if 43998158041234.41 > g second */ > yesterday++; > for 39385397001446.54 > se it > */ > tlp = lp; > lp 18139557014551.83 > they came in on a pseudo-tty, t 17978822565419.97 > */ > head = log_out(head, &usr); > 0.00 > total762774013172614.25 utmp(5) is a binary format, so crazy input causes crazy output. I don't think we should be trying to outsmart the user. At least, not trying too hard. > What if we still add the check secs < 0 || secs > LLONG_MAX but instead of > erroring out just set secs to 0? It's the same behaviour as we have right now > if we use 'ac -w ./bogus_file user', this will call update_user with secs set > to 0L and the program won't segfault as it does if 'user' is not specified. First, there's no reason to check secs > LLONG_MAX. time_t is a signed 64-bit value on all platforms, as is long long, so that comparison is always false. Next, I don't think we should add a sanity check to handle a case where the user literally needs to feed the program garbage to trigger the behavior. The only case in which I see the check being useful is if the file is corrupted. And in that case I don't think we should truncate to to 0 and continue running. We should either handle the value as-is, allowing the user to see the crazy value in the output and reason about it, or fail closed and indicate that the file is invalid. I'm leaning toward not changing anything because it's at best a super rare edge case and I'm worried we're missing something. I want to think on it a bit more but at the moment I'm not ok with adding the check. Addressing the incorrect use of strlcpy/strchr is still totally ok though, as they are definitely bugs. More comments inline below. > Index: ac.c > === > RCS file: /cvs/src/usr.sbin/ac/ac.c,v > retrieving revision 1.25 > diff -u -p -u -r1.25 ac.c > --- ac.c 26 Apr 2018 12:42:51 - 1.25 > +++ ac.c 19 Aug 2018 08:26:54 - > @@ -29,6 +29,7 @@ > > #include > #include > +#include > #include > #include > #include > @@ -171,6 +172,9 @@ update_user(struct user_list *head, char > { > struct user_list *up; > > + if (secs < 0 || secs > LLONG_MAX) > + secs = 0; > + > for (up = head; up != NULL; up = up->next) { > if (strncmp(up->name, name, sizeof (up->name) - 1) == 0) { > up->secs += secs; > @@ -187,7 +191,8 @@ update_user(struct user_list *head, char > if ((up = malloc(sizeof(struct user_list))) == NULL) > err(1, "malloc"); While you're here could you allocate sizeof(*up) and change the err(3) from "malloc" -> NULL? > up->next = head; > - strlcpy(up->name, name, sizeof (up->name)); > + strncpy(up->name, name, sizeof (up->name)); > + up->name[sizeof(up->name) - 1] = '\0'; That strncpy should be strncpy(up->name, name, sizeof(up->name) - 1); > up->secs = secs; > Total += secs; > return up; > @@ -446,7 +451,8 @@ ac(FILE *fp) >*/ > if (*usr.ut_name) { > if (strncmp(usr.ut_line, "tty", 3) != 0 || > - strchr("pqrstuvwxyzPQRST", usr.ut_line[3]) > != NULL || > + memchr("pqrstuvwxyzPQRST", usr.ut_line[3], > + sizeof("pqrstuvwxyzPQRST")) != NULL || The memchr length needs to be sizeof("p...") - 1, otherwise you'll include the NUL terminator in your search. > *usr.ut_host != '\0') > head = log_in(head, &usr); > } else > @@ -457,7 +463,8 @@ ac(FILE *fp) > (void)fclose(fp); > if (!(Flags & AC_W)) > usr.ut_time = time(NULL); > - (void)strlcpy(usr.ut_line, "~", sizeof usr.ut_line); > + (void)strncpy(usr.ut_line, "~", sizeof usr.ut_line); > + usr.ut_line[sizeof(usr.ut_line) - 1] = '\0'; Same deal here, use sizeof(usr.ut_line) - 1 for the strncpy length. > > if (Flags & AC_D) { > ltm = localtime(&usr.ut_time); >
Re: inteldrm(4) regression from 6.1 to 6.2: wrong console resolution
Mark Kettenis wrote: >I have to think through the consequences of simply doing a delay >without checking the condition here though. Right now __wait_event_intr_timeout has a KASSERT(!cold), so, if its code is changed to have an "if (cold) { delay(tick); ret = 1; }" then we know that this new code is only going to be called from drm_wait_one_vblank because that's the only place where a corresponding "if (... || cold) return;" is removed. So we always know exactly what the condition is going to be in __wait_event_intr_timeout when "cold" is 1: the condition from drm_wait_one_vblank, which we know we can ignore because it was never used before in drm_wait_one_vblank when "cold" is 1 anyway (because drm_wait_one_vblank simply directly returns in that case). Doing it this way might be a problem in the future though: if code somewhere else is changed to call __wait_event_intr_timeout when "cold" is 1 and with a different condition then the condition will be silently ignored, which is not great. I still think the patch ought to be directly in drm_wait_one_vblank in drm_irq.c, because that's the function that promises to wait for one vblank but does not when "cold" is 1. So that's where I think "if (cold) { delay(tick); return; }" ought to be, and then there would be no need to worry about __wait_event_intr_timeout's condition or about vblank interrupts. I'm not the one who has to merge that code with the Linux code though :-) Philippe
Re: radeondrm(4) fix
On Sun, Aug 19, 2018 at 07:38:44PM +0200, Mark Kettenis wrote: > If memory is supposed to be cached, we should make it so. Matches > what the Linux version of this code does. > > ok? Sure, I had something similiar in a tree here. Could you keep the comment in the original code as well? Index: ttm_bo_util.c === RCS file: /cvs/src/sys/dev/pci/drm/ttm/ttm_bo_util.c,v retrieving revision 1.19 diff -u -p -r1.19 ttm_bo_util.c --- ttm_bo_util.c 25 Apr 2018 01:27:46 - 1.19 +++ ttm_bo_util.c 20 Aug 2018 01:32:45 - @@ -514,6 +514,9 @@ EXPORT_SYMBOL(ttm_io_prot); pgprot_t ttm_io_prot(uint32_t caching_flags, pgprot_t tmp) { + /* Cached mappings need no adjustment */ + if (caching_flags & TTM_PL_FLAG_CACHED) + return tmp; #ifdef PMAP_WC if (caching_flags & TTM_PL_FLAG_WC) return PMAP_WC;
[patch] switchd(8) on sparc64: panic: trap type 0x34
Hi, At BSDCan, someone reported that a sparc64 machine would panic if it was receiving any traffic on a member interface of a switch(4) during reboot. We got as far as getting this trace: panic: trap type 0x34 (mem address not aligned): pc=15864ec npc=15864f0 pstate=99820006 Stopped at db_enter+0x8: nop TIDPIDUID PRFLAGS PFLAGS CPU COMMAND 1 86809 00x12 01 ld 22929608 490x100012 08 switchd *136438 67954 0 0x14000 0x2003K softnet trap(404f9b994f0, 34, 15864ec, 99820006, 14, 0) at trap+0x2e0 Lslowtrap_reenter(4000d9c9b88, 4c, 1, 0, 4000d9c9b70, 0) at Lslowtrap_reenter+0xf8 swofp_action_output_controller(4000d135000, 4000d13d100, 4000c2db5e0, , 0, 3b9ac800) at swofp_action_output_controller+0x1f4 swofp_action_output(4000d135000, 4000d425400, 4000c2db5e0, 4000cd300a8, 4000c2db5e0, 6) at swofp_action_output+0x228 swofp_execute_action(4000d135000, 4000d425400, 4000c2db5e0, 4000cd300a8, 0, 1c289c0) at swofp_execute_action+0x34 swofp_apply_actions(4000d135000, 4000d425400, 4000c2db5e0, 4000cd300a0, 404f9b99ae8, 40079ac8000) at swofp_apply_actions+0x78 swofp_forward_ofs(4000c2db5e0, 4000d0a0d40, 4000d425400, 0, 404f9b99ae8, 40079ac8000) at swofp_forward_ofs+0xd8 switch_process(4000d128000, 4000d425400, 0, 2, 4000d128590, 16c8710) at switch_process+0x118 switchintr(1cb5560, 3c4, 20, 0, 0, 6) at switchintr+0x94 if_netisr(1c00, 404f9b99de0, 1c2ad38, 800, 4000, 2000) at if_netisr+0x1f0 taskq_thread(4000cd3c040, 4000cd04000, 17de528, 165f968, 0, 3b9ac800) at taskq_thread+0x6c proc_trampoline(0, 0, 0, 0, 0, 0) at proc_trampoline+0x14 o...@eigenstate.org and I put together the following diff. I haven't been able to check with the original reporter, and I don't have a machine to test it on, so comments and/or tests would be appreciated! Thanks, Ayaka Index: switchofp.c === RCS file: /cvs/src/sys/net/switchofp.c,v retrieving revision 1.70 diff -u -p -u -r1.70 switchofp.c --- switchofp.c 19 Feb 2018 08:59:52 - 1.70 +++ switchofp.c 19 Jun 2018 04:14:04 - @@ -2455,12 +2455,12 @@ swofp_ox_match_put_uint32(struct ofp_mat int off = ntohs(om->om_length); struct ofp_ox_match *oxm; + val = htonl(val); oxm = (struct ofp_ox_match *)((caddr_t)om + off); oxm->oxm_class = htons(OFP_OXM_C_OPENFLOW_BASIC); OFP_OXM_SET_FIELD(oxm, type); oxm->oxm_length = sizeof(uint32_t); - *(uint32_t *)oxm->oxm_value = htonl(val); - + memcpy(oxm->oxm_value, &val, sizeof(val)); om->om_length = htons(ntohs(om->om_length) + sizeof(*oxm) + sizeof(uint32_t)); @@ -2473,12 +2473,12 @@ swofp_ox_match_put_uint64(struct ofp_mat struct ofp_ox_match *oxm; int off = ntohs(om->om_length); + val = htobe64(val); oxm = (struct ofp_ox_match *)((caddr_t)om + off); oxm->oxm_class = htons(OFP_OXM_C_OPENFLOW_BASIC); OFP_OXM_SET_FIELD(oxm, type); oxm->oxm_length = sizeof(uint64_t); - *(uint64_t *)oxm->oxm_value = htobe64(val); - + memcpy(oxm->oxm_value, &val, sizeof(val)); om->om_length = htons(ntohs(om->om_length) + sizeof(*oxm) + sizeof(uint64_t));
radeondrm(4) fix
If memory is supposed to be cached, we should make it so. Matches what the Linux version of this code does. ok? Index: dev/pci/drm/ttm/ttm_bo_util.c === RCS file: /cvs/src/sys/dev/pci/drm/ttm/ttm_bo_util.c,v retrieving revision 1.19 diff -u -p -r1.19 ttm_bo_util.c --- dev/pci/drm/ttm/ttm_bo_util.c 25 Apr 2018 01:27:46 - 1.19 +++ dev/pci/drm/ttm/ttm_bo_util.c 19 Aug 2018 17:32:13 - @@ -514,6 +514,9 @@ EXPORT_SYMBOL(ttm_io_prot); pgprot_t ttm_io_prot(uint32_t caching_flags, pgprot_t tmp) { + if (caching_flags & TTM_PL_FLAG_CACHED) + return 0; + #ifdef PMAP_WC if (caching_flags & TTM_PL_FLAG_WC) return PMAP_WC;
Re: inteldrm(4) regression from 6.1 to 6.2: wrong console resolution
> Date: Sun, 19 Aug 2018 13:08:15 -0400 > From: Philippe Meunier > > Philippe Meunier wrote: > >Yes, the delay is okay. The problem is that when "cold" is 1, the vblank > >counter never changes during a call to drm_wait_one_vblank, so the > >condition inside __wait_event_intr_timeout is never true and there is a > >timeout. In fact the vblank counter does not even change across multiple > >calls to drm_wait_one_vblank, unless there is in-between a call to > >vblank_disable_and_save followed by a call to drm_vblank_enable. > > I looked at that vblank stuff more in details: > > - If you search in the (very long) debugging output below, you'll see that > the first call to drm_handle_vblank, which is the callback into the DRM > code when there's a vblank interrupt, appears long after the first call > to intel_tv_detect_type, which is the function call that triggers the > wrong console resolution problem on my computer. > So vblank interrupts, which change vblank->count (which is what the > condition inside __wait_event_intr_timeout depends on), start happening > too late to help solve the console resolution problem. > > - There's also a hardware vblank counter which is used inside > drm_update_vblank_count to change vblank->count too, to simulate the > ticking of vblank interrupts in the early stage of the DRM code before > the interrupts actually start being handled, but drm_update_vblank_count > is only called from the vblank interrupt handler drm_handle_vblank, so > too late for our purpose (see the previous paragraph) or from > vblank_disable_and_save / drm_vblank_enable, which is at the wrong time > (see the quote from my previous email at the top). > > Conclusion: the condition inside __wait_event_intr_timeout, which depends > on vblank->count changing, can never be true when intel_tv_detect_type is > called for the first time. > > I think it would be possible to change the condition "last != > drm_vblank_count(dev, pipe)" given by drm_wait_one_vblank as argument to > __wait_event_intr_timeout (through wait_event_timeout) to use the hardware > vblank counter instead of using the software vblank->count, but that would > require again some changes to drm_irq.c and it would probably be a bit messy > (see how cur_vblank and diff are computed inside drm_update_vblank_count). > > So I think using delay(tick) to fake a single vblank interval in > __wait_event_intr_timeout when "cold" is true is the simplest way to go. > > The DRM code also defines vblank->framedur_ns, which is the duration of a > frame in nanoseconds, so using delay(vblank->framedur_ns / 1000) instead of > delay(tick) would work too. In practice vblank->framedur_ns is 13 to 17 > milliseconds (see the output below) while one tick is 10 milliseconds, so I > think a tick is close enough and simpler. > > Anyway, at this point I feel I've kind of beaten this horse to death, so > it's up to you. Thoughts? Yeah. On OpenBSD we enable interrupts late. And the vblank counter updates happen from the interrupt handler. I have to think through the consequences of simply doing a delay without checking the condition here though.
Re: CID #183499: don't leak db in RSA_padding_check_PKCS1_OAEP()
On Sunday 19 August 2018 08:44:24 Theo Buehler wrote: > Coverity complains about the case where EVP_Digest() fails, but there > are a couple more. One thing worth mentioning... previously it would return -1 without setting an error, whereas now it will always set RSA_R_OAEP_DECODING_ERROR (even if the underlying failure was something unrelated like a malloc failure). I'm not overly concerned by this, but we could (if we wanted) keep the previous behaviour by adding an 'err' label that jumps to the 'free(db);' line and use that for non-decoding failures. Either way, ok jsing@ > Index: rsa/rsa_oaep.c > === > RCS file: /var/cvs/src/lib/libcrypto/rsa/rsa_oaep.c,v > retrieving revision 1.27 > diff -u -p -r1.27 rsa_oaep.c > --- rsa/rsa_oaep.c5 Aug 2018 13:30:04 - 1.27 > +++ rsa/rsa_oaep.c19 Aug 2018 06:38:52 - > @@ -126,8 +126,7 @@ RSA_padding_check_PKCS1_OAEP(unsigned ch > } > > dblen = num - SHA_DIGEST_LENGTH; > - db = malloc(dblen + num); > - if (db == NULL) { > + if ((db = malloc(dblen + num)) == NULL) { > RSAerror(ERR_R_MALLOC_FAILURE); > return -1; > } > @@ -143,17 +142,17 @@ RSA_padding_check_PKCS1_OAEP(unsigned ch > maskeddb = padded_from + SHA_DIGEST_LENGTH; > > if (MGF1(seed, SHA_DIGEST_LENGTH, maskeddb, dblen)) > - return -1; > + goto decoding_err; > for (i = 0; i < SHA_DIGEST_LENGTH; i++) > seed[i] ^= padded_from[i]; > > if (MGF1(db, dblen, seed, SHA_DIGEST_LENGTH)) > - return -1; > + goto decoding_err; > for (i = 0; i < dblen; i++) > db[i] ^= maskeddb[i]; > > if (!EVP_Digest((void *)param, plen, phash, NULL, EVP_sha1(), NULL)) > - return -1; > + goto decoding_err; > > if (timingsafe_memcmp(db, phash, SHA_DIGEST_LENGTH) != 0 || bad) > goto decoding_err; > @@ -177,7 +176,7 @@ RSA_padding_check_PKCS1_OAEP(unsigned ch > free(db); > return mlen; > > -decoding_err: > + decoding_err: > /* >* To avoid chosen ciphertext attacks, the error message should not >* reveal which kind of decoding error happened
Re: openssl(1) s_socket.c: don't leak `name' (CID #154702)
OK @inoguchi 2018/08/19 22:44 "Theo Buehler" : > On Sun, Aug 19, 2018 at 09:53:32PM +0900, Kinichiro Inoguchi wrote: > > I feel that "error case free" should be done in do_accept() rather than > caller. > > After strdup(), there are 2 "return (0)". > > How about adding "free(*host)" before these 2 "return (0)" ? > > You're right, that makes more sense. > > > I worried that error return occurs before strdup() in do_accept(). > > That would be harmless as name = NULL. > > > On Sun, Aug 19, 2018 at 10:40:55AM +0200, Theo Buehler wrote: > > > do_accept() may strdup() the host name and store it in `name', so we > > > need to free it before exiting. Perhaps a refactor might be more > > > appropriate, but I'm not sure I want to touch this mess. > > Index: s_socket.c > === > RCS file: /var/cvs/src/usr.bin/openssl/s_socket.c,v > retrieving revision 1.9 > diff -u -p -r1.9 s_socket.c > --- s_socket.c 7 Feb 2018 05:47:55 - 1.9 > +++ s_socket.c 19 Aug 2018 13:42:59 - > @@ -276,11 +276,13 @@ do_accept(int acc_sock, int *sock, char > if (h2 == NULL) { > BIO_printf(bio_err, "gethostbyname failure\n"); > close(ret); > + free(*host); > return (0); > } > if (h2->h_addrtype != AF_INET) { > BIO_printf(bio_err, "gethostbyname addr is not > AF_INET\n"); > close(ret); > + free(*host); > return (0); > } > } >
Re: openssl(1) s_socket.c: don't leak `name' (CID #154702)
On Sun, Aug 19, 2018 at 09:53:32PM +0900, Kinichiro Inoguchi wrote: > I feel that "error case free" should be done in do_accept() rather than > caller. > After strdup(), there are 2 "return (0)". > How about adding "free(*host)" before these 2 "return (0)" ? You're right, that makes more sense. > I worried that error return occurs before strdup() in do_accept(). That would be harmless as name = NULL. > On Sun, Aug 19, 2018 at 10:40:55AM +0200, Theo Buehler wrote: > > do_accept() may strdup() the host name and store it in `name', so we > > need to free it before exiting. Perhaps a refactor might be more > > appropriate, but I'm not sure I want to touch this mess. Index: s_socket.c === RCS file: /var/cvs/src/usr.bin/openssl/s_socket.c,v retrieving revision 1.9 diff -u -p -r1.9 s_socket.c --- s_socket.c 7 Feb 2018 05:47:55 - 1.9 +++ s_socket.c 19 Aug 2018 13:42:59 - @@ -276,11 +276,13 @@ do_accept(int acc_sock, int *sock, char if (h2 == NULL) { BIO_printf(bio_err, "gethostbyname failure\n"); close(ret); + free(*host); return (0); } if (h2->h_addrtype != AF_INET) { BIO_printf(bio_err, "gethostbyname addr is not AF_INET\n"); close(ret); + free(*host); return (0); } }
Re: openssl(1) s_socket.c: don't leak `name' (CID #154702)
I feel that "error case free" should be done in do_accept() rather than caller. After strdup(), there are 2 "return (0)". How about adding "free(*host)" before these 2 "return (0)" ? I worried that error return occurs before strdup() in do_accept(). On Sun, Aug 19, 2018 at 10:40:55AM +0200, Theo Buehler wrote: > do_accept() may strdup() the host name and store it in `name', so we > need to free it before exiting. Perhaps a refactor might be more > appropriate, but I'm not sure I want to touch this mess. > > Index: s_socket.c > === > RCS file: /var/cvs/src/usr.bin/openssl/s_socket.c,v > retrieving revision 1.9 > diff -u -p -r1.9 s_socket.c > --- s_socket.c7 Feb 2018 05:47:55 - 1.9 > +++ s_socket.c19 Aug 2018 07:13:49 - > @@ -151,6 +151,7 @@ do_server(int port, int type, int *ret, > if (do_accept(accept_socket, &sock, &name) == 0) { > shutdown(accept_socket, SHUT_RD); > close(accept_socket); > + free(name); > return (0); > } > } else
Re: CID #183499: don't leak db in RSA_padding_check_PKCS1_OAEP()
It looks good to me. OK inoguchi@ On Sun, Aug 19, 2018 at 08:44:24AM +0200, Theo Buehler wrote: > Coverity complains about the case where EVP_Digest() fails, but there > are a couple more. > > Index: rsa/rsa_oaep.c > === > RCS file: /var/cvs/src/lib/libcrypto/rsa/rsa_oaep.c,v > retrieving revision 1.27 > diff -u -p -r1.27 rsa_oaep.c > --- rsa/rsa_oaep.c5 Aug 2018 13:30:04 - 1.27 > +++ rsa/rsa_oaep.c19 Aug 2018 06:38:52 - > @@ -126,8 +126,7 @@ RSA_padding_check_PKCS1_OAEP(unsigned ch > } > > dblen = num - SHA_DIGEST_LENGTH; > - db = malloc(dblen + num); > - if (db == NULL) { > + if ((db = malloc(dblen + num)) == NULL) { > RSAerror(ERR_R_MALLOC_FAILURE); > return -1; > } > @@ -143,17 +142,17 @@ RSA_padding_check_PKCS1_OAEP(unsigned ch > maskeddb = padded_from + SHA_DIGEST_LENGTH; > > if (MGF1(seed, SHA_DIGEST_LENGTH, maskeddb, dblen)) > - return -1; > + goto decoding_err; > for (i = 0; i < SHA_DIGEST_LENGTH; i++) > seed[i] ^= padded_from[i]; > > if (MGF1(db, dblen, seed, SHA_DIGEST_LENGTH)) > - return -1; > + goto decoding_err; > for (i = 0; i < dblen; i++) > db[i] ^= maskeddb[i]; > > if (!EVP_Digest((void *)param, plen, phash, NULL, EVP_sha1(), NULL)) > - return -1; > + goto decoding_err; > > if (timingsafe_memcmp(db, phash, SHA_DIGEST_LENGTH) != 0 || bad) > goto decoding_err; > @@ -177,7 +176,7 @@ RSA_padding_check_PKCS1_OAEP(unsigned ch > free(db); > return mlen; > > -decoding_err: > + decoding_err: > /* >* To avoid chosen ciphertext attacks, the error message should not >* reveal which kind of decoding error happened
Re: ac(8) can segfault in two different ways
Hi, With your suggestion now it doesn't segfault anymore, but it still outputs crazy stuff, do we really want this? Also -p still shows contents of the file as per below: 0 serial@skoll:/usr/src/usr.sbin/ac$ obj/ac -w ./ac.c total762774013172614.25 0 serial@skoll:/usr/src/usr.sbin/ac$ obj/ac -w ./ac.c -p opyright (c) 1994 Christopher G. 338687831669364.19 etain the above copyright * 258230964833358.44 user_list *Users = NULL; static 46353282047238.85 -") == 0) fp = stdin; else if 43998158041234.41 g second */ yesterday++; for 39385397001446.54 se it */ tlp = lp; lp 18139557014551.83 they came in on a pseudo-tty, t 17978822565419.97 */ head = log_out(head, &usr); 0.00 total762774013172614.25 What if we still add the check secs < 0 || secs > LLONG_MAX but instead of erroring out just set secs to 0? It's the same behaviour as we have right now if we use 'ac -w ./bogus_file user', this will call update_user with secs set to 0L and the program won't segfault as it does if 'user' is not specified. Thank you for the time spent looking at this as well! :) Index: ac.c === RCS file: /cvs/src/usr.sbin/ac/ac.c,v retrieving revision 1.25 diff -u -p -u -r1.25 ac.c --- ac.c26 Apr 2018 12:42:51 - 1.25 +++ ac.c19 Aug 2018 08:26:54 - @@ -29,6 +29,7 @@ #include #include +#include #include #include #include @@ -171,6 +172,9 @@ update_user(struct user_list *head, char { struct user_list *up; + if (secs < 0 || secs > LLONG_MAX) + secs = 0; + for (up = head; up != NULL; up = up->next) { if (strncmp(up->name, name, sizeof (up->name) - 1) == 0) { up->secs += secs; @@ -187,7 +191,8 @@ update_user(struct user_list *head, char if ((up = malloc(sizeof(struct user_list))) == NULL) err(1, "malloc"); up->next = head; - strlcpy(up->name, name, sizeof (up->name)); + strncpy(up->name, name, sizeof (up->name)); + up->name[sizeof(up->name) - 1] = '\0'; up->secs = secs; Total += secs; return up; @@ -446,7 +451,8 @@ ac(FILE *fp) */ if (*usr.ut_name) { if (strncmp(usr.ut_line, "tty", 3) != 0 || - strchr("pqrstuvwxyzPQRST", usr.ut_line[3]) != NULL || + memchr("pqrstuvwxyzPQRST", usr.ut_line[3], + sizeof("pqrstuvwxyzPQRST")) != NULL || *usr.ut_host != '\0') head = log_in(head, &usr); } else @@ -457,7 +463,8 @@ ac(FILE *fp) (void)fclose(fp); if (!(Flags & AC_W)) usr.ut_time = time(NULL); - (void)strlcpy(usr.ut_line, "~", sizeof usr.ut_line); + (void)strncpy(usr.ut_line, "~", sizeof usr.ut_line); + usr.ut_line[sizeof(usr.ut_line) - 1] = '\0'; if (Flags & AC_D) { ltm = localtime(&usr.ut_time);
openssl(1) s_socket.c: don't leak `name' (CID #154702)
do_accept() may strdup() the host name and store it in `name', so we need to free it before exiting. Perhaps a refactor might be more appropriate, but I'm not sure I want to touch this mess. Index: s_socket.c === RCS file: /var/cvs/src/usr.bin/openssl/s_socket.c,v retrieving revision 1.9 diff -u -p -r1.9 s_socket.c --- s_socket.c 7 Feb 2018 05:47:55 - 1.9 +++ s_socket.c 19 Aug 2018 07:13:49 - @@ -151,6 +151,7 @@ do_server(int port, int type, int *ret, if (do_accept(accept_socket, &sock, &name) == 0) { shutdown(accept_socket, SHUT_RD); close(accept_socket); + free(name); return (0); } } else
X509_verify_cert() don't leak sktmp (CID #118791)
At this point sktmp may be allocated by sk_dup(), so we need to free it. Remove a free guard at the end while there. Index: x509/x509_vfy.c === RCS file: /var/cvs/src/lib/libcrypto/x509/x509_vfy.c,v retrieving revision 1.70 diff -u -p -r1.70 x509_vfy.c --- x509/x509_vfy.c 8 Apr 2018 16:57:57 - 1.70 +++ x509/x509_vfy.c 19 Aug 2018 08:26:57 - @@ -496,9 +496,10 @@ X509_verify_cert(X509_STORE_CTX *ctx) ctx->current_cert = x; } else { if (!sk_X509_push(ctx->chain, chain_ss)) { - X509_free(chain_ss); X509error(ERR_R_MALLOC_FAILURE); - return 0; + ctx->error = X509_V_ERR_OUT_OF_MEM; + ok = 0; + goto end; } num++; ctx->last_untrusted = num; @@ -548,8 +549,7 @@ X509_verify_cert(X509_STORE_CTX *ctx) ok = ctx->check_policy(ctx); end: - if (sktmp != NULL) - sk_X509_free(sktmp); + sk_X509_free(sktmp); X509_free(chain_ss); /* Safety net, error returns must set ctx->error */