On Sat, 19 Nov 2022 14:41:18 +0900 (JST)
YASUOKA Masahiko <[email protected]> wrote:
> On Wed, 12 Oct 2022 07:58:20 +0900 (JST)
> YASUOKA Masahiko <[email protected]> wrote:
>> On Wed, 05 Oct 2022 13:37:35 +0900 (JST)
>> Masato Asou <[email protected]> wrote:
>>> From: "Theo de Raadt" <[email protected]>
>>> 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.
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);
