Re: Fix ipsp_spd_lookup() for transport mode
I don't know much about l2tp, pipex or npppd. So I cannot say if
the new logic is correct. But I guess you have tested that.
The tdb mutex and ref counting looks correct.
> + struct tdb *tdb, *tdblocal = NULL;
The variable names tdb and tdbp are used very inconsistently within
IPsec. Don't use both. I think tdpb and a tdbflow are sufficient.
> + if (ipsecflowinfo != 0)
> + ids = ipsp_ids_lookup(ipsecflowinfo);
Can you move that to the place where it is needed?
> + /*
> + * Prepare tdb for searching the correct SPD by rn_lookup().
> + * "tdb_filtemask" of the tdb is used to find the correct SPD when
> + * multiple policies are overlapped.
> + */
> + tdb = tdbp;
> + if (ipsecflowinfo != 0 && ids != NULL) {
> + KASSERT(tdbp == NULL);
> + KASSERT(direction == IPSP_DIRECTION_OUT);
> + if ((tdblocal = gettdbbyflow(rdomain, direction, ,
> + IPPROTO_ESP, ids)) != NULL)
> + tdb = tdblocal;
> + }
> +
> /* Actual SPD lookup. */
> - if ((rnh = spd_table_get(rdomain)) == NULL ||
> - (rn = rn_match((caddr_t), rnh)) == NULL) {
> + rnh = spd_table_get(rdomain);
> + if (rnh != NULL) {
> + if (tdb != NULL)
> + rn = rn_lookup((caddr_t)>tdb_filter,
> + (caddr_t)>tdb_filtermask, rnh);
> + else
> + rn = rn_match((caddr_t), rnh);
> + }
> + tdb_unref(tdblocal);
Perhaps it is easier to understand this way:
if (ipsecflowinfo != 0) {
ids = ipsp_ids_lookup(ipsecflowinfo);
if (ids != NULL)
tdbflow = gettdbbyflow(rdomain, direction, ,
IPPROTO_ESP, ids);
}
/* Actual SPD lookup. */
rnh = spd_table_get(rdomain);
if (rnh != NULL) {
if (tdbflow != NULL)
rn = rn_lookup((caddr_t)>tdb_filter,
(caddr_t)>tdb_filtermask, rnh);
else if (tdb != NULL)
rn = rn_lookup((caddr_t)>tdb_filter,
(caddr_t)>tdb_filtermask, rnh);
else
rn = rn_match((caddr_t), rnh);
}
tdb_unref(tdbflow);
It is hard to say whether the new
rn_lookup(tdbp->tdb_filter/tdbp->tdb_filtermask) changes existing
IPsec behavior for setups without l2tp. Do we need it there?
I never ran into problems patching the correct policy.
bluhm
Re: com(4) at acpi(4) on amd64
> Date: Fri, 10 Dec 2021 07:56:44 +0100 > From: Anton Lindqvist > > On Tue, Dec 07, 2021 at 01:08:45PM +0100, Mark Kettenis wrote: > > > Date: Tue, 7 Dec 2021 11:30:48 +0100 > > > From: Anton Lindqvist > > > > > > On Mon, Dec 06, 2021 at 10:25:34PM +0100, Mark Kettenis wrote: > > > > > Date: Mon, 6 Dec 2021 21:45:03 +0100 > > > > > From: Anton Lindqvist > > > > > > > > > > On Mon, Dec 06, 2021 at 09:23:45PM +0100, Mark Kettenis wrote: > > > > > > > Date: Mon, 6 Dec 2021 21:08:04 +0100 > > > > > > > From: Patrick Wildt > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > On one machine I had the pleasure of having to try and use the > > > > > > > Serial-over-LAN feature which shows up as just another com(4) > > > > > > > device. Instead of having to manually add a com(4) at isa(4) > > > > > > > I figured it would be nicer to have them attach via ACPI. At > > > > > > > least on that machine, the SOL definitely shows up in the DSDT. > > > > > > > > > > > > > > Since I don't want to break any legacy machines, I figured I'd > > > > > > > keep ignoring the isa(4) addresses specified in amd64's GENERIC. > > > > > > > > > > > > > > Right now this diff is more about putting it out there, not about > > > > > > > asking for OKs, as amd64 isn't really my strong suit. If people > > > > > > > are interested, I can definitely put in all the feedback there is. > > > > > > > > > > > > > > Patrick > > > > > > > > > > > > anton@ has a better diff he's working on > > > > > > > > > > Here's the diff in its current state. There's one thing I haven't had > > > > > the time to figure out yet: in order to get interrupts working on my > > > > > apu2 I had to explicit pass LR_EXTIRQ_MODE (aaa_irq_flags is zero) to > > > > > acpi_intr_establish() which causes IST_EDGE to be passed to > > > > > intr_establish(). Worth noting is that this matches what com at isa > > > > > already does. This was however not necessary on my amd64 build machine > > > > > where aaa_irq_flags also is zero. > > > > > > > > Actually, it seems we're parsing the ACPI interrupt resource > > > > descriptor wrong. The decompiled DSDTs I have here seem to use > > > > IRQNoFlags() for the interrupts, which apparently implies the > > > > interrupt is edge-triggered. > > > > > > > > Let me see if I can cook a diff. > > > > > > Updated diff now that kettenis@ fixed parsing of the irq flags. > > > > A few comments below. > > > > > diff --git share/man/man4/com.4 share/man/man4/com.4 > > > index 73b421f2ca7..e54255fe0d6 100644 > > > --- share/man/man4/com.4 > > > +++ share/man/man4/com.4 > > > @@ -61,11 +61,13 @@ > > > .Cd "com0 at isa? port 0x3f8 irq 4" > > > .Cd "com1 at isa? port 0x2f8 irq 3" > > > .Pp > > > +.Cd "# arm64 and amd64" > > > +.Cd "com* at acpi?" > > > +.Pp > > > .Cd "# armv7" > > > .Cd "com* at fdt?" > > > .Pp > > > .Cd "# arm64" > > > -.Cd "com* at acpi?" > > > .Cd "com* at fdt?" > > > > The armv7 and arm64 entries can be combined now > > > > > .Pp > > > .Cd "# hppa" > > > diff --git sys/arch/amd64/conf/GENERIC sys/arch/amd64/conf/GENERIC > > > index ecccd1323d9..87fcaced306 100644 > > > --- sys/arch/amd64/conf/GENERIC > > > +++ sys/arch/amd64/conf/GENERIC > > > @@ -65,6 +65,11 @@ amdgpio* at acpi? > > > aplgpio* at acpi? > > > bytgpio* at acpi? > > > chvgpio* at acpi? > > > +com0 at acpi? > > > +com1 at acpi? > > > +com2 at acpi? > > > +com3 at acpi? > > > +com* at acpi? > > > glkgpio* at acpi? > > > pchgpio* at acpi? > > > sdhc*at acpi? > > > diff --git sys/arch/amd64/conf/RAMDISK sys/arch/amd64/conf/RAMDISK > > > index 6041293b287..a5f10b357dd 100644 > > > --- sys/arch/amd64/conf/RAMDISK > > > +++ sys/arch/amd64/conf/RAMDISK > > > @@ -34,6 +34,9 @@ acpipci*at acpi? > > > acpiprt* at acpi? > > > acpimadt0at acpi? > > > #acpitz* at acpi? > > > +com0 at acpi? > > > +com1 at acpi? > > > +com* at acpi? > > > > > > mpbios0 at bios0 > > > > As Theo pointed out, this may not be entirely fool-proof. We probe > > acpi(4) before isa(4), which is good. Since we prevent ISA devices > > from attaching to addresses that have been claimed by something else, > > this means that we won't attach a com(4) to isa(4) if we have attached > > an ACPI com(4) attached at the same address. But if there is no ACPI > > com(4) at an address the an ISA com(4) may still attach. As long as > > the ACPI serial ports are listed in canonical order and use the > > standard IRQ, everything should be fine. If the ACPI com(4) ports are > > not in canonical order any additional ISA com(4) may no longer attach. > > For example, if there is a single ACPI com(4) at address 0x2f8, we > > will now attach it as com0. But since com0 is now attached, the > > isa(4) code will skip its com0 and won't probe com(4) at address > > 0x3f8. I think this is fine. I expect ACPI to list all usable com(4) > >
cat(1): drop "rpath" promise in no-file case
We don't need the "rpath" promise if there are no file arguments. We
drop this promise in other utilities that default to stdin in the
no-file case.
If we consolidate raw_args() and cook_args() into a single function,
cat_file(), we can drop the "rpath" promise in main(), in a single
place in cat.c.
The alternative is to do pledge("stdio", NULL) in two places in cat.c.
I'm not a fan of that.
ok?
Index: cat.c
===
RCS file: /cvs/src/bin/cat/cat.c,v
retrieving revision 1.32
diff -u -p -r1.32 cat.c
--- cat.c 24 Oct 2021 21:24:21 - 1.32
+++ cat.c 13 Dec 2021 17:51:39 -
@@ -50,9 +50,8 @@
int bflag, eflag, nflag, sflag, tflag, vflag;
int rval;
-void cook_args(char *argv[]);
+void cat_file(const char *);
void cook_buf(FILE *, const char *);
-void raw_args(char *argv[]);
void raw_cat(int, const char *);
int
@@ -94,38 +93,58 @@ main(int argc, char *argv[])
}
argv += optind;
- if (bflag || eflag || nflag || sflag || tflag || vflag)
- cook_args(argv);
- else
- raw_args(argv);
+ if (*argv == NULL) {
+ if (pledge("stdio", NULL) == -1)
+ err(1, "pledge");
+ cat_file(NULL);
+ } else {
+ for (; *argv != NULL; argv++)
+ cat_file(*argv);
+ }
if (fclose(stdout))
err(1, "stdout");
return rval;
}
void
-cook_args(char **argv)
+cat_file(const char *path)
{
+ const char *name;
FILE *fp;
+ int fd;
- if (*argv == NULL) {
- cook_buf(stdin, "stdin");
- return;
- }
-
- for (; *argv != NULL; argv++) {
- if (!strcmp(*argv, "-")) {
- cook_buf(stdin, "stdin");
- clearerr(stdin);
- continue;
+ if (bflag || eflag || nflag || sflag || tflag || vflag) {
+ if (path == NULL || strcmp(path, "-") == 0) {
+ name = "(stdin)";
+ fp = stdin;
+ } else {
+ name = path;
+ if ((fp = fopen(name, "r")) == NULL) {
+ warn("%s", name);
+ rval = 1;
+ return;
+ }
}
- if ((fp = fopen(*argv, "r")) == NULL) {
- warn("%s", *argv);
- rval = 1;
- continue;
+ cook_buf(fp, name);
+ if (fp == stdin)
+ clearerr(stdin);
+ else
+ fclose(fp);
+ } else {
+ if (path == NULL || strcmp(path, "-") == 0) {
+ name = "(stdin)";
+ fd = STDIN_FILENO;
+ } else {
+ name = path;
+ if ((fd = open(name, O_RDONLY)) == -1) {
+ warn("%s", name);
+ rval = 1;
+ return;
+ }
}
- cook_buf(fp, *argv);
- fclose(fp);
+ raw_cat(fd, name);
+ if (fd != STDIN_FILENO)
+ close(fd);
}
}
@@ -191,31 +210,6 @@ cook_buf(FILE *fp, const char *filename)
}
if (ferror(stdout))
err(1, "stdout");
-}
-
-void
-raw_args(char **argv)
-{
- int fd;
-
- if (*argv == NULL) {
- raw_cat(fileno(stdin), "stdin");
- return;
- }
-
- for (; *argv != NULL; argv++) {
- if (!strcmp(*argv, "-")) {
- raw_cat(fileno(stdin), "stdin");
- continue;
- }
- if ((fd = open(*argv, O_RDONLY)) == -1) {
- warn("%s", *argv);
- rval = 1;
- continue;
- }
- raw_cat(fd, *argv);
- close(fd);
- }
}
void
Re: cat(1): always use a 64K buffer
I think Todd is right, and this doesn't need to change. Todd C. Miller wrote: > On Sun, 12 Dec 2021 19:15:51 -0600, Scott Cheloha wrote: > > > cat(1) sizes its I/O buffer according to the st_blksize of the first > > file it processes. We don't do this very often in the tree. I'm not > > sure if we should trust st_blksize. > > It sizes the buffer based on st_blksize of stdout, not the input > file. Since st_blksize is the "optimal" blocksize for that device. > Since cat only has a single output, the output buffer only needs > to be sized once. > > > It would be simpler to just choose a value that works in practice and > > always use it. > > I prefer it the way it is now. By using st_blksize you get the > proper block size regardless of whether output is to a pipe, a file > or a tty. I didn't suggest this for tee since it supports multiple > output files. > > - todd >
Re: cat(1): always use a 64K buffer
On Sun, 12 Dec 2021 19:15:51 -0600, Scott Cheloha wrote: > cat(1) sizes its I/O buffer according to the st_blksize of the first > file it processes. We don't do this very often in the tree. I'm not > sure if we should trust st_blksize. It sizes the buffer based on st_blksize of stdout, not the input file. Since st_blksize is the "optimal" blocksize for that device. Since cat only has a single output, the output buffer only needs to be sized once. > It would be simpler to just choose a value that works in practice and > always use it. I prefer it the way it is now. By using st_blksize you get the proper block size regardless of whether output is to a pipe, a file or a tty. I didn't suggest this for tee since it supports multiple output files. - todd
Re: unveil(2) usbhidaction(1)
Hi,
and of course this was also missing unveil(NULL, NULL). ok now?
this one opens the default table file "/usr/share/misc/usb_hid_usages" through
hid_start(3) from libusbhid, then `dev' (will have the fd used on the ioctls)
and finally `conf' which is the file with the actions to be monitored. `conf'
needs to be unveiled with read perms since usbhidaction(1) can run as daemon and
this file will be re-read if a SIGHUP is catched.
Index: usbhidaction.c
===
RCS file: /cvs/src/usr.bin/usbhidaction/usbhidaction.c,v
retrieving revision 1.23
diff -u -p -u -r1.23 usbhidaction.c
--- usbhidaction.c 28 Jun 2019 13:35:05 - 1.23
+++ usbhidaction.c 13 Dec 2021 15:13:32 -
@@ -164,6 +164,11 @@ main(int argc, char **argv)
isdemon = 1;
}
+ if (unveil(conf, "r") == -1)
+ err(1, "unveil %s", conf);
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
+
for(;;) {
n = read(fd, buf, sz);
if (verbose > 2) {
Re: unveil(2) usbhidctl(1)
Hi,
of course it needs that, sorry. ok now?
a little bit more of info here, hid_start(3) opens `table' through libusbhid,
then usbhidctl(1) itself opens `dev', after that it's just performing ioctls on
the fd left opened by the latter in this case `hidfd'.
Index: usbhid.c
===
RCS file: /cvs/src/usr.bin/usbhidctl/usbhid.c,v
retrieving revision 1.17
diff -u -p -u -r1.17 usbhid.c
--- usbhid.c31 May 2021 18:30:11 - 1.17
+++ usbhid.c13 Dec 2021 15:06:26 -
@@ -941,6 +941,11 @@ main(int argc, char **argv)
if (hidfd == -1)
err(1, "%s", dev);
+ if (unveil("/", "") == -1)
+ err(1, "unveil /");
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
+
if (ioctl(hidfd, USB_GET_REPORT_ID, ) == -1)
reportid = -1;
if (verbose > 1)
>
> You need this, too, no?
>
> if (unveil(NULL, NULL) == -1)
> err(1, "unveil");
>
> > if (ioctl(hidfd, USB_GET_REPORT_ID, ) == -1)
> > reportid = -1;
> > if (verbose > 1)
> >
>
> --
> I'm not entirely sure you are real.
>
Re: msk(4) not working with trunk(4) (Marvell Yukon 88E8042)
Friendly reminder. I really would like to proceed in debugging this issue with a more suitable report, but I definitely need directions... Copying tech@, hoping to attract the attention of some developers. All the best On 2021-12-07 19:39, Alessandro De Laurenzis wrote: Greetings, I recently installed OpenBSD 7.0 on an old CoreDuo2 machine (Compaq 610, complete dmesg in attach), which was powered by 5.5/5.6/5.7 some years ago, without any relevant issues (after that, it has been used as home server with Debian). mskc0 at pci4 dev 0 function 0 "Marvell Yukon 88E8042" rev 0x10, Yukon-2 FE+ rev. A0 (0x0): msi msk0 at mskc0 port A: address 18:a9:05:94:ab:19 eephy0 at msk0 phy 0: 88E3016 10/100 PHY, rev. 0 I noticed that the trunk(4) failover protocol is broken when the Ethernet cable is plugged in (starting in this configuration, no lease is acquired from DHCP server, switching to Ethernet from wifi breaks the connection; in both cases, trunk and msk0 status is: no carrier). It's worth noting that when msk0 is configured as "stand-alone" (i.e., without trunk(4) failover), the connection is pretty functional and stable. Since I didn't remember any similar problems showing up with 5.x, I made a bit of bisecting, and my conclusion is that the functionality got broken b/w 6.2 and 6.3 and, specifically, after the following commit: RCS file: /cvs/src/sys/dev/pci/if_msk.c,v revision 1.131 date: 2018/01/06 03:11:04; author: dlg; state: Exp; lines: +251 -311; commitid: BhB8LisF92o4xfOK; rework the transmit and receive paths to address reliability issues. phessler@ has been having trouble with msk on overdrive 1000s. some of the issues relate to the driver not coping with exhaustion of mbufs for the rx ring, the other issues are corruption of the mcl9k pool that msk uses. this diff adds a timeout that the rx refill code uses when the rx ring is empty and cannot be filled. it'll periodically retry the ring refill until it can get some mbufs in the air again. the current code made hunting for the mcl9k issue too hard, so this rewrites it to be simpler and more like other drivers. there's now just arrays of mbuf pointers and dmamaps to shadow the hardware ring entries, and producer and consumer indexes. what was there before had linkes lists of something to hold mbuf pointers and dmamaps, and some way to go from the ring to go back to that. i think, it was hard to tell what was happening. this also copies the ADDR64 handling on the tx ring to the rx ring. this potentially makes more rx descriptors available, but that can happen later. in hindsight the mcl9k problem could have been from letting if_rxr allocate the entier ring. if every descriptor was filled, the chip may have run around the ring when it shouldnt have. giving rxr one less descriptor than there is on the ring may have fixed the problem too. this work also makes it easier to make msk mpsafe. tested by an ok phessler@ ok kettenis@ deraadt@ = and the corresponding one for sys/dev/pci/if_mskvar.h (revision 1.14, same log). On a fresh 6.3 install, which was showing the issue, I reverted the 2 files to the revisions 1.130 and 1.13 respectively, observing a functional trunk(4) failover again. The diff is too long and complex, so I cannot say where the problem lies exactly, but I hope this report contains enough information to start an analysis (I'm copying the involved developers, just in case they are not reading this list); of course, I'm available to test any patches (on 7.0 or -current) and add further details if needed. Please note that the dmesg is from OBSD 6.3, since that is the version currently installed on the laptop; in case you're interested in the 7.0/current's dmesg, just let me know. All the best
iked: cleanup libcrypto *_free calls
Hey,
tb@ noticed that we do a lot of redundant explicit NULL checks before
calling libcrypto *_free() functions. A few of the free() calls can also
be avoided by using X509_get0_pubkey() instead of X509_get_pubkey().
ok?
Index: ca.c
===
RCS file: /cvs/src/sbin/iked/ca.c,v
retrieving revision 1.83
diff -u -p -r1.83 ca.c
--- ca.c8 Dec 2021 19:17:35 - 1.83
+++ ca.c13 Dec 2021 13:25:55 -
@@ -187,10 +187,8 @@ ca_reset(struct privsep *ps)
store->ca_pubkey.id_type == IKEV2_ID_NONE)
fatalx("ca_reset: keys not loaded");
- if (store->ca_cas != NULL)
- X509_STORE_free(store->ca_cas);
- if (store->ca_certs != NULL)
- X509_STORE_free(store->ca_certs);
+ X509_STORE_free(store->ca_cas);
+ X509_STORE_free(store->ca_certs);
if ((store->ca_cas = X509_STORE_new()) == NULL)
fatalx("ca_reset: failed to get ca store");
@@ -483,9 +481,8 @@ ca_getcert(struct iked *env, struct imsg
cert = ca_by_subjectaltname(store->ca_certs, );
if (cert) {
log_debug("%s: found local cert", __func__);
- if ((certkey = X509_get_pubkey(cert)) != NULL) {
+ if ((certkey = X509_get0_pubkey(cert)) != NULL) {
ret = ca_pubkey_serialize(certkey, );
- EVP_PKEY_free(certkey);
if (ret == 0) {
ptr = ibuf_data(key.id_buf);
len = ibuf_length(key.id_buf);
@@ -1045,7 +1042,7 @@ ca_cert_local(struct iked *env, X509 *c
if ((localpub = ca_id_to_pkey(>ca_pubkey)) == NULL)
goto done;
- if ((certkey = X509_get_pubkey(cert)) == NULL) {
+ if ((certkey = X509_get0_pubkey(cert)) == NULL) {
log_info("%s: no public key in cert", __func__);
goto done;
}
@@ -1057,10 +1054,8 @@ ca_cert_local(struct iked *env, X509 *c
ret = 1;
done:
- if (certkey != NULL)
- EVP_PKEY_free(certkey);
- if (localpub != NULL)
- EVP_PKEY_free(localpub);
+ EVP_PKEY_free(localpub);
+
return (ret);
}
@@ -1092,8 +1087,7 @@ ca_cert_info(const char *msg, X509 *cert
log_info("%s: subject: %s", msg, buf);
ca_x509_subjectaltname_log(cert, msg);
out:
- if (rawserial)
- BIO_free(rawserial);
+ BIO_free(rawserial);
}
int
@@ -1112,10 +1106,9 @@ ca_subjectpubkey_digest(X509 *x509, uint
* that includes the public key type (eg. RSA) and the
* public key value (see 3.7 of RFC7296).
*/
- if ((pkey = X509_get_pubkey(x509)) == NULL)
+ if ((pkey = X509_get0_pubkey(x509)) == NULL)
return (-1);
buflen = i2d_PUBKEY(pkey, );
- EVP_PKEY_free(pkey);
if (buflen == 0)
return (-1);
if (!EVP_Digest(buf, buflen, md, size, EVP_sha1(), NULL)) {
@@ -1245,10 +1238,9 @@ ca_pubkey_serialize(EVP_PKEY *key, struc
ret = 0;
done:
- if (rsa != NULL)
- RSA_free(rsa);
- if (ec != NULL)
- EC_KEY_free(ec);
+ RSA_free(rsa);
+ EC_KEY_free(ec);
+
return (ret);
}
@@ -1317,10 +1309,9 @@ ca_privkey_serialize(EVP_PKEY *key, stru
ret = 0;
done:
- if (rsa != NULL)
- RSA_free(rsa);
- if (ec != NULL)
- EC_KEY_free(ec);
+ RSA_free(rsa);
+ EC_KEY_free(ec);
+
return (ret);
}
@@ -1354,14 +1345,11 @@ ca_id_to_pkey(struct iked_id *pubkey)
out = localkey;
localkey = NULL;
done:
- if (localkey != NULL)
- EVP_PKEY_free(localkey);
- if (localrsa != NULL)
- RSA_free(localrsa);
- if (localec != NULL)
- EC_KEY_free(localec);
- if (rawcert != NULL)
- BIO_free(rawcert);
+ EVP_PKEY_free(localkey);
+ RSA_free(localrsa);
+ EC_KEY_free(localec);
+ BIO_free(rawcert);
+
return (out);
}
@@ -1403,11 +1391,8 @@ ca_privkey_to_method(struct iked_id *pri
print_map(method, ikev2_auth_map));
out:
- if (ec != NULL)
- EC_KEY_free(ec);
- if (rawcert != NULL)
- BIO_free(rawcert);
-
+ EC_KEY_free(ec);
+ BIO_free(rawcert);
return (method);
}
@@ -1630,19 +1615,12 @@ ca_validate_pubkey(struct iked *env, str
ca_sslerror(__func__);
done:
ibuf_release(idp.id_buf);
- if (localkey != NULL)
- EVP_PKEY_free(localkey);
- if (peerrsa != NULL)
- RSA_free(peerrsa);
- if (peerec != NULL)
- EC_KEY_free(peerec);
- if (localrsa != NULL)
- RSA_free(localrsa);
- if (rawcert !=
Re: ipsec ipo tdb mutex
On 12.12.2021. 16:37, Hrvoje Popovski wrote: > On 11.12.2021. 20:03, Alexander Bluhm wrote: >> On Sat, Dec 11, 2021 at 12:53:35AM +0100, Alexander Bluhm wrote: >>> To cache lookups, the policy ipo is linked to its SA tdb. There >>> is a list of SAs that belong to a policy. To make it MP safe we >>> need a mutex around these pointers. >>> >>> Hrvoje: Can you test this alone and together with the ip forwarding >>> diff. I protects the data structure where the latter crashed. >>> Wonder if this helps. >> updated diff, merged with -current > > > Hi, > > i'm hitting this and ip forwarding diff for 20 hours and boxes didn't panic. > Will try now with plain source .. > Same with clean source, i can't trigger panic ... tnx ..
dhcpleased(8): keep xid
Only generate a new xid on state change.
When we first request a lease (INIT or REBOOTING state) we run with very
short timeouts. If the dhcp server is slow to respond we already have a
new xid and ignore the server's response. This goes on until we increase
the timeout high enough. If we just stick to an xid this will not happen
and we accept "late" responses.
RFC 2131 has:
Selecting a new 'xid' for each retransmission is an implementation
decision. A client may choose to reuse the same 'xid' or select a new
'xid' for each retransmitted message.
Problem seen by phessler on german train wifi.
OK?
diff --git engine.c engine.c
index 4fbdf2f16ef..784c32b6e37 100644
--- engine.c
+++ engine.c
@@ -616,6 +616,7 @@ engine_update_iface(struct imsg_ifinfo *imsg_ifinfo)
if ((iface = calloc(1, sizeof(*iface))) == NULL)
fatal("calloc");
iface->state = IF_DOWN;
+ iface->xid = arc4random();
iface->timo.tv_usec = arc4random_uniform(100);
evtimer_set(>timer, iface_timeout, iface);
iface->if_index = imsg_ifinfo->if_index;
@@ -1309,6 +1310,9 @@ state_transition(struct dhcpleased_iface *iface, enum
if_state new_state)
char ifnamebuf[IF_NAMESIZE], *if_name;
iface->state = new_state;
+ if (new_state != old_state)
+ iface->xid = arc4random();
+
switch (new_state) {
case IF_DOWN:
if (iface->requested_ip.s_addr == INADDR_ANY) {
@@ -1468,7 +1472,6 @@ request_dhcp_discover(struct dhcpleased_iface *iface)
memset(, 0, sizeof(imsg));
- iface->xid = arc4random();
imsg.if_index = iface->if_index;
imsg.xid = iface->xid;
@@ -1495,7 +1498,6 @@ request_dhcp_request(struct dhcpleased_iface *iface)
{
struct imsg_req_dhcp imsg;
- iface->xid = arc4random();
imsg.if_index = iface->if_index;
imsg.xid = iface->xid;
--
I'm not entirely sure you are real.
Re: dhcpleased(8): use struct assignment
On Mon, Dec 13, 2021 at 11:25:02AM +0100, Florian Obser wrote:
> Replace struct member assignment with struct assignment to make the code
> more compact. No binary change (on amd64).
>
> OK? Or is there a reason not to do this?
Looks good to me and I also see no reason why this should not be done.
> diff --git dhcpleased.c dhcpleased.c
> index c8cc8e14d04..00bbffc1699 100644
> --- dhcpleased.c
> +++ dhcpleased.c
> @@ -785,16 +785,14 @@ configure_interface(struct imsg_configure_interface
> *imsg)
> if (ifa->ifa_addr->sa_family != AF_INET)
> continue;
>
> - addr.s_addr = ((struct sockaddr_in *)ifa->ifa_addr)
> - ->sin_addr.s_addr;
> - mask.s_addr = ((struct sockaddr_in *)ifa->ifa_netmask)
> - ->sin_addr.s_addr;
> + addr = ((struct sockaddr_in *)ifa->ifa_addr)->sin_addr;
> + mask = ((struct sockaddr_in *)ifa->ifa_netmask)->sin_addr;
>
> if (imsg->addr.s_addr == addr.s_addr) {
> if (imsg->mask.s_addr == mask.s_addr)
> found = 1;
> else {
> - req_sin_addr->sin_addr.s_addr = addr.s_addr;
> + req_sin_addr->sin_addr = addr;
> if (ioctl(ioctl_sock, SIOCDIFADDR, )
> == -1) {
> if (errno != EADDRNOTAVAIL)
> @@ -805,12 +803,12 @@ configure_interface(struct imsg_configure_interface
> *imsg)
> }
> }
>
> - req_sin_addr->sin_addr.s_addr = imsg->addr.s_addr;
> + req_sin_addr->sin_addr = imsg->addr;
> if (!found) {
> req_sin_mask = (struct sockaddr_in *)_mask;
> req_sin_mask->sin_family = AF_INET;
> req_sin_mask->sin_len = sizeof(*req_sin_mask);
> - req_sin_mask->sin_addr.s_addr = imsg->mask.s_addr;
> + req_sin_mask->sin_addr = imsg->mask;
> if (ioctl(ioctl_sock, SIOCAIFADDR, ) == -1)
> log_warn("SIOCAIFADDR");
> }
> @@ -931,7 +929,7 @@ deconfigure_interface(struct imsg_configure_interface
> *imsg)
> req_sin_addr->sin_family = AF_INET;
> req_sin_addr->sin_len = sizeof(*req_sin_addr);
>
> - req_sin_addr->sin_addr.s_addr = imsg->addr.s_addr;
> + req_sin_addr->sin_addr = imsg->addr;
> if (ioctl(ioctl_sock, SIOCDIFADDR, ) == -1) {
> if (errno != EADDRNOTAVAIL)
> log_warn("SIOCDIFADDR");
> @@ -948,7 +946,7 @@ configure_routes(uint8_t rtm_type, struct
> imsg_configure_interface *imsg)
> memset(, 0, sizeof(ifa));
> ifa.sin_family = AF_INET;
> ifa.sin_len = sizeof(ifa);
> - ifa.sin_addr.s_addr = imsg->addr.s_addr;
> + ifa.sin_addr = imsg->addr;
>
> memset(, 0, sizeof(dst));
> dst.sin_family = AF_INET;
> @@ -965,9 +963,9 @@ configure_routes(uint8_t rtm_type, struct
> imsg_configure_interface *imsg)
> addrnet = imsg->addr.s_addr & imsg->mask.s_addr;
>
> for (i = 0; i < imsg->routes_len; i++) {
> - dst.sin_addr.s_addr = imsg->routes[i].dst.s_addr;
> - mask.sin_addr.s_addr = imsg->routes[i].mask.s_addr;
> - gw.sin_addr.s_addr = imsg->routes[i].gw.s_addr;
> + dst.sin_addr = imsg->routes[i].dst;
> + mask.sin_addr = imsg->routes[i].mask;
> + gw.sin_addr = imsg->routes[i].gw;
>
> if (gw.sin_addr.s_addr == INADDR_ANY) {
> /* direct route */
> @@ -988,8 +986,7 @@ configure_routes(uint8_t rtm_type, struct
> imsg_configure_interface *imsg)
> configure_route(rtm_type, imsg->if_index,
> imsg->rdomain, , , , NULL,
> RTF_CLONING);
> - mask.sin_addr.s_addr =
> - imsg->routes[i].mask.s_addr;
> + mask.sin_addr = imsg->routes[i].mask;
> }
>
> if (gw.sin_addr.s_addr == ifa.sin_addr.s_addr) {
> diff --git engine.c engine.c
> index 09867c595b4..60f81d6f3c8 100644
> --- engine.c
> +++ engine.c
> @@ -570,10 +570,10 @@ send_interface_info(struct dhcpleased_iface *iface,
> pid_t pid)
> strlcpy(cei.state, if_state_name[iface->state], sizeof(cei.state));
> memcpy(_time, >request_time,
> sizeof(cei.request_time));
> - cei.server_identifier.s_addr = iface->server_identifier.s_addr;
> - cei.dhcp_server.s_addr = iface->dhcp_server.s_addr;
> - cei.requested_ip.s_addr = iface->requested_ip.s_addr;
> - cei.mask.s_addr = iface->mask.s_addr;
> + cei.server_identifier = iface->server_identifier;
> + cei.dhcp_server = iface->dhcp_server;
> + cei.requested_ip = iface->requested_ip;
> + cei.mask = iface->mask;
> cei.routes_len = iface->routes_len;
>
Re: dhcpleased(8): network byte order for xid
On Mon, Dec 13, 2021 at 11:27:20AM +0100, Florian Obser wrote:
> Treat xid as a uint32_t in network byte order on the wire.
>
> Internally this doesn't matter since we only care about equality.
> This makes logging output comparable to tcpdump(8).
>
> Pointed out by joel@
>
> OK?
OK claudio@
> diff --git engine.c engine.c
> index 60f81d6f3c8..4fbdf2f16ef 100644
> --- engine.c
> +++ engine.c
> @@ -848,7 +848,7 @@ parse_dhcp(struct dhcpleased_iface *iface, struct
> imsg_dhcp *dhcp)
> return;
> }
>
> - if (dhcp_hdr->xid != iface->xid)
> + if (ntohl(dhcp_hdr->xid) != iface->xid)
> return; /* silently ignore wrong xid */
>
> if (rem < sizeof(cookie))
> @@ -1770,7 +1770,7 @@ log_dhcp_hdr(struct dhcp_hdr *dhcp_hdr)
> "Unknown", dhcp_hdr->htype);
> log_debug("dhcp_hdr hlen: %d", dhcp_hdr->hlen);
> log_debug("dhcp_hdr hops: %d", dhcp_hdr->hops);
> - log_debug("dhcp_hdr xid: 0x%x", dhcp_hdr->xid);
> + log_debug("dhcp_hdr xid: 0x%x", ntohl(dhcp_hdr->xid));
> log_debug("dhcp_hdr secs: %u", dhcp_hdr->secs);
> log_debug("dhcp_hdr flags: 0x%x", dhcp_hdr->flags);
> log_debug("dhcp_hdr ciaddr: %s", inet_ntop(AF_INET, _hdr->ciaddr,
> diff --git frontend.c frontend.c
> index 53b61cafb77..ab644285451 100644
> --- frontend.c
> +++ frontend.c
> @@ -935,7 +935,7 @@ build_packet(uint8_t message_type, char *if_name,
> uint32_t xid,
> hdr->htype = HTYPE_ETHER;
> hdr->hlen = 6;
> hdr->hops = 0;
> - hdr->xid = xid;
> + hdr->xid = htonl(xid);
> hdr->secs = 0;
> hdr->ciaddr = *ciaddr;
> memcpy(hdr->chaddr, hw_address, sizeof(*hw_address));
>
> --
> I'm not entirely sure you are real.
>
--
:wq Claudio
dhcpleased(8): network byte order for xid
Treat xid as a uint32_t in network byte order on the wire.
Internally this doesn't matter since we only care about equality.
This makes logging output comparable to tcpdump(8).
Pointed out by joel@
OK?
diff --git engine.c engine.c
index 60f81d6f3c8..4fbdf2f16ef 100644
--- engine.c
+++ engine.c
@@ -848,7 +848,7 @@ parse_dhcp(struct dhcpleased_iface *iface, struct imsg_dhcp
*dhcp)
return;
}
- if (dhcp_hdr->xid != iface->xid)
+ if (ntohl(dhcp_hdr->xid) != iface->xid)
return; /* silently ignore wrong xid */
if (rem < sizeof(cookie))
@@ -1770,7 +1770,7 @@ log_dhcp_hdr(struct dhcp_hdr *dhcp_hdr)
"Unknown", dhcp_hdr->htype);
log_debug("dhcp_hdr hlen: %d", dhcp_hdr->hlen);
log_debug("dhcp_hdr hops: %d", dhcp_hdr->hops);
- log_debug("dhcp_hdr xid: 0x%x", dhcp_hdr->xid);
+ log_debug("dhcp_hdr xid: 0x%x", ntohl(dhcp_hdr->xid));
log_debug("dhcp_hdr secs: %u", dhcp_hdr->secs);
log_debug("dhcp_hdr flags: 0x%x", dhcp_hdr->flags);
log_debug("dhcp_hdr ciaddr: %s", inet_ntop(AF_INET, _hdr->ciaddr,
diff --git frontend.c frontend.c
index 53b61cafb77..ab644285451 100644
--- frontend.c
+++ frontend.c
@@ -935,7 +935,7 @@ build_packet(uint8_t message_type, char *if_name, uint32_t
xid,
hdr->htype = HTYPE_ETHER;
hdr->hlen = 6;
hdr->hops = 0;
- hdr->xid = xid;
+ hdr->xid = htonl(xid);
hdr->secs = 0;
hdr->ciaddr = *ciaddr;
memcpy(hdr->chaddr, hw_address, sizeof(*hw_address));
--
I'm not entirely sure you are real.
dhcpleased(8): use struct assignment
Replace struct member assignment with struct assignment to make the code
more compact. No binary change (on amd64).
OK? Or is there a reason not to do this?
diff --git dhcpleased.c dhcpleased.c
index c8cc8e14d04..00bbffc1699 100644
--- dhcpleased.c
+++ dhcpleased.c
@@ -785,16 +785,14 @@ configure_interface(struct imsg_configure_interface *imsg)
if (ifa->ifa_addr->sa_family != AF_INET)
continue;
- addr.s_addr = ((struct sockaddr_in *)ifa->ifa_addr)
- ->sin_addr.s_addr;
- mask.s_addr = ((struct sockaddr_in *)ifa->ifa_netmask)
- ->sin_addr.s_addr;
+ addr = ((struct sockaddr_in *)ifa->ifa_addr)->sin_addr;
+ mask = ((struct sockaddr_in *)ifa->ifa_netmask)->sin_addr;
if (imsg->addr.s_addr == addr.s_addr) {
if (imsg->mask.s_addr == mask.s_addr)
found = 1;
else {
- req_sin_addr->sin_addr.s_addr = addr.s_addr;
+ req_sin_addr->sin_addr = addr;
if (ioctl(ioctl_sock, SIOCDIFADDR, )
== -1) {
if (errno != EADDRNOTAVAIL)
@@ -805,12 +803,12 @@ configure_interface(struct imsg_configure_interface *imsg)
}
}
- req_sin_addr->sin_addr.s_addr = imsg->addr.s_addr;
+ req_sin_addr->sin_addr = imsg->addr;
if (!found) {
req_sin_mask = (struct sockaddr_in *)_mask;
req_sin_mask->sin_family = AF_INET;
req_sin_mask->sin_len = sizeof(*req_sin_mask);
- req_sin_mask->sin_addr.s_addr = imsg->mask.s_addr;
+ req_sin_mask->sin_addr = imsg->mask;
if (ioctl(ioctl_sock, SIOCAIFADDR, ) == -1)
log_warn("SIOCAIFADDR");
}
@@ -931,7 +929,7 @@ deconfigure_interface(struct imsg_configure_interface *imsg)
req_sin_addr->sin_family = AF_INET;
req_sin_addr->sin_len = sizeof(*req_sin_addr);
- req_sin_addr->sin_addr.s_addr = imsg->addr.s_addr;
+ req_sin_addr->sin_addr = imsg->addr;
if (ioctl(ioctl_sock, SIOCDIFADDR, ) == -1) {
if (errno != EADDRNOTAVAIL)
log_warn("SIOCDIFADDR");
@@ -948,7 +946,7 @@ configure_routes(uint8_t rtm_type, struct
imsg_configure_interface *imsg)
memset(, 0, sizeof(ifa));
ifa.sin_family = AF_INET;
ifa.sin_len = sizeof(ifa);
- ifa.sin_addr.s_addr = imsg->addr.s_addr;
+ ifa.sin_addr = imsg->addr;
memset(, 0, sizeof(dst));
dst.sin_family = AF_INET;
@@ -965,9 +963,9 @@ configure_routes(uint8_t rtm_type, struct
imsg_configure_interface *imsg)
addrnet = imsg->addr.s_addr & imsg->mask.s_addr;
for (i = 0; i < imsg->routes_len; i++) {
- dst.sin_addr.s_addr = imsg->routes[i].dst.s_addr;
- mask.sin_addr.s_addr = imsg->routes[i].mask.s_addr;
- gw.sin_addr.s_addr = imsg->routes[i].gw.s_addr;
+ dst.sin_addr = imsg->routes[i].dst;
+ mask.sin_addr = imsg->routes[i].mask;
+ gw.sin_addr = imsg->routes[i].gw;
if (gw.sin_addr.s_addr == INADDR_ANY) {
/* direct route */
@@ -988,8 +986,7 @@ configure_routes(uint8_t rtm_type, struct
imsg_configure_interface *imsg)
configure_route(rtm_type, imsg->if_index,
imsg->rdomain, , , , NULL,
RTF_CLONING);
- mask.sin_addr.s_addr =
- imsg->routes[i].mask.s_addr;
+ mask.sin_addr = imsg->routes[i].mask;
}
if (gw.sin_addr.s_addr == ifa.sin_addr.s_addr) {
diff --git engine.c engine.c
index 09867c595b4..60f81d6f3c8 100644
--- engine.c
+++ engine.c
@@ -570,10 +570,10 @@ send_interface_info(struct dhcpleased_iface *iface, pid_t
pid)
strlcpy(cei.state, if_state_name[iface->state], sizeof(cei.state));
memcpy(_time, >request_time,
sizeof(cei.request_time));
- cei.server_identifier.s_addr = iface->server_identifier.s_addr;
- cei.dhcp_server.s_addr = iface->dhcp_server.s_addr;
- cei.requested_ip.s_addr = iface->requested_ip.s_addr;
- cei.mask.s_addr = iface->mask.s_addr;
+ cei.server_identifier = iface->server_identifier;
+ cei.dhcp_server = iface->dhcp_server;
+ cei.requested_ip = iface->requested_ip;
+ cei.mask = iface->mask;
cei.routes_len = iface->routes_len;
memcpy(cei.routes, iface->routes, sizeof(cei.routes));
memcpy(cei.nameservers, iface->nameservers, sizeof(cei.nameservers));
@@ -1169,9 +1169,9 @@ parse_dhcp(struct dhcpleased_iface *iface,
Re: cat(1): always use a 64K buffer
On Mon, Dec 13, 2021 at 02:52:50AM -0600, Scott Cheloha wrote: > > On Dec 13, 2021, at 01:13, Otto Moerbeek wrote: > > > > On Sun, Dec 12, 2021 at 07:15:51PM -0600, Scott Cheloha wrote: > > > >> cat(1) sizes its I/O buffer according to the st_blksize of the first > >> file it processes. We don't do this very often in the tree. I'm not > >> sure if we should trust st_blksize. > >> > >> It would be simpler to just choose a value that works in practice and > >> always use it. > >> > >> 64K works well. We settled on that for tee(1) recently. > > > > Why are you also change the allocation to be per-file? You might as > > well go for a static buffer if you make it fixed size. > > Ottomalloc does canary checks when free(3) > is called if malloc_options is set up for it. > I always thought that was useful when > auditing my code. In this case (large allocation an exact multiple of the page size) any overflow will be caught by the MMU, independent of malloc options. So no big gain here to be gained from calling free. I'd say keep the original one time malloc call. -Otto
Re: DNS in acme-client
Change of plans...
I went throught the code and it seems to handle multiple addresses
correct through all the calls / processes.
(It is beyond me why it passes IP addresses as strings around though.)
OK florian
On 2021-12-13 01:56 +01, Jeremie Courreges-Anglas wrote:
> On Sun, Dec 12 2021, Florian Obser wrote:
>> On 12 December 2021 21:19:21 CET, Jeremie Courreges-Anglas
>> wrote:
>>>
>>>dnsproc.c only returns a single address even if the code pretends to
>>>support multiple addresses. This leads to weird behavior in edge cases,
>>>as experienced by a user on IRC.
>>>
>>>Take a machine with both IPv4 and IPv6 addresses configured, but no
>>>IPv4 default route (on purpose). Since there is at least one IPv4
>>>address different from 127.0.0.1, AI_ADDRCONFIG doesn't filter out
>>>A records. Let's encrypt ACME service is dual stacked but the first and
>>>only address returned by dnsproc.c is always IPv4 with our "family inet
>>>inet6". In this situation acme-client can't connect over IPv4 and errors
>>>out even though there's a working IPv6 default route.
>>>
>>
>> Doctor, Doctor! When I do this, it hurts!
>
> Ah well, the user wasn't complaining, really. :)
> I just got curious because the behavior and the code seemed strange.
>
>>>I don't know much about ACME and its requirements / good practices for
>>
>> I can't think of a reason to not try all address families.
>
> Also I can't think of a reason not to try multiple addresses in the same
> address family.
>
>>>clients, but clearly acme-client doesn't behave like most of our
>>>programs which try to connect to all available addresses. This break
>>>statement has been there since import, but was it added on purpose?
>>>Input welcome.
>>>
>>>
>>
>> I probably won't be able to look at this for a week. I am very surprised
>> that this is the correct fix though.
>
> Well I was puzzled too, I stared perhaps 30 seconds trying to understand
> what this statement was doing there. Its presence defeats the point of
> the loop, variables, etc around it. To me this looks like a leftover
> from initial experiments. The initial copy in ntpd/config.c doesn't
> have this break statement.
>
>> I trust you checked that multiple IP addresses can be passed between
>> processes?
>
> I added some extra debug statements and could see that 2 adresses (v4
> and v6) were passed to netproc.c. I was able to reproduce the problem
> of the original reporter, and see that the diff fixed renewing
> a certificate.
>
>>>diff --git a/usr.sbin/acme-client/dnsproc.c b/usr.sbin/acme-client/dnsproc.c
>>>index 664ef8d9b8b..c4c612e521a 100644
>>>--- a/usr.sbin/acme-client/dnsproc.c
>>>+++ b/usr.sbin/acme-client/dnsproc.c
>>>@@ -102,7 +102,6 @@ host_dns(const char *s, struct addr *vec)
>>>
>>> dodbg("%s: DNS: %s", s, vec[vecsz].ip);
>>> vecsz++;
>>>-break;
>>> }
>>>
>>> freeaddrinfo(res0);
>>>
>>>
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
>
--
I'm not entirely sure you are real.
Re: cat(1): always use a 64K buffer
> On Dec 13, 2021, at 01:13, Otto Moerbeek wrote: > > On Sun, Dec 12, 2021 at 07:15:51PM -0600, Scott Cheloha wrote: > >> cat(1) sizes its I/O buffer according to the st_blksize of the first >> file it processes. We don't do this very often in the tree. I'm not >> sure if we should trust st_blksize. >> >> It would be simpler to just choose a value that works in practice and >> always use it. >> >> 64K works well. We settled on that for tee(1) recently. > > Why are you also change the allocation to be per-file? You might as > well go for a static buffer if you make it fixed size. Ottomalloc does canary checks when free(3) is called if malloc_options is set up for it. I always thought that was useful when auditing my code.
