Re: Properly check if ACPI devices are enabled
On Mon, Jan 24, 2022 at 12:15:58PM -0800, Philip Guenther wrote: > On Mon, Jan 24, 2022 at 11:41 AM Mark Kettenis > wrote: > > > > Date: Mon, 24 Jan 2022 20:19:46 +0100 > > > From: Anton Lindqvist > > > > > > On Mon, Jan 24, 2022 at 05:31:49PM +0100, Mark Kettenis wrote: > > > > Currently we attach ACPI devices that are present in a machine. > > > > However, in some cases ACPI devices can be present, but not enabled. > > > > Attaching a device driver to devices that are not enabled is not a > > > > good idea since reading and writing from/to its registers will fail > > > > and the driver will malfunction in interesting ways. Such as a com(4) > > > > serial port that is misdetected and hangs the kernel when it is > > > > actually opened. > > > > > > > > The diff below makes sure we only enable devices that are actually > > > > enabled. This may cause some devices to disappear in OpenBSD. > > > > However those devices should have been unusable anyway, so that isn't > > > > an issue. > > > > > > > > ok? > > > > > > According to the ACPI specification[1]: > > > > > > > A device can only decode its hardware resources if both bits 0 and 1 > > are > > > > set. If the device is not present (bit [0] cleared) or not enabled (bit > > > > [1] cleared), then the device must not decode its resources. > > > > Just before that it says: > > > > If bit [0] is cleared, then bit 1 must also be cleared (in other > > words, a device that is not present cannot be enabled). > > > > > Should we therefore check for presence of both STA_PRESENT and > > > STA_ENABLED? > > > > So according to the ACPI specification we don't need to do that. > > Should we do it just to be safe? > > > > Unless you're taking money bets about this being the one thing in the ACPI > spec that some vendor won't screw up, doing both seems "can't be worse; can > be better". > > Philip A bit late to the party here, but we should all remember the HP bios with the FACS table version of "1" and not 1. (0x31 and not 0x01). So yes, someone will indeed screw it up if it's possible to screw it up.
Re: touch(1): don't leak descriptor if futimens(2) fails
On Thu, Jan 27, 2022 at 4:03 PM Scott Cheloha wrote: > If futimens(2) fails here then close(2) is not called and we leak the > descriptor. > Good catch. I think futimens(2) and close(2) failures are exotic enough to warrant > printing the system call name. > I don't understand this. Can you give an example of an error message that touch currently might emit where knowing that the failed call was futimens() or close() would affect the analysis of how to deal with it? I mean, it looks like the only errors that futimens() could really return are EROFS, EIO, and EPERM (implies a race by different users to create the file), and close() could only return EIO. For any of those errors, you're going to handle them the same whether they're from open, futimens, or close, no? Philip Guenther
touch(1): don't leak descriptor if futimens(2) fails
If futimens(2) fails here then close(2) is not called and we leak the
descriptor.
I think futimens(2) and close(2) failures are exotic enough to warrant
printing the system call name.
ok?
Index: touch.c
===
RCS file: /cvs/src/usr.bin/touch/touch.c,v
retrieving revision 1.26
diff -u -p -r1.26 touch.c
--- touch.c 10 Mar 2019 15:11:52 - 1.26
+++ touch.c 27 Jan 2022 23:55:50 -
@@ -137,9 +137,18 @@ main(int argc, char *argv[])
/* Create the file. */
fd = open(*argv, O_WRONLY | O_CREAT, DEFFILEMODE);
- if (fd == -1 || futimens(fd, ts) || close(fd)) {
+ if (fd == -1) {
rval = 1;
warn("%s", *argv);
+ continue;
+ }
+ if (futimens(fd, ts) == -1) {
+ warn("futimens %s", *argv);
+ rval = 1;
+ }
+ if (close(fd) == -1) {
+ warn("close %s", *argv);
+ rval = 1;
}
}
return rval;
mention rpki-client(8) on openbgpd/index.html
Hi, I think rpki-client is now an important piece of the DFZ, so it makes sense to mention it. Comments? OK? BTW there's no mention of eigrpd, should we add it? Or there's no need to list them all? Cheers, Daniel Index: index.html === RCS file: /cvs/www/openbgpd/index.html,v retrieving revision 1.61 diff -u -p -r1.61 index.html --- index.html 5 Nov 2021 00:38:58 - 1.61 +++ index.html 27 Jan 2022 23:41:09 - @@ -64,6 +64,8 @@ OpenBGPD's companions, add support for the respective protocols. https://man.openbsd.org/ldpd;>ldpd(8) and https://man.openbsd.org/mpe;>mpe(4) add MPLS support. +https://man.openbsd.org/rpki-client;>rpki-client(8) facilitates +validation of the Route Origin of a BGP announcement. OpenBGPD is primarily developed by Henning Brauer, Peter Hessler, and
kubsan nd6
Hi,
kubsan: netinet6/nd6.c:948:42: type mismatch: member access within null pointer
of type 'struct in6_ifaddr'
kubsan: netinet6/nd6_nbr.c:640:43: type mismatch: member access within null
pointer of type 'struct in6_ifaddr'
This codes works as ifaddr ia_ifa is the first field of in6_ifaddr.
So the pointers are the same, and one NULL check works for both.
But in ISO C NULL has a type and this is undefined behavior. So
add a second NULL check that the compiler can optimize away. The
resulting assembler is the same.
ok?
bluhm
Index: netinet6/nd6.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6.c,v
retrieving revision 1.236
diff -u -p -r1.236 nd6.c
--- netinet6/nd6.c 7 Nov 2021 19:38:25 - 1.236
+++ netinet6/nd6.c 27 Jan 2022 22:20:06 -
@@ -792,6 +792,7 @@ nd6_rtrequest(struct ifnet *ifp, int req
struct sockaddr *gate = rt->rt_gateway;
struct llinfo_nd6 *ln = (struct llinfo_nd6 *)rt->rt_llinfo;
struct ifaddr *ifa;
+ struct in6_ifaddr *ifa6;
if (ISSET(rt->rt_flags, RTF_GATEWAY|RTF_MULTICAST|RTF_MPLS))
return;
@@ -944,8 +945,9 @@ nd6_rtrequest(struct ifnet *ifp, int req
* check if rt_key(rt) is one of my address assigned
* to the interface.
*/
- ifa = _ifpwithaddr(ifp,
- (rt_key(rt))->sin6_addr)->ia_ifa;
+ ifa6 = in6ifa_ifpwithaddr(ifp,
+ (rt_key(rt))->sin6_addr);
+ ifa = ifa6 ? >ia_ifa : NULL;
if (ifa) {
ln->ln_state = ND6_LLINFO_REACHABLE;
ln->ln_byhint = 0;
Index: netinet6/nd6_nbr.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6_nbr.c,v
retrieving revision 1.130
diff -u -p -r1.130 nd6_nbr.c
--- netinet6/nd6_nbr.c 13 Dec 2021 14:30:16 - 1.130
+++ netinet6/nd6_nbr.c 27 Jan 2022 22:20:06 -
@@ -568,6 +568,7 @@ nd6_na_input(struct mbuf *m, int off, in
char *lladdr = NULL;
int lladdrlen = 0;
struct ifaddr *ifa;
+ struct in6_ifaddr *ifa6;
struct llinfo_nd6 *ln;
struct rtentry *rt = NULL;
struct sockaddr_dl *sdl;
@@ -637,7 +638,8 @@ nd6_na_input(struct mbuf *m, int off, in
lladdrlen = ndopts.nd_opts_tgt_lladdr->nd_opt_len << 3;
}
- ifa = _ifpwithaddr(ifp, )->ia_ifa;
+ ifa6 = in6ifa_ifpwithaddr(ifp, );
+ ifa = ifa6 ? >ia_ifa : NULL;
/*
* Target address matches one of my interface address.
Index: arch/amd64/conf/GENERIC
===
RCS file: /data/mirror/openbsd/cvs/src/sys/arch/amd64/conf/GENERIC,v
retrieving revision 1.510
diff -u -p -r1.510 GENERIC
--- arch/amd64/conf/GENERIC 4 Jan 2022 05:50:43 - 1.510
+++ arch/amd64/conf/GENERIC 27 Jan 2022 22:19:55 -
@@ -381,23 +381,23 @@ wsmouse* at pms? mux 0
#mmuagp* at pchb?# amd64 mmu agp.
#agp* at mmuagp?
-inteldrm* at pci? # Intel i915, i945 DRM driver
-intagp*at inteldrm?
-agp* at intagp?
-drm0 at inteldrm? primary 1
-drm* at inteldrm?
-wsdisplay0 at inteldrm? primary 1
-wsdisplay* at inteldrm? mux -1
-radeondrm* at pci? # ATI Radeon DRM driver
-drm0 at radeondrm? primary 1
-drm* at radeondrm?
-wsdisplay0 at radeondrm? primary 1
-wsdisplay* at radeondrm? mux -1
-amdgpu*at pci?
-drm0 at amdgpu? primary 1
-drm* at amdgpu?
-wsdisplay0 at amdgpu? primary 1
-wsdisplay* at amdgpu? mux -1
+#inteldrm* at pci? # Intel i915, i945 DRM driver
+#intagp* at inteldrm?
+#agp* at intagp?
+#drm0 at inteldrm? primary 1
+#drm* at inteldrm?
+#wsdisplay0at inteldrm? primary 1
+#wsdisplay*at inteldrm? mux -1
+#radeondrm*at pci? # ATI Radeon DRM driver
+#drm0 at radeondrm? primary 1
+#drm* at radeondrm?
+#wsdisplay0at radeondrm? primary 1
+#wsdisplay*at radeondrm? mux -1
+#amdgpu* at pci?
+#drm0 at amdgpu? primary 1
+#drm* at amdgpu?
+#wsdisplay0at amdgpu? primary 1
+#wsdisplay*at amdgpu? mux -1
pcppi0 at isa?
Index: arch/amd64/conf/GENERIC.MP
===
RCS file: /data/mirror/openbsd/cvs/src/sys/arch/amd64/conf/GENERIC.MP,v
retrieving revision 1.16
diff -u -p -r1.16 GENERIC.MP
--- arch/amd64/conf/GENERIC.MP 9 Feb 2021 14:06:19 - 1.16
+++ arch/amd64/conf/GENERIC.MP 27 Jan 2022 22:19:55 -
@@ -5,5 +5,6 @@ include "arch/amd64/conf/GENERIC"
option MULTIPROCESSOR
#optionMP_LOCKDEBUG
#optionWITNESS
+option KUBSAN
cpu* at mainbus?
amd64: simplify TSC sync testing
Hi, sthen@ complained recently about a multisocket system not being able to use the TSC in userspace because the sync test measured too much skew and disabled it. I don't think there is any real skew on that system. I think the sync test is confusing NUMA overhead for skew and issuing a false positive result. Now, we _could_ change the test to account for NUMA overhead. I don't know exactly how you would do it, but I imagine you could devise a handshake to compute an estimate and then factor that into your skew measurement. Another approach is to drop the current skew measurement handshake in favor of a dumber approach without that particular false positive case. This patch changes our sync test to a dumber approach. Instead of trying to measure and correct for skew we only test for lag and do not attempt to correct for it if we detect it. Two CPUs enter a loop and continuously check whether the other CPU's TSC lags their own. With this approach the false positive we are seeing on sthen@'s machine is impossible because we are only checking whether one value lags the other, not whether their difference exceeds an arbitrary value. Synchronization is tested to within a margin of error because both CPUs are checking for lag at the same time. To keep the margin of error is as small as possible for a given clock rate we spin for a relatively long time. Right now we spin for 1 millisecond per test round. This is arbitrary but it seems to work. There is a point of diminishing returns for round duration. This sync test approach takes much more time than the current handshake approach and I'd like to minimize our impact on boot time. To actually shrink the margin of error you need to run the CPUs at the highest possible clock rate. If they are underclocked due to e.g. SpeedStep your margin of error grows and the test may fail to detect lag. We do two rounds of testing for each CPU. This is arbitrary. You could do more. I think at least one additional test round is a good idea, just in case we "get lucky" in the first round. I think this could help mitigate measurement problems introduced by factors beyond our control. For example, if one of the CPUs blacks out for the duration of the test because it is preempted by the hypervisor the test will pass but the result is bogus. If we do more test rounds we have a better chance of getting a meaningful result even if we get unlucky with hypervisor preemption or something similar. Misc. notes: - We no longer have a per-CPU skew value. If any TSC lags another we just set the timecounter quality to a negative number. We don't disable userspace or attempt to correct the skew in the kernel. I think that bears repeating: we are no longer correcting the skew. If we detect any lag the TSC is effectively broken. - There is no longer any concept of TSC drift. Drift is computed from skew change and we have no skew value anymore. - Am I allowed to printf(9) from a secondary CPU during boot? It seems to hang the kernel but I don't know why. - I have no idea how to use the volatile keyword correctly. I am trying to keep the compiler from optimizing away my stores. I don't think it implicitly understands that two threads are looking at these variables at the same time If I am abusing it please tell me. I'm trying to avoid a situation where some later compiler change subtly breaks the test. If I sprinkle volatile everywhere my impression is that it forces the compiler to actually do the store. - I have aligned the global TSC values to 64 bytes to try to minimize "cache bounce". Each value has one reader and one writer so if the two values are on different cache lines a write to one value shouldn't cause a cache miss for the writer of the other value. ... right? I'm not an expert on cache stuff. Can someone shed light on whether I am doing the right thing here? - I rolled my own thread barriers for the test. Would a generic thread barrier be useful elsewhere in the kernel? Feels wrong to roll my own synchronization primitive, but maybe we actually don't need them anywhere else? - I would like to forcibly reset IA32_TSC_ADJUST before running the test but I don't think the CPU feature flags are set early enough for tsc_reset_adjust() to see the relevant flag even if the CPU has that register. Could we initialize the flags earlier in boot, before the sync test? Testing notes: - Tests on multisocket systems, multiprocessor VMs on various hypervisors, and on systems where the TSC is currently disabled by the kernel are appreciated! Just send your dmesg. - TSC_DEBUG is set for now to force the test to run on all CPUs even if a prior test has already failed. This is useful for testing but in practice if we fail one test there is no point in running further tests. I'm actually unsure about this. If we take the time to run the test on every processor it might give us a better
Re: passwd(1) does not need librpcsvc
On Thu, 27 Jan 2022 15:17:01 +
Miod Vallat wrote:
> > Hi,
> >
> > linking with librpcsvc should be superfluous now.
>
> Then you should also update DPADD to remove $(LIBRPCSVC}...
>
Patch V2
Index: Makefile
===
RCS file: /var/cvs/src/usr.bin/passwd/Makefile,v
retrieving revision 1.41
diff -u -p -r1.41 Makefile
--- Makefile26 Nov 2015 19:01:47 - 1.41
+++ Makefile27 Jan 2022 16:20:05 -
@@ -6,8 +6,8 @@ PROG= passwd
SRCS= local_passwd.c passwd.c getpwent.c \
pwd_check.c
.PATH: ${.CURDIR}/../../lib/libc/gen
-DPADD+= ${LIBRPCSVC} ${LIBUTIL}
-LDADD+= -lrpcsvc -lutil
+DPADD+= ${LIBUTIL}
+LDADD+= -lutil
CFLAGS+= -I${.CURDIR}
CFLAGS+=-I${.CURDIR}/../../lib/libc/include
Re: head(1): refactor main loop
From:
https://en.cppreference.com/w/c/language/main_function
argv- Pointer to the first element of an array of argc + 1 pointers,
of which the last one is null and the previous ones, if any, point to strings
that represent the arguments passed to the program from the host environment.
If argv[0] is not a null pointer (or, equivalently, if argc > 0), it points to
a string that represents the program name, which is empty if the program name
is not available from the host environment.
There are other programs like /usr/src/bin/echo/echo.c that take advantage of
this behavior.
Cheers,
-Allen
From: owner-t...@openbsd.org on behalf of flint pyrite
Sent: Thursday, January 27, 2022 8:31 AM
To: Scott Cheloha
Cc: tech@openbsd.org
Subject: Re: head(1): refactor main loop
+ for (; *argv != NULL; argv++)
is argv[1] guaranteed? I have never seen this before.
On Wed, Jan 26, 2022 at 9:52 PM Scott Cheloha
wrote:
> The main loop in head(1) is obfuscated. In particular, the path
> through the loop to exit(3) is extremely clever. Clever in a bad way.
>
> This patch moves the open/read/write/close portions of the loop out
> into a separate function, head_file(). I think the result is easier
> to understand. We only loop in main() if we have file arguments, the
> argv[] loop is idiomatic and simple, we actually reach the end of
> main(), etc.
>
> In a subsequent patch I intend to switch to getline(3) and add more
> error checking to head_file(). For now I have just moved the
> getc/putchar loop into head_file() as-is.
>
> ok?
>
> Index: head.c
> ===
> RCS file: /cvs/src/usr.bin/head/head.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 head.c
> --- head.c 10 Oct 2021 15:57:25 - 1.22
> +++ head.c 27 Jan 2022 04:41:09 -
> @@ -37,6 +37,7 @@
> #include
> #include
>
> +int head_file(const char *, long, int);
> static void usage(void);
>
> /*
> @@ -49,9 +50,7 @@ int
> main(int argc, char *argv[])
> {
> const char *errstr;
> - FILE*fp;
> - longcnt;
> - int ch, firsttime;
> + int ch;
> longlinecnt = 10;
> int status = 0;
>
> @@ -81,33 +80,46 @@ main(int argc, char *argv[])
> }
> argc -= optind, argv += optind;
>
> - for (firsttime = 1; ; firsttime = 0) {
> - if (!*argv) {
> - if (!firsttime)
> - exit(status);
> - fp = stdin;
> - if (pledge("stdio", NULL) == -1)
> - err(1, "pledge");
> - } else {
> - if ((fp = fopen(*argv, "r")) == NULL) {
> - warn("%s", *argv++);
> - status = 1;
> - continue;
> - }
> - if (argc > 1) {
> - if (!firsttime)
> - putchar('\n');
> - printf("==> %s <==\n", *argv);
> - }
> - ++argv;
> - }
> - for (cnt = linecnt; cnt && !feof(fp); --cnt)
> - while ((ch = getc(fp)) != EOF)
> - if (putchar(ch) == '\n')
> - break;
> - fclose(fp);
> + if (argc == 0) {
> + if (pledge("stdio", NULL) == -1)
> + err(1, "pledge");
> +
> + status = head_file(NULL, linecnt, 0);
> + } else {
> + for (; *argv != NULL; argv++)
> + status |= head_file(*argv, linecnt, argc > 1);
> }
> - /*NOTREACHED*/
> +
> + return status;
> +}
> +
> +int
> +head_file(const char *path, long count, int need_header)
> +{
> + FILE *fp;
> + int ch;
> + static int first = 1;
> +
> + if (path != NULL) {
> + fp = fopen(path, "r");
> + if (fp == NULL) {
> + warn("%s", path);
> + return 1;
> + }
> + if (need_header) {
> + printf("%s==> %s <==\n", first ? "" : "\n", path);
> + first = 0;
> + }
> + } else
> + fp = stdin;
> +
> + for (; count > 0 && !feof(fp); --count)
> + while ((ch = getc(fp)) != EOF)
> + if (putchar(ch) == '\n')
> + break;
> + fclose(fp);
> +
> + return 0;
> }
>
>
>
>
Re: head(1): refactor main loop
+ for (; *argv != NULL; argv++)
is argv[1] guaranteed? I have never seen this before.
On Wed, Jan 26, 2022 at 9:52 PM Scott Cheloha
wrote:
> The main loop in head(1) is obfuscated. In particular, the path
> through the loop to exit(3) is extremely clever. Clever in a bad way.
>
> This patch moves the open/read/write/close portions of the loop out
> into a separate function, head_file(). I think the result is easier
> to understand. We only loop in main() if we have file arguments, the
> argv[] loop is idiomatic and simple, we actually reach the end of
> main(), etc.
>
> In a subsequent patch I intend to switch to getline(3) and add more
> error checking to head_file(). For now I have just moved the
> getc/putchar loop into head_file() as-is.
>
> ok?
>
> Index: head.c
> ===
> RCS file: /cvs/src/usr.bin/head/head.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 head.c
> --- head.c 10 Oct 2021 15:57:25 - 1.22
> +++ head.c 27 Jan 2022 04:41:09 -
> @@ -37,6 +37,7 @@
> #include
> #include
>
> +int head_file(const char *, long, int);
> static void usage(void);
>
> /*
> @@ -49,9 +50,7 @@ int
> main(int argc, char *argv[])
> {
> const char *errstr;
> - FILE*fp;
> - longcnt;
> - int ch, firsttime;
> + int ch;
> longlinecnt = 10;
> int status = 0;
>
> @@ -81,33 +80,46 @@ main(int argc, char *argv[])
> }
> argc -= optind, argv += optind;
>
> - for (firsttime = 1; ; firsttime = 0) {
> - if (!*argv) {
> - if (!firsttime)
> - exit(status);
> - fp = stdin;
> - if (pledge("stdio", NULL) == -1)
> - err(1, "pledge");
> - } else {
> - if ((fp = fopen(*argv, "r")) == NULL) {
> - warn("%s", *argv++);
> - status = 1;
> - continue;
> - }
> - if (argc > 1) {
> - if (!firsttime)
> - putchar('\n');
> - printf("==> %s <==\n", *argv);
> - }
> - ++argv;
> - }
> - for (cnt = linecnt; cnt && !feof(fp); --cnt)
> - while ((ch = getc(fp)) != EOF)
> - if (putchar(ch) == '\n')
> - break;
> - fclose(fp);
> + if (argc == 0) {
> + if (pledge("stdio", NULL) == -1)
> + err(1, "pledge");
> +
> + status = head_file(NULL, linecnt, 0);
> + } else {
> + for (; *argv != NULL; argv++)
> + status |= head_file(*argv, linecnt, argc > 1);
> }
> - /*NOTREACHED*/
> +
> + return status;
> +}
> +
> +int
> +head_file(const char *path, long count, int need_header)
> +{
> + FILE *fp;
> + int ch;
> + static int first = 1;
> +
> + if (path != NULL) {
> + fp = fopen(path, "r");
> + if (fp == NULL) {
> + warn("%s", path);
> + return 1;
> + }
> + if (need_header) {
> + printf("%s==> %s <==\n", first ? "" : "\n", path);
> + first = 0;
> + }
> + } else
> + fp = stdin;
> +
> + for (; count > 0 && !feof(fp); --count)
> + while ((ch = getc(fp)) != EOF)
> + if (putchar(ch) == '\n')
> + break;
> + fclose(fp);
> +
> + return 0;
> }
>
>
>
>
Re: rev(1): refactor main loop
On Thu, 27 Jan 2022 08:52:52 -0600, Scott Cheloha wrote: > Just like with head(1), rev(1)'s main loop is obfuscated. > > This patch moves the open/read/write/close portion of the main loop > out of main() into a separate function, rev_file(). "multibyte" > becomes a global. OK millert@ - todd
Re: head(1): refactor main loop
On Wed, 26 Jan 2022 22:50:27 -0600, Scott Cheloha wrote: > The main loop in head(1) is obfuscated. In particular, the path > through the loop to exit(3) is extremely clever. Clever in a bad way. > > This patch moves the open/read/write/close portions of the loop out > into a separate function, head_file(). I think the result is easier > to understand. We only loop in main() if we have file arguments, the > argv[] loop is idiomatic and simple, we actually reach the end of > main(), etc. OK millert@ - todd
Re: passwd(1) does not need librpcsvc
> Hi,
>
> linking with librpcsvc should be superfluous now.
Then you should also update DPADD to remove $(LIBRPCSVC}...
> Index: Makefile
> ===
> RCS file: /var/cvs/src/usr.bin/passwd/Makefile,v
> retrieving revision 1.41
> diff -u -p -r1.41 Makefile
> --- Makefile 26 Nov 2015 19:01:47 - 1.41
> +++ Makefile 27 Jan 2022 14:59:09 -
> @@ -7,7 +7,7 @@ SRCS= local_passwd.c passwd.c getpwent.c
> pwd_check.c
> .PATH: ${.CURDIR}/../../lib/libc/gen
> DPADD+= ${LIBRPCSVC} ${LIBUTIL}
> -LDADD+= -lrpcsvc -lutil
> +LDADD+= -lutil
> CFLAGS+= -I${.CURDIR}
>
> CFLAGS+=-I${.CURDIR}/../../lib/libc/include
>
> --
> Greetings Ben
>
passwd(1) does not need librpcsvc
Hi,
linking with librpcsvc should be superfluous now.
Index: Makefile
===
RCS file: /var/cvs/src/usr.bin/passwd/Makefile,v
retrieving revision 1.41
diff -u -p -r1.41 Makefile
--- Makefile26 Nov 2015 19:01:47 - 1.41
+++ Makefile27 Jan 2022 14:59:09 -
@@ -7,7 +7,7 @@ SRCS= local_passwd.c passwd.c getpwent.c
pwd_check.c
.PATH: ${.CURDIR}/../../lib/libc/gen
DPADD+= ${LIBRPCSVC} ${LIBUTIL}
-LDADD+= -lrpcsvc -lutil
+LDADD+= -lutil
CFLAGS+= -I${.CURDIR}
CFLAGS+=-I${.CURDIR}/../../lib/libc/include
--
Greetings Ben
rev(1): refactor main loop
Just like with head(1), rev(1)'s main loop is obfuscated.
This patch moves the open/read/write/close portion of the main loop
out of main() into a separate function, rev_file(). "multibyte"
becomes a global.
I think the result is easier to understand at a glance.
In a subsequent patch I want to drop the "rpath" promise in the
no-file branch, but that needs to wait.
ok?
Index: rev.c
===
RCS file: /cvs/src/usr.bin/rev/rev.c,v
retrieving revision 1.14
diff -u -p -r1.14 rev.c
--- rev.c 13 Jan 2022 05:10:46 - 1.14
+++ rev.c 27 Jan 2022 14:50:41 -
@@ -40,17 +40,16 @@
#include
#include
+int multibyte;
+
int isu8cont(unsigned char);
+int rev_file(const char *);
void usage(void);
int
main(int argc, char *argv[])
{
- char *filename, *p = NULL, *t, *te, *u;
- FILE *fp;
- ssize_t len;
- size_t ps = 0;
- int ch, multibyte, rval;
+ int ch, rval;
setlocale(LC_CTYPE, "");
multibyte = MB_CUR_MAX > 1;
@@ -67,40 +66,13 @@ main(int argc, char *argv[])
argc -= optind;
argv += optind;
- fp = stdin;
- filename = "stdin";
rval = 0;
- do {
- if (*argv) {
- if ((fp = fopen(*argv, "r")) == NULL) {
- warn("%s", *argv);
- rval = 1;
- ++argv;
- continue;
- }
- filename = *argv++;
- }
- while ((len = getline(, , fp)) != -1) {
- if (p[len - 1] == '\n')
- --len;
- for (t = p + len - 1; t >= p; --t) {
- te = t;
- if (multibyte)
- while (t > p && isu8cont(*t))
- --t;
- for (u = t; u <= te; ++u)
- if (putchar(*u) == EOF)
- err(1, "stdout");
- }
- if (putchar('\n') == EOF)
- err(1, "stdout");
- }
- if (ferror(fp)) {
- warn("%s", filename);
- rval = 1;
- }
- (void)fclose(fp);
- } while(*argv);
+ if (argc == 0) {
+ rval = rev_file(NULL);
+ } else {
+ for (; *argv != NULL; argv++)
+ rval |= rev_file(*argv);
+ }
return rval;
}
@@ -108,6 +80,54 @@ int
isu8cont(unsigned char c)
{
return (c & (0x80 | 0x40)) == 0x80;
+}
+
+int
+rev_file(const char *path)
+{
+ char *p = NULL, *t, *te, *u;
+ const char *filename;
+ FILE *fp;
+ size_t ps = 0;
+ ssize_t len;
+ int rval = 0;
+
+ if (path != NULL) {
+ fp = fopen(path, "r");
+ if (fp == NULL) {
+ warn("%s", path);
+ return 1;
+ }
+ filename = path;
+ } else {
+ fp = stdin;
+ filename = "stdin";
+ }
+
+ while ((len = getline(, , fp)) != -1) {
+ if (p[len - 1] == '\n')
+ --len;
+ for (t = p + len - 1; t >= p; --t) {
+ te = t;
+ if (multibyte)
+ while (t > p && isu8cont(*t))
+ --t;
+ for (u = t; u <= te; ++u)
+ if (putchar(*u) == EOF)
+ err(1, "stdout");
+ }
+ if (putchar('\n') == EOF)
+ err(1, "stdout");
+ }
+ free(p);
+ if (ferror(fp)) {
+ warn("%s", filename);
+ rval = 1;
+ }
+
+ (void)fclose(fp);
+
+ return rval;
}
void
Re: unlock mmap(2) for anonymous mappings
On Tue, Jan 25, 2022 at 07:54:52AM +, Klemens Nanni wrote:
> > Could we use a vm_map_assert_locked() or something similar that reflect
> > the exclusive or shared (read) lock comments? I don't trust comments.
> > It's too easy to miss a lock in a code path.
>
> This survives desktop usage, running regress and building kernels on
> amd64.
>
> I'll throw it at sparc64 soon.
>
> >
> > > So annotate functions using `size' wrt. the required lock.
Here's a better diff that asserts for read, write or any type of vm_map
lock.
vm_map_lock() and vm_map_lock_read() are used for exclusive and shared
access respectively; unless I missed something, no code path is purely
protected by vm_map_lock_read() alone, i.e. functions called with a read
lock held by the callee are also called with a write lock elsewhere.
That means my new vm_map_assert_locked_read() is currently unused, but
vm_map_assert_locked_any() and vm_map_assert_locked_write() are now used
to validate existing function comments as well as a few new places.
amd64 and sparc64 are happy with this, both daily drivers as well as
build/regress machines.
I'd appreciate more tests and reports about running with this diff and
mmap(2) unlocked.
Index: uvm_map.c
===
RCS file: /cvs/src/sys/uvm/uvm_map.c,v
retrieving revision 1.282
diff -u -p -r1.282 uvm_map.c
--- uvm_map.c 21 Dec 2021 22:21:32 - 1.282
+++ uvm_map.c 27 Jan 2022 10:45:47 -
@@ -805,6 +805,8 @@ uvm_map_sel_limits(vaddr_t *min, vaddr_t
* Fills in *start_ptr and *end_ptr to be the first and last entry describing
* the space.
* If called with prefilled *start_ptr and *end_ptr, they are to be correct.
+ *
+ * map must be at least read-locked.
*/
int
uvm_map_isavail(struct vm_map *map, struct uvm_addr_state *uaddr,
@@ -815,6 +817,8 @@ uvm_map_isavail(struct vm_map *map, stru
struct uvm_map_addr *atree;
struct vm_map_entry *i, *i_end;
+ vm_map_assert_locked_any(map);
+
if (addr + sz < addr)
return 0;
@@ -1821,6 +1825,8 @@ boolean_t
uvm_map_lookup_entry(struct vm_map *map, vaddr_t address,
struct vm_map_entry **entry)
{
+ vm_map_assert_locked_any(map);
+
*entry = uvm_map_entrybyaddr(>addr, address);
return *entry != NULL && !UVM_ET_ISHOLE(*entry) &&
(*entry)->start <= address && (*entry)->end > address;
@@ -2206,6 +2212,8 @@ uvm_unmap_kill_entry(struct vm_map *map,
* If remove_holes, then remove ET_HOLE entries as well.
* If markfree, entry will be properly marked free, otherwise, no replacement
* entry will be put in the tree (corrupting the tree).
+ *
+ * map must be locked.
*/
void
uvm_unmap_remove(struct vm_map *map, vaddr_t start, vaddr_t end,
@@ -2214,6 +,8 @@ uvm_unmap_remove(struct vm_map *map, vad
{
struct vm_map_entry *prev_hint, *next, *entry;
+ vm_map_assert_locked_write(map);
+
start = MAX(start, map->min_offset);
end = MIN(end, map->max_offset);
if (start >= end)
@@ -2976,12 +2986,17 @@ uvm_tree_sanity(struct vm_map *map, char
UVM_ASSERT(map, addr == vm_map_max(map), file, line);
}
+/*
+ * map must be at least read-locked.
+ */
void
uvm_tree_size_chk(struct vm_map *map, char *file, int line)
{
struct vm_map_entry *iter;
vsize_t size;
+ vm_map_assert_locked_any(map);
+
size = 0;
RBT_FOREACH(iter, uvm_map_addr, >addr) {
if (!UVM_ET_ISHOLE(iter))
@@ -4268,7 +4283,7 @@ uvm_map_submap(struct vm_map *map, vaddr
* uvm_map_checkprot: check protection in map
*
* => must allow specific protection in a fully allocated region.
- * => map mut be read or write locked by caller.
+ * => map must be read or write locked by caller.
*/
boolean_t
uvm_map_checkprot(struct vm_map *map, vaddr_t start, vaddr_t end,
@@ -4276,6 +4291,8 @@ uvm_map_checkprot(struct vm_map *map, va
{
struct vm_map_entry *entry;
+ vm_map_assert_locked_any(map);
+
if (start < map->min_offset || end > map->max_offset || start > end)
return FALSE;
if (start == end)
@@ -5557,6 +5577,46 @@ vm_map_unbusy_ln(struct vm_map *map, cha
mtx_leave(>flags_lock);
if (oflags & VM_MAP_WANTLOCK)
wakeup(>flags);
+}
+
+void
+vm_map_assert_locked_any_ln(struct vm_map *map, char *file, int line)
+{
+ LPRINTF(("map assert read or write locked: %p (at %s %d)\n", map, file,
line));
+ if ((map->flags & VM_MAP_INTRSAFE) == 0)
+ rw_assert_anylock(>lock);
+ else
+ MUTEX_ASSERT_LOCKED(>mtx);
+}
+
+void
+vm_map_assert_locked_read_ln(struct vm_map *map, char *file, int line)
+{
+ LPRINTF(("map assert read locked: %p (at %s %d)\n", map, file, line));
+ if ((map->flags & VM_MAP_INTRSAFE) == 0)
+ rw_assert_rdlock(>lock);
+ else
+ MUTEX_ASSERT_LOCKED(>mtx);
+}
+
+void
Re: rpki-client RFC "compliant" MFT parsing
On Thu, Jan 27, 2022 at 07:46:32AM +0100, Theo Buehler wrote:
> On Wed, Jan 26, 2022 at 04:42:04PM +0100, Claudio Jeker wrote:
> > So the RFC is not very clear but in general the idea is that if multiple
> > MFTs are available the newest one (highest manifest number) should be
> > used.
> >
> > In our case there are two possible MFTs available the previously valid on
> > and the now downloaded one. So adjust the parser code so that both files
> > are opened and parsed and the x509 is verified. Checks like the
> > thisUpdate/nextUpdate validity and FileAndHash sequence are postponed.
> > Compare these two mfts and decide which one should be used.
> > Now check everything that was postponed.
> >
> > When checking the hash of files in the MFT check both locations and
> > remember which file was the actual match. It is important that later on
> > the same file is opened.
> >
> > The error checking around MFTs had to be adjusted in some places since it
> > turned out to be too noisy on stale caches.
> >
> > Please test and report unexpected behaviour.
>
> This seems to work fine here. I have read the diff and it looks good,
> but have not reviewed it thoroughly. Do you consider it ready for that?
I have parts I'm not super happy with but have no better idea yet.
Mainly the changes to parse_entity() make that function even more complex
and error prone. proc_parser_mft_check() is the other function where I'm
not sure. So happy for any feedback to improve those bits.
> One thing that stood out in mft_compare():
>
> > +int
> > +mft_compare(const struct mft *a, const struct mft *b)
> > +{
> > + BIGNUM *abn = NULL, *bbn = NULL;
> > + int r;
> > +
> > + if (b == NULL)
> > + return 1;
> > + if (a == NULL)
> > + return 0;
> > +
> > + BN_hex2bn(, a->seqnum);
> > + BN_hex2bn(, b->seqnum);
>
> These need error checking.
>
> Is it necessary to convert these back into BIGNUMs? Can't we compare
> first the string length and if it's equal do a strcmp?
I think it is possible but I was not sure if it is always correct so I
bailed on that plan and used BN_cmp().
I have to look at how OpenSSL does the BN_bn2hex() conversion. I noticed
that the numbers seen are mostly 2 or 4 bytes long with leding 0.
I agree that this comparision should be easier. Another option would be to
not convert seqnum and keep it as BIGNUM but I'm not a fan of that.
Especially inside struct mft.
> > + r = BN_cmp(abn, bbn);
> > + BN_free(abn);
> > + BN_free(bbn);
> > +
> > + if (r < 0)
> > + return 0;
> > +
> > + /*
> > +* Equal sequence numbers should not happen for different content.
> > +* In this case we prefer the newer MFT.
> > +*/
> > + return 1;
> > }
>
--
:wq Claudio
