use shared socket^Wnet lock in get* syscalls
so{,un}lock_shared() take the shared net lock for PF_INET and PF_INET6
while sticking to the exclusive rwlock elsewhere.
getsockopt(2), getsockname(2) and getpeername(2) (all UNLOCK) do not
write, so the exclusive net lock is overkill here.
Did I miss anything?
Feedback? OK?
diff --git a/sys/kern/uipc_syscalls.c b/sys/kern/uipc_syscalls.c
index ddb612a8043..ea5b07b71e7 100644
--- a/sys/kern/uipc_syscalls.c
+++ b/sys/kern/uipc_syscalls.c
@@ -1268,9 +1268,9 @@ sys_getsockopt(struct proc *p, void *v, register_t
*retval)
valsize = 0;
m = m_get(M_WAIT, MT_SOOPTS);
so = fp->f_data;
- solock(so);
+ solock_shared(so);
error = sogetopt(so, SCARG(uap, level), SCARG(uap, name), m);
- sounlock(so);
+ sounlock_shared(so);
if (error == 0 && SCARG(uap, val) && valsize && m != NULL) {
if (valsize > m->m_len)
valsize = m->m_len;
@@ -1320,9 +1320,9 @@ sys_getsockname(struct proc *p, void *v, register_t
*retval)
goto bad;
}
m = m_getclr(M_WAIT, MT_SONAME);
- solock(so);
+ solock_shared(so);
error = pru_sockaddr(so, m);
- sounlock(so);
+ sounlock_shared(so);
if (error)
goto bad;
error = copyaddrout(p, m, SCARG(uap, asa), len, SCARG(uap, alen));
@@ -1367,9 +1367,9 @@ sys_getpeername(struct proc *p, void *v, register_t
*retval)
if (error)
goto bad;
m = m_getclr(M_WAIT, MT_SONAME);
- solock(so);
+ solock_shared(so);
error = pru_peeraddr(so, m);
- sounlock(so);
+ sounlock_shared(so);
if (error)
goto bad;
error = copyaddrout(p, m, SCARG(uap, asa), len, SCARG(uap, alen));
Remove constant basereachable and retrans members from struct nd_ifinfo
Both are initalised with compile-time constants and never written to.
They are part of the Neighbour Discovery machinery and only surface
through the single-user SIOCGIFINFO_IN6:
$ ndp -i lo0
basereachable=30s0ms, reachable=39s, retrans=1s0ms
These values are read-only since 2017 sys/netinet6/nd6.c r1.217
usr.sbin/ndp/ndp.c r1.85
Remove knob and always do neighbor unreachable detection
Inline the macros (to keep meaningful names), shrink the per-interface
allocated struct nd_ifinfo to what is actually needed and inline
nd6_dad_starttimer()'s constant `msec' argument.
This makes it clear how SIOCGIFINFO_IN6 is less useful than it seems:
$ ./obj/ndp -i lo0
reachable=39s
Nothing else in base, incl. regress, uses SIOCGIFINFO_IN6 or `ndp -i'.
Feedback? Objection? OK?
(Sorry for the header/ABI churn, I could've cleaned this up earlier...
if I had seen this right away between all the other dead code.)
diff --git a/sys/netinet6/nd6.c b/sys/netinet6/nd6.c
index 10753026d4b..0a1d5fb24c7 100644
--- a/sys/netinet6/nd6.c
+++ b/sys/netinet6/nd6.c
@@ -124,9 +124,7 @@ nd6_ifattach(struct ifnet *ifp)
nd = malloc(sizeof(*nd), M_IP6NDP, M_WAITOK | M_ZERO);
- nd->basereachable = REACHABLE_TIME;
- nd->reachable = ND_COMPUTE_RTIME(nd->basereachable);
- nd->retrans = RETRANS_TIMER;
+ nd->reachable = ND_COMPUTE_RTIME(REACHABLE_TIME);
ifp->if_nd = nd;
}
@@ -349,7 +347,7 @@ nd6_llinfo_timer(struct rtentry *rt)
case ND6_LLINFO_INCOMPLETE:
if (ln->ln_asked < nd6_mmaxtries) {
ln->ln_asked++;
- nd6_llinfo_settimer(ln, ifp->if_nd->retrans / 1000);
+ nd6_llinfo_settimer(ln, RETRANS_TIMER / 1000);
nd6_ns_output(ifp, NULL, >sin6_addr, ln, 0);
} else {
struct mbuf *m = ln->ln_hold;
@@ -396,13 +394,13 @@ nd6_llinfo_timer(struct rtentry *rt)
/* We need NUD */
ln->ln_asked = 1;
ln->ln_state = ND6_LLINFO_PROBE;
- nd6_llinfo_settimer(ln, ifp->if_nd->retrans / 1000);
+ nd6_llinfo_settimer(ln, RETRANS_TIMER / 1000);
nd6_ns_output(ifp, >sin6_addr, >sin6_addr, ln, 0);
break;
case ND6_LLINFO_PROBE:
if (ln->ln_asked < nd6_umaxtries) {
ln->ln_asked++;
- nd6_llinfo_settimer(ln, ifp->if_nd->retrans / 1000);
+ nd6_llinfo_settimer(ln, RETRANS_TIMER / 1000);
nd6_ns_output(ifp, >sin6_addr,
>sin6_addr, ln, 0);
} else {
@@ -1281,8 +1279,7 @@ nd6_slowtimo(void *ignored_arg)
TAILQ_FOREACH(ifp, , if_list) {
nd6if = ifp->if_nd;
- if (nd6if->basereachable && /* already initialized */
- (nd6if->recalctm -= ND6_SLOWTIMER_INTERVAL) <= 0) {
+ if ((nd6if->recalctm -= ND6_SLOWTIMER_INTERVAL) <= 0) {
/*
* Since reachable time rarely changes by router
* advertisements, we SHOULD insure that a new random
@@ -1290,7 +1287,7 @@ nd6_slowtimo(void *ignored_arg)
* (RFC 2461, 6.3.4)
*/
nd6if->recalctm = ND6_RECALC_REACHTM_INTERVAL;
- nd6if->reachable =
ND_COMPUTE_RTIME(nd6if->basereachable);
+ nd6if->reachable = ND_COMPUTE_RTIME(REACHABLE_TIME);
}
}
NET_UNLOCK();
@@ -1399,7 +1396,7 @@ nd6_resolve(struct ifnet *ifp, struct rtentry *rt0,
struct mbuf *m,
*/
if (!ND6_LLINFO_PERMANENT(ln) && ln->ln_asked == 0) {
ln->ln_asked++;
- nd6_llinfo_settimer(ln, ifp->if_nd->retrans / 1000);
+ nd6_llinfo_settimer(ln, RETRANS_TIMER / 1000);
nd6_ns_output(ifp, NULL, (dst)->sin6_addr, ln, 0);
}
return (EAGAIN);
diff --git a/sys/netinet6/nd6.h b/sys/netinet6/nd6.h
index d55bab0027e..897147e5501 100644
--- a/sys/netinet6/nd6.h
+++ b/sys/netinet6/nd6.h
@@ -45,14 +45,11 @@
/*
* Locks used to protect struct members in this file:
- * I immutable after creation
* N net lock
*/
struct nd_ifinfo {
- u_int32_t basereachable;/* [I] BaseReachableTime */
u_int32_t reachable;/* [N] Reachable Time */
- u_int32_t retrans; /* [I] Retrans Timer */
int recalctm; /* [N] BaseReachable recalc timer */
};
diff --git a/sys/netinet6/nd6_nbr.c b/sys/netinet6/nd6_nbr.c
index e68ccbd4944..6046b90dcb9 100644
--- a/sys/netinet6/nd6_nbr.c
+++ b/sys/netinet6/nd6_nbr.c
@@ -76,7 +76,7 @@ struct dadq {
};
struct dadq *nd6_dad_find(struct ifaddr *);
-void nd6_dad_starttimer(struct dadq
Re: Improve rpki-client warnings on .mft files
On Mon, Nov 28, 2022 at 07:18:24PM +0100, Claudio Jeker wrote:
> On Mon, Nov 28, 2022 at 06:02:56PM +0100, Theo Buehler wrote:
> > On Mon, Nov 28, 2022 at 05:14:48PM +0100, Claudio Jeker wrote:
> > > On Mon, Nov 28, 2022 at 04:50:24PM +0100, Theo Buehler wrote:
> > > > On Mon, Nov 28, 2022 at 04:02:11PM +0100, Claudio Jeker wrote:
> > > > > Since a long time any problem that caused rpki-client to not load a
> > > > > manifest resulted in the non helpful:
> > > > > rpki-client:
> > > > > rpki.afrinic.net/repository/member_repository/F36505B2/0569917622D711ED862FD6E0F1222468/0nALpPtwFyntPHjkS8xt-VQrqLw.mft:
> > > > > no valid mft available
> > > > >
> > > > > This hides in most cases the cause why the manifest verificatin
> > > > > failed.
> > > > > The following diff exposes the error from valid_x509() and with that
> > > > > some
> > > > > manifest errors change to e.g.:
> > > > > rpki-client:
> > > > > parent.rov.koenvanhove.nl/repo/KoenvanHove/1/C1F7424F77FBF669FB750C6DC7B649C5969DCD55.mft:
> > > > > CRL has expired
> > > > >
> > > > > or if the CRL is missing
> > > > >
> > > > > rpki-client:
> > > > > repo.pedjoeang.group/repo/localname/0/EF79F8E55B6A248EF9CF4CE70FF60C017BF1A3B0.mft:
> > > > > unable to get certificate CRL
> > > > >
> > > > > If the certificate is pointing to a manifest that does not exist the
> > > > > old
> > > > > "no valid mft available" is shown.
> > > > >
> > > > > I tried to keep original behaviour as much as possible but I think
> > > > > filemode can be further improved. And maybe we can remove verbose from
> > > > > other warnings as well.
> > > >
> > > > I like this a lot.
> > > >
> > > > I was wondering if valid_x509() should have a const char **errstr
> > > > instead of an int * as last argument. valid_x509() could do the
> > > > conversion from error code to error string itself. This way we don't
> > > > have to sprinkle X509_verify_cert_error_string() calls everywhere.
> > > >
> > > > Or we could introduce a warn_invalid_x509(const char *str, int err) that
> > > > does the conversion from err using X509_verify_cert_error_string().
> > > >
> > > > One downside of X509_verify_cert_error_string() is that it isn't thread
> > > > safe since it might return a pointer to a static buffer -- it should
> > > > not, but who can be sure...
> > >
> > > Tough call. It may also help other code paths to do the same. But in many
> > > cases a dynamic buffer would be needed.
> > >
> > > Not sure if it makes sense to introduce warn_invalid_x509(). What I don't
> > > like is the verbose check before the warning. I wonder if we still need
> > > that. My last run has 11 failed roas and 51 failed mfts. The mft errors
> > > already show up. Shouldn't the roa errors be shown as well?
> >
> > They should. Unless I'm completely confusing myself, this is a bug in
> > the diff and all the added if (verbose > 1) should be dropped.
> >
> > If the last argument (nowarn) of valid_x509() was 0 (everywhere except
> > in proc_parser_pre()), valid_x509() would print the error independently
> > of verbose. verbose > 1 would force printing the warning also for mfts,
> > but there it would be drowned in the other noise.
> >
> > - if (!valid_x509(file, ctx, x509, a, crl, 0)) {
> >
> > ...
> >
> > - if (!nowarn || verbose > 1)
> > - warnx("%s: %s", file, X509_verify_cert_error_string(c));
> >
>
> Indeed. Better diff below.
> Still thinking about the idea with the const char **.
One of the main reasons for suggesting it was the amount of awkward line
wrapping. There's now much less of this. We can easily switch if you
should change your mind.
ok tb
> It seems OpenSSL made X509_verify_cert_error_string() thread safe by only
> returning static strings. So maybe we can ignore the issue and just assume
> all is well.
>
> --
> :wq Claudio
>
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.161
> diff -u -p -r1.161 extern.h
> --- extern.h 26 Nov 2022 12:02:36 - 1.161
> +++ extern.h 28 Nov 2022 10:42:11 -
> @@ -629,7 +629,7 @@ intvalid_filename(const char *, size_
> int valid_uri(const char *, size_t, const char *);
> int valid_origin(const char *, const char *);
> int valid_x509(char *, X509_STORE_CTX *, X509 *, struct auth *,
> - struct crl *, int);
> + struct crl *, int *);
> int valid_rsc(const char *, struct cert *, struct rsc *);
> int valid_econtent_version(const char *, const ASN1_INTEGER *);
> int valid_aspa(const char *, struct cert *, struct aspa *);
> Index: filemode.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 filemode.c
> --- filemode.c26 Nov 2022 12:02:37 -
Re: vmd: fix booting 7.2 ramdisks with >= 4G mem
> Date: Mon, 28 Nov 2022 18:26:47 +0100
> From: Claudio Jeker
>
> On Mon, Nov 28, 2022 at 11:32:32AM -0500, Dave Voutila wrote:
> > tech@ et. al.,
> >
> > When kettenis@ introduced a newer version of BOOTARG_CONSDEV to add
> > additional params for the AMD Ryzen V1000 family, vmd's code that
> > configures bootargs to support direct booting a ramdisk kernel didn't
> > adjust with it.
> >
> > Mischa Peters found this and shared a simple reproducer on 7.2 and
> > -current:
> >
> > # vmctl start -c -b /bsd.rd -m 4G test
> >
> > Where /bsd.rd is a 7.2 or -current ramdisk kernel.
> >
> > Interestingly, this is only seen when using 4G (or more) memory for the
> > guest. I think it's just a happy coincedence it works < 4G because of
> > the resulting BOOTARG_MEMMAP sizing things so the BOOTARG_CONSDEV works.
> >
> > Diff below fixes the issue by simply zero'ing the BOOTARG_CONSDEV
> > structure before assigning to members.
> >
> > While here, I also cleaned up some things like using literal values that
> > could be more descriptive boot arg names and also made the arithmetic
> > explicitly use the same type (uint32_t) throughout instead of mixing it
> > with int.
> >
> > ok?
>
> OK claudio@
ok kettenis@ too FWIW
> PS: diff did not apply, a space got stripped below diff should apply
> --
> :wq Claudio
>
> Index: loadfile_elf.c
> ===
> RCS file: /cvs/src/usr.sbin/vmd/loadfile_elf.c,v
> retrieving revision 1.42
> diff -u -p -r1.42 loadfile_elf.c
> --- loadfile_elf.c28 Jan 2022 06:33:27 - 1.42
> +++ loadfile_elf.c28 Nov 2022 17:07:20 -
> @@ -382,9 +382,10 @@ create_bios_memmap(struct vm_create_para
> * Parameters:
> * memmap: the BIOS memory map
> * n: number of entries in memmap
> + * bootmac: optional PXE boot MAC address
> *
> * Return values:
> - * The size of the bootargs
> + * The size of the bootargs in bytes
> */
> static uint32_t
> push_bootargs(bios_memmap_t *memmap, size_t n, bios_bootmac_t *bootmac)
> @@ -393,40 +394,41 @@ push_bootargs(bios_memmap_t *memmap, siz
> bios_consdev_t consdev;
> uint32_t ba[1024];
>
> - memmap_sz = 3 * sizeof(int) + n * sizeof(bios_memmap_t);
> - ba[0] = 0x0;/* memory map */
> + memmap_sz = 3 * sizeof(uint32_t) + n * sizeof(bios_memmap_t);
> + ba[0] = BOOTARG_MEMMAP;
> ba[1] = memmap_sz;
> - ba[2] = memmap_sz; /* next */
> + ba[2] = memmap_sz;
> memcpy([3], memmap, n * sizeof(bios_memmap_t));
> - i = memmap_sz / sizeof(int);
> + i = memmap_sz / sizeof(uint32_t);
>
> /* Serial console device, COM1 @ 0x3f8 */
> - consdev.consdev = makedev(8, 0);/* com1 @ 0x3f8 */
> + memset(, 0, sizeof(consdev));
> + consdev.consdev = makedev(8, 0);
> consdev.conspeed = 115200;
> consdev.consaddr = 0x3f8;
> - consdev.consfreq = 0;
>
> - consdev_sz = 3 * sizeof(int) + sizeof(bios_consdev_t);
> - ba[i] = 0x5; /* consdev */
> + consdev_sz = 3 * sizeof(uint32_t) + sizeof(bios_consdev_t);
> + ba[i] = BOOTARG_CONSDEV;
> ba[i + 1] = consdev_sz;
> ba[i + 2] = consdev_sz;
> memcpy([i + 3], , sizeof(bios_consdev_t));
> - i += consdev_sz / sizeof(int);
> + i += consdev_sz / sizeof(uint32_t);
>
> if (bootmac) {
> - bootmac_sz = 3 * sizeof(int) + (sizeof(bios_bootmac_t) + 3) &
> ~3;
> - ba[i] = 0x7; /* bootmac */
> + bootmac_sz = 3 * sizeof(uint32_t) +
> + (sizeof(bios_bootmac_t) + 3) & ~3;
> + ba[i] = BOOTARG_BOOTMAC;
> ba[i + 1] = bootmac_sz;
> ba[i + 2] = bootmac_sz;
> memcpy([i + 3], bootmac, sizeof(bios_bootmac_t));
> - i += bootmac_sz / sizeof(int);
> - }
> + i += bootmac_sz / sizeof(uint32_t);
> + }
>
> ba[i++] = 0x; /* BOOTARG_END */
>
> write_mem(BOOTARGS_PAGE, ba, PAGE_SIZE);
>
> - return (i * sizeof(int));
> + return (i * sizeof(uint32_t));
> }
>
> /*
>
>
Re: Improve rpki-client warnings on .mft files
On Mon, Nov 28, 2022 at 06:02:56PM +0100, Theo Buehler wrote:
> On Mon, Nov 28, 2022 at 05:14:48PM +0100, Claudio Jeker wrote:
> > On Mon, Nov 28, 2022 at 04:50:24PM +0100, Theo Buehler wrote:
> > > On Mon, Nov 28, 2022 at 04:02:11PM +0100, Claudio Jeker wrote:
> > > > Since a long time any problem that caused rpki-client to not load a
> > > > manifest resulted in the non helpful:
> > > > rpki-client:
> > > > rpki.afrinic.net/repository/member_repository/F36505B2/0569917622D711ED862FD6E0F1222468/0nALpPtwFyntPHjkS8xt-VQrqLw.mft:
> > > > no valid mft available
> > > >
> > > > This hides in most cases the cause why the manifest verificatin failed.
> > > > The following diff exposes the error from valid_x509() and with that
> > > > some
> > > > manifest errors change to e.g.:
> > > > rpki-client:
> > > > parent.rov.koenvanhove.nl/repo/KoenvanHove/1/C1F7424F77FBF669FB750C6DC7B649C5969DCD55.mft:
> > > > CRL has expired
> > > >
> > > > or if the CRL is missing
> > > >
> > > > rpki-client:
> > > > repo.pedjoeang.group/repo/localname/0/EF79F8E55B6A248EF9CF4CE70FF60C017BF1A3B0.mft:
> > > > unable to get certificate CRL
> > > >
> > > > If the certificate is pointing to a manifest that does not exist the old
> > > > "no valid mft available" is shown.
> > > >
> > > > I tried to keep original behaviour as much as possible but I think
> > > > filemode can be further improved. And maybe we can remove verbose from
> > > > other warnings as well.
> > >
> > > I like this a lot.
> > >
> > > I was wondering if valid_x509() should have a const char **errstr
> > > instead of an int * as last argument. valid_x509() could do the
> > > conversion from error code to error string itself. This way we don't
> > > have to sprinkle X509_verify_cert_error_string() calls everywhere.
> > >
> > > Or we could introduce a warn_invalid_x509(const char *str, int err) that
> > > does the conversion from err using X509_verify_cert_error_string().
> > >
> > > One downside of X509_verify_cert_error_string() is that it isn't thread
> > > safe since it might return a pointer to a static buffer -- it should
> > > not, but who can be sure...
> >
> > Tough call. It may also help other code paths to do the same. But in many
> > cases a dynamic buffer would be needed.
> >
> > Not sure if it makes sense to introduce warn_invalid_x509(). What I don't
> > like is the verbose check before the warning. I wonder if we still need
> > that. My last run has 11 failed roas and 51 failed mfts. The mft errors
> > already show up. Shouldn't the roa errors be shown as well?
>
> They should. Unless I'm completely confusing myself, this is a bug in
> the diff and all the added if (verbose > 1) should be dropped.
>
> If the last argument (nowarn) of valid_x509() was 0 (everywhere except
> in proc_parser_pre()), valid_x509() would print the error independently
> of verbose. verbose > 1 would force printing the warning also for mfts,
> but there it would be drowned in the other noise.
>
> - if (!valid_x509(file, ctx, x509, a, crl, 0)) {
>
> ...
>
> - if (!nowarn || verbose > 1)
> - warnx("%s: %s", file, X509_verify_cert_error_string(c));
>
Indeed. Better diff below.
Still thinking about the idea with the const char **.
It seems OpenSSL made X509_verify_cert_error_string() thread safe by only
returning static strings. So maybe we can ignore the issue and just assume
all is well.
--
:wq Claudio
Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.161
diff -u -p -r1.161 extern.h
--- extern.h26 Nov 2022 12:02:36 - 1.161
+++ extern.h28 Nov 2022 10:42:11 -
@@ -629,7 +629,7 @@ int valid_filename(const char *, size_
int valid_uri(const char *, size_t, const char *);
int valid_origin(const char *, const char *);
int valid_x509(char *, X509_STORE_CTX *, X509 *, struct auth *,
- struct crl *, int);
+ struct crl *, int *);
int valid_rsc(const char *, struct cert *, struct rsc *);
int valid_econtent_version(const char *, const ASN1_INTEGER *);
int valid_aspa(const char *, struct cert *, struct aspa *);
Index: filemode.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
retrieving revision 1.17
diff -u -p -r1.17 filemode.c
--- filemode.c 26 Nov 2022 12:02:37 - 1.17
+++ filemode.c 28 Nov 2022 18:04:57 -
@@ -141,7 +141,7 @@ parse_load_certchain(char *uri)
struct cert *cert;
struct crl *crl;
struct auth *a;
- int i;
+ int i, e;
for (i = 0; i < MAX_CERT_DEPTH; i++) {
filestack[i] = uri;
@@ -171,9 +171,11 @@ parse_load_certchain(char *uri)
uri = filestack[i];
crl = crl_get(, a);
- if
Re: vmd: fix booting 7.2 ramdisks with >= 4G mem
On Mon, Nov 28, 2022 at 11:32:32AM -0500, Dave Voutila wrote:
> tech@ et. al.,
>
> When kettenis@ introduced a newer version of BOOTARG_CONSDEV to add
> additional params for the AMD Ryzen V1000 family, vmd's code that
> configures bootargs to support direct booting a ramdisk kernel didn't
> adjust with it.
>
> Mischa Peters found this and shared a simple reproducer on 7.2 and
> -current:
>
> # vmctl start -c -b /bsd.rd -m 4G test
>
> Where /bsd.rd is a 7.2 or -current ramdisk kernel.
>
> Interestingly, this is only seen when using 4G (or more) memory for the
> guest. I think it's just a happy coincedence it works < 4G because of
> the resulting BOOTARG_MEMMAP sizing things so the BOOTARG_CONSDEV works.
>
> Diff below fixes the issue by simply zero'ing the BOOTARG_CONSDEV
> structure before assigning to members.
>
> While here, I also cleaned up some things like using literal values that
> could be more descriptive boot arg names and also made the arithmetic
> explicitly use the same type (uint32_t) throughout instead of mixing it
> with int.
>
> ok?
>
ok mlarkin
> -dv
>
> diff refs/heads/master refs/heads/vmd-ramdisk
> commit - 8cbcfb178c36f28f6fcb28289719a4f0547eabb4
> commit + 0be12dfaa063ded82837d3a6b2ce8df7ea7e1c2d
> blob - b367721e32b61892955bbf835b873034875c85ec
> blob + d560b8e8eb2cdd87a60c63e8ecb7fed56e5c60dc
> --- usr.sbin/vmd/loadfile_elf.c
> +++ usr.sbin/vmd/loadfile_elf.c
> @@ -382,9 +382,10 @@ create_bios_memmap(struct vm_create_params *vcp, bios_
> * Parameters:
> * memmap: the BIOS memory map
> * n: number of entries in memmap
> + * bootmac: optional PXE boot MAC address
> *
> * Return values:
> - * The size of the bootargs
> + * The size of the bootargs in bytes
> */
> static uint32_t
> push_bootargs(bios_memmap_t *memmap, size_t n, bios_bootmac_t *bootmac)
> @@ -393,40 +394,41 @@ push_bootargs(bios_memmap_t *memmap, size_t n, bios_bo
> bios_consdev_t consdev;
> uint32_t ba[1024];
>
> - memmap_sz = 3 * sizeof(int) + n * sizeof(bios_memmap_t);
> - ba[0] = 0x0;/* memory map */
> + memmap_sz = 3 * sizeof(uint32_t) + n * sizeof(bios_memmap_t);
> + ba[0] = BOOTARG_MEMMAP;
> ba[1] = memmap_sz;
> - ba[2] = memmap_sz; /* next */
> + ba[2] = memmap_sz;
> memcpy([3], memmap, n * sizeof(bios_memmap_t));
> - i = memmap_sz / sizeof(int);
> + i = memmap_sz / sizeof(uint32_t);
>
> /* Serial console device, COM1 @ 0x3f8 */
> - consdev.consdev = makedev(8, 0);/* com1 @ 0x3f8 */
> + memset(, 0, sizeof(consdev));
> + consdev.consdev = makedev(8, 0);
> consdev.conspeed = 115200;
> consdev.consaddr = 0x3f8;
> - consdev.consfreq = 0;
>
> - consdev_sz = 3 * sizeof(int) + sizeof(bios_consdev_t);
> - ba[i] = 0x5; /* consdev */
> + consdev_sz = 3 * sizeof(uint32_t) + sizeof(bios_consdev_t);
> + ba[i] = BOOTARG_CONSDEV;
> ba[i + 1] = consdev_sz;
> ba[i + 2] = consdev_sz;
> memcpy([i + 3], , sizeof(bios_consdev_t));
> - i += consdev_sz / sizeof(int);
> + i += consdev_sz / sizeof(uint32_t);
>
> if (bootmac) {
> - bootmac_sz = 3 * sizeof(int) + (sizeof(bios_bootmac_t) + 3) &
> ~3;
> - ba[i] = 0x7; /* bootmac */
> + bootmac_sz = 3 * sizeof(uint32_t) +
> + (sizeof(bios_bootmac_t) + 3) & ~3;
> + ba[i] = BOOTARG_BOOTMAC;
> ba[i + 1] = bootmac_sz;
> ba[i + 2] = bootmac_sz;
> memcpy([i + 3], bootmac, sizeof(bios_bootmac_t));
> - i += bootmac_sz / sizeof(int);
> + i += bootmac_sz / sizeof(uint32_t);
> }
>
> ba[i++] = 0x; /* BOOTARG_END */
>
> write_mem(BOOTARGS_PAGE, ba, PAGE_SIZE);
>
> - return (i * sizeof(int));
> + return (i * sizeof(uint32_t));
> }
>
> /*
Unlock getsockopt(2)
Subj.
At sockets layer we touch only per-socket data, which is solock()
protected().
At protocol layer, unix(4) and key management sockets have no
(*pr_ctloutput)() handlers. route_ctloutput() touches only per socket
data, which is solock() protected. inet{,6} globals are protected by
netlock, which is solock() backend for corresponding sockets.
Index: sys/kern/syscalls.master
===
RCS file: /cvs/src/sys/kern/syscalls.master,v
retrieving revision 1.236
diff -u -p -r1.236 syscalls.master
--- sys/kern/syscalls.master9 Nov 2022 10:26:28 - 1.236
+++ sys/kern/syscalls.master28 Nov 2022 17:29:03 -
@@ -249,7 +249,7 @@
struct timespec *timeout); }
117STD NOLOCK { int sys_sendmmsg(int s, struct mmsghdr *mmsg,\
unsigned int vlen, int flags); }
-118STD { int sys_getsockopt(int s, int level, int name, \
+118STD NOLOCK { int sys_getsockopt(int s, int level, int name, \
void *val, socklen_t *avalsize); }
119STD { int sys_thrkill(pid_t tid, int signum, void *tcb); }
120STD NOLOCK { ssize_t sys_readv(int fd, \
Re: vmd: fix booting 7.2 ramdisks with >= 4G mem
On Mon, Nov 28, 2022 at 11:32:32AM -0500, Dave Voutila wrote:
> tech@ et. al.,
>
> When kettenis@ introduced a newer version of BOOTARG_CONSDEV to add
> additional params for the AMD Ryzen V1000 family, vmd's code that
> configures bootargs to support direct booting a ramdisk kernel didn't
> adjust with it.
>
> Mischa Peters found this and shared a simple reproducer on 7.2 and
> -current:
>
> # vmctl start -c -b /bsd.rd -m 4G test
>
> Where /bsd.rd is a 7.2 or -current ramdisk kernel.
>
> Interestingly, this is only seen when using 4G (or more) memory for the
> guest. I think it's just a happy coincedence it works < 4G because of
> the resulting BOOTARG_MEMMAP sizing things so the BOOTARG_CONSDEV works.
>
> Diff below fixes the issue by simply zero'ing the BOOTARG_CONSDEV
> structure before assigning to members.
>
> While here, I also cleaned up some things like using literal values that
> could be more descriptive boot arg names and also made the arithmetic
> explicitly use the same type (uint32_t) throughout instead of mixing it
> with int.
>
> ok?
OK claudio@
PS: diff did not apply, a space got stripped below diff should apply
--
:wq Claudio
Index: loadfile_elf.c
===
RCS file: /cvs/src/usr.sbin/vmd/loadfile_elf.c,v
retrieving revision 1.42
diff -u -p -r1.42 loadfile_elf.c
--- loadfile_elf.c 28 Jan 2022 06:33:27 - 1.42
+++ loadfile_elf.c 28 Nov 2022 17:07:20 -
@@ -382,9 +382,10 @@ create_bios_memmap(struct vm_create_para
* Parameters:
* memmap: the BIOS memory map
* n: number of entries in memmap
+ * bootmac: optional PXE boot MAC address
*
* Return values:
- * The size of the bootargs
+ * The size of the bootargs in bytes
*/
static uint32_t
push_bootargs(bios_memmap_t *memmap, size_t n, bios_bootmac_t *bootmac)
@@ -393,40 +394,41 @@ push_bootargs(bios_memmap_t *memmap, siz
bios_consdev_t consdev;
uint32_t ba[1024];
- memmap_sz = 3 * sizeof(int) + n * sizeof(bios_memmap_t);
- ba[0] = 0x0;/* memory map */
+ memmap_sz = 3 * sizeof(uint32_t) + n * sizeof(bios_memmap_t);
+ ba[0] = BOOTARG_MEMMAP;
ba[1] = memmap_sz;
- ba[2] = memmap_sz; /* next */
+ ba[2] = memmap_sz;
memcpy([3], memmap, n * sizeof(bios_memmap_t));
- i = memmap_sz / sizeof(int);
+ i = memmap_sz / sizeof(uint32_t);
/* Serial console device, COM1 @ 0x3f8 */
- consdev.consdev = makedev(8, 0);/* com1 @ 0x3f8 */
+ memset(, 0, sizeof(consdev));
+ consdev.consdev = makedev(8, 0);
consdev.conspeed = 115200;
consdev.consaddr = 0x3f8;
- consdev.consfreq = 0;
- consdev_sz = 3 * sizeof(int) + sizeof(bios_consdev_t);
- ba[i] = 0x5; /* consdev */
+ consdev_sz = 3 * sizeof(uint32_t) + sizeof(bios_consdev_t);
+ ba[i] = BOOTARG_CONSDEV;
ba[i + 1] = consdev_sz;
ba[i + 2] = consdev_sz;
memcpy([i + 3], , sizeof(bios_consdev_t));
- i += consdev_sz / sizeof(int);
+ i += consdev_sz / sizeof(uint32_t);
if (bootmac) {
- bootmac_sz = 3 * sizeof(int) + (sizeof(bios_bootmac_t) + 3) &
~3;
- ba[i] = 0x7; /* bootmac */
+ bootmac_sz = 3 * sizeof(uint32_t) +
+ (sizeof(bios_bootmac_t) + 3) & ~3;
+ ba[i] = BOOTARG_BOOTMAC;
ba[i + 1] = bootmac_sz;
ba[i + 2] = bootmac_sz;
memcpy([i + 3], bootmac, sizeof(bios_bootmac_t));
- i += bootmac_sz / sizeof(int);
- }
+ i += bootmac_sz / sizeof(uint32_t);
+ }
ba[i++] = 0x; /* BOOTARG_END */
write_mem(BOOTARGS_PAGE, ba, PAGE_SIZE);
- return (i * sizeof(int));
+ return (i * sizeof(uint32_t));
}
/*
Re: Improve rpki-client warnings on .mft files
On Mon, Nov 28, 2022 at 05:14:48PM +0100, Claudio Jeker wrote:
> On Mon, Nov 28, 2022 at 04:50:24PM +0100, Theo Buehler wrote:
> > On Mon, Nov 28, 2022 at 04:02:11PM +0100, Claudio Jeker wrote:
> > > Since a long time any problem that caused rpki-client to not load a
> > > manifest resulted in the non helpful:
> > > rpki-client:
> > > rpki.afrinic.net/repository/member_repository/F36505B2/0569917622D711ED862FD6E0F1222468/0nALpPtwFyntPHjkS8xt-VQrqLw.mft:
> > > no valid mft available
> > >
> > > This hides in most cases the cause why the manifest verificatin failed.
> > > The following diff exposes the error from valid_x509() and with that some
> > > manifest errors change to e.g.:
> > > rpki-client:
> > > parent.rov.koenvanhove.nl/repo/KoenvanHove/1/C1F7424F77FBF669FB750C6DC7B649C5969DCD55.mft:
> > > CRL has expired
> > >
> > > or if the CRL is missing
> > >
> > > rpki-client:
> > > repo.pedjoeang.group/repo/localname/0/EF79F8E55B6A248EF9CF4CE70FF60C017BF1A3B0.mft:
> > > unable to get certificate CRL
> > >
> > > If the certificate is pointing to a manifest that does not exist the old
> > > "no valid mft available" is shown.
> > >
> > > I tried to keep original behaviour as much as possible but I think
> > > filemode can be further improved. And maybe we can remove verbose from
> > > other warnings as well.
> >
> > I like this a lot.
> >
> > I was wondering if valid_x509() should have a const char **errstr
> > instead of an int * as last argument. valid_x509() could do the
> > conversion from error code to error string itself. This way we don't
> > have to sprinkle X509_verify_cert_error_string() calls everywhere.
> >
> > Or we could introduce a warn_invalid_x509(const char *str, int err) that
> > does the conversion from err using X509_verify_cert_error_string().
> >
> > One downside of X509_verify_cert_error_string() is that it isn't thread
> > safe since it might return a pointer to a static buffer -- it should
> > not, but who can be sure...
>
> Tough call. It may also help other code paths to do the same. But in many
> cases a dynamic buffer would be needed.
>
> Not sure if it makes sense to introduce warn_invalid_x509(). What I don't
> like is the verbose check before the warning. I wonder if we still need
> that. My last run has 11 failed roas and 51 failed mfts. The mft errors
> already show up. Shouldn't the roa errors be shown as well?
They should. Unless I'm completely confusing myself, this is a bug in
the diff and all the added if (verbose > 1) should be dropped.
If the last argument (nowarn) of valid_x509() was 0 (everywhere except
in proc_parser_pre()), valid_x509() would print the error independently
of verbose. verbose > 1 would force printing the warning also for mfts,
but there it would be drowned in the other noise.
- if (!valid_x509(file, ctx, x509, a, crl, 0)) {
...
- if (!nowarn || verbose > 1)
- warnx("%s: %s", file, X509_verify_cert_error_string(c));
>
> > >
> > > --
> > > :wq Claudio
> > >
> > > Index: extern.h
> > > ===
> > > RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> > > retrieving revision 1.161
> > > diff -u -p -r1.161 extern.h
> > > --- extern.h 26 Nov 2022 12:02:36 - 1.161
> > > +++ extern.h 28 Nov 2022 10:42:11 -
> > > @@ -629,7 +629,7 @@ intvalid_filename(const char *, size_
> > > int valid_uri(const char *, size_t, const char *);
> > > int valid_origin(const char *, const char *);
> > > int valid_x509(char *, X509_STORE_CTX *, X509 *, struct
> > > auth *,
> > > - struct crl *, int);
> > > + struct crl *, int *);
> > > int valid_rsc(const char *, struct cert *, struct rsc *);
> > > int valid_econtent_version(const char *, const
> > > ASN1_INTEGER *);
> > > int valid_aspa(const char *, struct cert *, struct aspa *);
> > > Index: filemode.c
> > > ===
> > > RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
> > > retrieving revision 1.17
> > > diff -u -p -r1.17 filemode.c
> > > --- filemode.c26 Nov 2022 12:02:37 - 1.17
> > > +++ filemode.c28 Nov 2022 11:08:31 -
> > > @@ -141,7 +141,7 @@ parse_load_certchain(char *uri)
> > > struct cert *cert;
> > > struct crl *crl;
> > > struct auth *a;
> > > - int i;
> > > + int i, e;
> > >
> > > for (i = 0; i < MAX_CERT_DEPTH; i++) {
> > > filestack[i] = uri;
> > > @@ -171,9 +171,13 @@ parse_load_certchain(char *uri)
> > > uri = filestack[i];
> > >
> > > crl = crl_get(, a);
> > > - if (!valid_x509(uri, ctx, cert->x509, a, crl, 0) ||
> > > - !valid_cert(uri, a, cert))
> > > + if (!valid_x509(uri, ctx, cert->x509, a, crl, ) ||
> > > + !valid_cert(uri, a, cert)) {
> > > +
vmd: fix booting 7.2 ramdisks with >= 4G mem
tech@ et. al.,
When kettenis@ introduced a newer version of BOOTARG_CONSDEV to add
additional params for the AMD Ryzen V1000 family, vmd's code that
configures bootargs to support direct booting a ramdisk kernel didn't
adjust with it.
Mischa Peters found this and shared a simple reproducer on 7.2 and
-current:
# vmctl start -c -b /bsd.rd -m 4G test
Where /bsd.rd is a 7.2 or -current ramdisk kernel.
Interestingly, this is only seen when using 4G (or more) memory for the
guest. I think it's just a happy coincedence it works < 4G because of
the resulting BOOTARG_MEMMAP sizing things so the BOOTARG_CONSDEV works.
Diff below fixes the issue by simply zero'ing the BOOTARG_CONSDEV
structure before assigning to members.
While here, I also cleaned up some things like using literal values that
could be more descriptive boot arg names and also made the arithmetic
explicitly use the same type (uint32_t) throughout instead of mixing it
with int.
ok?
-dv
diff refs/heads/master refs/heads/vmd-ramdisk
commit - 8cbcfb178c36f28f6fcb28289719a4f0547eabb4
commit + 0be12dfaa063ded82837d3a6b2ce8df7ea7e1c2d
blob - b367721e32b61892955bbf835b873034875c85ec
blob + d560b8e8eb2cdd87a60c63e8ecb7fed56e5c60dc
--- usr.sbin/vmd/loadfile_elf.c
+++ usr.sbin/vmd/loadfile_elf.c
@@ -382,9 +382,10 @@ create_bios_memmap(struct vm_create_params *vcp, bios_
* Parameters:
* memmap: the BIOS memory map
* n: number of entries in memmap
+ * bootmac: optional PXE boot MAC address
*
* Return values:
- * The size of the bootargs
+ * The size of the bootargs in bytes
*/
static uint32_t
push_bootargs(bios_memmap_t *memmap, size_t n, bios_bootmac_t *bootmac)
@@ -393,40 +394,41 @@ push_bootargs(bios_memmap_t *memmap, size_t n, bios_bo
bios_consdev_t consdev;
uint32_t ba[1024];
- memmap_sz = 3 * sizeof(int) + n * sizeof(bios_memmap_t);
- ba[0] = 0x0;/* memory map */
+ memmap_sz = 3 * sizeof(uint32_t) + n * sizeof(bios_memmap_t);
+ ba[0] = BOOTARG_MEMMAP;
ba[1] = memmap_sz;
- ba[2] = memmap_sz; /* next */
+ ba[2] = memmap_sz;
memcpy([3], memmap, n * sizeof(bios_memmap_t));
- i = memmap_sz / sizeof(int);
+ i = memmap_sz / sizeof(uint32_t);
/* Serial console device, COM1 @ 0x3f8 */
- consdev.consdev = makedev(8, 0);/* com1 @ 0x3f8 */
+ memset(, 0, sizeof(consdev));
+ consdev.consdev = makedev(8, 0);
consdev.conspeed = 115200;
consdev.consaddr = 0x3f8;
- consdev.consfreq = 0;
- consdev_sz = 3 * sizeof(int) + sizeof(bios_consdev_t);
- ba[i] = 0x5; /* consdev */
+ consdev_sz = 3 * sizeof(uint32_t) + sizeof(bios_consdev_t);
+ ba[i] = BOOTARG_CONSDEV;
ba[i + 1] = consdev_sz;
ba[i + 2] = consdev_sz;
memcpy([i + 3], , sizeof(bios_consdev_t));
- i += consdev_sz / sizeof(int);
+ i += consdev_sz / sizeof(uint32_t);
if (bootmac) {
- bootmac_sz = 3 * sizeof(int) + (sizeof(bios_bootmac_t) + 3) &
~3;
- ba[i] = 0x7; /* bootmac */
+ bootmac_sz = 3 * sizeof(uint32_t) +
+ (sizeof(bios_bootmac_t) + 3) & ~3;
+ ba[i] = BOOTARG_BOOTMAC;
ba[i + 1] = bootmac_sz;
ba[i + 2] = bootmac_sz;
memcpy([i + 3], bootmac, sizeof(bios_bootmac_t));
- i += bootmac_sz / sizeof(int);
+ i += bootmac_sz / sizeof(uint32_t);
}
ba[i++] = 0x; /* BOOTARG_END */
write_mem(BOOTARGS_PAGE, ba, PAGE_SIZE);
- return (i * sizeof(int));
+ return (i * sizeof(uint32_t));
}
/*
Re: Improve rpki-client warnings on .mft files
On Mon, Nov 28, 2022 at 04:50:24PM +0100, Theo Buehler wrote:
> On Mon, Nov 28, 2022 at 04:02:11PM +0100, Claudio Jeker wrote:
> > Since a long time any problem that caused rpki-client to not load a
> > manifest resulted in the non helpful:
> > rpki-client:
> > rpki.afrinic.net/repository/member_repository/F36505B2/0569917622D711ED862FD6E0F1222468/0nALpPtwFyntPHjkS8xt-VQrqLw.mft:
> > no valid mft available
> >
> > This hides in most cases the cause why the manifest verificatin failed.
> > The following diff exposes the error from valid_x509() and with that some
> > manifest errors change to e.g.:
> > rpki-client:
> > parent.rov.koenvanhove.nl/repo/KoenvanHove/1/C1F7424F77FBF669FB750C6DC7B649C5969DCD55.mft:
> > CRL has expired
> >
> > or if the CRL is missing
> >
> > rpki-client:
> > repo.pedjoeang.group/repo/localname/0/EF79F8E55B6A248EF9CF4CE70FF60C017BF1A3B0.mft:
> > unable to get certificate CRL
> >
> > If the certificate is pointing to a manifest that does not exist the old
> > "no valid mft available" is shown.
> >
> > I tried to keep original behaviour as much as possible but I think
> > filemode can be further improved. And maybe we can remove verbose from
> > other warnings as well.
>
> I like this a lot.
>
> I was wondering if valid_x509() should have a const char **errstr
> instead of an int * as last argument. valid_x509() could do the
> conversion from error code to error string itself. This way we don't
> have to sprinkle X509_verify_cert_error_string() calls everywhere.
>
> Or we could introduce a warn_invalid_x509(const char *str, int err) that
> does the conversion from err using X509_verify_cert_error_string().
>
> One downside of X509_verify_cert_error_string() is that it isn't thread
> safe since it might return a pointer to a static buffer -- it should
> not, but who can be sure...
Tough call. It may also help other code paths to do the same. But in many
cases a dynamic buffer would be needed.
Not sure if it makes sense to introduce warn_invalid_x509(). What I don't
like is the verbose check before the warning. I wonder if we still need
that. My last run has 11 failed roas and 51 failed mfts. The mft errors
already show up. Shouldn't the roa errors be shown as well?
> >
> > --
> > :wq Claudio
> >
> > Index: extern.h
> > ===
> > RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> > retrieving revision 1.161
> > diff -u -p -r1.161 extern.h
> > --- extern.h26 Nov 2022 12:02:36 - 1.161
> > +++ extern.h28 Nov 2022 10:42:11 -
> > @@ -629,7 +629,7 @@ int valid_filename(const char *, size_
> > int valid_uri(const char *, size_t, const char *);
> > int valid_origin(const char *, const char *);
> > int valid_x509(char *, X509_STORE_CTX *, X509 *, struct
> > auth *,
> > - struct crl *, int);
> > + struct crl *, int *);
> > int valid_rsc(const char *, struct cert *, struct rsc *);
> > int valid_econtent_version(const char *, const
> > ASN1_INTEGER *);
> > int valid_aspa(const char *, struct cert *, struct aspa *);
> > Index: filemode.c
> > ===
> > RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
> > retrieving revision 1.17
> > diff -u -p -r1.17 filemode.c
> > --- filemode.c 26 Nov 2022 12:02:37 - 1.17
> > +++ filemode.c 28 Nov 2022 11:08:31 -
> > @@ -141,7 +141,7 @@ parse_load_certchain(char *uri)
> > struct cert *cert;
> > struct crl *crl;
> > struct auth *a;
> > - int i;
> > + int i, e;
> >
> > for (i = 0; i < MAX_CERT_DEPTH; i++) {
> > filestack[i] = uri;
> > @@ -171,9 +171,13 @@ parse_load_certchain(char *uri)
> > uri = filestack[i];
> >
> > crl = crl_get(, a);
> > - if (!valid_x509(uri, ctx, cert->x509, a, crl, 0) ||
> > - !valid_cert(uri, a, cert))
> > + if (!valid_x509(uri, ctx, cert->x509, a, crl, ) ||
> > + !valid_cert(uri, a, cert)) {
> > + if (verbose > 1)
> > + warnx("%s: %s", uri,
> > + X509_verify_cert_error_string(e));
> > goto fail;
> > + }
> > cert->talid = a->cert->talid;
> > a = auth_insert(, cert, a);
> > stack[i] = NULL;
> > @@ -275,7 +279,7 @@ proc_parser_file(char *file, unsigned ch
> > char filehash[SHA256_DIGEST_LENGTH];
> > char *hash;
> > enum rtype type;
> > - int is_ta = 0;
> > + int is_ta = 0, e;
> >
> > if (num++ > 0) {
> > if ((outformats & FORMAT_JSON) == 0)
> > @@ -418,7 +422,7 @@ proc_parser_file(char *file, unsigned ch
> > a = auth_find(, aki);
> > c = crl_get(, a);
> >
> > - if ((status =
Re: pfsync panic after pf_purge backout
Hello,
>
>
> Hi,
>
> here's panic with WITNESS and this diff on tech@
> https://www.mail-archive.com/tech@openbsd.org/msg72582.html
>
> I will stop now because I'm not sure what I'm doing and which diffs I'm
> testing...
>
>
> r620-1# uvm_fault(0x8248ea28, 0x17, 0, 2) -> e
> kernel: page fault trap, code=0
> Stopped at pfsync_q_del+0x96: movq%rdx,0x8(%rax)
> TIDPIDUID PRFLAGS PFLAGS CPU COMMAND
> *300703 35643 0 0x14000 0x2001K systq
> 237790 10061 0 0x14000 0x42000 softclock
> pfsync_q_del(fd8323dc3900) at pfsync_q_del+0x96
> pfsync_delete_state(fd8323dc3900) at pfsync_delete_state+0x118
> pf_remove_state(fd8323dc3900) at pf_remove_state+0x14e
> pf_purge_expired_states(c3501) at pf_purge_expired_states+0x1b3
> pf_purge(823ae080) at pf_purge+0x28
> taskq_thread(822cbe30) at taskq_thread+0x11a
> end trace frame: 0x0, count: 9
> https://www.openbsd.org/ddb.html describes the minimum info required in
> bug reports. Insufficient info makes it difficult to find and fix bugs.
> ddb{1}>
>
>
diff below should avoid panic above (and similar panics in pfsync_q_del().
It also prints some 'error' system message buffer (a.k.a. dmesg)
We panic because we attempt to remove state from psync queue which is
already empty. the pfsync_q_del() must be acting on some stale information
kept in `st` argument (state).
the information we print into dmesg buffer should help us understand
what's going on. At the moment I can not explain how does it come
there is a state which claims its presence on state queue, while the
queue in question is empty.
I'd like to ask you to give a try diff below and repeat your test.
Let it run for some time and collect 'dmesg' output for me after usual
uptime-to-panic elapses during a test run.
thanks a lot
regards
sashan
8<---8<---8<--8<
diff --git a/sys/net/if_pfsync.c b/sys/net/if_pfsync.c
index f69790ee98d..d4be84a1f57 100644
--- a/sys/net/if_pfsync.c
+++ b/sys/net/if_pfsync.c
@@ -2254,6 +2254,74 @@ pfsync_q_ins(struct pf_state *st, int q)
} while (0);
}
+const char *
+pfsync_qname(int q)
+{
+ switch (q) {
+ case PFSYNC_S_IACK:
+ return ("PFSYNC_S_IACK");
+
+ case PFSYNC_S_UPD_C:
+ return ("PFSYNC_S_UPD_C");
+
+ case PFSYNC_S_DEL:
+ return ("PFSYNC_S_DEL");
+
+ case PFSYNC_S_INS:
+ return ("PFSYNC_S_INS");
+
+ case PFSYNC_S_UPD:
+ return ("PFSYNC_S_UPD");
+
+ case PFSYNC_S_COUNT:
+ return ("PFSYNC_S_COUNT");
+
+ case PFSYNC_S_DEFER:
+ return ("PFSYNC_S_DEFER");
+
+ case PFSYNC_S_NONE:
+ return ("PFSYNC_S_NONE");
+
+ default:
+ return ("???");
+ }
+}
+
+const char *
+pfsync_timeout_name(unsigned int timeout)
+{
+ const char *timeout_name[] = {
+ "PFTM_TCP_FIRST_PACKET",
+ "PFTM_TCP_OPENING",
+ "PFTM_TCP_ESTABLISHED",
+ "PFTM_TCP_CLOSING",
+ "PFTM_TCP_FIN_WAIT",
+ "PFTM_TCP_CLOSED",
+ "PFTM_UDP_FIRST_PACKET",
+ "PFTM_UDP_SINGLE",
+ "PFTM_UDP_MULTIPLE",
+ "PFTM_ICMP_FIRST_PACKET",
+ "PFTM_ICMP_ERROR_REPLY",
+ "PFTM_OTHER_FIRST_PACKET",
+ "PFTM_OTHER_SINGLE",
+ "PFTM_OTHER_MULTIPLE",
+ "PFTM_FRAG",
+ "PFTM_INTERVAL",
+ "PFTM_ADAPTIVE_START",
+ "PFTM_ADAPTIVE_END",
+ "PFTM_SRC_NODE",
+ "PFTM_TS_DIFF",
+ "PFTM_MAX",
+ "PFTM_PURGE",
+ "PFTM_UNLINKED"
+ };
+
+ if (timeout > PFTM_UNLINKED)
+ return ("???");
+ else
+ return (timeout_name[timeout]);
+}
+
void
pfsync_q_del(struct pf_state *st)
{
@@ -2273,6 +2341,19 @@ pfsync_q_del(struct pf_state *st)
mtx_leave(>sc_st_mtx);
return;
}
+
+ if (TAILQ_EMPTY(>sc_qs[q])) {
+ mtx_leave(>sc_st_mtx);
+ DPFPRINTF(LOG_ERR,
+ "%s: stale queue (%s) in state (id %llu)"
+ "nosync: %s unlinked: %s timeout: %s", __func__,
+ pfsync_qname(q), st->id,
+ (st->state_flags & PFSTATE_NOSYNC) ? "yes" : "no",
+ (st->timeout == PFTM_UNLINKED) ? "yes" : "no",
+ pfsync_timeout_name(st->timeout));
+ return;
+ }
+
atomic_sub_long(>sc_len, pfsync_qs[q].len);
TAILQ_REMOVE(>sc_qs[q], st, sync_list);
if (TAILQ_EMPTY(>sc_qs[q]))
Re: Improve rpki-client warnings on .mft files
On Mon, Nov 28, 2022 at 04:02:11PM +0100, Claudio Jeker wrote:
> Since a long time any problem that caused rpki-client to not load a
> manifest resulted in the non helpful:
> rpki-client:
> rpki.afrinic.net/repository/member_repository/F36505B2/0569917622D711ED862FD6E0F1222468/0nALpPtwFyntPHjkS8xt-VQrqLw.mft:
> no valid mft available
>
> This hides in most cases the cause why the manifest verificatin failed.
> The following diff exposes the error from valid_x509() and with that some
> manifest errors change to e.g.:
> rpki-client:
> parent.rov.koenvanhove.nl/repo/KoenvanHove/1/C1F7424F77FBF669FB750C6DC7B649C5969DCD55.mft:
> CRL has expired
>
> or if the CRL is missing
>
> rpki-client:
> repo.pedjoeang.group/repo/localname/0/EF79F8E55B6A248EF9CF4CE70FF60C017BF1A3B0.mft:
> unable to get certificate CRL
>
> If the certificate is pointing to a manifest that does not exist the old
> "no valid mft available" is shown.
>
> I tried to keep original behaviour as much as possible but I think
> filemode can be further improved. And maybe we can remove verbose from
> other warnings as well.
I like this a lot.
I was wondering if valid_x509() should have a const char **errstr
instead of an int * as last argument. valid_x509() could do the
conversion from error code to error string itself. This way we don't
have to sprinkle X509_verify_cert_error_string() calls everywhere.
Or we could introduce a warn_invalid_x509(const char *str, int err) that
does the conversion from err using X509_verify_cert_error_string().
One downside of X509_verify_cert_error_string() is that it isn't thread
safe since it might return a pointer to a static buffer -- it should
not, but who can be sure...
>
> --
> :wq Claudio
>
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.161
> diff -u -p -r1.161 extern.h
> --- extern.h 26 Nov 2022 12:02:36 - 1.161
> +++ extern.h 28 Nov 2022 10:42:11 -
> @@ -629,7 +629,7 @@ intvalid_filename(const char *, size_
> int valid_uri(const char *, size_t, const char *);
> int valid_origin(const char *, const char *);
> int valid_x509(char *, X509_STORE_CTX *, X509 *, struct auth *,
> - struct crl *, int);
> + struct crl *, int *);
> int valid_rsc(const char *, struct cert *, struct rsc *);
> int valid_econtent_version(const char *, const ASN1_INTEGER *);
> int valid_aspa(const char *, struct cert *, struct aspa *);
> Index: filemode.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 filemode.c
> --- filemode.c26 Nov 2022 12:02:37 - 1.17
> +++ filemode.c28 Nov 2022 11:08:31 -
> @@ -141,7 +141,7 @@ parse_load_certchain(char *uri)
> struct cert *cert;
> struct crl *crl;
> struct auth *a;
> - int i;
> + int i, e;
>
> for (i = 0; i < MAX_CERT_DEPTH; i++) {
> filestack[i] = uri;
> @@ -171,9 +171,13 @@ parse_load_certchain(char *uri)
> uri = filestack[i];
>
> crl = crl_get(, a);
> - if (!valid_x509(uri, ctx, cert->x509, a, crl, 0) ||
> - !valid_cert(uri, a, cert))
> + if (!valid_x509(uri, ctx, cert->x509, a, crl, ) ||
> + !valid_cert(uri, a, cert)) {
> + if (verbose > 1)
> + warnx("%s: %s", uri,
> + X509_verify_cert_error_string(e));
> goto fail;
> + }
> cert->talid = a->cert->talid;
> a = auth_insert(, cert, a);
> stack[i] = NULL;
> @@ -275,7 +279,7 @@ proc_parser_file(char *file, unsigned ch
> char filehash[SHA256_DIGEST_LENGTH];
> char *hash;
> enum rtype type;
> - int is_ta = 0;
> + int is_ta = 0, e;
>
> if (num++ > 0) {
> if ((outformats & FORMAT_JSON) == 0)
> @@ -418,7 +422,7 @@ proc_parser_file(char *file, unsigned ch
> a = auth_find(, aki);
> c = crl_get(, a);
>
> - if ((status = valid_x509(file, ctx, x509, a, c, 0))) {
> + if ((status = valid_x509(file, ctx, x509, a, c, ))) {
> switch (type) {
> case RTYPE_ROA:
> status = roa->valid;
> @@ -438,8 +442,12 @@ proc_parser_file(char *file, unsigned ch
> }
> if (status)
> printf("OK");
> - else
> + else {
> + if (verbose > 1)
> + warnx("%s: %s", file,
> + X509_verify_cert_error_string(e));
> printf("Failed");
> + }
>
Do not store unused ICMPv6 Option PREFIX_INFORMATION
Dead since 2017 sys/netinet6/nd6_rtr.c r1.163
Remove sending of router solicitations and processing of router
advertisements from the kernel. It's handled by slaacd(8) these days.
Multiple prefixes are fine, so sysctl(2) net.inet6.icmp6.nd6_debug does not
warn about it like it does for, e.g., duplicate MTU options, so don't do
anything with this option.
Remove access macros for other unused options while here.
All under _KERNEL.
tcpdump(8)/rad(8)/slaacd(8) keep showing/sending/receiving this option when
running this diff on both router and client.
Feedback? Objection? OK?
NB: I suspect that nd6.h's union nd_opt can be cleaned up/reduced in
size, but nd6.c's nd6_option() and nd6_options() are not trivial, so
here goes the option handling alone for now.
diff --git a/sys/netinet6/nd6.c b/sys/netinet6/nd6.c
index 1c7bf7d985b..9c7f2c422ca 100644
--- a/sys/netinet6/nd6.c
+++ b/sys/netinet6/nd6.c
@@ -251,13 +251,6 @@ nd6_options(union nd_opts *ndopts)
}
break;
case ND_OPT_PREFIX_INFORMATION:
- if (ndopts->nd_opt_array[nd_opt->nd_opt_type] == 0) {
- ndopts->nd_opt_array[nd_opt->nd_opt_type]
- = nd_opt;
- }
- ndopts->nd_opts_pi_end =
- (struct nd_opt_prefix_info *)nd_opt;
- break;
case ND_OPT_DNSSL:
case ND_OPT_RDNSS:
/* Don't warn */
diff --git a/sys/netinet6/nd6.h b/sys/netinet6/nd6.h
index 2b441bd97e7..8d67c66d961 100644
--- a/sys/netinet6/nd6.h
+++ b/sys/netinet6/nd6.h
@@ -127,10 +127,6 @@ union nd_opts {
};
#define nd_opts_src_lladdr nd_opt_each.src_lladdr
#define nd_opts_tgt_lladdr nd_opt_each.tgt_lladdr
-#define nd_opts_pi nd_opt_each.pi_beg
-#define nd_opts_pi_end nd_opt_each.pi_end
-#define nd_opts_rh nd_opt_each.rh
-#define nd_opts_mtund_opt_each.mtu
#define nd_opts_search nd_opt_each.search
#define nd_opts_last nd_opt_each.last
#define nd_opts_done nd_opt_each.done
Improve rpki-client warnings on .mft files
Since a long time any problem that caused rpki-client to not load a
manifest resulted in the non helpful:
rpki-client:
rpki.afrinic.net/repository/member_repository/F36505B2/0569917622D711ED862FD6E0F1222468/0nALpPtwFyntPHjkS8xt-VQrqLw.mft:
no valid mft available
This hides in most cases the cause why the manifest verificatin failed.
The following diff exposes the error from valid_x509() and with that some
manifest errors change to e.g.:
rpki-client:
parent.rov.koenvanhove.nl/repo/KoenvanHove/1/C1F7424F77FBF669FB750C6DC7B649C5969DCD55.mft:
CRL has expired
or if the CRL is missing
rpki-client:
repo.pedjoeang.group/repo/localname/0/EF79F8E55B6A248EF9CF4CE70FF60C017BF1A3B0.mft:
unable to get certificate CRL
If the certificate is pointing to a manifest that does not exist the old
"no valid mft available" is shown.
I tried to keep original behaviour as much as possible but I think
filemode can be further improved. And maybe we can remove verbose from
other warnings as well.
--
:wq Claudio
Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.161
diff -u -p -r1.161 extern.h
--- extern.h26 Nov 2022 12:02:36 - 1.161
+++ extern.h28 Nov 2022 10:42:11 -
@@ -629,7 +629,7 @@ int valid_filename(const char *, size_
int valid_uri(const char *, size_t, const char *);
int valid_origin(const char *, const char *);
int valid_x509(char *, X509_STORE_CTX *, X509 *, struct auth *,
- struct crl *, int);
+ struct crl *, int *);
int valid_rsc(const char *, struct cert *, struct rsc *);
int valid_econtent_version(const char *, const ASN1_INTEGER *);
int valid_aspa(const char *, struct cert *, struct aspa *);
Index: filemode.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
retrieving revision 1.17
diff -u -p -r1.17 filemode.c
--- filemode.c 26 Nov 2022 12:02:37 - 1.17
+++ filemode.c 28 Nov 2022 11:08:31 -
@@ -141,7 +141,7 @@ parse_load_certchain(char *uri)
struct cert *cert;
struct crl *crl;
struct auth *a;
- int i;
+ int i, e;
for (i = 0; i < MAX_CERT_DEPTH; i++) {
filestack[i] = uri;
@@ -171,9 +171,13 @@ parse_load_certchain(char *uri)
uri = filestack[i];
crl = crl_get(, a);
- if (!valid_x509(uri, ctx, cert->x509, a, crl, 0) ||
- !valid_cert(uri, a, cert))
+ if (!valid_x509(uri, ctx, cert->x509, a, crl, ) ||
+ !valid_cert(uri, a, cert)) {
+ if (verbose > 1)
+ warnx("%s: %s", uri,
+ X509_verify_cert_error_string(e));
goto fail;
+ }
cert->talid = a->cert->talid;
a = auth_insert(, cert, a);
stack[i] = NULL;
@@ -275,7 +279,7 @@ proc_parser_file(char *file, unsigned ch
char filehash[SHA256_DIGEST_LENGTH];
char *hash;
enum rtype type;
- int is_ta = 0;
+ int is_ta = 0, e;
if (num++ > 0) {
if ((outformats & FORMAT_JSON) == 0)
@@ -418,7 +422,7 @@ proc_parser_file(char *file, unsigned ch
a = auth_find(, aki);
c = crl_get(, a);
- if ((status = valid_x509(file, ctx, x509, a, c, 0))) {
+ if ((status = valid_x509(file, ctx, x509, a, c, ))) {
switch (type) {
case RTYPE_ROA:
status = roa->valid;
@@ -438,8 +442,12 @@ proc_parser_file(char *file, unsigned ch
}
if (status)
printf("OK");
- else
+ else {
+ if (verbose > 1)
+ warnx("%s: %s", file,
+ X509_verify_cert_error_string(e));
printf("Failed");
+ }
} else if (is_ta) {
if ((tal = find_tal(cert)) != NULL) {
cert = ta_parse(file, cert, tal->pkey, tal->pkeysz);
Index: parser.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
retrieving revision 1.78
diff -u -p -r1.78 parser.c
--- parser.c2 Nov 2022 12:43:02 - 1.78
+++ parser.c28 Nov 2022 14:39:37 -
@@ -132,6 +132,7 @@ proc_parser_roa(char *file, const unsign
struct auth *a;
struct crl *crl;
X509*x509;
+ int e;
if ((roa = roa_parse(, file, der, len)) == NULL)
return NULL;
@@ -139,7 +140,9 @@ proc_parser_roa(char *file,
Re: Remove useless nd6_init_done from nd6_init()
On Mon, Nov 28, 2022 at 02:54:31PM +0100, Alexander Bluhm wrote: > > NB: While here, nd6 and frag6 could statically initalise their lists > > right away (they're both static/local to nd6.c and frag6.c anyway), but > > I see no big adavantage of variable initializer over ..._init() > function. The latter is there anyway. > > > that should probably be another diff, if at all, with more static added > > to those files. > > Why is static in the kernel a benefit? There were times when static > was no allowed to have more complete symbol list. Seeing a static variable immediately tells me that it is not used outside the function or compilation unit. Imho, this makes it easier to work with the code, as you don't have to cross-check and grep for variables, whether they're used somewhere else, behind some language construct which made your grep not show it... On the other hand, I do see how changing static around for existing code is quite some churn, so I'll refrain from doing so here.
Re: Get rid of UVM_VNODE_CANPERSIST
> Date: Wed, 23 Nov 2022 17:33:26 +0100
> From: Martin Pieuchot
>
> On 23/11/22(Wed) 16:34, Mark Kettenis wrote:
> > > Date: Wed, 23 Nov 2022 10:52:32 +0100
> > > From: Martin Pieuchot
> > >
> > > On 22/11/22(Tue) 23:40, Mark Kettenis wrote:
> > > > > Date: Tue, 22 Nov 2022 17:47:44 +
> > > > > From: Miod Vallat
> > > > >
> > > > > > Here is a diff. Maybe bluhm@ can try this on the macppc machine
> > > > > > that
> > > > > > triggered the original "vref used where vget required" problem?
> > > > >
> > > > > On a similar machine it panics after a few hours with:
> > > > >
> > > > > panic: uvn_flush: PGO_SYNCIO return 'try again' error (impossible)
> > > > >
> > > > > The trace (transcribed by hand) is
> > > > > uvn_flush+0x820
> > > > > uvm_vnp_terminate+0x79
> > > > > vclean+0xdc
> > > > > vgonel+0x70
> > > > > getnewvnode+0x240
> > > > > ffs_vget+0xcc
> > > > > ffs_inode_alloc+0x13c
> > > > > ufs_makeinode+0x94
> > > > > ufs_create+0x58
> > > > > VOP_CREATE+0x48
> > > > > vn_open+0x188
> > > > > doopenat+0x1b4
> > > >
> > > > Ah right, there is another path where we end up with a refcount of
> > > > zero. Should be fixable, but I need to think about this for a bit.
> > >
> > > Not sure to understand what you mean with refcount of 0. Could you
> > > elaborate?
> >
> > Sorry, I was thinking ahead a bit. I'm pretty much convinced that the
> > issue we're dealing with is a race between a vnode being
> > recycled/cleaned and the pagedaemon paging out pages associated with
> > that same vnode.
> >
> > The crashes we've seen before were all in the pagedaemon path where we
> > end up calling into the VFS layer with a vnode that has v_usecount ==
> > 0. My "fix" avoids that, but hits the issue that when we are in the
> > codepath that is recycling/cleaning the vnode, we can't use vget() to
> > get a reference to the vnode since it checks that the vnode isn't in
> > the process of being cleaned.
> >
> > But if we avoid that issue (by for example) skipping the vget() call
> > if the UVM_VNODE_DYING flag is set, we run into the same scenario
> > where we call into the VFS layer with v_usecount == 0. Now that may
> > not actually be a problem, but I need to investigate this a bit more.
>
> When the UVM_VNODE_DYING flag is set the caller always own a valid
> reference to the vnode. Either because it is in the process of cleaning
> it via uvm_vnp_terminate() or because it uvn_detach() has been called
> which means the reference to the vnode hasn't been dropped yet. So I
> believe `v_usecount' for such vnode is positive.
I don't think so. The vnode that can be recycled is sitting on the
freelist with v_usecount == 0. When getnewvnode() decides to recycle
a vnode it takes it off the freelist and calls vgonel(), which i turn
calls vclean(), which only increases v_usecount if it is non-zero. So
at the point where uvn_vnp_terminate() gets called v_usecount
definitely is 0.
That said, the vnode is no longer on the freelist at that point and
since UVM_VNODE_DYING is set, uvn_vnp_uncache() will return
immediately without calling vref() to get another reference. So that
is fine.
> > Or maybe calling into the VFS layer with a vnode that has v_usecount
> > == 0 is perfectly fine and we should do the vget() dance I propose in
> > uvm_vnp_unache() instead of in uvn_put().
>
> I'm not following. uvm_vnp_uncache() is always called with a valid
> vnode, no?
Not sure what you mean with a "valid vnode"; uvm_vnp_uncache() checks
the UVM_VNODE_VALID flag at the start, which suggests that it can be
called in cases where that flag is not set. But it will unlock and
return immediately in that case, so it should be safe.
Anyway, I think I have convinced myself that in the case where the
pagedaemon ends up calling uvn_put() for a persisting vnode vm object,
we do need to get a refence before calling uvn_io(). Otherwise we run
the risk of the vnode being recycled under our feet while we're doing
I/O.
So here is an updated diff that checks the UVM_VNODE_DYING flag and
skips the refcount manipulation if it is set.
What do you think?
Index: uvm/uvm_vnode.c
===
RCS file: /cvs/src/sys/uvm/uvm_vnode.c,v
retrieving revision 1.130
diff -u -p -r1.130 uvm_vnode.c
--- uvm/uvm_vnode.c 20 Oct 2022 13:31:52 - 1.130
+++ uvm/uvm_vnode.c 28 Nov 2022 14:01:20 -
@@ -899,11 +899,21 @@ uvn_cluster(struct uvm_object *uobj, vof
int
uvn_put(struct uvm_object *uobj, struct vm_page **pps, int npages, int flags)
{
- int retval;
+ struct uvm_vnode *uvn = (struct uvm_vnode *)uobj;
+ int dying, retval;
KASSERT(rw_write_held(uobj->vmobjlock));
+ dying = (uvn->u_flags & UVM_VNODE_DYING);
+ if (!dying) {
+ if (vget(uvn->u_vnode, LK_NOWAIT))
+ return VM_PAGER_AGAIN;
+ }
+
retval = uvn_io((struct uvm_vnode*)uobj, pps, npages, flags, UIO_WRITE);
+
+ if
Re: Remove useless nd6_init_done from nd6_init()
On Sun, Nov 27, 2022 at 09:23:25PM +, Klemens Nanni wrote:
> Only ip6_init() calls nd6_init(), exactly once, just like it calls
> frag6_init() which on the other hand does not have some fra6_init_done
> to guard against itself.
>
> Like all other domains, ip6_init() is called in domaininit(), early in
> the kernel's main().
>
> This variable was probably never useful and dates back to nd6.c r1.1:
> bring in KAME IPv6 code, dated 19991208.
>
> Feedback? OK?
OK bluhm@
> NB: While here, nd6 and frag6 could statically initalise their lists
> right away (they're both static/local to nd6.c and frag6.c anyway), but
I see no big adavantage of variable initializer over ..._init()
function. The latter is there anyway.
> that should probably be another diff, if at all, with more static added
> to those files.
Why is static in the kernel a benefit? There were times when static
was no allowed to have more complete symbol list.
> diff --git a/sys/netinet6/nd6.c b/sys/netinet6/nd6.c
> index 9fdaf8138d1..07ccf615746 100644
> --- a/sys/netinet6/nd6.c
> +++ b/sys/netinet6/nd6.c
> @@ -104,21 +104,12 @@ struct task nd6_expire_task;
> void
> nd6_init(void)
> {
> - static int nd6_init_done = 0;
> -
> - if (nd6_init_done) {
> - log(LOG_NOTICE, "%s called more than once\n", __func__);
> - return;
> - }
> -
> TAILQ_INIT(_list);
> pool_init(_pool, sizeof(struct llinfo_nd6), 0,
> IPL_SOFTNET, 0, "nd6", NULL);
>
> task_set(_expire_task, nd6_expire, NULL);
>
> - nd6_init_done = 1;
> -
> /* start timer */
> timeout_set_proc(_timer_to, nd6_timer, NULL);
> timeout_set_proc(_slowtimo_ch, nd6_slowtimo, NULL);
Re: nd6: DAD: Remove useless variable, simplify code
On Sun, Nov 27, 2022 at 06:42:48PM +, Klemens Nanni wrote: > Using a local `duplicate' variable to defer the actual checks by a few > lines, interleaved with comments (saying the same thing but negated), > is harder to follow that neccessary. > > Fold the logic and merge comments (remove the last obvious one missing > a negation) to save 20 LOC. > > Feedback? Objection? OK? OK bluhm@ > - * We have transmitted sufficient number of DAD packets. > + * We have transmitted enough DAD packets. Could you leave this line as it is? Not changing comment makes cvs blame easier to use. And "enough" is not better to understand than "sufficient number of".
