Re: sysupgrade(8) and http_proxy
Anthony Coulter wrote:
> Hello @tech,
>
> When I manually upgrade OpenBSD using bsd.rd, I have to set http_proxy
> to fetch the file sets. When I reboot after installing, fw_update
> succeeds because theinstall script was clever enough to export
> http_proxy in /etc/rc.firsttime.
>
> Unfortunately sysupgrade(8) does not remember that http_proxy was set
> when it fetched the file sets, and so when I run sysupgrade I have to
> either wait for fw_update to manually time out on first reboot, or kill
> it manually with ^C.
>
> Adding the line:
>
> [ ${http_proxy} ] && echo "export http_proxy=${http_proxy}"
> >>/etc/rc.firsttime
>
> to a spot near the bottom of /usr/sbin/sysupgrade fixes my fw_update
> problem, at least until the upgrade restores all of my files to their
> stock defaults. Is this something we could integrate into the official
> repository?
>
> Thanks and regards,
> Anthony Coulter
Here it is in patch form in case there is interest. This also pulls in the
quote function from install.sub to make sure a proxy username or password is
quoted properly. I haven't tested this since I could only use it at work.
Tim.
Index: sysupgrade.sh
===
RCS file: /cvs/src/usr.sbin/sysupgrade/sysupgrade.sh,v
retrieving revision 1.29
diff -u -p -r1.29 sysupgrade.sh
--- sysupgrade.sh 26 Oct 2019 04:04:20 - 1.29
+++ sysupgrade.sh 2 Nov 2019 00:39:05 -
@@ -73,6 +73,16 @@ rmel() {
echo -n "$_c"
}
+# Prints the supplied parameters properly escaped for future sh/ksh parsing.
+# Quotes are added if needed, so you should not do that yourself.
+quote() (
+ # Since this is a subshell we won't pollute the calling namespace.
+ for _a; do
+ alias Q=$_a; _a=$(alias Q); print -rn -- " ${_a#Q=}"
+ done | sed '1s/ //'
+ echo
+)
+
RELEASE=false
SNAP=false
FORCE=false
@@ -199,6 +209,9 @@ if ! ${KEEP}; then
rm -f /home/_sysupgrade/{${CLEAN}}
__EOT
fi
+
+[[ -n $http_proxy ]] &&
+ quote export "http_proxy=$http_proxy" >>/etc/rc.firsttime
install -F -m 700 bsd.rd /bsd.upgrade
sync
Re: _pbuild user to have priority=5
Ted Unangst wrote: > Theo de Raadt wrote: > > What about all the other users who aren't in staff? > > > > I think the approach is right. Push non-interactive down. > > The same then for src build user? Well, that's different. Most of us building the src tree are waiting eagerly for it to finish aren't we?
Re: _pbuild user to have priority=5
Theo de Raadt wrote: > What about all the other users who aren't in staff? > > I think the approach is right. Push non-interactive down. The same then for src build user?
Time-out like behaviour for ICMP port unreachable errors
Hello,
I'm sending same patch I've sent to bugs@ to thread [1], which got started by
Paul de Weerd few days ago. I've received few questions off-list, which made me
to shovel bit more through TCP code in OpenBSD. I hope I can better explain
impact of change introduced by patch below. I think it's worth if I'll post
answers to those off-list questions to tech@, as it is seems better place to
discuss the matter.
> Are you not undoing all the hard work by cloder and fgsch and previously
> markus -- to make icmp errors become "soft errors" rather than "hard errors",
> otherwise I can spoof icmp's and tear down your connections?
the tcp_notify() function gets called from tcp*_ctlinput() [2]. In our
particular case of ICMP error message we arrive to tcp*_ctlinput() from
icmp_input_if() [3], which handles inbound ICMP packet.
the tcp*_ctlinput() unwraps payload of ICMP error message. The payload
is header portion of packet, which triggered the ICMP error. *UNREACHABLE
ICMP errors are handled at the bottom of tcp*_ctlinput() [4]:
792
793 if (ip) {
794 th = (struct tcphdr *)((caddr_t)ip + (ip->ip_hl << 2));
795 inp = in_pcbhashlookup(&tcbtable,
796 ip->ip_dst, th->th_dport, ip->ip_src, th->th_sport,
797 rdomain);
798 if (inp) {
799 seq = ntohl(th->th_seq);
800 if (inp->inp_socket &&
801 (tp = intotcpcb(inp)) &&
802 SEQ_GEQ(seq, tp->snd_una) &&
803 SEQ_LT(seq, tp->snd_max))
804 notify(inp, errno);
805 } else if (inetctlerrmap[cmd] == EHOSTUNREACH ||
806 inetctlerrmap[cmd] == ENETUNREACH ||
807 inetctlerrmap[cmd] == EHOSTDOWN) {
808 struct sockaddr_in sin;
809
810 bzero(&sin, sizeof(sin));
811 sin.sin_len = sizeof(sin);
812 sin.sin_family = AF_INET;
813 sin.sin_port = th->th_sport;
814 sin.sin_addr = ip->ip_src;
815 syn_cache_unreach(sintosa(&sin), sa, th, rdomain);
816 }
817 } else
818 in_pcbnotifyall(&tcbtable, sa, rdomain, errno, notify);
if we could find `inp` at line 795, then ICMP message comes as a response
to packet sent by bound network client socket. If there is a matching socket
waiting for response we call tcp_notify() at 804. `notify()` is a pointer,
which
points to tcp_notify()
The else branch at 805 deals with case, where we deal with ICMP error,
which comes as a response to packet sent by listen socket. This typically
happens when we receive error to our SYN-ACK.
> Could it be that this was something we changed when the various TCP RST
> attacks were a big deal? That was in 2004/2005 IIRC. We for sure made sure
I think it's just omission in old code. The tcp_notify() has not
change sine 1995.
> that the TCP RST needs to be at the correct place. Not sure how well the
> ICMP is checked to ensure that nobody can use them for attacks (we did for
> sure made the checks more strict).
yes those checks happen in tcp_ctlinput() before tcp_notify() gets
called at lines 802, 803
>
> Your diff seems to only pass the error up if the connection is not
> established which is probably safe.
I think we should also consider to report ECONNREFUSED immediately for
sockets, which are in established state. Currently this case is handled
as 'softerror' case. The softerror is still kind of mystery. It seems
to me we just poke/wake blocked caller, which won't spot any error and
will most likely call read()/write() again. I hope tech@ will be able
to shed some light here...
> i found the same code when i read Pauls mail. I also found
> syn_cache_unreach(), which gets called with
>
> } else if (inetctlerrmap[cmd] == EHOSTUNREACH ||
>inetctlerrmap[cmd] == ENETUNREACH ||
>inetctlerrmap[cmd] == EHOSTDOWN) {
> ...
> syn_cache_unreach(sintosa(&sin), sa, th, rdomain);
>
> and contains this:
>
>/*
> * If we've retransmitted 3 times and this is our second error,
> * we remove the entry. Otherwise, we allow it to continue on.
> * This prevents us from incorrectly nuking an entry during a
> * spurious network outage.
> *
> * See tcp_notify().
> */
>if ((sc->sc_flags & SCF_UNREACH) == 0 || sc->sc_rxtshift < 3) {
>sc->sc_flags |= SCF_UNREACH;
>return;
>}
My understanding is this was just copied from tcp_notify(), which is older
code than syn cache. Perhaps the reason was to keep behavio
Re: _pbuild user to have priority=5
Stuart Henderson wrote: > On 2019/11/01 23:32, Solene Rapenne wrote: > > On Fri, Nov 01, 2019 at 10:06:48PM -, Christian Weisgerber wrote: > > > On 2019-10-31, Solene Rapenne wrote: > > > > > > > I suggest adding a priority=5 to _pbuild user. > > > > > > Is this the right place? Or should dpb do this? > > > > > > -- > > > Christian "naddy" Weisgerber na...@mips.inka.de > > > > > > > using the _pbuild class it applies for package building using make and > > also when using dpb (which uses make). I don't understand why it should > > be handled by dpb. > > > > Building ports isn't the only thing that places demands on the system > that can result in slow interactive behaviour. If working around this > via priority in login.conf is considered acceptable (I don't like it > much myself..) would it be better to make the default class lower- > priority and set e.g. staff to something higher? What about all the other users who aren't in staff? I think the approach is right. Push non-interactive down.
Re: _pbuild user to have priority=5
On 2019/11/01 23:32, Solene Rapenne wrote: > On Fri, Nov 01, 2019 at 10:06:48PM -, Christian Weisgerber wrote: > > On 2019-10-31, Solene Rapenne wrote: > > > > > I suggest adding a priority=5 to _pbuild user. > > > > Is this the right place? Or should dpb do this? > > > > -- > > Christian "naddy" Weisgerber na...@mips.inka.de > > > > using the _pbuild class it applies for package building using make and > also when using dpb (which uses make). I don't understand why it should > be handled by dpb. > Building ports isn't the only thing that places demands on the system that can result in slow interactive behaviour. If working around this via priority in login.conf is considered acceptable (I don't like it much myself..) would it be better to make the default class lower- priority and set e.g. staff to something higher?
Re: _pbuild user to have priority=5
On Fri, Nov 01, 2019 at 10:06:48PM -, Christian Weisgerber wrote: > On 2019-10-31, Solene Rapenne wrote: > > > I suggest adding a priority=5 to _pbuild user. > > Is this the right place? Or should dpb do this? > > -- > Christian "naddy" Weisgerber na...@mips.inka.de > using the _pbuild class it applies for package building using make and also when using dpb (which uses make). I don't understand why it should be handled by dpb.
Re: uvm_map_inentry() & KERNEL_LOCK()
> Date: Fri, 1 Nov 2019 14:13:26 +0100
> From: Martin Pieuchot
>
> On 01/11/19(Fri) 13:12, Mark Kettenis wrote:
> > > From: "Ted Unangst"
> > > Date: Fri, 01 Nov 2019 00:18:35 -0400
> > >
> > > Theo de Raadt wrote:
> > > > In uvm_map_inentry_fix(), two variables in the map are now being read
> > > > without a per-map read lock, this was previously protected by the
> > > > kernel lock
> > > >
> > > > if (addr < map->min_offset || addr >= map->max_offset)
> > > > return (FALSE);
> > > >
> > > > When I wrote this I was told to either use KERNEL_LOCK() or
> > > > vm_map_lock_read(), which is vm_map_lock_read_ln() .. if
> > > > KERNEL_LOCK() is no longer good should vm_map_lock_read be moved
> > > > upwards, or are you confident those offsets are safe to read
> > > > independently without any locking??
> > >
> > > indeed, another thread can expand the map, so that should have the read
> > > lock.
> >
> > That can't happen. The map limits are only touched by uvm_map_setup()
> > and uvmspace_exec(). The former is when the map gets created and
> > isn't known to other threads yet. The latter is called from exec(2)
> > and the process is guaranteed to be single-threaded at that point.
>
> Diff below starts documenting which locking primitives apply to the
> members of "struct uvm_map", ok?
That matches my understanding of the code.
ok kettenis@
> Index: uvm/uvm_map.h
> ===
> RCS file: /cvs/src/sys/uvm/uvm_map.h,v
> retrieving revision 1.62
> diff -u -p -r1.62 uvm_map.h
> --- uvm/uvm_map.h 14 Jun 2019 05:52:43 - 1.62
> +++ uvm/uvm_map.h 1 Nov 2019 13:10:49 -
> @@ -287,15 +287,19 @@ RBT_PROTOTYPE(uvm_map_addr, vm_map_entry
> *
> *
> * read_locks and write_locks are used in lock debugging code.
> + *
> + * Locks used to protect struct members in this file:
> + * I immutable after creation or exec(2)
> + * v `vm_map_lock' (this map `lock' or `mtx')
> */
> struct vm_map {
> - struct pmap * pmap; /* Physical map */
> + struct pmap *pmap; /* [I] Physical map */
> struct rwlock lock; /* Lock for map data */
> struct mutexmtx;
> - u_long sserial;/* counts stack changes */
> - u_long wserial;/* counts PROT_WRITE increases
> */
> + u_long sserial;/* [v] # stack changes */
> + u_long wserial;/* [v] # PROT_WRITE increases */
>
> - struct uvm_map_addr addr; /* Entry tree, by addr */
> + struct uvm_map_addr addr; /* [v] Entry tree, by addr */
>
> vsize_t size; /* virtual size */
> int ref_count; /* Reference count */
> @@ -303,16 +307,16 @@ struct vm_map {
> struct mutexflags_lock; /* flags lock */
> unsigned inttimestamp; /* Version number */
>
> - vaddr_t min_offset; /* First address in map. */
> - vaddr_t max_offset; /* Last address in map. */
> + vaddr_t min_offset; /* [I] First address in map. */
> + vaddr_t max_offset; /* [I] Last address in map. */
>
> /*
>* Allocation overflow regions.
>*/
> - vaddr_t b_start;/* Start for brk() alloc. */
> - vaddr_t b_end; /* End for brk() alloc. */
> - vaddr_t s_start;/* Start for stack alloc. */
> - vaddr_t s_end; /* End for stack alloc. */
> + vaddr_t b_start;/* [v] Start for brk() alloc. */
> + vaddr_t b_end; /* [v] End for brk() alloc. */
> + vaddr_t s_start;/* [v] Start for stack alloc. */
> + vaddr_t s_end; /* [v] End for stack alloc. */
>
> /*
>* Special address selectors.
>
ospfd: more explicit error message
Newer version after a comment by claudio@.
Currently ospfd logs routing message type code instead of name.
Make it more explicit.
remi@ is OK but wonders if rtm_type_name() will be updated as needed.
What do you think ?
Denis
Index: kroute.c
===
RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v
retrieving revision 1.112
diff -u -p -r1.112 kroute.c
--- kroute.c28 Dec 2018 19:25:10 - 1.112
+++ kroute.c1 Nov 2019 22:12:18 -
@@ -1269,7 +1269,8 @@ retry:
return (0);
}
}
- log_warn("send_rtmsg: action %u, prefix %s/%u", hdr.rtm_type,
+ log_warn("send_rtmsg: action %s, prefix %s/%u",
+ rtm_type_name(hdr.rtm_type),
inet_ntoa(kroute->prefix), kroute->prefixlen);
return (0);
}
Index: logmsg.c
===
RCS file: /cvs/src/usr.sbin/ospfd/logmsg.c,v
retrieving revision 1.1
diff -u -p -r1.1 logmsg.c
--- logmsg.c2 Sep 2016 14:04:25 - 1.1
+++ logmsg.c1 Nov 2019 22:12:18 -
@@ -23,6 +23,8 @@
#include
#include
#include
+#include
+#include
#include "ospfd.h"
#include "log.h"
@@ -140,4 +142,36 @@ path_type_name(enum path_type type)
}
/* NOTREACHED */
return ("unknown");
+}
+
+const char *
+rtm_type_name(int type)
+{
+ static char buf[13];
+
+ switch(type) {
+ case RTM_ADD:
+ return("RTM_ADD");
+ case RTM_GET:
+ return("RTM_GET");
+ case RTM_CHANGE:
+ return("RTM_CHANGE");
+ case RTM_DELETE:
+ return("RTM_DELETE");
+ case RTM_IFINFO:
+ return("RTM_IFINFO");
+ case RTM_NEWADDR:
+ return("RTM_NEWADDR");
+ case RTM_DELADDR:
+ return("RTM_DELADDR");
+ case RTM_IFANNOUNCE:
+ return("RTM_IFANNOUNCE");
+ case RTM_DESYNC:
+ return("RTM_DESYNC");
+ case RTM_BFD:
+ return("RTM_BFD");
+ default:
+ snprintf(buf, sizeof(buf), "#%d", type);
+ return(buf);
+ }
}
Index: ospfd.h
===
RCS file: /cvs/src/usr.sbin/ospfd/ospfd.h,v
retrieving revision 1.104
diff -u -p -r1.104 ospfd.h
--- ospfd.h 16 May 2019 05:49:22 - 1.104
+++ ospfd.h 1 Nov 2019 22:12:18 -
@@ -598,6 +598,7 @@ const char *if_type_name(enum iface_type
const char *if_auth_name(enum auth_type);
const char *dst_type_name(enum dst_type);
const char *path_type_name(enum path_type);
+const char *rtm_type_name(int);
/* name2id.c */
u_int16_t rtlabel_name2id(const char *);
Re: _pbuild user to have priority=5
On 2019-10-31, Solene Rapenne wrote: > I suggest adding a priority=5 to _pbuild user. Is this the right place? Or should dpb do this? -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: Opportunistic DoT for unwind(8)
On Fri, Nov 01, 2019 at 09:53:28PM +0100, Florian Obser wrote:
> On Fri, Nov 01, 2019 at 09:45:37PM +0100, Florian Obser wrote:
> > On Fri, Nov 01, 2019 at 09:35:07PM +0100, Remi Locherer wrote:
> > > On Thu, Oct 31, 2019 at 08:14:04PM +0100, Otto Moerbeek wrote:
> > > > Hi,
> > > >
> > > > So here's a new diff that incorporates the bug fix mentioned plus
> > > > debug printf line changes suggested by Stuart.
> > > >
> > > > Please note that this is a diff on top of very recent current, i.e.
> > > > florian's work he committed today. That means that you need to be
> > > > up-to-date (including a recent libc update that was committed a few
> > > > days ago) to be able to test this version.
> > >
> > > I upgraded to a snapshot from today, updated the source and applied
> > > your diff. Then I did the same test as last time using pf to block port 53
> > > (block return out log inet proto {tcp udp} to !9.9.9.9 port 53).
> > >
> > > Result: the non functional type asr is selected instead of the forwarder.
> > >
> > > $ doas unwindctl status
> > > captive portal is unknown
> > >
> > > selected type status
> > > recursor dead
> > > forwarder validating (OppDoT)
> > > dhcp unknown (OppDoT)
> > >* asr dead
> > > $
> > > $ getent hosts undeadly.org
> > > $ echo $?
> > > 2
> > > $ dig +short undeadly.org @9.9.9.9
> > > 94.142.241.173
> > > $
> > >
> > > Without your patch unwind behaves similar regarding the type selection:
> > >
> > > $ doas unwindctl status
> > > captive portal is unknown
> >
> > ^ you are creating a not supported configuration.
> >
> > When we are behind a captive portal or don't know yet if we are behind
> > a captive portal resolving is forced to asr.
> >
> > That might not be very wise if asr is dead but I currently don't see
> > how this can happen in practice except with a well aimed foot-gun.
>
> Actually, I have an idea how this can happen in practice, please try this:
>
> diff --git resolver.c resolver.c
> index f59860a5e98..5bbc63f60fa 100644
> --- resolver.c
> +++ resolver.c
> @@ -1282,7 +1282,8 @@ best_resolver(void)
>
> if (captive_portal_state == PORTAL_UNKNOWN || captive_portal_state ==
> BEHIND) {
> - if (resolvers[UW_RES_ASR] != NULL) {
> + if (resolvers[UW_RES_ASR] != NULL && resolvers[UW_RES_ASR]->
> +state != DEAD) {
> res = resolvers[UW_RES_ASR];
> goto out;
> }
>
>
Yes, this makes unwind cope with this situation:
$ unwindctl status
not behind captive portal
selected type status
recursor dead
*forwarder validating
dhcp dead
asr dead
$
OK remi@
Re: [PATCH: 1/3] MMIO handler in vmm(4)
On Tue, Oct 29, 2019 at 02:17:28AM -0700, Mike Larkin wrote: > On Thu, Oct 24, 2019 at 08:54:58AM +0900, Iori YONEJI wrote: > > Hello tech@, > > > > Here is the patch discussed in the previous email. This part mainly > > covers changes in the declaration part and fault handlers. > > > > Hello, > > I read through the three diffs and have some feedback. > > First, please reformat all 3 diffs using style(9) guidelines. There are many > spaces vs tabs issues in the diffs. Thank you for your review. I'll fix the issues on this weekend. First of all, the reason of the spaces vs. tabs issues was due to mangling by the mailer. I fixed this now, but I'm not aware of other types of style issues, even I will go through the patches to find other style mismatches after started working on them. > Second, there seems to be quite a bit of important code missing here: Most of them convinced me, but let me leave a few comment for them. 1. Segment base and limit check > 1. There appears to be no handling of segment base and limits for 32 bit >instructions. These need to be read from the gdt and strictly adhered >to. For 64 bit mode, it's not as important, unless the guest has enabled >LMSLE (long mode segment limit enable) on AMD CPUs, in which case the >limits need to be checked also. If I recall correctly, there are also >some different rules that need to be followed for 32 bit segment use >relating to permission checks and segment types. I wasn't aware of limit check, but I think vcpu would be triggered #GP if the segment was out of range instead of EPT violation, wouldn't it? 2. Privilege level check > 2. For that matter, there appears to be no handling of any permission or >privilege checks in the instruction emulator. This means any privilege >level can read or write any memory in the VM. Sorry, it is my very big fault. I must check CPU mode in next post. 3-4. 'A'ccessed and 'D'irty bit management > 3. There appears to be no handling of the updating of the 'A'ccessed or >'D'irty bits on successful page table walk and writing to a page >computed by that translation. Granted, this probably needs to go into >the existing translate_gva function for the 'A' bit, but the 'D' bit >needs to also be handled here. > > 4. There appears to be no handling of the updating of the 'A' bit in the >GDT for the segment descriptor in use. Yes, but I'm not sure what would be the expected use of A/D bit management on MMIO region because it wouldn't be subject to swap in/out. I think I will understand the expected behavior after working on it, however, I couldn't get how it compromise the virtual machine features. 5-7. Prefixes > 5. The code seems to ignore any segment prefix override bytes. > > 6. The code seems to ignore forbidden instruction prefixes (like rex.w >or lock prefixes being placed before instructions where they don't >make sense or are prohibited by the SDM) > > 7. For that matter, operand size encoding prefixes don't seem to be >honoured at all Yes, there are some prefixes I missed in the patches. I will enhance them to handle those prefixes. One thing to confirm: Is it OK not to calculate in the segment registers (whether override-ed or not) but just count the bytes, because we know the address the instruction intended to access? 8. %rflags > 8. %rflags is not being updated in any scenario. It looks like there were >a few #defines for things like VMM_EMUL_ADD/SUB, which would require >setting various flags in %rflags, but I can't seem to find the code that >actually does these instructions, so maybe these are just old #defines? MOV is the only instruction in my 'top-priority' to-do list, but I will handle these instructions as well as %rflags if needed. For now, the #defines has no role but just suggesting how the parser indicates different types of the instructions, so I delete those until they get needed. > > While this appears to be a step in the right direction, I'm afraid there > is quite a bit that needs to get done before we can commit this. The > items above are not nit-pick items; they are critical for security and > without implementing these we will end up opening huge security holes > inside the VM. > > Please let me know if any of these are actually implemented somewhere > in the three diffs; it's certainly possible I missed something. > > How would you like to proceed? If you would like to clean up the style > and start implementing the items above, I'm happy to take another look > once you make some progress. Let me know. > > Thanks again for trying to make progress here. I hope we can get things > moving forward. > > -ml > Thanks again too for your precise review and feedback. I will work on those issues on this weekend (and might take a couple of weeks). In my mind, 2 is the most urgent, and 5-7 are also considered as bugs, not 'unimplemented' things. So I will start with them. I will skip supporting non-MOV i
Re: Opportunistic DoT for unwind(8)
On Fri, Nov 01, 2019 at 09:45:37PM +0100, Florian Obser wrote:
> On Fri, Nov 01, 2019 at 09:35:07PM +0100, Remi Locherer wrote:
> > On Thu, Oct 31, 2019 at 08:14:04PM +0100, Otto Moerbeek wrote:
> > > Hi,
> > >
> > > So here's a new diff that incorporates the bug fix mentioned plus
> > > debug printf line changes suggested by Stuart.
> > >
> > > Please note that this is a diff on top of very recent current, i.e.
> > > florian's work he committed today. That means that you need to be
> > > up-to-date (including a recent libc update that was committed a few
> > > days ago) to be able to test this version.
> >
> > I upgraded to a snapshot from today, updated the source and applied
> > your diff. Then I did the same test as last time using pf to block port 53
> > (block return out log inet proto {tcp udp} to !9.9.9.9 port 53).
> >
> > Result: the non functional type asr is selected instead of the forwarder.
> >
> > $ doas unwindctl status
> > captive portal is unknown
> >
> > selected type status
> > recursor dead
> > forwarder validating (OppDoT)
> > dhcp unknown (OppDoT)
> >* asr dead
> > $
> > $ getent hosts undeadly.org
> > $ echo $?
> > 2
> > $ dig +short undeadly.org @9.9.9.9
> > 94.142.241.173
> > $
> >
> > Without your patch unwind behaves similar regarding the type selection:
> >
> > $ doas unwindctl status
> > captive portal is unknown
>
> ^ you are creating a not supported configuration.
>
> When we are behind a captive portal or don't know yet if we are behind
> a captive portal resolving is forced to asr.
>
> That might not be very wise if asr is dead but I currently don't see
> how this can happen in practice except with a well aimed foot-gun.
Actually, I have an idea how this can happen in practice, please try this:
diff --git resolver.c resolver.c
index f59860a5e98..5bbc63f60fa 100644
--- resolver.c
+++ resolver.c
@@ -1282,7 +1282,8 @@ best_resolver(void)
if (captive_portal_state == PORTAL_UNKNOWN || captive_portal_state ==
BEHIND) {
- if (resolvers[UW_RES_ASR] != NULL) {
+ if (resolvers[UW_RES_ASR] != NULL && resolvers[UW_RES_ASR]->
+state != DEAD) {
res = resolvers[UW_RES_ASR];
goto out;
}
>
> >
> > selected type status
> > recursor dead
> > forwarder validating
> > dhcp dead
> >* asr dead
> > $
> >
>
> --
> I'm not entirely sure you are real.
>
--
I'm not entirely sure you are real.
Re: Opportunistic DoT for unwind(8)
On Fri, Nov 01, 2019 at 09:35:07PM +0100, Remi Locherer wrote:
> On Thu, Oct 31, 2019 at 08:14:04PM +0100, Otto Moerbeek wrote:
> > Hi,
> >
> > So here's a new diff that incorporates the bug fix mentioned plus
> > debug printf line changes suggested by Stuart.
> >
> > Please note that this is a diff on top of very recent current, i.e.
> > florian's work he committed today. That means that you need to be
> > up-to-date (including a recent libc update that was committed a few
> > days ago) to be able to test this version.
>
> I upgraded to a snapshot from today, updated the source and applied
> your diff. Then I did the same test as last time using pf to block port 53
> (block return out log inet proto {tcp udp} to !9.9.9.9 port 53).
>
> Result: the non functional type asr is selected instead of the forwarder.
>
> $ doas unwindctl status
> captive portal is unknown
>
> selected type status
> recursor dead
> forwarder validating (OppDoT)
> dhcp unknown (OppDoT)
>* asr dead
> $
> $ getent hosts undeadly.org
> $ echo $?
> 2
> $ dig +short undeadly.org @9.9.9.9
> 94.142.241.173
> $
>
> Without your patch unwind behaves similar regarding the type selection:
>
> $ doas unwindctl status
> captive portal is unknown
^ you are creating a not supported configuration.
When we are behind a captive portal or don't know yet if we are behind
a captive portal resolving is forced to asr.
That might not be very wise if asr is dead but I currently don't see
how this can happen in practice except with a well aimed foot-gun.
>
> selected type status
> recursor dead
> forwarder validating
> dhcp dead
>* asr dead
> $
>
--
I'm not entirely sure you are real.
Re: Opportunistic DoT for unwind(8)
On Thu, Oct 31, 2019 at 08:14:04PM +0100, Otto Moerbeek wrote:
> Hi,
>
> So here's a new diff that incorporates the bug fix mentioned plus
> debug printf line changes suggested by Stuart.
>
> Please note that this is a diff on top of very recent current, i.e.
> florian's work he committed today. That means that you need to be
> up-to-date (including a recent libc update that was committed a few
> days ago) to be able to test this version.
I upgraded to a snapshot from today, updated the source and applied
your diff. Then I did the same test as last time using pf to block port 53
(block return out log inet proto {tcp udp} to !9.9.9.9 port 53).
Result: the non functional type asr is selected instead of the forwarder.
$ doas unwindctl status
captive portal is unknown
selected type status
recursor dead
forwarder validating (OppDoT)
dhcp unknown (OppDoT)
* asr dead
$
$ getent hosts undeadly.org
$ echo $?
2
$ dig +short undeadly.org @9.9.9.9
94.142.241.173
$
Without your patch unwind behaves similar regarding the type selection:
$ doas unwindctl status
captive portal is unknown
selected type status
recursor dead
forwarder validating
dhcp dead
* asr dead
$
Re: /bin/cp: overwrite symlink with file pointed to by symlink
> Hi,
Hello Christopher,
> I admit this is a very special case, but anyway this is what I hit:
>
> $ touch blub; ln -s blub blab; ls -l; cp -f blub blab
> total 0
> lrwxr-xr-x 1 madroach wheel 4 Oct 30 17:39 blab -> blub
> -rw-r--r-- 1 madroach wheel 0 Oct 30 17:39 blub
> cp: blab and blub are identical (not copied).
This is expected behaviour, blab and blub are indeed the same file and
so copy should be skipped.
From POSIX:
“If source_file references the same file as dest_file, cp may write a
diagnostic message to standard error; it shall do nothing more with
source_file and shall go on to any remaining files.”
> Now I had a look at our cp.c, our cp(1) and at POSIX:
>
> -f For each existing destination pathname, remove it and
> create a new file, without prompting for confirmation, regardless of
> its permissions. The -f option overrides any previous -i options.
>
> POSIX says cp must always first try to open(2)(O_TRUNC) the
> destination and only if this fails AND -f was specified should it try
> unlink(2).
>
> At least regarding this special case our cp behaves according to
> POSIX, not according to our manpage.
>
> As long as we cannot change the POSIX standard I don't know whether
> our manpage should mention this or cp.c be changed to use lstat(),
> too. Maybe this is such a special case we could just ignore it...
If you're interested in adapting to POSIX, here's a proposition patch:
Index: bin/cp//cp.1
===
RCS file: /var/cvs/src/bin/cp/cp.1,v
retrieving revision 1.40
diff -u -r1.40 cp.1
--- bin/cp//cp.128 Jan 2019 18:58:42 - 1.40
+++ bin/cp//cp.11 Nov 2019 19:41:26 -
@@ -79,9 +79,8 @@
Same as
.Fl RpP .
.It Fl f
-For each existing destination pathname, remove it and
-create a new file, without prompting for confirmation,
-regardless of its permissions.
+For each existing destination pathname which cannot be opened, remove
+it and create a new file, without prompting for confirmation.
The
.Fl f
option overrides any previous
Index: bin/cp//utils.c
===
RCS file: /var/cvs/src/bin/cp/utils.c,v
retrieving revision 1.47
diff -u -r1.47 utils.c
--- bin/cp//utils.c 28 Jan 2019 18:58:42 - 1.47
+++ bin/cp//utils.c 1 Nov 2019 19:25:02 -
@@ -55,7 +55,7 @@
static char *buf;
static char *zeroes;
struct stat to_stat, *fs;
- int from_fd, rcount, rval, to_fd, wcount;
+ int from_fd, rcount, rval, to_fd, wcount, create = 1;
#ifdef VM_AND_BUFFER_CACHE_SYNCHRONIZED
char *p;
#endif
@@ -79,26 +79,24 @@
fs = entp->fts_statp;
/*
-* In -f (force) mode, we always unlink the destination first
-* if it exists. Note that -i and -f are mutually exclusive.
-*/
- if (exists && fflag)
- (void)unlink(to.p_path);
-
- /*
* If the file DNE, set the mode to be the from file, minus setuid
* bits, modified by the umask; arguably wrong, but it makes copying
* executables work right and it's been that way forever. (The
* other choice is 666 or'ed with the execute bits on the from file
* modified by the umask.)
*/
- if (exists && !fflag) {
- if (!copy_overwrite()) {
+ if (exists) {
+ /* Note that -i and -f are mutually exclusive. */
+ if (!fflag && !copy_overwrite()) {
(void)close(from_fd);
return 2;
}
to_fd = open(to.p_path, O_WRONLY | O_TRUNC, 0);
- } else
+ if (to_fd != -1 || fflag && unlink(to.p_path) == -1)
+ create = 0;
+ }
+
+ if (create)
to_fd = open(to.p_path, O_WRONLY | O_TRUNC | O_CREAT,
fs->st_mode & ~(S_ISTXT | S_ISUID | S_ISGID));
Re: vmm disk unavailable after forceful vm termination
On Fri, Nov 01, 2019 at 09:20:58AM -0400, Johan Huldtgren wrote: > hello, > > I have vmd running on -current, in it I have an Ubuntu vm (18.04.3 LTS), > every now and then the Ubuntu vm will hang hard, console is dead, only > option is to restart it. Now at that point a graceful restart won't > work, 'vmctl stop n' will run and return but nothing will happen. So > the only option is 'vmctl stop -f n', this will kill the vm. However > after that the vm will disapear from the list of vms > > Before killing: > > $ vmctl status > ID PID VCPUS MAXMEM CURMEM TTYOWNERSTATE NAME > 2 71924 12.0G1.1G ttyp2johan running plex.vm > 1 18308 18.0G6.5G ttyp0johan running monitor.vm > 3 - 11.0G - -johan stopped > amd64-ports.vm > 4 - 1512M - -johan stopped > i386-ports.vm > > After killing: > > $ vmctl stop -f 2 > stopping vm: forced to terminate vm 2 > > $ vmctl status > ID PID VCPUS MAXMEM CURMEM TTYOWNERSTATE NAME > 1 18308 18.0G6.5G ttyp0johan running monitor.vm > 3 - 11.0G - -johan stopped > amd64-ports.vm > 4 - 1512M - -johan stopped > i386-ports.vm > > The vm is now gone, I can't just start it with 'vmctl start n', > but it's defined in vm.conf so let's try starting it by name: > > $ vmctl start plex.vm > vmctl: start vm command failed: Operation already in progress > > ok.. so let's restart vmd. > > $ doas rcctl stop vmd > vmd(ok) > $ doas rcctl start vmd > vmd(ok) > > but it's not actually running. > > $ vmctl status > vmctl: connect: /var/run/vmd.sock: Connection refused > > Let's try starting manually with debug > > $ doas vmd -d > startup > warning: macro 'sets' not used > can't open disk /ftp/vm/plex.img: Resource temporarily unavailable > failed to start vm plex.vm > parent: configuration failed > priv exiting, pid 9509 > control exiting, pid 63731 > vmm exiting, pid 28106 > > So it seems the disk image is now in some state which won't let it be read > again? > > $ ls -al /ftp/vm/plex.img > -rw--- 1 root wheel 32212254720 Nov 1 08:07 /ftp/vm/plex.img > > $ doas file /ftp/vm/plex.img > /ftp/vm/plex.img: x86 boot sector; partition 1: ID=0x83, active, starthead > 32, startsector 2048, 62910464 sectors > > At this point the only solution is rebooting the host running vmd. > After that everything will work just fine again until the next time > this happens. I don't know if this is known or a bug, googling and > scanning through marc.info I couldn't find any reports. I don't know > if this is relevant but when I restarted this vm yesterday (along with > hanging hard it sometimes just dies and is reported as stopped). I see > this in /var/log/daemon > > Oct 31 07:58:25 absu vmd[17613]: plex.vm: started vm 2 successfully, tty > /dev/ttyp2 > Oct 31 07:58:29 absu vmd[71924]: vcpu_process_com_data: guest reading com1 > when not ready > Oct 31 07:58:35 absu vmd[71924]: vioblk_notifyq: unsupported command 0x8 > Oct 31 07:58:52 absu vmd[71924]: vcpu_process_com_data: guest reading com1 > when not ready > > Full dmesg of the vmd host below, let me know if I can provide any further > details. > > thanks, > > .jh > Sometimes vmd gets really stuck like this and even vmctl stop -f won't stop it and rcctl stop vmd also fails. You probably have a vmd spinning at 100%, kill that one manually and it should free things up. I know about the problem, but have not had a chance to fix it yet. -ml > --- > > OpenBSD 6.6-current (GENERIC.MP) #407: Mon Oct 28 00:42:58 MDT 2019 > dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP > real mem = 137318658048 (130957MB) > avail mem = 133144268800 (126976MB) > mpath0 at root > scsibus0 at mpath0: 256 targets > mainbus0 at root > bios0 at mainbus0: SMBIOS rev. 2.8 @ 0x7db4c000 (134 entries) > bios0: vendor American Megatrends Inc. version "3703" date 04/24/2018 > bios0: ASUSTeK COMPUTER INC. Z10PE-D16 Series > acpi0 at bios0: ACPI 5.0 > acpi0: sleep states S0 S4 S5 > acpi0: tables DSDT FACP APIC FPDT FIDT MCFG EINJ UEFI HPET MSCT SLIT SRAT > WDDT SSDT SPMI SSDT SSDT PRAD DMAR HEST BERT ERST > acpi0: wakeup devices IP2P(S3) EHC1(S4) BR1A(S4) BR1B(S4) BR2A(S4) BR2B(S4) > BR2C(S4) BR2D(S4) BR3A(S4) BR3B(S4) BR3C(S4) BR3D(S4) RP01(S4) RP02(S4) > RP03(S4) RP04(S4) [...] > acpitimer0 at acpi0: 3579545 Hz, 24 bits > acpimadt0 at acpi0 addr 0xfee0: PC-AT compat > cpu0 at mainbus0: apid 0 (boot processor) > cpu0: Intel(R) Xeon(R) CPU E5-2620 v3 @ 2.40GHz, 2394.84 MHz, 06-3f-02 > cpu0: > FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,DCA,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,A
ospfd: more explicit error message
Currently ospfd logs routing message type code instead of name.
Make it more explicit.
remi@ is OK but wonders if rtm_type_name() will be updated as needed (that's why
I also log the typecode)
What do you think ?
Denis
Index: kroute.c
===
RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v
retrieving revision 1.112
diff -u -p -r1.112 kroute.c
--- kroute.c28 Dec 2018 19:25:10 - 1.112
+++ kroute.c1 Nov 2019 18:00:31 -
@@ -1269,7 +1269,8 @@ retry:
return (0);
}
}
- log_warn("send_rtmsg: action %u, prefix %s/%u", hdr.rtm_type,
+ log_warn("send_rtmsg: action %s(%d), prefix %s/%u",
+ rtm_type_name(hdr.rtm_type), hdr.rtm_type,
inet_ntoa(kroute->prefix), kroute->prefixlen);
return (0);
}
Index: logmsg.c
===
RCS file: /cvs/src/usr.sbin/ospfd/logmsg.c,v
retrieving revision 1.1
diff -u -p -r1.1 logmsg.c
--- logmsg.c2 Sep 2016 14:04:25 - 1.1
+++ logmsg.c1 Nov 2019 18:00:31 -
@@ -23,6 +23,8 @@
#include
#include
#include
+#include
+#include
#include "ospfd.h"
#include "log.h"
@@ -137,6 +139,35 @@ path_type_name(enum path_type type)
return ("Type 1 ext");
case PT_TYPE2_EXT:
return ("Type 2 ext");
+ }
+ /* NOTREACHED */
+ return ("unknown");
+}
+
+const char *
+rtm_type_name(int type)
+{
+ switch(type) {
+ case RTM_ADD:
+ return("RTM_ADD");
+ case RTM_GET:
+ return("RTM_GET");
+ case RTM_CHANGE:
+ return("RTM_CHANGE");
+ case RTM_DELETE:
+ return("RTM_DELETE");
+ case RTM_IFINFO:
+ return("RTM_IFINFO");
+ case RTM_NEWADDR:
+ return("RTM_NEWADDR");
+ case RTM_DELADDR:
+ return("RTM_DELADDR");
+ case RTM_IFANNOUNCE:
+ return("RTM_IFANNOUNCE");
+ case RTM_DESYNC:
+ return("RTM_DESYNC");
+ case RTM_BFD:
+ return("RTM_BFD");
}
/* NOTREACHED */
return ("unknown");
Index: ospfd.h
===
RCS file: /cvs/src/usr.sbin/ospfd/ospfd.h,v
retrieving revision 1.104
diff -u -p -r1.104 ospfd.h
--- ospfd.h 16 May 2019 05:49:22 - 1.104
+++ ospfd.h 1 Nov 2019 18:00:31 -
@@ -598,6 +598,7 @@ const char *if_type_name(enum iface_type
const char *if_auth_name(enum auth_type);
const char *dst_type_name(enum dst_type);
const char *path_type_name(enum path_type);
+const char *rtm_type_name(int);
/* name2id.c */
u_int16_t rtlabel_name2id(const char *);
Re: pthread process-private futexes [Re: CVS: cvs.openbsd.org: src]
On 24/10/19(Thu) 13:30, Stuart Henderson wrote: > On 2019/10/21 04:06, Martin Pieuchot wrote: > > CVSROOT:/cvs > > Module name:src > > Changes by: m...@cvs.openbsd.org2019/10/21 04:06:31 > > > > Modified files: > > lib/librthread : synch.h > > > > Log message: > > Use process-private futexes to avoid the uvm_map lookup overhead. > > > > While here kill unused _wait() function. > > > > ok visa@ > > > > I'm going to back this one out for now - it causes hangs with Python > (seems to be 100% reproducible). Easy/fast test case: > > cd /usr/ports/graphics/py-Pillow > make > > With this commit, it hangs after > > writing manifest file 'src/Pillow.egg-info/SOURCES.txt' ltrace shows that sempahore are used. gdb says that sem_init() is always called with `pshared' set to 0. However python forks then sleeps in futex(2), via sem_wait(3), when private futexes are used. When non-private futexes are used it it seems that threads from a process wake up the sleeping thread from a parent process: 84294/104794 python2.7 CALL futex(0x1a49bbca3008,0x1,0,0,0) 84677/611464 python2.7 CALL futex(0x1a49bbca3008,0x2,1,0,0) To me it seems that shared semaphore, that are advertised in librthread as non-supported, are in fact used. Python has a second lock implementation using mutex and condvar, I'd be interested to know if that works.
Re: uvm_map_inentry() & KERNEL_LOCK()
Mark Kettenis wrote: > > From: "Ted Unangst" > > Date: Fri, 01 Nov 2019 00:18:35 -0400 > > > > Theo de Raadt wrote: > > > In uvm_map_inentry_fix(), two variables in the map are now being read > > > without a per-map read lock, this was previously protected by the > > > kernel lock > > > > > > if (addr < map->min_offset || addr >= map->max_offset) > > > return (FALSE); > > > > > > When I wrote this I was told to either use KERNEL_LOCK() or > > > vm_map_lock_read(), which is vm_map_lock_read_ln() .. if > > > KERNEL_LOCK() is no longer good should vm_map_lock_read be moved > > > upwards, or are you confident those offsets are safe to read > > > independently without any locking?? > > > > indeed, another thread can expand the map, so that should have the read > > lock. > > That can't happen. The map limits are only touched by uvm_map_setup() > and uvmspace_exec(). The former is when the map gets created and > isn't known to other threads yet. The latter is called from exec(2) > and the process is guaranteed to be single-threaded at that point. Ah, ok then.
sysupgrade(8) and http_proxy
Hello @tech,
When I manually upgrade OpenBSD using bsd.rd, I have to set http_proxy
to fetch the file sets. When I reboot after installing, fw_update
succeeds because theinstall script was clever enough to export
http_proxy in /etc/rc.firsttime.
Unfortunately sysupgrade(8) does not remember that http_proxy was set
when it fetched the file sets, and so when I run sysupgrade I have to
either wait for fw_update to manually time out on first reboot, or kill
it manually with ^C.
Adding the line:
[ ${http_proxy} ] && echo "export http_proxy=${http_proxy}" >>/etc/rc.firsttime
to a spot near the bottom of /usr/sbin/sysupgrade fixes my fw_update
problem, at least until the upgrade restores all of my files to their
stock defaults. Is this something we could integrate into the official
repository?
Thanks and regards,
Anthony Coulter
vmm disk unavailable after forceful vm termination
hello, I have vmd running on -current, in it I have an Ubuntu vm (18.04.3 LTS), every now and then the Ubuntu vm will hang hard, console is dead, only option is to restart it. Now at that point a graceful restart won't work, 'vmctl stop n' will run and return but nothing will happen. So the only option is 'vmctl stop -f n', this will kill the vm. However after that the vm will disapear from the list of vms Before killing: $ vmctl status ID PID VCPUS MAXMEM CURMEM TTYOWNERSTATE NAME 2 71924 12.0G1.1G ttyp2johan running plex.vm 1 18308 18.0G6.5G ttyp0johan running monitor.vm 3 - 11.0G - -johan stopped amd64-ports.vm 4 - 1512M - -johan stopped i386-ports.vm After killing: $ vmctl stop -f 2 stopping vm: forced to terminate vm 2 $ vmctl status ID PID VCPUS MAXMEM CURMEM TTYOWNERSTATE NAME 1 18308 18.0G6.5G ttyp0johan running monitor.vm 3 - 11.0G - -johan stopped amd64-ports.vm 4 - 1512M - -johan stopped i386-ports.vm The vm is now gone, I can't just start it with 'vmctl start n', but it's defined in vm.conf so let's try starting it by name: $ vmctl start plex.vm vmctl: start vm command failed: Operation already in progress ok.. so let's restart vmd. $ doas rcctl stop vmd vmd(ok) $ doas rcctl start vmd vmd(ok) but it's not actually running. $ vmctl status vmctl: connect: /var/run/vmd.sock: Connection refused Let's try starting manually with debug $ doas vmd -d startup warning: macro 'sets' not used can't open disk /ftp/vm/plex.img: Resource temporarily unavailable failed to start vm plex.vm parent: configuration failed priv exiting, pid 9509 control exiting, pid 63731 vmm exiting, pid 28106 So it seems the disk image is now in some state which won't let it be read again? $ ls -al /ftp/vm/plex.img -rw--- 1 root wheel 32212254720 Nov 1 08:07 /ftp/vm/plex.img $ doas file /ftp/vm/plex.img /ftp/vm/plex.img: x86 boot sector; partition 1: ID=0x83, active, starthead 32, startsector 2048, 62910464 sectors At this point the only solution is rebooting the host running vmd. After that everything will work just fine again until the next time this happens. I don't know if this is known or a bug, googling and scanning through marc.info I couldn't find any reports. I don't know if this is relevant but when I restarted this vm yesterday (along with hanging hard it sometimes just dies and is reported as stopped). I see this in /var/log/daemon Oct 31 07:58:25 absu vmd[17613]: plex.vm: started vm 2 successfully, tty /dev/ttyp2 Oct 31 07:58:29 absu vmd[71924]: vcpu_process_com_data: guest reading com1 when not ready Oct 31 07:58:35 absu vmd[71924]: vioblk_notifyq: unsupported command 0x8 Oct 31 07:58:52 absu vmd[71924]: vcpu_process_com_data: guest reading com1 when not ready Full dmesg of the vmd host below, let me know if I can provide any further details. thanks, .jh --- OpenBSD 6.6-current (GENERIC.MP) #407: Mon Oct 28 00:42:58 MDT 2019 dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP real mem = 137318658048 (130957MB) avail mem = 133144268800 (126976MB) mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: SMBIOS rev. 2.8 @ 0x7db4c000 (134 entries) bios0: vendor American Megatrends Inc. version "3703" date 04/24/2018 bios0: ASUSTeK COMPUTER INC. Z10PE-D16 Series acpi0 at bios0: ACPI 5.0 acpi0: sleep states S0 S4 S5 acpi0: tables DSDT FACP APIC FPDT FIDT MCFG EINJ UEFI HPET MSCT SLIT SRAT WDDT SSDT SPMI SSDT SSDT PRAD DMAR HEST BERT ERST acpi0: wakeup devices IP2P(S3) EHC1(S4) BR1A(S4) BR1B(S4) BR2A(S4) BR2B(S4) BR2C(S4) BR2D(S4) BR3A(S4) BR3B(S4) BR3C(S4) BR3D(S4) RP01(S4) RP02(S4) RP03(S4) RP04(S4) [...] acpitimer0 at acpi0: 3579545 Hz, 24 bits acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) cpu0: Intel(R) Xeon(R) CPU E5-2620 v3 @ 2.40GHz, 2394.84 MHz, 06-3f-02 cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,DCA,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,TSC_ADJUST,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,PQM,MD_CLEAR,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,MELTDOWN cpu0: 256KB 64b/line 8-way L2 cache cpu0: smt 0, core 0, package 0 mtrr: Pentium Pro MTRR support, 10 var ranges, 88 fixed ranges cpu0: apic clock running at 99MHz cpu0: mwait min=64, max=64, C-substates=0.2.1.2, IBE cpu1 at mainbus0: apid 2 (application processor) cpu1: Intel(R) Xeon(R) CPU E5-2620 v3 @ 2.40GHz, 2394.47 MHz, 06-3f-02 cpu1: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,P
Re: uvm_map_inentry() & KERNEL_LOCK()
On 01/11/19(Fri) 13:12, Mark Kettenis wrote:
> > From: "Ted Unangst"
> > Date: Fri, 01 Nov 2019 00:18:35 -0400
> >
> > Theo de Raadt wrote:
> > > In uvm_map_inentry_fix(), two variables in the map are now being read
> > > without a per-map read lock, this was previously protected by the
> > > kernel lock
> > >
> > > if (addr < map->min_offset || addr >= map->max_offset)
> > > return (FALSE);
> > >
> > > When I wrote this I was told to either use KERNEL_LOCK() or
> > > vm_map_lock_read(), which is vm_map_lock_read_ln() .. if
> > > KERNEL_LOCK() is no longer good should vm_map_lock_read be moved
> > > upwards, or are you confident those offsets are safe to read
> > > independently without any locking??
> >
> > indeed, another thread can expand the map, so that should have the read
> > lock.
>
> That can't happen. The map limits are only touched by uvm_map_setup()
> and uvmspace_exec(). The former is when the map gets created and
> isn't known to other threads yet. The latter is called from exec(2)
> and the process is guaranteed to be single-threaded at that point.
Diff below starts documenting which locking primitives apply to the
members of "struct uvm_map", ok?
Index: uvm/uvm_map.h
===
RCS file: /cvs/src/sys/uvm/uvm_map.h,v
retrieving revision 1.62
diff -u -p -r1.62 uvm_map.h
--- uvm/uvm_map.h 14 Jun 2019 05:52:43 - 1.62
+++ uvm/uvm_map.h 1 Nov 2019 13:10:49 -
@@ -287,15 +287,19 @@ RBT_PROTOTYPE(uvm_map_addr, vm_map_entry
*
*
* read_locks and write_locks are used in lock debugging code.
+ *
+ * Locks used to protect struct members in this file:
+ * I immutable after creation or exec(2)
+ * v `vm_map_lock' (this map `lock' or `mtx')
*/
struct vm_map {
- struct pmap * pmap; /* Physical map */
+ struct pmap *pmap; /* [I] Physical map */
struct rwlock lock; /* Lock for map data */
struct mutexmtx;
- u_long sserial;/* counts stack changes */
- u_long wserial;/* counts PROT_WRITE increases
*/
+ u_long sserial;/* [v] # stack changes */
+ u_long wserial;/* [v] # PROT_WRITE increases */
- struct uvm_map_addr addr; /* Entry tree, by addr */
+ struct uvm_map_addr addr; /* [v] Entry tree, by addr */
vsize_t size; /* virtual size */
int ref_count; /* Reference count */
@@ -303,16 +307,16 @@ struct vm_map {
struct mutexflags_lock; /* flags lock */
unsigned inttimestamp; /* Version number */
- vaddr_t min_offset; /* First address in map. */
- vaddr_t max_offset; /* Last address in map. */
+ vaddr_t min_offset; /* [I] First address in map. */
+ vaddr_t max_offset; /* [I] Last address in map. */
/*
* Allocation overflow regions.
*/
- vaddr_t b_start;/* Start for brk() alloc. */
- vaddr_t b_end; /* End for brk() alloc. */
- vaddr_t s_start;/* Start for stack alloc. */
- vaddr_t s_end; /* End for stack alloc. */
+ vaddr_t b_start;/* [v] Start for brk() alloc. */
+ vaddr_t b_end; /* [v] End for brk() alloc. */
+ vaddr_t s_start;/* [v] Start for stack alloc. */
+ vaddr_t s_end; /* [v] End for stack alloc. */
/*
* Special address selectors.
Re: uvm_map_inentry() & KERNEL_LOCK()
> Date: Fri, 1 Nov 2019 00:18:43 +0100
> From: Martin Pieuchot
>
> When a userland program triggers a fault or does a syscall its SP and/or
> PC are checked to be sure they are in expected regions. The result of
> this check is cached in the "struct proc" such that a lookup isn't
> always necessary.
>
> Currently when the cache is outdated the KERNEL_LOCK() is taken before
> doing the lookup. This shouldn't be necessary, uvm_map_lookup_entry()
> is protected by the `vm_map_lock' also taken before the lookup. This
> function is already called w/o KERNEL_LOCK(), like in the futex code,
> so it should be safe to push it down there.
>
> Removing the KERNEL_LOCK() from this code path reduces the overall time
> spinning when executing syscalls from 30% to 25% when building the libc
> on my 16 CPU sparc64.
>
> Even if it doesn't improve the performances significantly I'd like to
> have more code exercising the `vm_map_lock', cause it's one of the
> pieces to unlock uvm_fault().
>
> While here document that `p_spinentry' and `p_pcinentry' are owned by
> the current process and don't need any locking.
>
> Tests, comments and oks welcome :o)
As indicated in my reply to tedu@, I think the change is ok.
However, I think you should seperate out the unrelated changes into a
separate diff. Just in case this has to come out again; you don't
want to lose the header file changes.
> Index: uvm/uvm_map.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> retrieving revision 1.247
> diff -u -p -r1.247 uvm_map.c
> --- uvm/uvm_map.c 9 Sep 2019 20:02:26 - 1.247
> +++ uvm/uvm_map.c 31 Oct 2019 21:45:51 -
> @@ -158,6 +158,10 @@ int uvm_map_findspace(struct
> vm_map*,
> vsize_t uvm_map_addr_augment_get(struct vm_map_entry*);
> void uvm_map_addr_augment(struct vm_map_entry*);
>
> +int uvm_map_inentry_recheck(u_long, vaddr_t,
> + struct p_inentry *);
> +boolean_t uvm_map_inentry_fix(struct proc *, struct p_inentry *,
> + vaddr_t, int (*)(vm_map_entry_t), u_long);
> /*
> * Tree management functions.
> */
> @@ -1868,16 +1872,16 @@ uvm_map_inentry(struct proc *p, struct p
> boolean_t ok = TRUE;
>
> if (uvm_map_inentry_recheck(serial, addr, ie)) {
> - KERNEL_LOCK();
> ok = uvm_map_inentry_fix(p, ie, addr, fn, serial);
> if (!ok) {
> printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid,
> addr, ie->ie_start, ie->ie_end);
> + KERNEL_LOCK();
> p->p_p->ps_acflag |= AMAP;
> sv.sival_ptr = (void *)PROC_PC(p);
> trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv);
> + KERNEL_UNLOCK();
> }
> - KERNEL_UNLOCK();
> }
> return (ok);
> }
> Index: uvm/uvm_map.h
> ===
> RCS file: /cvs/src/sys/uvm/uvm_map.h,v
> retrieving revision 1.62
> diff -u -p -r1.62 uvm_map.h
> --- uvm/uvm_map.h 14 Jun 2019 05:52:43 - 1.62
> +++ uvm/uvm_map.h 31 Oct 2019 21:46:00 -
> @@ -414,12 +414,8 @@ void uvm_unmap_remove(struct vm_map*, v
>
> struct p_inentry;
>
> -int uvm_map_inentry_recheck(u_long serial, vaddr_t,
> - struct p_inentry *);
> int uvm_map_inentry_sp(vm_map_entry_t);
> int uvm_map_inentry_pc(vm_map_entry_t);
> -boolean_tuvm_map_inentry_fix(struct proc *, struct p_inentry *,
> - vaddr_t addr, int (*fn)(vm_map_entry_t), u_long serial);
> boolean_tuvm_map_inentry(struct proc *, struct p_inentry *, vaddr_t addr,
> const char *fmt, int (*fn)(vm_map_entry_t), u_long serial);
>
> Index: sys/proc.h
> ===
> RCS file: /cvs/src/sys/sys/proc.h,v
> retrieving revision 1.275
> diff -u -p -r1.275 proc.h
> --- sys/proc.h22 Oct 2019 21:19:22 - 1.275
> +++ sys/proc.h31 Oct 2019 21:27:43 -
> @@ -318,6 +318,7 @@ struct p_inentry {
> * I immutable after creation
> * s scheduler lock
> * l read only reference, see lim_read_enter()
> + * o owned (read/modified) by the current proc
> */
> struct proc {
> TAILQ_ENTRY(proc) p_runq; /* [s] current run/sleep queue */
> @@ -332,8 +333,8 @@ struct proc {
> /* substructures: */
> struct filedesc *p_fd; /* copy of p_p->ps_fd */
> struct vmspace *p_vmspace; /* [I] copy of p_p->ps_vmspace */
> - struct p_inentry p_spinentry;
> - struct p_inentry p_pcinentry;
> + struct p_inentry p_spinentry; /* [o] */
> + struct p_inentry p_pcinentry; /* [o] */
> struct plimit *p_limit;
Re: uvm_map_inentry() & KERNEL_LOCK()
> From: "Ted Unangst" > Date: Fri, 01 Nov 2019 00:18:35 -0400 > > Theo de Raadt wrote: > > In uvm_map_inentry_fix(), two variables in the map are now being read > > without a per-map read lock, this was previously protected by the > > kernel lock > > > > if (addr < map->min_offset || addr >= map->max_offset) > > return (FALSE); > > > > When I wrote this I was told to either use KERNEL_LOCK() or > > vm_map_lock_read(), which is vm_map_lock_read_ln() .. if > > KERNEL_LOCK() is no longer good should vm_map_lock_read be moved > > upwards, or are you confident those offsets are safe to read > > independently without any locking?? > > indeed, another thread can expand the map, so that should have the read lock. That can't happen. The map limits are only touched by uvm_map_setup() and uvmspace_exec(). The former is when the map gets created and isn't known to other threads yet. The latter is called from exec(2) and the process is guaranteed to be single-threaded at that point.
Re: apmd: log to stderr, remove unused code
On Fri, Nov 01, 2019 at 10:19:01AM +0100, Jeremie Courreges-Anglas wrote: > debug messages should go to stderr (currently debug messages are > buffered if you send them to a pipe) > > The sched.h/ncpu bits are remnants from rev 1.46. Most of this code was > then removed in rev 1.70. OK kn
apmd: log to stderr, remove unused code
debug messages should go to stderr (currently debug messages are
buffered if you send them to a pipe)
The sched.h/ncpu bits are remnants from rev 1.46. Most of this code was
then removed in rev 1.70.
ok?
--- apmd.c.~1.89.~ Thu Oct 31 22:23:07 2019
+++ apmd.c Fri Nov 1 10:07:21 2019
@@ -36,7 +36,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -67,9 +66,6 @@ void usage(void);
int power_status(int fd, int force, struct apm_power_info *pinfo);
int bind_socket(const char *sn);
enum apm_state handle_client(int sock_fd, int ctl_fd);
-int get_avg_idle_mp(int ncpu);
-int get_avg_idle_up(void);
-void perf_status(struct apm_power_info *pinfo, int ncpu);
void suspend(int ctl_fd);
void stand_by(int ctl_fd);
void hibernate(int ctl_fd);
@@ -96,8 +92,8 @@ logmsg(int prio, const char *msg, ...)
va_start(ap, msg);
if (debug) {
- vfprintf(stdout, msg, ap);
- fprintf(stdout, "\n");
+ vfprintf(stderr, msg, ap);
+ fprintf(stderr, "\n");
} else {
vsyslog(prio, msg, ap);
}
@@ -389,9 +385,6 @@ main(int argc, char *argv[])
const char *errstr;
int kq, nchanges;
struct kevent ev[2];
- int ncpu_mib[2] = { CTL_HW, HW_NCPU };
- int ncpu;
- size_t ncpu_sz = sizeof(ncpu);
while ((ch = getopt(argc, argv, "aACdHLsf:t:S:z:Z:")) != -1)
switch(ch) {
@@ -503,9 +496,6 @@ main(int argc, char *argv[])
}
if (kevent(kq, ev, nchanges, NULL, 0, &sts) == -1)
error("kevent", NULL);
-
- if (sysctl(ncpu_mib, 2, &ncpu, &ncpu_sz, NULL, 0) == -1)
- error("cannot read hw.ncpu", NULL);
for (;;) {
int rv;
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
