From nobody Mon Nov 03 15:10:43 2025 X-Original-To: dev-commits-src-all@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 4d0Zln17S1z652cY; Mon, 03 Nov 2025 15:10:45 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R13" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4d0Zlm6CB9z3X2B; Mon, 03 Nov 2025 15:10:44 +0000 (UTC) (envelope-from jhb@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1762182644; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=YmwjRA3aBcaAaKICny14qz4yavgDf3mlWRcQ/Pkemdo=; b=Q8YX3Zo8kDFmb+lKnJcCu5YCm8mjKIgH0vFyyPDqtcgAmaVX06IzaKB68C09tc/qFHDzfZ ts95UqptiOhKKut1Sd7XV4zl9cUHxoIpcGEs9J1BgZQMmAcpr9rIk/Kedoan3lR8ffozzo ZX/zMj7juAFNeK9ryf3JLwKX2WHtZ3un3C17eCAqXn5UPf8VUMV7lV6n9wiYCJpm94p4Jg lkcqtDfdntXhX7QBapemKrlk6v4e/sjIQKL8DGE2A/aq1XTa/zCvjIv5md7oTxS0Kxktcn GsX+zY19UARKVBsVgsMYS66N9hqE3FGZTRm5c6X2MFM4qk1bH406CCG2js1cVg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1762182644; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=YmwjRA3aBcaAaKICny14qz4yavgDf3mlWRcQ/Pkemdo=; b=OETX6zC2PkWWpZHbp2D/h4IFprdWufV2zuladudvAcjHZa6JAYLnmiGmO29YIQ4rTQK0K2 ML0NRZjdBFPsVWFUlDRSE3T8T9js1DQEXBBEijf1dDOckBotk3Dvgt2dBPe7ZHyw02KBmv db+9xaqW7OJ7mEWh40UADILgndan4KboGbWrTaCXmRVU8BBCO2rUnfAFQqYqiL26SjFwW1 b3XO6NG4KMZUIng0AcQEdtDjRhFXTdognU//yU7ZxgLE+3fFk9bxXvBGOB08RTUGsigXnb qfTYDn6R37EGHQx8Za6Bb0lRUsXpjFYsgTBy/NNNuNxgvh4fvwWNR86MSa53kw== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1762182644; a=rsa-sha256; cv=none; b=fdSZ6A0HbPI7tIgidccn6lK9nWcT+oDnmKUP/nWgKXS0dMq7HWXJowDuwNlBil4K9olB0t pnEpQbjRqTcs13HNTb3YBkhCL4wZ/gpJLXw+lFrD1poK4yqIo4YGE+/zINxCKPx6BX6pEp QADXR+WrxpTumJINHizbDJSrHcodoXSOH8GOG3qHExGdaFqWNMDqAXaJdleZlyI6X20wrw k/9U6wtZtL56UQ/rQ4AbzLcdrprC42N4kcJF4nJ6wdrZKOa2WmnJ07QT+57WwkIZj+91FG 0/FvzAuD1oLO7hMV++P8DfMWoZTz5+873R0toC8+jbrjOJ2ANS7QtNrSogJ0aw== ARC-Authentication-Results: i=1; mx1.freebsd.org; none Received: from [IPV6:2601:5c0:4202:5670:ac49:a53f:963:aaba] (unknown [IPv6:2601:5c0:4202:5670:ac49:a53f:963:aaba]) (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 did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 4d0Zlm40sJz16sY; Mon, 03 Nov 2025 15:10:44 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: Date: Mon, 3 Nov 2025 10:10:43 -0500 List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: git: 9404c479946c - main - pci_user: Report NUMA domain Content-Language: en-US To: Jake Freeland , Warner Losh , src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org References: <202509040245.5842jXvV085511@gitrepo.freebsd.org> From: John Baldwin In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 11/2/25 21:01, Jake Freeland wrote: >> For future reference: you didn't need to bump to a new number for the ioctl >> since the size of the argument is encoded in the ioctl as well. This >> feature of ioctls also means that we generally don't add padding (as you did >> in the followup commit) as you will naturally get a new ioctl cmd value >> anytime you add more fields in the future. This avoids the one point Warner >> raised in the review about how do you define the semantics of the padding >> as that approach defers reserving space in the structure until the semantics >> of that space is known. > > Hey John, > > I just took another look at this and understand your point about not > needing to bump the ioctl number, but am struggling to see why that > would remove the need for padding. > > The point of the padding was to minimize the in-kernel churn needed > when adding new members to struct pci_conf. Without it, adding a new > member means we need to account for the old ioctl (with the old length) > in every switch statement in pci_user.c and there are a lot. > > The only solution I can think of to get around this is to switch on the > IOCGROUP of every command and copy out IOCPARM_LEN number of bytes. > Then modifying pci_conf wouldn't require an additional case for every > switch statement. There may be a reason, that I'm not aware of, for why > we don't already do this. Perhaps this is what you were thinking? Hummmm. So after reading more, you did indeed need to add a new number. First, a general thought about adding padding in structures used as ioctl arguments. With padding you have to be sure that all calling code follows good sanitation of the padding (e.g. all client code know needs to know that it has to memset/bzero the entire structure before assigning members to ensure the padding is zeroed) as otherwise you can't depend on using zeroes in the padding to determine which API the calling application believes it is using. Most of the userspace ioctl code I've seen in general doesn't use this, it is often: struct foo; foo.field_one = x; foo.field_two = y; ioctl(fd, SIOCSFOO, &foo); So that is the origin of the general rule that we don't tend to use padding for ioctl structures. However, this case is a bit more unusual. Here, `struct pci_conf_io` is the structure we pass to the ioctl, and it's size did not in fact change. Rather, this structure contains a pointer to an array of `struct pci_conf` objects. So, you had to bump the number to deal with the alternate ABI of `struct pci_conf`. Hmm, but you didn't actually have to. `pci_conf_io` already has these fields: u_int32_t match_buf_len; /* match buffer length */ u_int32_t num_matches; /* number of matches returned */ struct pci_conf *matches; /* match buffer */ So you can infer the caller's size of `struct pci_conf` by doing match_buf_len / num_matches. We really should just be passing that size as an argument to `pci_conf_for_copyout` so that only in that one place do you need to deal with the different layouts of `struct pci_conf`. It does mean you would need to switch on the structure size inside of the current switches for the ioctl command, but you wouldn't need the ioctl command, instead your prior patch would look something like: static void pci_conf_for_copyout(const struct pci_conf *pcp, union pci_conf_union *pcup, u_long cmd, size_t conf_size) { ... switch (cmd) { case PCIOCGETCONF: switch (conf_size) { #ifdef COMPAT_FREEBSD14 case sizeof(pcup->pc14): ... #endif case sizeof(pcpu->pc): pcup->pc = *pcp; return; default: __assert_unreachable(); } I see that we currently do some lazy-ish handling for the size of pci_conf now though: /* * Determine how much room we have for pci_conf structures. * Round the user's buffer size down to the nearest * multiple of sizeof(struct pci_conf) in case the user * didn't specify a multiple of that size. */ confsz = pci_conf_size(cmd); iolen = min(cio->match_buf_len - (cio->match_buf_len % confsz), pci_numdevs * confsz); I think in practice the user always provides an exact array and we should depend on that, so this should be rewritten more like: if (cio->match_buf_len % cio-num_matches != 0) { error = EINVAL; goto confexit; } confsz = cio->match_buf_len / cio->num_matches; error = pci_validate_conf_size(cmd, confsz); if (error != 0) goto confexit; ionum = min(cio->num_matches, pci_numdevs); And pci_validate_conf_size would use a similar structure of a switch on cmd with nested commands on the size to check for supported ABIs of pci_conf. -- John Baldwin