delete mbuhl from Cc:. From: YASUOKA Masahiko <yasu...@openbsd.org> Date: Sat, 19 Nov 2022 16:37:47 +0900 (JST)
> On Sat, 19 Nov 2022 14:41:18 +0900 (JST) > YASUOKA Masahiko <yasu...@openbsd.org> wrote: >> On Wed, 12 Oct 2022 07:58:20 +0900 (JST) >> YASUOKA Masahiko <yasu...@openbsd.org> wrote: >>> On Wed, 05 Oct 2022 13:37:35 +0900 (JST) >>> Masato Asou <a...@soum.co.jp> wrote: >>>> From: "Theo de Raadt" <dera...@openbsd.org> >>>> Date: Tue, 04 Oct 2022 21:58:13 -0600 >>>>> Userland may not ask the kernel to allocate such a huge object. The >>>>> kernel address space is quite small. First off, huge allocations will >>>>> fail. >>>>> >>>>> But it is worse -- the small kernel address space is shared for many >>>>> purposes, so large allocations will harm the other subsystems. >>>>> >>>>> As written, this diff is too dangerous. Arbitrary allocation inside >>>>> the kernel is not reasonable. object sizes requested by userland must >>>>> be small, or the operations must be cut up, which does create impact >>>>> on atomicity or other things. >>>> >>>> As you pointed out, it is not a good idea to allocate large spaces >>>> in kernel. >>>> >>>> Would it be better to keep the current fixed length? >>> >>> Currently the value on VMware may be truncated silently. It's simply >>> broken. I think we should fix it by having a way to know if the value >>> is reached the limit. >>> >>> Also I think we should be able to pass larger size of data. Since at >>> least on VMware, people is useing for parameters when deployment >>> through OVF tamplate. Sometimes the parameter includes large data >>> like X.509 certificate. >>> >>> https://docs.vmware.com/en/VMware-vSphere/7.0/com.vmware.vsphere.vm_admin.doc/GUID-D0F9E3B9-B77B-4DEF-A982-49B9F6358FF3.html >>> >>> What do you think? >>> >>>> Prepare a variable like kern.maxpvbus and default it to >>>> 4096. Futhermore, how about free() after copyout() to user space? >>> >>> I suppose we can use the space prepared by the userland directly. >> >> I admit there is no way to use the user space directly in case of the >> vmware. >> >> But current vmt(4) uses the buffer in vmt_softc for all RPC commands. >> The buffer seems to have beeen created for RPC done by the tclo >> process. The tclo process executes RPC periodically, so having a long >> term buffer does make sense. >> >> On the otherhand, pvbus(4) prepares a buffer for >> PVBUSIOC_KV{READ|WRITE} and pass it to the driver handler. So vmt(4) >> can use the buffer. >> >> The diff is to change vmt(4) to use the buffer given by pvbuf(4) for >> the rpc output directly. Also make it return EMSGSIZE when the buffer >> is not enough instead of truncating silently. >> >> The diff is first step. We need to more hack for pvbus(4) and >> vmt(4). For example, the buffer size pvbuf(4) allocates is not enough >> to store the size user requested, since vmt(4) neeeds extra 2 bytes >> for the RPC output. >> >> + bcopy(value + 2, value, valuelen - 2); >> >> >> But I'd like to commit this first. >> >> ok? > > sorry, using existing vm_rpc_send_rpci_tx_buf() or > vm_rpci_response_successful() wasn't good idea and had a bug. > > Let me update the diff. ok asou@ > Index: sys/dev/pv/vmt.c > =================================================================== > RCS file: /var/cvs/openbsd/src/sys/dev/pv/vmt.c,v > retrieving revision 1.26 > diff -u -p -r1.26 vmt.c > --- sys/dev/pv/vmt.c 8 Sep 2022 10:22:06 -0000 1.26 > +++ sys/dev/pv/vmt.c 19 Nov 2022 07:32:47 -0000 > @@ -491,9 +491,12 @@ int > vmt_kvop(void *arg, int op, char *key, char *value, size_t valuelen) > { > struct vmt_softc *sc = arg; > - char *buf = NULL, *ptr; > + struct vm_rpc rpci; > + char *buf = NULL; > size_t bufsz; > int error = 0; > + uint32_t rlen; > + uint16_t ack; > > bufsz = VMT_RPC_BUFLEN; > buf = malloc(bufsz, M_TEMP, M_WAITOK | M_ZERO); > @@ -520,25 +523,52 @@ vmt_kvop(void *arg, int op, char *key, c > goto done; > } > > - if (vm_rpc_send_rpci_tx(sc, "%s", buf) != 0) { > - DPRINTF("%s: error sending command: %s\n", DEVNAME(sc), buf); > + if (vm_rpc_open(&rpci, VM_RPC_OPEN_RPCI) != 0) { > + DPRINTF("%s: rpci channel open failed\n", DEVNAME(sc)); > sc->sc_rpc_error = 1; > error = EIO; > goto done; > } > > - if (vm_rpci_response_successful(sc) == 0) { > - DPRINTF("%s: host rejected command: %s\n", DEVNAME(sc), buf); > - error = EINVAL; > + if (vm_rpc_send(&rpci, buf, bufsz) != 0) { > + DPRINTF("%s: unable to send rpci command\n", DEVNAME(sc)); > + sc->sc_rpc_error = 1; > + error = EIO; > goto done; > } > > - /* skip response that was tested in vm_rpci_response_successful() */ > - ptr = sc->sc_rpc_buf + 2; > + if (vm_rpc_get_length(&rpci, &rlen, &ack) != 0) { > + DPRINTF("%s: failed to get length of rpci response data\n", > + DEVNAME(sc)); > + sc->sc_rpc_error = 1; > + error = EIO; > + goto done; > + } > > - /* might truncate, copy anyway but return error */ > - if (strlcpy(value, ptr, valuelen) >= valuelen) > - error = ENOMEM; > + if (rlen > 0) { > + if (rlen + 1 > valuelen) { > + error = EMSGSIZE; > + goto done; > + } > + > + if (vm_rpc_get_data(&rpci, value, rlen, ack) != 0) { > + DPRINTF("%s: failed to get rpci response data\n", > + DEVNAME(sc)); > + sc->sc_rpc_error = 1; > + error = EIO; > + goto done; > + } > + /* test if response success */ > + if (rlen < 2 || value[0] != '1' || value[1] != ' ') { > + DPRINTF("%s: host rejected command: %s\n", DEVNAME(sc), > + buf); > + error = EINVAL; > + goto done; > + } > + /* skip response that was tested */ > + bcopy(value + 2, value, valuelen - 2); > + value[rlen - 2] = '\0'; > + } > > done: > free(buf, M_TEMP, bufsz);
