From nobody Sun Nov 6 20:10:34 2022 X-Original-To: freebsd-virtualization@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4N558K2Ldhz4h14F for ; Sun, 6 Nov 2022 20:10:41 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-qv1-xf34.google.com (mail-qv1-xf34.google.com [IPv6:2607:f8b0:4864:20::f34]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4N558J3sW3z449k; Sun, 6 Nov 2022 20:10:40 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: by mail-qv1-xf34.google.com with SMTP id j6so6876789qvn.12; Sun, 06 Nov 2022 12:10:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:sender :from:to:cc:subject:date:message-id:reply-to; bh=bLIQc1g47SnH0Hc1HCd5UhVvCOi87JxjBAm9Ua+J/CI=; b=lRGtNbMh8Xhax2wA6nkVmWgyc+b+fTP8+PPowA7KNsSfXeZ14lBgLl+sz2A3MLeh7Z 14Yb0K3AFrrAt8+/r1VkecnWipex4d/FAObocHKDojjMu3MpdBTtScrcHQixCCctnJR3 By9z7zqOubUE1t4mRIfcok5q2xrwumM9Rb9w8pWsh0qK9N9T0AsV8sjiwZu2V9XpmOKL s6RyrqL8yhDviNvcZAJ5nBzcrmlSprLcYMH9wEruepHgxHo0py+zv+VyJMaccjDB0Xlb NGL0bC8+2SszpUY7uoQyLDt6Kc3xfnWbhlCTEstl32oza0EiZF9ZxRPRKj/vr+1z7ktD YI7Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:sender :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=bLIQc1g47SnH0Hc1HCd5UhVvCOi87JxjBAm9Ua+J/CI=; b=GWGgOQdpSGyFaNfWvmyKWLipoyEMwp8fU+lnE4etIeNmMcveX1ic0c/+CoKWfvgFq4 5U1gozH1Kz4ZIzlCfKSjv1ZK4QcHOI1m2WkkBb9dLGMmnhfwUAhCWsrM07yWnlu723RO AF2nSeJncTqy9XRr/VdwY6bQSo0tiMOtP3uJW4IaMa9JLxvSqmIpXgAwJo7+fbDQYUgl L8fwWuSwK4QGhWM5p8vb/j6Lzirjs4WbZv3wMkuoT5bh4XaXtGBxnIrc5f1o10rlolbO azO60WnYoAb+NpcfVu89I0XqpbL778OihBrUKgHHb9FgNptYvF3kXRN8db8b0dtDgeve uGtQ== X-Gm-Message-State: ACrzQf3f1SmyJOmaUJ+dGDUnefD3nTsh+VrJvYFLuppeqwI3ELXpgucf tO6nxBbKt8tmossVDLySWbAnnCOi7fY= X-Google-Smtp-Source: AMsMyM6g9pFObHPCvgo8BgJW67WD7cL8vjulBQcxcDlROnFpvCqDp1k1Ksbu1wzZ+X/gB5foXvpXyQ== X-Received: by 2002:a05:6214:f63:b0:4b8:c0bc:c43e with SMTP id iy3-20020a0562140f6300b004b8c0bcc43emr41731373qvb.119.1667765437742; Sun, 06 Nov 2022 12:10:37 -0800 (PST) Received: from nuc (192-0-220-237.cpe.teksavvy.com. [192.0.220.237]) by smtp.gmail.com with ESMTPSA id bm26-20020a05620a199a00b006ed138e89f2sm4970509qkb.123.2022.11.06.12.10.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 06 Nov 2022 12:10:36 -0800 (PST) Date: Sun, 6 Nov 2022 15:10:34 -0500 From: Mark Johnston To: Corvin =?iso-8859-1?Q?K=F6hne?= Cc: freebsd-virtualization@freebsd.org Subject: Re: bhyve nvlist constness bug Message-ID: References: <924528458d3b4265a9bf36050c949c3f@beckhoff.com> <80881e5b-74d8-3831-85ac-cb8b8d3d0413@FreeBSD.org> List-Id: Discussion List-Archive: https://lists.freebsd.org/archives/freebsd-virtualization List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-virtualization@freebsd.org X-BeenThere: freebsd-virtualization@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <80881e5b-74d8-3831-85ac-cb8b8d3d0413@FreeBSD.org> X-Rspamd-Queue-Id: 4N558J3sW3z449k X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20210112 header.b=lRGtNbMh; dmarc=none; spf=pass (mx1.freebsd.org: domain of markjdb@gmail.com designates 2607:f8b0:4864:20::f34 as permitted sender) smtp.mailfrom=markjdb@gmail.com X-Spamd-Result: default: False [-2.69 / 15.00]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_MEDIUM(-1.00)[-0.999]; NEURAL_HAM_SHORT(-0.99)[-0.992]; MID_RHS_NOT_FQDN(0.50)[]; FORGED_SENDER(0.30)[markj@freebsd.org,markjdb@gmail.com]; R_DKIM_ALLOW(-0.20)[gmail.com:s=20210112]; R_SPF_ALLOW(-0.20)[+ip6:2607:f8b0:4000::/36:c]; MIME_GOOD(-0.10)[text/plain]; RCVD_IN_DNSWL_NONE(0.00)[2607:f8b0:4864:20::f34:from]; MLMMJ_DEST(0.00)[freebsd-virtualization@freebsd.org]; ARC_NA(0.00)[]; FREEMAIL_ENVFROM(0.00)[gmail.com]; MIME_TRACE(0.00)[0:+]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; RCPT_COUNT_TWO(0.00)[2]; RCVD_COUNT_THREE(0.00)[3]; FROM_NEQ_ENVFROM(0.00)[markj@freebsd.org,markjdb@gmail.com]; FROM_HAS_DN(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; DKIM_TRACE(0.00)[gmail.com:+]; TO_DN_SOME(0.00)[]; RCVD_TLS_LAST(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; DMARC_NA(0.00)[freebsd.org]; DWL_DNSWL_NONE(0.00)[gmail.com:dkim] X-ThisMailContainsUnwantedMimeParts: N On Fri, Nov 04, 2022 at 11:22:55AM +0100, Corvin Köhne wrote: > > On 10/23/22 11:18 AM, Mark Johnston wrote: > >> I'm going through compiler warnings in the bhyve code with the aim of > >> bumping WARNS, since the current setting hides bugs. There's one in the > >> config code that looks a bit tricky to resolve and I was hoping for some > >> guidance on the right way to do that. > >> > >> The basic problem is _lookup_config_node() may return an nvlist via > >> nvlist_get_nvlist(), but nvlist_get_nvlist() returns a const nvlist_t > >> *, so _lookup_config_node() is discarding the const qualifier. And > >> indeed, some callers will modify the returned node. The use of > >> nvlist_move_nvlist() in _lookup_config_node() has a similar problem: > >> nvlist_move_* "consumes" the passed value and is not supposed to be > >> mutated after. > >> > >> I'm pretty sure this is actually a non-issue with our nvlist > >> implementation; when adding an nvlist value to an nvlist, it doesn't > >> store anything except for the pointer itself, so you can mutate it > >> safely. Note, this is not true for all value types, specifically > >> arrays. However, there are multiple nvlist implementations out there, > >> and ours might change such that bhyve's config code becomes incorrect. > >> > >> The bug seems annoying to fix because consumers can do this: > >> > >> nvlist_t *nvl = find_config_node(path); > >> set_config_value_node(nvl, "foo", "bar"); > >> > >> So, if we naively changed find_config_node() to return a copy of the > >> nvlist at "path", set_config_value_node() would have to somehow figure > >> out where in the config tree to insert the updated nvlist, but it > >> doesn't have the path anymore. > >> > >> To fix it properly could change find_config_node() to return some opaque > >> type which contains the source path of the node so that we can DTRT when > >> mutating the node. IMO it's better if the config node type is opaque to > >> consumers. Or, make each nvlist node store its source path by storing > >> it in the nvlist itself. e.g., the nvlist node describing the device > >> model at "pci.0.1.2" would have a "__path" key with value "pci.0.1.2", > >> so that set_config_value_node() can figure out where in the tree to > >> replace an existing copy of "pci.0.1.2". Or is there a simpler way to > >> fix this that I missed? > >> > >> Any thoughts? I'm happy to implement the suggestions above but didn't > >> want to do that work without some buy-in. > > The config stuff uses nvlist on the premise that it is a useful way to > > store the data. However, it is true that nvlists seem to not be as > > useful for storing mutable data. Rather, they are useful as a way to > > pass structured data across domain boundaries (e.g. serialized over > > a socket, or between kernel and userland). If nvlist doesn't work well > > I'd rather just change the data structures we use in bhyve rather than > > add hidden nodes with path names, etc. To that end, if nvlist is too > > limiting then we can do what my first intution was which was to turn > > bhyve into a C++ program and use something like std::map<> to represent > > the tree instead. > > > > -- > > John Baldwin > > > I do agree with John. It looks like nvlist isn't made for mutable data. > So, when using nvlist we have to make sure that we don't modify the data > which is already stored in it or we have to use nvlist_take, modify the > data and write it back with nvlist_move/nvlist_add. This requires that > we patch the consumers to follow these rules. The consumers don't need to be patched with my "__path" proposal above since they use config.c accessors to get and set config values. Though, I admit I didn't implement it though so might be missing something. > I don't know how much work would be required to turn bhyve into a C++ > program. If it's doable, I vote for this approach. Ok, I'll try that. The first step is probably to make config nodes an opaque type; today all consumers have to pass nvlist_t *s around even though they use accessors in config.c for everything. Since my immediate goal was to raise WARNS for bhyve, and I'm quite close to that, I posted a patch which simply disarms the warning for the time being: https://reviews.freebsd.org/D37293