Re: [httpd] Fix issues related to tls/ssl
On Fri, Oct 02, 2020 at 02:04:15AM +1000, Joshua Sing wrote: > Hello there, I was adding a site to httpd when I experienced a bug that > would cause httpd to crash whenever someone accessed the site while the TLS > cert of the first server entry was missing. > > Steps to reproduce: (httpd.conf provided at bottom of email) > - Have a server entry at the top of the httpd.conf file with the TLS > certificate missing. > - Second server entry below the first one with a valid TLS certificate and > key. > - Visit the server on HTTPS. > > The crash occurs because the server is listening on port 443 without setting > up the TLS context first. > > The cleanest fix is to move the handling of missing certificates to the > configuration parser. > > See attachment for the diff to fix this bug (this also reverts r1.117 of > server.c which is no longer neccessary). Nice. This makes sense and looks like a superior approach for solving the acme issue from r1.117. The diff is against 6.7-stable, so the parse.y part has some offset, but it applies and works as intended. Please make sure that you send diffs against -current. ok tb (the s/setup/accept tweak is unrelated and should probably be committed separately).
fix: ospf6d(8): wrong intra area announcement
Hi,
The new intra area db entry has to be saved into the tree before
orig_intra_area_prefix_lsas() is called. If not, the ospf6d will not
announce the new intra area db for a newly learned link from another
ospf router of the broadcast domain.
This bug is triggered, if you add new addresses an ospf interface while
the ospf6d is already running as a backup designated router. The
opposite designated ospf6d will get your new link announcement and
return an old intra area db without the new address.
Beside of the fix, the diff removes redundant code. I made the same
diff for the ospfd to keep code in sync and remove redundant code there,
too. ospfd does not have the bug explained above, as far as I know.
Both regression tests passes with this diff.
OK?
Bye,
Jan
Index: ospf6d/rde_lsdb.c
===
RCS file: /cvs//src/usr.sbin/ospf6d/rde_lsdb.c,v
retrieving revision 1.45
diff -u -p -r1.45 rde_lsdb.c
--- ospf6d/rde_lsdb.c 21 Aug 2020 10:17:35 - 1.45
+++ ospf6d/rde_lsdb.c 1 Oct 2020 23:09:38 -
@@ -467,6 +467,7 @@ lsa_add(struct rde_nbr *nbr, struct lsa
struct lsa_tree *tree;
struct vertex *new, *old;
struct timeval tv, now, res;
+ int update = 1;
if (LSA_IS_SCOPE_AS(ntohs(lsa->hdr.type)))
tree = &asext_tree;
@@ -495,16 +496,13 @@ lsa_add(struct rde_nbr *nbr, struct lsa
fatal("lsa_add");
return (1);
}
- if (!lsa_equal(new->lsa, old->lsa)) {
- if (ntohs(lsa->hdr.type) == LSA_TYPE_LINK)
- orig_intra_area_prefix_lsas(nbr->area);
- if (ntohs(lsa->hdr.type) != LSA_TYPE_EXTERNAL)
- nbr->area->dirty = 1;
- start_spf_timer();
- }
+ if (lsa_equal(new->lsa, old->lsa))
+ update = 0;
vertex_free(old);
RB_INSERT(lsa_tree, tree, new);
- } else {
+ }
+
+ if (update) {
if (ntohs(lsa->hdr.type) == LSA_TYPE_LINK)
orig_intra_area_prefix_lsas(nbr->area);
if (ntohs(lsa->hdr.type) != LSA_TYPE_EXTERNAL)
Index: ospfd/rde_lsdb.c
===
RCS file: /cvs//src/usr.sbin/ospfd/rde_lsdb.c,v
retrieving revision 1.50
diff -u -p -r1.50 rde_lsdb.c
--- ospfd/rde_lsdb.c22 Nov 2015 13:09:10 - 1.50
+++ ospfd/rde_lsdb.c1 Oct 2020 23:06:57 -
@@ -383,6 +383,7 @@ lsa_add(struct rde_nbr *nbr, struct lsa
struct lsa_tree *tree;
struct vertex *new, *old;
struct timeval tv, now, res;
+ int update = 1;
if (lsa->hdr.type == LSA_TYPE_EXTERNAL ||
lsa->hdr.type == LSA_TYPE_AS_OPAQ)
@@ -410,15 +411,13 @@ lsa_add(struct rde_nbr *nbr, struct lsa
fatal("lsa_add");
return (1);
}
- if (!lsa_equal(new->lsa, old->lsa)) {
- if (lsa->hdr.type != LSA_TYPE_EXTERNAL &&
- lsa->hdr.type != LSA_TYPE_AS_OPAQ)
- nbr->area->dirty = 1;
- start_spf_timer();
- }
+ if (lsa_equal(new->lsa, old->lsa))
+ update = 0;
vertex_free(old);
RB_INSERT(lsa_tree, tree, new);
- } else {
+ }
+
+ if (update) {
if (lsa->hdr.type != LSA_TYPE_EXTERNAL &&
lsa->hdr.type != LSA_TYPE_AS_OPAQ)
nbr->area->dirty = 1;
Make df output more human friendly in daily(8)
Hi, Currently daily(8) runs `df -ikl`. I find reading daily(8) emails hard because in today's disk size, kilobyte counts are not sensible. I'd like to replace -k by -h so the output is more human friendly. I doubt anyone parses daily(8) so this shouldn't break anyone setup. It seems that historically it was `df -k`, back when it was imported from NetBSD in October 95. On the other hand, df's -h was added only in April 97. Comments? OK? Cheers, Daniel Index: etc/daily === RCS file: /cvs/src/etc/daily,v retrieving revision 1.93 diff -u -p -r1.93 daily --- etc/daily 9 Sep 2019 20:02:26 - 1.93 +++ etc/daily 2 Oct 2020 02:17:33 - @@ -140,7 +140,7 @@ next_part "Checking subsystem status:" if [ "X$VERBOSESTATUS" != X0 ]; then echo "" echo "disks:" - df -ikl + df -hil echo "" dump W else
[httpd] Fix issues related to tls/ssl
Hello there, I was adding a site to httpd when I experienced a bug that
would cause httpd to crash whenever someone accessed the site while the
TLS cert of the first server entry was missing.
Steps to reproduce: (httpd.conf provided at bottom of email)
- Have a server entry at the top of the httpd.conf file with the TLS
certificate missing.
- Second server entry below the first one with a valid TLS certificate
and key.
- Visit the server on HTTPS.
The crash occurs because the server is listening on port 443 without
setting up the TLS context first.
The cleanest fix is to move the handling of missing certificates to the
configuration parser.
See attachment for the diff to fix this bug (this also reverts r1.117 of
server.c which is no longer neccessary).
Thanks, Joshua Sing.
jos...@hypera.dev
# $OpenBSD: httpd.conf,v 1.20 2018/06/13 15:08:24 reyk Exp $
server "example.com" {
listen on * tls port 443
tls {
certificate "/etc/ssl/example2.com.fullchain.pem" # missing
key "/etc/ssl/private/example2.com.key" # exists
}
location "/pub/*" {
directory auto index
}
location "/.well-known/acme-challenge/*" {
root "/acme"
request strip 2
}
}
server "example2.com" {
listen on * tls port 443
tls {
certificate "/etc/ssl/example.com.fullchain.pem" # exists
key "/etc/ssl/private/example.com.key" # exists
}
location "/pub/*" {
directory auto index
}
location "/.well-known/acme-challenge/*" {
root "/acme"
request strip 2
}
}
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/httpd/parse.y,v
retrieving revision 1.114
diff -u -p -r1.114 parse.y
--- parse.y 9 Feb 2020 09:44:04 - 1.114
+++ parse.y 1 Oct 2020 15:42:23 -
@@ -345,10 +345,17 @@ server: SERVER optmatch STRING{
YYERROR;
}
- if (server_tls_load_keypair(srv) == -1)
+ if (server_tls_load_keypair(srv) == -1) {
+ /* Soft fail as there may be no certificate. */
log_warnx("%s:%d: server \"%s\": failed to "
"load public/private keys", file->name,
yylval.lineno, srv->srv_conf.name);
+ serverconfig_free(srv_conf);
+ srv_conf = NULL;
+ free(srv);
+ srv = NULL;
+ break;
+ }
if (server_tls_load_ca(srv) == -1) {
yyerror("server \"%s\": failed to load "
Index: server.c
===
RCS file: /cvs/src/usr.sbin/httpd/server.c,v
retrieving revision 1.120
diff -u -p -r1.120 server.c
--- server.c14 Oct 2019 11:07:08 - 1.120
+++ server.c1 Oct 2020 15:42:24 -
@@ -119,13 +119,6 @@ server_privinit(struct server *srv)
}
/* Open listening socket in the privileged process */
- if ((srv->srv_conf.flags & SRVFLAG_TLS) && srv->srv_conf.tls_cert ==
- NULL) {
- /* soft fail if cert is not there yet */
- srv->srv_s = -1;
- return (0);
- }
-
if ((srv->srv_s = server_socket_listen(&srv->srv_conf.ss,
srv->srv_conf.port, &srv->srv_conf)) == -1)
return (-1);
@@ -257,10 +250,6 @@ server_tls_init(struct server *srv)
if ((srv->srv_conf.flags & SRVFLAG_TLS) == 0)
return (0);
- if (srv->srv_conf.tls_cert == NULL)
- /* soft fail if cert is not there yet */
- return (0);
-
log_debug("%s: setting up tls for %s", __func__, srv->srv_conf.name);
if (tls_init() != 0) {
@@ -1160,7 +1149,7 @@ server_accept(int fd, short event, void
if (srv->srv_conf.flags & SRVFLAG_TLS) {
if (tls_accept_socket(srv->srv_tls_ctx, &clt->clt_tls_ctx,
clt->clt_s) != 0) {
- server_close(clt, "failed to setup tls context");
+ server_close(clt, "failed to accept tls socket");
return;
}
event_again(&clt->clt_ev, clt->clt_s, EV_TIMEOUT|EV_READ,
Re: mmap: Do not push KERNEL_LOCK() too far
> Date: Thu, 1 Oct 2020 14:10:56 +0200
> From: Martin Pieuchot
>
> While studying a bug report from naddy@ in 2017 when testing guenther@'s
> amap/anon locking diff I figured out that we have been too optimistic in
> the !MAP_ANON case.
>
> The reported panic involves, I'd guess, a race between fd_getfile() and
> vref():
>
> panic: vref used where vget required
> db_enter() at db_enter+0x5
> panic() at panic+0x129
> vref(ff03b20d29e8) at vref+0x5d
> uvn_attach(101,ff03a5879dc0) at uvn_attach+0x11d
> uvm_mmapfile(7,ff03a5879dc0,2,1,13,10012) at uvm_mmapfile+0x12c
> sys_mmap(c50,8000225f82a0,1) at sys_mmap+0x604
> syscall() at syscall+0x279
> --- syscall (number 198) ---
> end of kernel
>
> Removing the KERNEL_LOCK() from file mapping was out of the scope of this
> previous work, so I'd like to go back to a single KERNEL_LOCK/UNLOCK dance
> in this code path to remove any false positive.
>
> Note that this code is currently always run under KERNEL_LOCK() so this
> will only have effect once the syscall will be unlocked.
>
> ok?
Hmm, I thought fd_getfile() was fully mpsafe.
But I suppose the kernel lock needs to be grabbed before we start
looking at the vnode?
Your diff makes the locking a bit convoluted, but I suppose adding a
KERNEL_UNLOCK() before every "goto out" is worse?
> Index: uvm/uvm_mmap.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_mmap.c,v
> retrieving revision 1.161
> diff -u -p -r1.161 uvm_mmap.c
> --- uvm/uvm_mmap.c4 Mar 2020 21:15:39 - 1.161
> +++ uvm/uvm_mmap.c28 Sep 2020 09:48:26 -
> @@ -288,8 +288,11 @@ sys_mmap(struct proc *p, void *v, regist
>
> /* check for file mappings (i.e. not anonymous) and verify file. */
> if ((flags & MAP_ANON) == 0) {
> - if ((fp = fd_getfile(fdp, fd)) == NULL)
> - return (EBADF);
> + KERNEL_LOCK();
> + if ((fp = fd_getfile(fdp, fd)) == NULL) {
> + error = EBADF;
> + goto out;
> + }
>
> if (fp->f_type != DTYPE_VNODE) {
> error = ENODEV; /* only mmap vnodes! */
> @@ -313,6 +316,7 @@ sys_mmap(struct proc *p, void *v, regist
> flags |= MAP_ANON;
> FRELE(fp, p);
> fp = NULL;
> + KERNEL_UNLOCK();
> goto is_anon;
> }
>
> @@ -362,9 +366,7 @@ sys_mmap(struct proc *p, void *v, regist
>* EPERM.
>*/
> if (fp->f_flag & FWRITE) {
> - KERNEL_LOCK();
> error = VOP_GETATTR(vp, &va, p->p_ucred, p);
> - KERNEL_UNLOCK();
> if (error)
> goto out;
> if ((va.va_flags & (IMMUTABLE|APPEND)) == 0)
> @@ -390,9 +392,9 @@ sys_mmap(struct proc *p, void *v, regist
> goto out;
> }
> }
> - KERNEL_LOCK();
> error = uvm_mmapfile(&p->p_vmspace->vm_map, &addr, size, prot,
> maxprot, flags, vp, pos, lim_cur(RLIMIT_MEMLOCK), p);
> + FRELE(fp, p);
> KERNEL_UNLOCK();
> } else {/* MAP_ANON case */
> if (fd != -1)
> @@ -428,7 +430,10 @@ is_anon: /* label for SunOS style /dev/z
> /* remember to add offset */
> *retval = (register_t)(addr + pageoff);
>
> + return (error);
> +
> out:
> + KERNEL_UNLOCK();
> if (fp)
> FRELE(fp, p);
> return (error);
>
>
[diff] Allow preferred source IP selection
This updated diff unbreak P2P links where local address was not the same as
preferred source address.
Sending to tech@ may help get more feedback on what I broke.
Example usage :
Set 2001:db8::1 as source : route source 2001:db8::1
Unset previously set IPv6 address on rdomain 10 : route -T10 source -inet6
default
Show set address : route source
Comments ? OK ?
Denis
Index: sbin/route/keywords.h
===
RCS file: /cvs/src/sbin/route/keywords.h,v
retrieving revision 1.34
diff -u -p -r1.34 keywords.h
--- sbin/route/keywords.h 10 Aug 2017 13:44:48 - 1.34
+++ sbin/route/keywords.h 17 Sep 2020 09:59:25 -
@@ -1,4 +1,4 @@
-/* $OpenBSD: keywords.h,v 1.34 2017/08/10 13:44:48 benno Exp $ */
+/* $OpenBSD$ */
/* WARNING! This file was generated by keywords.sh */
@@ -66,6 +66,7 @@ enum {
K_SA,
K_SENDPIPE,
K_SHOW,
+ K_SOURCE,
K_SSTHRESH,
K_STATIC,
K_SWAP,
@@ -129,6 +130,7 @@ struct keytab keywords[] = {
{ "sa", K_SA },
{ "sendpipe", K_SENDPIPE },
{ "show", K_SHOW },
+ { "source", K_SOURCE },
{ "ssthresh", K_SSTHRESH },
{ "static", K_STATIC },
{ "swap", K_SWAP },
Index: sbin/route/keywords.sh
===
RCS file: /cvs/src/sbin/route/keywords.sh,v
retrieving revision 1.32
diff -u -p -r1.32 keywords.sh
--- sbin/route/keywords.sh 10 Aug 2017 13:44:48 - 1.32
+++ sbin/route/keywords.sh 17 Sep 2020 09:59:25 -
@@ -67,6 +67,7 @@ rttvar
sa
sendpipe
show
+source
ssthresh
static
swap
Index: sbin/route/route.8
===
RCS file: /cvs/src/sbin/route/route.8,v
retrieving revision 1.91
diff -u -p -r1.91 route.8
--- sbin/route/route.8 19 Jan 2020 18:22:31 - 1.91
+++ sbin/route/route.8 17 Sep 2020 09:59:25 -
@@ -195,6 +195,17 @@ or
.Cm bgp .
If the priority is negative, then routes that do not match the numeric
priority are shown.
+.It Xo
+.Nm route
+.Op Fl T Ar rtable
+.Tg
+.Cm source
+.Ar address
+.Xc
+Set the preferred source address. If
+.Ar address
+is the word "default", 0.0.0.0 or ::, source address will be chosen by
+the kernel for the matching address family.
.El
.Pp
.Tg destination
Index: sbin/route/route.c
===
RCS file: /cvs/src/sbin/route/route.c,v
retrieving revision 1.248
diff -u -p -r1.248 route.c
--- sbin/route/route.c 7 Jul 2020 14:53:36 - 1.248
+++ sbin/route/route.c 17 Sep 2020 09:59:25 -
@@ -68,7 +68,8 @@
const struct if_status_description
if_status_descriptions[] = LINK_STATE_DESCRIPTIONS;
-union sockunion so_dst, so_gate, so_mask, so_ifa, so_ifp, so_src, so_label;
+union sockunion so_dst, so_gate, so_mask, so_ifa, so_ifp, so_src, so_label,
+so_source;
typedef union sockunion *sup;
pid_t pid;
@@ -85,6 +86,7 @@ struct rt_metrics rt_metrics;
int flushroutes(int, char **);
int newroute(int, char **);
+int setsource(int, char **);
int show(int, char *[]);
int keycmp(const void *, const void *);
int keyword(char *);
@@ -132,7 +134,8 @@ usage(char *cp)
"usage: %s [-dnqtv] [-T rtable] command [[modifiers] args]\n",
__progname);
fprintf(stderr,
- "commands: add, change, delete, exec, flush, get, monitor, show\n");
+ "commands: add, change, delete, exec, flush, get, monitor, show, "
+ "source\n");
exit(1);
}
@@ -258,6 +261,10 @@ main(int argc, char **argv)
case K_FLUSH:
exit(flushroutes(argc, argv));
break;
+ case K_SOURCE:
+ nflag = 1;
+ exit(setsource(argc, argv));
+ break;
}
if (pledge("stdio dns", NULL) == -1)
@@ -450,6 +457,52 @@ set_metric(char *value, int key)
locking = 0;
}
+
+int
+setsource(int argc, char **argv)
+{
+ char *cmd, *srcaddr = "";
+ int af = AF_UNSPEC, ret = 0;
+ struct hostent *hp = NULL;
+ int key;
+
+ if (uid)
+ errx(1, "must be root to alter source address");
+ cmd = argv[0];
+ while (--argc > 0) {
+ if (**(++argv)== '-') {
+ switch (key = keyword(1 + *argv)) {
+ case K_INET:
+ af = AF_INET;
+ aflen = sizeof(struct sockaddr_in);
+ break;
+ case K_INET6:
+ af = AF_INET6;
+ aflen = sizeof(struct sockaddr_in6);
+ break;
+ }
+ } else if ((rtm_addrs & RTA_IFA) == 0) {
+ srcaddr = *argv;
+
ifconfig: consistent display of P2P link
All tunnels & point-to-point addresses are separated by "->" but inet.
Denis
Index: ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.426
diff -u -p -r1.426 ifconfig.c
--- ifconfig.c 15 Sep 2020 15:23:11 - 1.426
+++ ifconfig.c 17 Sep 2020 14:41:34 -
@@ -3552,7 +3552,7 @@ in_status(int force)
}
(void) strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));
sin = (struct sockaddr_in *)&ifr.ifr_dstaddr;
- printf(" --> %s", inet_ntoa(sin->sin_addr));
+ printf(" -> %s", inet_ntoa(sin->sin_addr));
}
printf(" netmask 0x%x", ntohl(netmask.sin_addr.s_addr));
if (flags & IFF_BROADCAST) {
Re: KASSERT() for VOP_*
On 09/09/20(Wed) 08:41, Martin Pieuchot wrote: > This is mostly the same diff that has been backed out months ago with > the VOP_CLOSE() case fixed. VOP_CLOSE() can accept a NULL argument > instead of `curproc' when garbage collecting passed FDs. > > The intent is to stop passing a "struct proc *" when a function applies > only to `curproc'. Synchronization/locking primitives are obviously > different if a CPU can modify the fields of any thread or only of the > current one. Now that we're early in the release cycle I'd like to get this in. Any ok? > Index: kern/vfs_vops.c > === > RCS file: /cvs/src/sys/kern/vfs_vops.c,v > retrieving revision 1.28 > diff -u -p -r1.28 vfs_vops.c > --- kern/vfs_vops.c 8 Apr 2020 08:07:51 - 1.28 > +++ kern/vfs_vops.c 27 Apr 2020 08:10:02 - > @@ -145,6 +145,8 @@ VOP_OPEN(struct vnode *vp, int mode, str > a.a_cred = cred; > a.a_p = p; > > + KASSERT(p == curproc); > + > if (vp->v_op->vop_open == NULL) > return (EOPNOTSUPP); > > @@ -164,6 +166,7 @@ VOP_CLOSE(struct vnode *vp, int fflag, s > a.a_cred = cred; > a.a_p = p; > > + KASSERT(p == NULL || p == curproc); > ASSERT_VP_ISLOCKED(vp); > > if (vp->v_op->vop_close == NULL) > @@ -184,6 +187,7 @@ VOP_ACCESS(struct vnode *vp, int mode, s > a.a_cred = cred; > a.a_p = p; > > + KASSERT(p == curproc); > ASSERT_VP_ISLOCKED(vp); > > if (vp->v_op->vop_access == NULL) > @@ -202,6 +206,7 @@ VOP_GETATTR(struct vnode *vp, struct vat > a.a_cred = cred; > a.a_p = p; > > + KASSERT(p == curproc); > if (vp->v_op->vop_getattr == NULL) > return (EOPNOTSUPP); > > @@ -219,6 +224,7 @@ VOP_SETATTR(struct vnode *vp, struct vat > a.a_cred = cred; > a.a_p = p; > > + KASSERT(p == curproc); > ASSERT_VP_ISLOCKED(vp); > > if (vp->v_op->vop_setattr == NULL) > @@ -282,6 +288,7 @@ VOP_IOCTL(struct vnode *vp, u_long comma > a.a_cred = cred; > a.a_p = p; > > + KASSERT(p == curproc); > if (vp->v_op->vop_ioctl == NULL) > return (EOPNOTSUPP); > > @@ -300,6 +307,7 @@ VOP_POLL(struct vnode *vp, int fflag, in > a.a_events = events; > a.a_p = p; > > + KASSERT(p == curproc); > if (vp->v_op->vop_poll == NULL) > return (EOPNOTSUPP); > > @@ -344,6 +352,7 @@ VOP_FSYNC(struct vnode *vp, struct ucred > a.a_waitfor = waitfor; > a.a_p = p; > > + KASSERT(p == curproc); > ASSERT_VP_ISLOCKED(vp); > > if (vp->v_op->vop_fsync == NULL) > @@ -565,6 +574,7 @@ VOP_INACTIVE(struct vnode *vp, struct pr > a.a_vp = vp; > a.a_p = p; > > + KASSERT(p == curproc); > ASSERT_VP_ISLOCKED(vp); > > if (vp->v_op->vop_inactive == NULL) > @@ -581,6 +591,7 @@ VOP_RECLAIM(struct vnode *vp, struct pro > a.a_vp = vp; > a.a_p = p; > > + KASSERT(p == curproc); > if (vp->v_op->vop_reclaim == NULL) > return (EOPNOTSUPP); > >
amap: KASSERT()s and local variables
Use more KASSERT()s instead of the "if (x) panic()" idiom for sanity
checks and add a couple of local variables to reduce the difference
with NetBSD and help for upcoming locking.
ok?
Index: uvm/uvm_amap.c
===
RCS file: /cvs/src/sys/uvm/uvm_amap.c,v
retrieving revision 1.84
diff -u -p -r1.84 uvm_amap.c
--- uvm/uvm_amap.c 25 Sep 2020 08:04:48 - 1.84
+++ uvm/uvm_amap.c 1 Oct 2020 12:13:23 -
@@ -460,8 +460,7 @@ amap_wipeout(struct vm_amap *amap)
map ^= 1 << slot;
anon = chunk->ac_anon[slot];
- if (anon == NULL || anon->an_ref == 0)
- panic("amap_wipeout: corrupt amap");
+ KASSERT(anon != NULL && anon->an_ref != 0);
refs = --anon->an_ref;
if (refs == 0) {
@@ -669,9 +668,7 @@ ReStart:
pg = anon->an_page;
/* page must be resident since parent is wired */
- if (pg == NULL)
- panic("amap_cow_now: non-resident wired page"
- " in anon %p", anon);
+ KASSERT(pg != NULL);
/*
* if the anon ref count is one, we are safe (the child
@@ -740,24 +737,23 @@ ReStart:
void
amap_splitref(struct vm_aref *origref, struct vm_aref *splitref, vaddr_t
offset)
{
+ struct vm_amap *amap = origref->ar_amap;
int leftslots;
AMAP_B2SLOT(leftslots, offset);
- if (leftslots == 0)
- panic("amap_splitref: split at zero offset");
+ KASSERT(leftslots != 0);
- /* now: we have a valid am_mapped array. */
- if (origref->ar_amap->am_nslot - origref->ar_pageoff - leftslots <= 0)
- panic("amap_splitref: map size check failed");
+ KASSERT(amap->am_nslot - origref->ar_pageoff - leftslots > 0);
#ifdef UVM_AMAP_PPREF
-/* establish ppref before we add a duplicate reference to the amap */
- if (origref->ar_amap->am_ppref == NULL)
- amap_pp_establish(origref->ar_amap);
+/* Establish ppref before we add a duplicate reference to the amap. */
+ if (amap->am_ppref == NULL)
+ amap_pp_establish(amap);
#endif
- splitref->ar_amap = origref->ar_amap;
- splitref->ar_amap->am_ref++;/* not a share reference */
+ /* Note: not a share reference. */
+ amap->am_ref++;
+ splitref->ar_amap = amap;
splitref->ar_pageoff = origref->ar_pageoff + leftslots;
}
@@ -828,9 +824,7 @@ amap_pp_adjref(struct vm_amap *amap, int
* now adjust reference counts in range. merge the first
* changed entry with the last unchanged entry if possible.
*/
- if (lcv != curslot)
- panic("amap_pp_adjref: overshot target");
-
+ KASSERT(lcv == curslot);
for (/* lcv already set */; lcv < stopslot ; lcv += len) {
pp_getreflen(ppref, lcv, &ref, &len);
if (lcv + len > stopslot) { /* goes past end? */
@@ -840,8 +834,7 @@ amap_pp_adjref(struct vm_amap *amap, int
len = stopslot - lcv;
}
ref += adjval;
- if (ref < 0)
- panic("amap_pp_adjref: negative reference count");
+ KASSERT(ref >= 0);
if (lcv == prevlcv + prevlen && ref == prevref) {
pp_setreflen(ppref, prevlcv, ref, prevlen + len);
} else {
@@ -1104,20 +1097,17 @@ amap_add(struct vm_aref *aref, vaddr_t o
slot = UVM_AMAP_SLOTIDX(slot);
if (replace) {
- if (chunk->ac_anon[slot] == NULL)
- panic("amap_add: replacing null anon");
- if (chunk->ac_anon[slot]->an_page != NULL &&
- (amap->am_flags & AMAP_SHARED) != 0) {
- pmap_page_protect(chunk->ac_anon[slot]->an_page,
- PROT_NONE);
+ struct vm_anon *oanon = chunk->ac_anon[slot];
+
+ KASSERT(oanon != NULL);
+ if (oanon->an_page && (amap->am_flags & AMAP_SHARED) != 0) {
+ pmap_page_protect(oanon->an_page, PROT_NONE);
/*
* XXX: suppose page is supposed to be wired somewhere?
*/
}
} else { /* !replace */
- if (chunk->ac_anon[slot] != NULL)
- panic("amap_add: slot in use");
-
+ KASSERT(chunk->ac_anon[slot] == NULL);
chunk->ac_usedmap |= 1 << slot;
amap->am_nused++;
}
@@ -1140,12 +1130,10 @@ amap_unadd(struct vm_aref *aref, vaddr_t
slot += aref->ar_pageoff;
KASSERT(slot < amap->am_nslot);
chunk = ama
mmap: Do not push KERNEL_LOCK() too far
While studying a bug report from naddy@ in 2017 when testing guenther@'s
amap/anon locking diff I figured out that we have been too optimistic in
the !MAP_ANON case.
The reported panic involves, I'd guess, a race between fd_getfile() and
vref():
panic: vref used where vget required
db_enter() at db_enter+0x5
panic() at panic+0x129
vref(ff03b20d29e8) at vref+0x5d
uvn_attach(101,ff03a5879dc0) at uvn_attach+0x11d
uvm_mmapfile(7,ff03a5879dc0,2,1,13,10012) at uvm_mmapfile+0x12c
sys_mmap(c50,8000225f82a0,1) at sys_mmap+0x604
syscall() at syscall+0x279
--- syscall (number 198) ---
end of kernel
Removing the KERNEL_LOCK() from file mapping was out of the scope of this
previous work, so I'd like to go back to a single KERNEL_LOCK/UNLOCK dance
in this code path to remove any false positive.
Note that this code is currently always run under KERNEL_LOCK() so this
will only have effect once the syscall will be unlocked.
ok?
Index: uvm/uvm_mmap.c
===
RCS file: /cvs/src/sys/uvm/uvm_mmap.c,v
retrieving revision 1.161
diff -u -p -r1.161 uvm_mmap.c
--- uvm/uvm_mmap.c 4 Mar 2020 21:15:39 - 1.161
+++ uvm/uvm_mmap.c 28 Sep 2020 09:48:26 -
@@ -288,8 +288,11 @@ sys_mmap(struct proc *p, void *v, regist
/* check for file mappings (i.e. not anonymous) and verify file. */
if ((flags & MAP_ANON) == 0) {
- if ((fp = fd_getfile(fdp, fd)) == NULL)
- return (EBADF);
+ KERNEL_LOCK();
+ if ((fp = fd_getfile(fdp, fd)) == NULL) {
+ error = EBADF;
+ goto out;
+ }
if (fp->f_type != DTYPE_VNODE) {
error = ENODEV; /* only mmap vnodes! */
@@ -313,6 +316,7 @@ sys_mmap(struct proc *p, void *v, regist
flags |= MAP_ANON;
FRELE(fp, p);
fp = NULL;
+ KERNEL_UNLOCK();
goto is_anon;
}
@@ -362,9 +366,7 @@ sys_mmap(struct proc *p, void *v, regist
* EPERM.
*/
if (fp->f_flag & FWRITE) {
- KERNEL_LOCK();
error = VOP_GETATTR(vp, &va, p->p_ucred, p);
- KERNEL_UNLOCK();
if (error)
goto out;
if ((va.va_flags & (IMMUTABLE|APPEND)) == 0)
@@ -390,9 +392,9 @@ sys_mmap(struct proc *p, void *v, regist
goto out;
}
}
- KERNEL_LOCK();
error = uvm_mmapfile(&p->p_vmspace->vm_map, &addr, size, prot,
maxprot, flags, vp, pos, lim_cur(RLIMIT_MEMLOCK), p);
+ FRELE(fp, p);
KERNEL_UNLOCK();
} else {/* MAP_ANON case */
if (fd != -1)
@@ -428,7 +430,10 @@ is_anon: /* label for SunOS style /dev/z
/* remember to add offset */
*retval = (register_t)(addr + pageoff);
+ return (error);
+
out:
+ KERNEL_UNLOCK();
if (fp)
FRELE(fp, p);
return (error);
drm: avoid possible deadlock in kthread_stop
Hi,
Currently, when a process is calling kthread_stop(), it sets a flag
asking the thread to stop, and enters in sleep mode, but the code
doing the stop doesn't wakeup the caller of kthread_stop().
The thread should also be unparked as else it will not seen the
KTHREAD_SHOULDSTOP flag. it follows what Linux is doing.
While here, I added some comments in the locking logic for park/unpark
and stop.
Comments or OK ?
Thanks.
--
Sebastien Marie
---
commit 70e71461c8598e28820f1743923cac40670f7c33
from: Sébastien Marie
date: Thu Oct 1 07:02:46 2020 UTC
properly support kthread_stop()
- wakeup pthread_stop() caller
- unpark the thread if parked
while here, add comments for locking logic for park/unpark/stop
diff ec329a4429e2542bc24dd017b8001b22df43564c
ce2b5031503711bbdd7a3067c76c4f18b1d8da82
blob - 2cbd0905406ccc9d89c86cee38673a4e9c3fcf42
blob + f0e5a5a1b282c071c97505556510952ee7a6282a
--- sys/dev/pci/drm/drm_linux.c
+++ sys/dev/pci/drm/drm_linux.c
@@ -206,6 +206,10 @@ kthread_func(void *arg)
ret = thread->func(thread->data);
thread->flags |= KTHREAD_STOPPED;
+
+ /* wakeup thread waiting in kthread_stop() */
+ wakeup(thread);
+
kthread_exit(ret);
}
@@ -256,7 +260,14 @@ kthread_parkme(void)
while (thread->flags & KTHREAD_SHOULDPARK) {
thread->flags |= KTHREAD_PARKED;
+
+ /*
+* wakeup kthread_park() caller
+* to signal I am parked as asked.
+*/
wakeup(thread);
+
+ /* wait for someone to kthread_unpark() me */
tsleep_nsec(thread, PPAUSE, "parkme", INFSLP);
thread->flags &= ~KTHREAD_PARKED;
}
@@ -269,7 +280,13 @@ kthread_park(struct proc *p)
while ((thread->flags & KTHREAD_PARKED) == 0) {
thread->flags |= KTHREAD_SHOULDPARK;
+
wake_up_process(thread->proc);
+
+ /*
+* wait for thread to be parked.
+* the asked thread should call kthread_parkme()
+*/
tsleep_nsec(thread, PPAUSE, "park", INFSLP);
}
}
@@ -280,6 +297,8 @@ kthread_unpark(struct proc *p)
struct kthread *thread = kthread_lookup(p);
thread->flags &= ~KTHREAD_SHOULDPARK;
+
+ /* wakeup kthread_parkme() caller */
wakeup(thread);
}
@@ -297,7 +316,13 @@ kthread_stop(struct proc *p)
while ((thread->flags & KTHREAD_STOPPED) == 0) {
thread->flags |= KTHREAD_SHOULDSTOP;
+
+ /* kthread_unpark() the thread if parked */
+ kthread_unpark(p);
+
wake_up_process(thread->proc);
+
+ /* wait for thread to stop (func() should return) */
tsleep_nsec(thread, PPAUSE, "stop", INFSLP);
}
LIST_REMOVE(thread, next);
