Re: wall(1) unveil for non-root users
Philip Guenther wrote: > On Wed, May 15, 2019 at 10:54 PM Theo de Raadt wrote: > > Martijn van Duren wrote: > > > I don't see much point in the check. > > > > If we don't have write permissions open(2) will fail. > > If we open it based on S_IWOTH permissions than checking for S_IWGRP > > without considering who is in that group seems really absurd to me. > > > > So I'd be OK with patch 1 > > There are 3 write permission checks which could permit the open, > but then it checks the specific one. > > I believe this is similar to comsat / biff, which in those programs > are used to indicate "I am ok with messages to my tty". > > That is an old behaviour, but I bet that is the history of this S_IWGRP > check. > > Yep. mesg(1) manipulates your tty's group-write bit so that wall(1), > talk(1), and write(1) can check it. Amusing, write(1) has a very similar TOCTOU. These are not serious, but if they were fixes there are opportunities for unveil and pledge to maybe prevent other bad behaviour in the future..
Re: scheduler small changes
On Wed, May 15, 2019 at 09:05:32AM -0500, Amit Kulkarni wrote: > Hi, > > This effort is heavily based on top of Gregor's and Michal's diffs. Tried to > incorporate feedback given by different people to them in 2011/2016. Split > the new code in a ifdef, so people can do a straight comparison, tried very > hard not to delete existing code, just shifted it around. Main motivation was > to find if it is possible to do incremental improvements in scheduler. After > sometime, have still not understood completely the load avg part, > p_priority/p_usrpri calculations, the cpuset code. Looked heavily at > Dragonfly (simpler than FreeBSD) and FreeBSD code. As a newcomer, OpenBSD > code is way easier to read. Thanks for the detailed explanations given to > Michal and also in later diffs posted to tech@, they helped a lot when trying > to understand the decisions made by other devs, the commit messages help a > little but the explanations help a lot more. This invites discussions and end > users learn a lot more in the process. > > This diff survived multiple tens of kernel builds, a bsd.sp build, bsd.rd > build, a bsd.mp without these changes, userland/xenocara, a make regress a > few days ago all on 3 desktops on amd64. Ran under all possible scenarios > listed in previous sentence. No major invasive changes attempted, all tools > should work as is, if its broken, please let me know. This is faster than > current, but not sure by how much, placebo effect in play. > > I think there is a bug in resched_proc() which is fixed in mi_switch(), a > quick enhancement in cpu_switchto(), p_slptime, and precise round robin > interval for each proc unless preempted or it yields(). > > Tried to fiddle with different time slices other than hz/10, not sure if it > will work on other arches, but tried to stay within MI code, so it should > work. Other than counting no idea to verify a proc is actually switched away > after its rr interval is over. Just went by what is there in the existing > code. Tried giving higher slice like 200 ms, but didn't want to exceed the > rucheck() in kern_resource 1 sec limit. > > Tried to do rudimentary work on affinity without having a affinity field in > struct proc or struct schedstate_percpu (like Dragonfly/FreeBSD does). > Observed the unbalance in runqs. Affinity works most of the time under light > load. There's a problem when I try to comment out sched_steal_proc(), in > kern_sched, that is the problem with this iteration of the diff. > > Not sure if the reduction from 32 queues to a single TAILQ would be accepted, > but tried it anyway, it is definitely faster. This code tries to reduce the > complexity in deciding which queue to place the proc on. There is no current > support for a real-time queue or other types of scheduling classes, so IMHO > this is a simplification. > > Tried to give detailed explanation of thinking. Sent the complete diff, but > will split diff, if parts of it are found to be valid. > > In any case, a request to please accept a small change in top, to display > p_priority directly. > > Thanks > > Hi, I tried your diff. It makes game games/gzdoom unplayable because of too much stuttering. I did not feel any change apart for this case on my intel i7 8550-U.
Re: wall(1) unveil for non-root users
On Wed, May 15, 2019 at 10:54 PM Theo de Raadt wrote: > Martijn van Duren wrote: > > > I don't see much point in the check. > > > > If we don't have write permissions open(2) will fail. > > If we open it based on S_IWOTH permissions than checking for S_IWGRP > > without considering who is in that group seems really absurd to me. > > > > So I'd be OK with patch 1 > > There are 3 write permission checks which could permit the open, > but then it checks the specific one. > > I believe this is similar to comsat / biff, which in those programs > are used to indicate "I am ok with messages to my tty". > > That is an old behaviour, but I bet that is the history of this S_IWGRP > check. > Yep. mesg(1) manipulates your tty's group-write bit so that wall(1), talk(1), and write(1) can check it. Philip Guenther
Re: wall(1) unveil for non-root users
On 5/16/19 7:53 AM, Theo de Raadt wrote: > Martijn van Duren wrote: > >> I don't see much point in the check. >> >> If we don't have write permissions open(2) will fail. >> If we open it based on S_IWOTH permissions than checking for S_IWGRP >> without considering who is in that group seems really absurd to me. >> >> So I'd be OK with patch 1 > > There are 3 write permission checks which could permit the open, > but then it checks the specific one. > > I believe this is similar to comsat / biff, which in those programs > are used to indicate "I am ok with messages to my tty". > > That is an old behaviour, but I bet that is the history of this S_IWGRP check. > Sounds valid. Personally I think it's a rather obscure feature that can go as far as I'm concerned, but if people want it I'm OK with either diff. martijn@
Re: wall(1) unveil for non-root users
Martijn van Duren wrote: > I don't see much point in the check. > > If we don't have write permissions open(2) will fail. > If we open it based on S_IWOTH permissions than checking for S_IWGRP > without considering who is in that group seems really absurd to me. > > So I'd be OK with patch 1 There are 3 write permission checks which could permit the open, but then it checks the specific one. I believe this is similar to comsat / biff, which in those programs are used to indicate "I am ok with messages to my tty". That is an old behaviour, but I bet that is the history of this S_IWGRP check.
Re: wall(1) unveil for non-root users
I don't see much point in the check.
If we don't have write permissions open(2) will fail.
If we open it based on S_IWOTH permissions than checking for S_IWGRP
without considering who is in that group seems really absurd to me.
So I'd be OK with patch 1
martijn@
On 5/16/19 12:46 AM, Theo de Raadt wrote:
> Anton Borowka wrote:
>
>> wall(1) does not work correctly for non-root users at the moment because
>> ttymsg() needs read access for the tty devices, but only write access is
>> unveiled. Because it cannot access any devices nothing is printed.
>>
>> This patch adds read access for /dev. Don't know if it makes sense to
>> only unveil read access for non-root users.
>
> Apparently it stat()'s the devices [thereby desiring "r"], before calling
> open O_WRONLY [which only needs "w"].
>
> It is a TOCTOU. Here are two proposals.
>
> This one lets non-root write to any fd it can manage to open.
>
> Index: ttymsg.c
> ===
> RCS file: /cvs/src/usr.bin/wall/ttymsg.c,v
> retrieving revision 1.17
> diff -u -p -u -r1.17 ttymsg.c
> --- ttymsg.c 5 Nov 2015 22:20:11 - 1.17
> +++ ttymsg.c 15 May 2019 22:39:44 -
> @@ -87,13 +87,6 @@ ttymsg(iov, iovcnt, line, tmout)
> return (errbuf);
> }
>
> - if (getuid()) {
> - if (stat(device, &st) < 0)
> - return (NULL);
> - if ((st.st_mode & S_IWGRP) == 0)
> - return (NULL);
> - }
> -
> /*
>* open will fail on slip lines or exclusive-use lines
>* if not running as root; not an error.
>
> This one is a bit more cautious, it checks if specifically g+w
> is allowed on the tty, that is more true to the original behaviour
> isn't it?
>
> Index: ttymsg.c
> ===
> RCS file: /cvs/src/usr.bin/wall/ttymsg.c,v
> retrieving revision 1.17
> diff -u -p -u -r1.17 ttymsg.c
> --- ttymsg.c 5 Nov 2015 22:20:11 - 1.17
> +++ ttymsg.c 15 May 2019 22:42:58 -
> @@ -87,12 +87,6 @@ ttymsg(iov, iovcnt, line, tmout)
> return (errbuf);
> }
>
> - if (getuid()) {
> - if (stat(device, &st) < 0)
> - return (NULL);
> - if ((st.st_mode & S_IWGRP) == 0)
> - return (NULL);
> - }
>
> /*
>* open will fail on slip lines or exclusive-use lines
> @@ -104,6 +98,17 @@ ttymsg(iov, iovcnt, line, tmout)
> (void) snprintf(errbuf, sizeof(errbuf),
> "%s: %s", device, strerror(errno));
> return (errbuf);
> + }
> +
> + if (getuid()) {
> + if (fstat(fd, &st) < 0) {
> + close(fd);
> + return (NULL);
> + }
> + if ((st.st_mode & S_IWGRP) == 0) {
> + close(fd);
> + return (NULL);
> + }
> }
>
> for (cnt = left = 0; cnt < iovcnt; ++cnt)
>
Re: scheduler small changes
> Why did you decide to change the data structure of the runqueue? What > problem are you trying to solve? Thanks for your feedback. It forced me to do some introspection. I was trying to explore if we can tweak and make the current code faster, while still tryign to keep it as simple as it is currently. The explanation I gave in the diff for the removal of 32 TAILQs is wrong. That flawed analysis was set in my mind weeks ago, when I was just starting to read the scheduler code. I understand now that the current code is quite a bit better. It places a proc with low value of p_priority in lower array index and will return it first, and a higher value of p_priority will be placed in higher array index. So a proc is being carefully placed in sorted order of p_priority. I will revert this data structure change as the single TAILQ will not return a proc in the sorted p_priority order. > Regarding your work, if you want to continue in the scheduler area, may > I suggest you start by making the global counters per-spc and export > them to userland via a syscl? Add a new view to systat(1) to see what's > happening. Without more visibility it's hard to confirm any theory. This is an excellent idea. It will take me some time to understand the sysctl/systat part and implement, but I will try to address within a week or two. > > Regarding the choice of deriving quantum from the priority, are you sure > the priorities are correct? Should we keep priorities? Or if userland > needs priorities shouldn't we convert quantum into priority and not the > other way around? I am not entirely sure of the p_priority/usrpri/estcpu/load_avg calculations, as I am still trying to make sense of the code. But once we make sure all the p_priority calculations are consistent, I think the priorities are the way to go. If we go by deadlines, we will not have a way to understand how a proc is behaving in real time, as a p_deadline is a history variable, how much a proc used its time slices. But if we stay with priorities, it is way simpler, and ranges strictly from 0 -> 127, and will work with current code. So the code changes will be minimal and easy to understand. Also, UNIX has historically had a concept of priority/nice levels, so I think we should stick with it. IMHO, a p_deadline field is a better substitute for p_slptime. The only reason I added quantum, was that I stumbled on the round robin interval buglet. Initially added a fixed 100 ms per proc, and then decided how much I could explore this quantum idea while still trying to keep the code understandable. I would guess a lot of code in userland is based on priorities, the GNOME/KDE equivalent of top/classical top/htop etc... I would think of p_priority as a real time tracking field of how hot the proc is in using the CPU. In order to change the quantum, how would we decide to increment or decrement it, unless by a real time tracking field? There's code which already does this tracking for p_priority, it might be flawed or complex, so why not fix it and use it? > Regarding your diff, if you find a thread in RUN state inside the sleep > queue (wakeup_n()), something is really wrong there. I added that SRUN assert weeks ago when I was trying to add some code and figure out things, and broke it immediately on boot, the assert made the problem go away, and till your noticing it, I forgot about it. Removed it today and compiled kernel/userland while browsing on a desktop and a kernel build on another dekstop. It worked out fine. Thanks
Re: Reduce the scope of SCHED_LOCK()
On Sun, May 12, 2019 at 06:17:04PM -0400, Martin Pieuchot wrote:
> People started complaining that the SCHED_LOCK() is contended. Here's a
> first round at reducing its scope.
>
> Diff below introduces a per-process mutex to protect time accounting
> fields accessed in tuagg(). tuagg() is principally called in mi_switch()
> where the SCHED_LOCK() is currently held. Moving these fields out of
> its scope allows us to drop some SCHED_LOCK/UNLOCK dances in accounting
> path.
>
> Note that hardclock(9) still increments p_{u,s,i}ticks without holding a
> lock. I doubt it's worth doing anything so this diff doesn't change
> anything in that regard.
>
> Comments, oks?
>
I've been running this for 2 days on my main machine and it seems
solid.
Assuming you're confident you have mutexes in all the right places, ok
mlarkin when you're ready.
-ml
> Index: kern/init_main.c
> ===
> RCS file: /cvs/src/sys/kern/init_main.c,v
> retrieving revision 1.284
> diff -u -p -r1.284 init_main.c
> --- kern/init_main.c 26 Feb 2019 14:24:21 - 1.284
> +++ kern/init_main.c 12 May 2019 21:56:23 -
> @@ -518,7 +518,9 @@ main(void *framep)
> getnanotime(&pr->ps_start);
> TAILQ_FOREACH(p, &pr->ps_threads, p_thr_link) {
> nanouptime(&p->p_cpu->ci_schedstate.spc_runtime);
> + mtx_enter(&pr->ps_mtx);
> timespecclear(&p->p_rtime);
> + mtx_leave(&pr->ps_mtx);
> }
> }
>
> Index: kern/kern_acct.c
> ===
> RCS file: /cvs/src/sys/kern/kern_acct.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 kern_acct.c
> --- kern/kern_acct.c 28 Apr 2018 03:13:04 - 1.36
> +++ kern/kern_acct.c 12 May 2019 21:51:09 -
> @@ -179,7 +179,9 @@ acct_process(struct proc *p)
> memcpy(acct.ac_comm, pr->ps_comm, sizeof acct.ac_comm);
>
> /* (2) The amount of user and system time that was used */
> + mtx_enter(&pr->ps_mtx);
> calctsru(&pr->ps_tu, &ut, &st, NULL);
> + mtx_leave(&pr->ps_mtx);
> acct.ac_utime = encode_comp_t(ut.tv_sec, ut.tv_nsec);
> acct.ac_stime = encode_comp_t(st.tv_sec, st.tv_nsec);
>
> Index: kern/kern_exec.c
> ===
> RCS file: /cvs/src/sys/kern/kern_exec.c,v
> retrieving revision 1.203
> diff -u -p -r1.203 kern_exec.c
> --- kern/kern_exec.c 8 Feb 2019 12:51:57 - 1.203
> +++ kern/kern_exec.c 12 May 2019 21:53:56 -
> @@ -661,8 +661,10 @@ sys_execve(struct proc *p, void *v, regi
> }
>
> /* reset CPU time usage for the thread, but not the process */
> + mtx_enter(&pr->ps_mtx);
> timespecclear(&p->p_tu.tu_runtime);
> p->p_tu.tu_uticks = p->p_tu.tu_sticks = p->p_tu.tu_iticks = 0;
> + mtx_leave(&pr->ps_mtx);
>
> km_free(argp, NCARGS, &kv_exec, &kp_pageable);
>
> Index: kern/kern_exit.c
> ===
> RCS file: /cvs/src/sys/kern/kern_exit.c,v
> retrieving revision 1.173
> diff -u -p -r1.173 kern_exit.c
> --- kern/kern_exit.c 23 Jan 2019 22:39:47 - 1.173
> +++ kern/kern_exit.c 12 May 2019 21:51:22 -
> @@ -282,7 +282,7 @@ exit1(struct proc *p, int rv, int flags)
>
> /* add thread's accumulated rusage into the process's total */
> ruadd(rup, &p->p_ru);
> - tuagg(pr, p);
> + tuagg(p, NULL);
>
> /*
>* clear %cpu usage during swap
> @@ -294,7 +294,9 @@ exit1(struct proc *p, int rv, int flags)
>* Final thread has died, so add on our children's rusage
>* and calculate the total times
>*/
> + mtx_enter(&pr->ps_mtx);
> calcru(&pr->ps_tu, &rup->ru_utime, &rup->ru_stime, NULL);
> + mtx_leave(&pr->ps_mtx);
> ruadd(rup, &pr->ps_cru);
>
> /* notify interested parties of our demise and clean up */
> Index: kern/kern_fork.c
> ===
> RCS file: /cvs/src/sys/kern/kern_fork.c,v
> retrieving revision 1.209
> diff -u -p -r1.209 kern_fork.c
> --- kern/kern_fork.c 6 Jan 2019 12:59:45 - 1.209
> +++ kern/kern_fork.c 12 May 2019 21:55:57 -
> @@ -55,6 +55,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -209,6 +210,8 @@ process_initialize(struct process *pr, s
> LIST_INIT(&pr->ps_ftlist);
> LIST_INIT(&pr->ps_kqlist);
> LIST_INIT(&pr->ps_sigiolst);
> +
> + mtx_init(&pr->ps_mtx, IPL_MPFLOOR);
>
> timeout_set(&pr->ps_realit_to, realitexpire, pr);
> timeout_set(&pr->ps_rucheck_to, rucheck, pr);
> Index: kern/kern_resource.c
> ===
> RCS file: /cvs/src/sys/kern/kern_resource.c,v
> retrieving
Re: Unlock uvm a tiny bit more
On Tue, May 14, 2019 at 12:13:52AM +0200, Mark Kettenis wrote:
> This changes uvm_unmap_detach() to get rid of the "easy" entries first
> before grabbing the kernel lock. Probably doesn't help much with the
> lock contention, but it avoids a locking problem that happens with
> pools that use kernel_map() to allocate the kva for their pages.
>
> ok?
>
Reads ok to me, ok mlarkin
>
> Index: uvm/uvm_map.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> retrieving revision 1.243
> diff -u -p -r1.243 uvm_map.c
> --- uvm/uvm_map.c 23 Apr 2019 13:35:12 - 1.243
> +++ uvm/uvm_map.c 13 May 2019 22:09:26 -
> @@ -1538,8 +1538,18 @@ uvm_mapent_tryjoin(struct vm_map *map, s
> void
> uvm_unmap_detach(struct uvm_map_deadq *deadq, int flags)
> {
> - struct vm_map_entry *entry;
> + struct vm_map_entry *entry, *tmp;
> int waitok = flags & UVM_PLA_WAITOK;
> +
> + TAILQ_FOREACH_SAFE(entry, deadq, dfree.deadq, tmp) {
> + /* Skip entries for which we have to grab the kernel lock. */
> + if (entry->aref.ar_amap || UVM_ET_ISSUBMAP(entry) ||
> + UVM_ET_ISOBJ(entry))
> + continue;
> +
> + TAILQ_REMOVE(deadq, entry, dfree.deadq);
> + uvm_mapent_free(entry);
> + }
>
> if (TAILQ_EMPTY(deadq))
> return;
>
Re: sensorsd(8) unveil for -f option
That looks good.
Do others using sensorsd concur?
Anton Borowka wrote:
> sensorsd(8) currently only unveils /etc/sensorsd.conf for reading, but
> the config file can be changed with the -f option (which is currently
> not working).
>
> The patch moves unveil and pledge after the options handling and unveils
> the determined configfile.
>
> Index: usr.sbin/sensorsd/sensorsd.c
> ===
> RCS file: /cvs/src/usr.sbin/sensorsd/sensorsd.c,v
> retrieving revision 1.63
> diff -u -p -u -r1.63 sensorsd.c
> --- usr.sbin/sensorsd/sensorsd.c 10 Dec 2018 13:35:54 - 1.63
> +++ usr.sbin/sensorsd/sensorsd.c 15 May 2019 17:18:21 -
> @@ -114,14 +114,6 @@ main(int argc, char *argv[])
> int ch, check_period = CHECK_PERIOD;
> const char *errstr;
>
> - if (unveil("/etc/sensorsd.conf", "r") == -1)
> - err(1, "unveil");
> - if (unveil("/", "x") == -1)
> - err(1, "unveil");
> -
> - if (pledge("stdio rpath proc exec", NULL) == -1)
> - err(1, "pledge");
> -
> while ((ch = getopt(argc, argv, "c:df:")) != -1) {
> switch (ch) {
> case 'c':
> @@ -148,14 +140,23 @@ main(int argc, char *argv[])
> if (argc > 0)
> usage();
>
> - openlog("sensorsd", LOG_PID | LOG_NDELAY, LOG_DAEMON);
> -
> - create();
> -
> if (configfile == NULL)
> if (asprintf(&configfile, "/etc/sensorsd.conf") == -1)
> err(1, "out of memory");
> +
> + if (unveil(configfile, "r") == -1)
> + err(1, "unveil");
> + if (unveil("/", "x") == -1)
> + err(1, "unveil");
> +
> + if (pledge("stdio rpath proc exec", NULL) == -1)
> + err(1, "pledge");
> +
> parse_config(configfile);
> +
> + openlog("sensorsd", LOG_PID | LOG_NDELAY, LOG_DAEMON);
> +
> + create();
>
> if (debug == 0 && daemon(0, 0) == -1)
> err(1, "unable to fork");
>
sensorsd(8) unveil for -f option
sensorsd(8) currently only unveils /etc/sensorsd.conf for reading, but
the config file can be changed with the -f option (which is currently
not working).
The patch moves unveil and pledge after the options handling and unveils
the determined configfile.
Index: usr.sbin/sensorsd/sensorsd.c
===
RCS file: /cvs/src/usr.sbin/sensorsd/sensorsd.c,v
retrieving revision 1.63
diff -u -p -u -r1.63 sensorsd.c
--- usr.sbin/sensorsd/sensorsd.c10 Dec 2018 13:35:54 - 1.63
+++ usr.sbin/sensorsd/sensorsd.c15 May 2019 17:18:21 -
@@ -114,14 +114,6 @@ main(int argc, char *argv[])
int ch, check_period = CHECK_PERIOD;
const char *errstr;
- if (unveil("/etc/sensorsd.conf", "r") == -1)
- err(1, "unveil");
- if (unveil("/", "x") == -1)
- err(1, "unveil");
-
- if (pledge("stdio rpath proc exec", NULL) == -1)
- err(1, "pledge");
-
while ((ch = getopt(argc, argv, "c:df:")) != -1) {
switch (ch) {
case 'c':
@@ -148,14 +140,23 @@ main(int argc, char *argv[])
if (argc > 0)
usage();
- openlog("sensorsd", LOG_PID | LOG_NDELAY, LOG_DAEMON);
-
- create();
-
if (configfile == NULL)
if (asprintf(&configfile, "/etc/sensorsd.conf") == -1)
err(1, "out of memory");
+
+ if (unveil(configfile, "r") == -1)
+ err(1, "unveil");
+ if (unveil("/", "x") == -1)
+ err(1, "unveil");
+
+ if (pledge("stdio rpath proc exec", NULL) == -1)
+ err(1, "pledge");
+
parse_config(configfile);
+
+ openlog("sensorsd", LOG_PID | LOG_NDELAY, LOG_DAEMON);
+
+ create();
if (debug == 0 && daemon(0, 0) == -1)
err(1, "unable to fork");
Re: wall(1) unveil for non-root users
"Theo de Raadt" writes:
> Anton Borowka wrote:
>
>> wall(1) does not work correctly for non-root users at the moment because
>> ttymsg() needs read access for the tty devices, but only write access is
>> unveiled. Because it cannot access any devices nothing is printed.
>>
>> This patch adds read access for /dev. Don't know if it makes sense to
>> only unveil read access for non-root users.
>
> Apparently it stat()'s the devices [thereby desiring "r"], before calling
> open O_WRONLY [which only needs "w"].
>
> It is a TOCTOU. Here are two proposals.
>
> This one lets non-root write to any fd it can manage to open.
>
> Index: ttymsg.c
> ===
> RCS file: /cvs/src/usr.bin/wall/ttymsg.c,v
> retrieving revision 1.17
> diff -u -p -u -r1.17 ttymsg.c
> --- ttymsg.c 5 Nov 2015 22:20:11 - 1.17
> +++ ttymsg.c 15 May 2019 22:39:44 -
> @@ -87,13 +87,6 @@ ttymsg(iov, iovcnt, line, tmout)
> return (errbuf);
> }
>
> - if (getuid()) {
> - if (stat(device, &st) < 0)
> - return (NULL);
> - if ((st.st_mode & S_IWGRP) == 0)
> - return (NULL);
> - }
> -
> /*
>* open will fail on slip lines or exclusive-use lines
>* if not running as root; not an error.
>
> This one is a bit more cautious, it checks if specifically g+w
> is allowed on the tty, that is more true to the original behaviour
> isn't it?
>
> Index: ttymsg.c
> ===
> RCS file: /cvs/src/usr.bin/wall/ttymsg.c,v
> retrieving revision 1.17
> diff -u -p -u -r1.17 ttymsg.c
> --- ttymsg.c 5 Nov 2015 22:20:11 - 1.17
> +++ ttymsg.c 15 May 2019 22:42:58 -
> @@ -87,12 +87,6 @@ ttymsg(iov, iovcnt, line, tmout)
> return (errbuf);
> }
>
> - if (getuid()) {
> - if (stat(device, &st) < 0)
> - return (NULL);
> - if ((st.st_mode & S_IWGRP) == 0)
> - return (NULL);
> - }
>
> /*
>* open will fail on slip lines or exclusive-use lines
> @@ -104,6 +98,17 @@ ttymsg(iov, iovcnt, line, tmout)
> (void) snprintf(errbuf, sizeof(errbuf),
> "%s: %s", device, strerror(errno));
> return (errbuf);
> + }
> +
> + if (getuid()) {
> + if (fstat(fd, &st) < 0) {
> + close(fd);
> + return (NULL);
> + }
> + if ((st.st_mode & S_IWGRP) == 0) {
> + close(fd);
> + return (NULL);
> + }
> }
>
> for (cnt = left = 0; cnt < iovcnt; ++cnt)
While looking at the wall(1) code I also thought about the first diff,
but the comments above ttymesg() said something about it being used by
talkd(8). Apparently I misunderstood or the comment is outdated, but I
didn't look into it.
I don't know about the practical implications of the second diff. A user
can not read his own messages, because for example the tty is not group
writable (but writable by the user)?
As you said the second one is more conservative and keeps to the old
behaviour. I personally don't see anything wrong with the first diff,
but I'm not an expert.
Best regards
Anton
Re: wall(1) unveil for non-root users
Anton Borowka wrote:
> wall(1) does not work correctly for non-root users at the moment because
> ttymsg() needs read access for the tty devices, but only write access is
> unveiled. Because it cannot access any devices nothing is printed.
>
> This patch adds read access for /dev. Don't know if it makes sense to
> only unveil read access for non-root users.
Apparently it stat()'s the devices [thereby desiring "r"], before calling
open O_WRONLY [which only needs "w"].
It is a TOCTOU. Here are two proposals.
This one lets non-root write to any fd it can manage to open.
Index: ttymsg.c
===
RCS file: /cvs/src/usr.bin/wall/ttymsg.c,v
retrieving revision 1.17
diff -u -p -u -r1.17 ttymsg.c
--- ttymsg.c5 Nov 2015 22:20:11 - 1.17
+++ ttymsg.c15 May 2019 22:39:44 -
@@ -87,13 +87,6 @@ ttymsg(iov, iovcnt, line, tmout)
return (errbuf);
}
- if (getuid()) {
- if (stat(device, &st) < 0)
- return (NULL);
- if ((st.st_mode & S_IWGRP) == 0)
- return (NULL);
- }
-
/*
* open will fail on slip lines or exclusive-use lines
* if not running as root; not an error.
This one is a bit more cautious, it checks if specifically g+w
is allowed on the tty, that is more true to the original behaviour
isn't it?
Index: ttymsg.c
===
RCS file: /cvs/src/usr.bin/wall/ttymsg.c,v
retrieving revision 1.17
diff -u -p -u -r1.17 ttymsg.c
--- ttymsg.c5 Nov 2015 22:20:11 - 1.17
+++ ttymsg.c15 May 2019 22:42:58 -
@@ -87,12 +87,6 @@ ttymsg(iov, iovcnt, line, tmout)
return (errbuf);
}
- if (getuid()) {
- if (stat(device, &st) < 0)
- return (NULL);
- if ((st.st_mode & S_IWGRP) == 0)
- return (NULL);
- }
/*
* open will fail on slip lines or exclusive-use lines
@@ -104,6 +98,17 @@ ttymsg(iov, iovcnt, line, tmout)
(void) snprintf(errbuf, sizeof(errbuf),
"%s: %s", device, strerror(errno));
return (errbuf);
+ }
+
+ if (getuid()) {
+ if (fstat(fd, &st) < 0) {
+ close(fd);
+ return (NULL);
+ }
+ if ((st.st_mode & S_IWGRP) == 0) {
+ close(fd);
+ return (NULL);
+ }
}
for (cnt = left = 0; cnt < iovcnt; ++cnt)
wall(1) unveil for non-root users
wall(1) does not work correctly for non-root users at the moment because ttymsg() needs read access for the tty devices, but only write access is unveiled. Because it cannot access any devices nothing is printed. This patch adds read access for /dev. Don't know if it makes sense to only unveil read access for non-root users. Index: usr.bin/wall/wall.c === RCS file: /cvs/src/usr.bin/wall/wall.c,v retrieving revision 1.34 diff -u -p -u -r1.34 wall.c --- usr.bin/wall/wall.c 28 Jan 2019 20:17:51 - 1.34 +++ usr.bin/wall/wall.c 15 May 2019 17:06:31 - @@ -117,7 +117,7 @@ main(int argc, char **argv) if (unveil(_PATH_UTMP, "r") == -1) err(1, "unveil"); - if (unveil(_PATH_DEV, "w") == -1) + if (unveil(_PATH_DEV, "rw") == -1) err(1, "unveil"); if (unveil(_PATH_DEVDB, "r") == -1) err(1, "unveil");
Re: ospfd: allow specifying area by number as well as id
On Tue, Apr 30, 2019 at 11:10:37PM +0200, Remi Locherer wrote:
> On Mon, Apr 29, 2019 at 11:10:31AM +0100, Stuart Henderson wrote:
> > On 2019/04/29 11:58, Sebastian Benoit wrote:
> > > David Gwynne(da...@gwynne.id.au) on 2019.04.29 19:36:51 +1000:
> > > >
> > > >
> > > > > On 29 Apr 2019, at 4:59 pm, Remi Locherer
> > > > > wrote:
> > > > >
> > > > > Hi David
> > > > >
> > > > > On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote:
> > > > >> it's always bothered me that i config areas on a crisco using a
> > > > >> number,
> > > > >> but then have to think hard to convert that number to an address for
> > > > >> use
> > > > >> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188
> > > > >> as an address. super annoying.
> > > > >>
> > > > >> so this changes the ospfd parser so it accepts both a number or
> > > > >> address.
> > > > >> i also changed it so it prints the number by default, which may be
> > > > >> contentious. the manpage is slightly tweaked too.
> > > > >>
> > > > >> thoughts?
> > > > >
> > > > > I like it to be able to use a number instead of an address!
> > > > >
> > > > > It worked fine in my short test I performed.
> > > > >
> > > > > The output with the comment looks a bit strange to me.
> > > >
> > > > Are you sure it doesn't look... awesome?
> > >
> > > I like it!
> >
> > I don't really, but if we change this it needs to be displayed somehow
> > and I don't have an idea to make it look nicer than this (cisco's method
> > seems pretty horrible and wouldn't work for us anyway - looks like they
> > remember which format was used to configure an area and use that as
> > the output format...)
> >
>
> Maybe it's better when we just allow both input formats but don't change
> any output.
Any opinions or comments on this? I think this would be a valuable addition
to ospfd.
>
> Below diff changes ospfctl to accept the address and number format for
> "ospfct show database area XXX".
>
>
> Index: parser.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfctl/parser.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 parser.c
> --- parser.c 9 May 2011 12:25:35 - 1.20
> +++ parser.c 30 Apr 2019 20:28:18 -
> @@ -39,7 +39,8 @@ enum token_type {
> ADDRESS,
> FLAG,
> PREFIX,
> - IFNAME
> + IFNAME,
> + AREA
> };
>
> struct token {
> @@ -107,7 +108,7 @@ static const struct token t_show_db[] =
> };
>
> static const struct token t_show_area[] = {
> - {ADDRESS, "", NONE, NULL},
> + {AREA, "", NONE, NULL},
> {ENDTOKEN, "", NONE, NULL}
> };
>
> @@ -218,6 +219,14 @@ match_token(const char *word, const stru
> res->action = t->value;
> }
> break;
> + case AREA:
> + if (parse_area(word, &res->addr)) {
> + match++;
> + t = &table[i];
> + if (t->value)
> + res->action = t->value;
> + }
> + break;
> case PREFIX:
> if (parse_prefix(word, &res->addr, &res->prefixlen)) {
> match++;
> @@ -274,6 +283,9 @@ show_valid_args(const struct token *tabl
> case ADDRESS:
> fprintf(stderr, " \n");
> break;
> + case AREA:
> + fprintf(stderr, " \n");
> + break;
> case PREFIX:
> fprintf(stderr, " [/]\n");
> break;
> @@ -298,6 +310,32 @@ parse_addr(const char *word, struct in_a
> bzero(&ina, sizeof(ina));
>
> if (inet_pton(AF_INET, word, &ina)) {
> + addr->s_addr = ina.s_addr;
> + return (1);
> + }
> +
> + return (0);
> +}
> +
> +int
> +parse_area(const char *word, struct in_addr *addr)
> +{
> + struct in_addr ina;
> + const char *errstr;
> +
> + if (word == NULL)
> + return (0);
> +
> + bzero(addr, sizeof(struct in_addr));
> + bzero(&ina, sizeof(ina));
> +
> + if (inet_pton(AF_INET, word, &ina)) {
> + addr->s_addr = ina.s_addr;
> + return (1);
> + }
> +
> + ina.s_addr = htonl(strtonum(word, 0, 0x, &errstr));
> + if (errstr == NULL) {
> addr->s_addr = ina.s_addr;
> return (1);
> }
> Index: parser.h
> ===
> RCS file: /cvs/src/usr.sbin/ospfctl/parser.h,v
> retrieving revision 1.13
> diff -u -p -r1.13 parser.h
> --- parser.h 9 May 2011 12:25:35 - 1.13
> +++ parser.h 30 Apr 2019 20:28:52 -
> @@ -64,6 +64,7 @@ struct parse_result {
>
>
Re: enable pfctl to flush all rules and tables
On Wed, May 15, 2019 at 09:08:20PM +0200, Alexandr Nedvedicky wrote: > completely agree with you. my diff indeed ignores '-a'. Thanks for > spotting that. With change below the complete patch behaves as you > expect. Finishing touch below adds makes pfctl_recurse() to > accept an 'anchorname' as an argument Not quite. Starting with my usual ruleset; Note that "misc" is empty. "vm" does NAT and various other stuff for local VMs. # pfctl -sr anchor "misc" all anchor "vm" quick all received-on tap block drop all pass all flags S/SA block return in on ! lo0 proto tcp from any to any port 6000:6010 block return out log proto tcp all user = 55 block return out log proto udp all user = 55 Pollute the system: # make -C/usr/src/regress/sbin/pfctl obj/sbin/pfctl/pfctl 1>/dev/null 2>&1< # pfctl -sA 1 misc regress relative vm Clean up: # ./obj/pfctl -Fa -aregress/* 0 tables deleted. rules cleared 0 tables deleted. rules cleared 1 tables deleted. rules cleared 0 tables deleted. rules cleared 0 tables deleted. rules cleared 0 tables deleted. rules cleared 0 tables deleted. rules cleared 0 tables deleted. rules cleared 0 tables deleted. rules cleared 0 tables deleted. rules cleared 0 tables deleted. rules cleared 0 tables deleted. rules cleared 0 tables deleted. rules cleared 0 tables deleted. rules cleared 0 tables deleted. rules cleared 0 tables deleted. rules cleared 0 tables deleted. rules cleared 0 tables deleted. rules cleared 0 tables deleted. rules cleared 11 states cleared Now that we recurse, it is not clear any longer which of those outputs belong to which anchor. We should print the anchor names additionally or alternatively, print those lines only if `-v' was passed. Either ways, information is missing, making the output rather annoying than helpful. source tracking entries cleared pf: statistics cleared pf: interface flags reset These steps should not happen if `-a' is given. They effect other anchors and rules as well. Think of NAT in my example, or perhaps `once' rules creating specific state: All of this should only ever be cleared and reset when the admin requests a full wipe, e.g. `pfctl -Fa -a*'. Now, besides the content of "regress" itself, "misc" is accidently lost: # pfctl -sA regress vm So is my main ruleset: # pfctl -sr ; echo $? 0 This must not happen. Did your main ruleset survive `pfctl -Fa -aregress/*' when testing? As for the current diff, see further comments inline. If you want, we can start committing first hunks as already prepared by your split up, so further work can focus on the relevant bits. I'll send separate replies with explicit OKs, but feel free to adapt this to whatever works best for you. > diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c > index f56f6f9e90b..aea093c426e 100644 > --- a/sbin/pfctl/pfctl.c > +++ b/sbin/pfctl/pfctl.c > @@ -54,6 +54,8 @@ > > #include > Can you zap this empty line? > +#include > + > #include "pfctl_parser.h" > #include "pfctl.h" > > @@ -63,7 +65,7 @@ int pfctl_disable(int, int); > void pfctl_clear_queues(struct pf_qihead *); > void pfctl_clear_stats(int, const char *, int); > void pfctl_clear_interface_flags(int, int); > -void pfctl_clear_rules(int, int, char *); > +int pfctl_clear_rules(int, int, char *); > void pfctl_clear_src_nodes(int, int); > void pfctl_clear_states(int, const char *, int); > struct addrinfo * > @@ -106,6 +108,17 @@ const char *pfctl_lookup_option(char *, const char > **); > void pfctl_state_store(int, const char *); > void pfctl_state_load(int, const char *); > void pfctl_reset(int, int); > +int pfctl_walk_show(int, struct pfioc_ruleset *, void *); > +int pfctl_walk_get(int, struct pfioc_ruleset *, void *); > +int pfctl_walk_anchors(int, int, const char *, void *, > +int(*)(int, struct pfioc_ruleset *, void *)); > +struct pfr_anchors * > + pfctl_get_anchors(int, const char *, int); > +int pfctl_recurse(int, int, const char *, > + int(*)(int, int, struct pfr_anchoritem *)); > +int pfctl_call_clearrules(int, int, struct pfr_anchoritem *); > +int pfctl_call_cleartables(int, int, struct pfr_anchoritem *); > +int pfctl_call_clearanchors(int, int, struct pfr_anchoritem *); > > const char *clearopt; > char *rulesopt; > @@ -125,6 +138,7 @@ char *state_kill[2]; > int dev = -1; > int first_title = 1; > int labels = 0; > +int ex
Re: ospfd: do not change router-id on reload if unspecified
On Wed, May 15, 2019 at 03:52:57PM +0200, Denis Fondras wrote:
> When router-id is unspecified, ospfd will choose the lowest IP address of the
> host. I added an area and an IP lower than the existing ones and on reload
> ospfd asked me to restart and did not activate the new area.
>
> Why would it update the router-id in such a case ?
>
> This diff changes this behaviour. When router-id is not explicitely changed,
> keep the existing setting.
makes sense to me.
OK remi@
>
> Index: ospfd.c
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
> retrieving revision 1.107
> diff -u -p -r1.107 ospfd.c
> --- ospfd.c 26 Mar 2019 20:39:33 - 1.107
> +++ ospfd.c 15 May 2019 13:19:52 -
> @@ -185,6 +185,8 @@ main(int argc, char *argv[])
> kif_clear();
> exit(1);
> }
> +if (ospfd_conf->rtr_id.s_addr == 0)
> +ospfd_conf->rtr_id.s_addr = get_rtr_id();
>
> if (sockname == NULL) {
> if (asprintf(&sockname, "%s.%d", OSPFD_SOCKET,
> @@ -641,6 +643,10 @@ ospf_reload(void)
>
> if ((xconf = parse_config(conffile, ospfd_conf->opts)) == NULL)
> return (-1);
> +
> + /* No router-id was specified, keep existing value */
> +if (xconf->rtr_id.s_addr == 0)
> +xconf->rtr_id.s_addr = ospfd_conf->rtr_id.s_addr;
>
> /* Abort the reload if rtr_id changed */
> if (ospfd_conf->rtr_id.s_addr != xconf->rtr_id.s_addr) {
> Index: ospfd.h
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/ospfd.h,v
> retrieving revision 1.103
> diff -u -p -r1.103 ospfd.h
> --- ospfd.h 28 Dec 2018 19:25:10 - 1.103
> +++ ospfd.h 15 May 2019 13:19:52 -
> @@ -561,6 +561,7 @@ intcarp_demote_set(char *, int);
>
> /* parse.y */
> struct ospfd_conf*parse_config(char *, int);
> +u_int32_t get_rtr_id(void);
> int cmdline_symset(char *);
> void conf_clear_redist_list(struct redist_list *);
>
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v
> retrieving revision 1.96
> diff -u -p -r1.96 parse.y
> --- parse.y 29 Apr 2019 05:14:38 - 1.96
> +++ parse.y 15 May 2019 13:19:52 -
> @@ -83,7 +83,6 @@ int symset(const char *, const char *,
> char *symget(const char *);
>
> void clear_config(struct ospfd_conf *xconf);
> -u_int32_t get_rtr_id(void);
> int host(const char *, struct in_addr *, struct in_addr *);
>
> static struct ospfd_conf *conf;
> @@ -1253,9 +1252,6 @@ parse_config(char *filename, int opts)
> clear_config(conf);
> return (NULL);
> }
> -
> - if (conf->rtr_id.s_addr == 0)
> - conf->rtr_id.s_addr = get_rtr_id();
>
> return (conf);
> }
>
Re: enable pfctl to flush all rules and tables
Hello Klemens,
On Mon, May 13, 2019 at 12:22:34AM +0200, Klemens Nanni wrote:
> On Wed, Apr 17, 2019 at 01:28:57AM +0200, Alexandr Nedvedicky wrote:
> > The idea has been already discussed few weeks ago [1]. Reusing "-a '*'"
> > option
> > to tell pfctl to flush everything is sthen's idea [2]. The patch below makes
> > pfctl to understand
> > pfctl -a '*' -FT
> > pfctl -a '*' -Fr
> > pfctl -a '*' -Fa
> This must be possible for specific anchors only, I'd say. Consider
> running the regress suite, which leaves lots of junk behind:
>
> # pfctl -vsA -aregress
> regress/_1
> regress/_1/_2
> regress/_1/_3
> regress/_1/_3/_4
> regress/_1/_3/_4/_5
> regress/_1/_3/_4/_5/_6
> regress/_1/_3/_4/_5/_6/_7
> regress/_1/_3/_4/_5/_6/_7/_8
> regress/_1/_3/_4/_5/_6/_7/_8/_9
> regress/_1/_3/_4/_5/_6/_7/_8/_9/_10
> regress/_1/foo
> regress/_1/foo/_3
> regress/foo
> regress/foo/_4
> regress/foo/bar
> regress/one
> regress/one/two
> regress/relative
>
> `-Fa -a\*' is a nice idea, but your diff fatally ignores `-a'. Without
> recursion, all is fine as the current in-tree version already does:
completely agree with you. my diff indeed ignores '-a'. Thanks for
spotting that. With change below the complete patch behaves as you
expect. Finishing touch below adds makes pfctl_recurse() to
accept an 'anchorname' as an argument
8<---8<---8<--8<
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 918832b73cf..aea093c426e 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -110,11 +110,12 @@ void pfctl_state_load(int, const char *);
void pfctl_reset(int, int);
intpfctl_walk_show(int, struct pfioc_ruleset *, void *);
intpfctl_walk_get(int, struct pfioc_ruleset *, void *);
-intpfctl_walk_anchors(int, int, char *, void *,
+intpfctl_walk_anchors(int, int, const char *, void *,
int(*)(int, struct pfioc_ruleset *, void *));
struct pfr_anchors *
- pfctl_get_anchors(int, int);
-intpfctl_recurse(int, int, int(*nuke)(int, int, struct
pfr_anchoritem *));
+ pfctl_get_anchors(int, const char *, int);
+intpfctl_recurse(int, int, const char *,
+ int(*)(int, int, struct pfr_anchoritem *));
intpfctl_call_clearrules(int, int, struct pfr_anchoritem *);
intpfctl_call_cleartables(int, int, struct pfr_anchoritem *);
intpfctl_call_clearanchors(int, int, struct pfr_anchoritem *);
@@ -2204,7 +2205,7 @@ pfctl_walk_get(int opts, struct pfioc_ruleset *pr,
void *warg)
}
int
-pfctl_walk_anchors(int dev, int opts, char *anchorname, void *warg,
+pfctl_walk_anchors(int dev, int opts, const char *anchorname, void *warg,
int(walkf)(int, struct pfioc_ruleset *, void *))
{
struct pfioc_ruleset pr;
@@ -2260,7 +2261,7 @@ pfctl_show_anchors(int dev, int opts, char
*anchorname)
}
struct pfr_anchors *
-pfctl_get_anchors(int dev, int opts)
+pfctl_get_anchors(int dev, const char *anchorname, int opts)
{
struct pfioc_rulesetpr;
struct {
@@ -2282,7 +2283,7 @@ pfctl_get_anchors(int dev, int opts)
SLIST_INSERT_HEAD(&anchors, pfra.pfra, pfra_sle);
opts |= PF_OPT_VERBOSE;
- if (pfctl_walk_anchors(dev, opts, "", &pfra, pfctl_walk_get))
+ if (pfctl_walk_anchors(dev, opts, anchorname, &pfra, pfctl_walk_get))
errx(1,
"%s failed to retrieve list of anchors, can't
continue\n",
__func__);
@@ -2315,19 +2316,20 @@ pfctl_call_clearanchors(int dev, int opts, struct
pfr_anchoritem *pfra)
}
int
-pfctl_recurse(int dev, int opts, int(*nuke)(int, int, struct
pfr_anchoritem *))
+pfctl_recurse(int dev, int opts, const char *anchorname,
+int(*walkf)(int, int, struct pfr_anchoritem *))
{
int rv = 0;
struct pfr_anchors *anchors;
struct pfr_anchoritem *pfra, *pfra_save;
- anchors = pfctl_get_anchors(dev, opts);
+ anchors = pfctl_get_anchors(dev, anchorname, opts);
/*
* don't let pfctl_clear_*() function to fail with exit
*/
opts |= PF_OPT_IGNFAIL;
SLIST_FOREACH_SAFE(pfra, anchors, pfra_sle, pfra_save) {
- rv |= nuke(dev, opts, pfra);
+ rv |= walkf(dev, opts, pfra);
SLIST_REMOVE(anchors, pfra, pfr_anchoritem, pfra_sle);
free(pfra->pfra_anchorname);
free(pfra);
@@ -2765,7 +2767,7 @@ main(int argc, char *argv[])
if ((opts & PF_OPT_RECURSE)
Re: caesar(6) to accept negative argument
On Wed, May 15, 2019 at 02:33:45PM -0400, Ted Unangst wrote: > Otto Moerbeek wrote: > > We're computing modulo 26 here. Negative numbers have a positive > > equivalent. So you diff adds code for no benefit. > > I think the amount of code added is an acceptable cost for improved user > experience. We could use this argument to remove subtraction from bc, but that > would be silly. > Ok, agreed. -Otto
Re: caesar(6) to accept negative argument
Otto Moerbeek wrote: > We're computing modulo 26 here. Negative numbers have a positive > equivalent. So you diff adds code for no benefit. I think the amount of code added is an acceptable cost for improved user experience. We could use this argument to remove subtraction from bc, but that would be silly.
Re: caesar(6) to accept negative argument
Hi, tleguern wrote on Wed, May 15, 2019 at 03:36:57PM +0100: > This little patch makes caesar(6) useful at both encrypting and > decrypting texts by allowing a negative rotation. Committed, thanks. > A similar patch was proposed by Dieter Rauschenberger in 2008 with > little response Well, it's a game, so people sometimes forget how critical it is to publish errata for it as fastly as possible. ;-) Yours, Ingo CVSROOT:/cvs Module name:src Changes by: schwa...@cvs.openbsd.org2019/05/15 09:59:24 Modified files: games/caesar : caesar.c Log message: patch from to support backward rotation, hoping to save somebody's life from the Leather Goddesses of Phobos
Re: caesar(6) to accept negative argument
On Wed, May 15, 2019 at 03:36:57PM +0100, tleguern wrote:
> Hi,
>
> This little patch makes caesar(6) useful at both encrypting and
> decrypting texts by allowing a negative rotation.
>
> Example:
>
> $ echo Ceci est un test | caesar 10
> Moms ocd ex docd
> $ echo Ceci est un test | caesar 10 | caesar -10
> Ceci est un test
>
> A similar patch was proposed by Dieter Rauschenberger in 2008 with
> little response so perhaps explaining why this is useful will help.
> Without spoiling too much: one of the puzzle in the Infocom game Leather
> Goddesses of Phobos requires to decipher such a text.
>
> I did not touch the man page as it was unclear about rotation boundaries
> anyway.
$ echo Ceci est un test | caesar 10 | caesar 16
Ceci est un test
We're computing modulo 26 here. Negative numbers have a positive
equivalent. So you diff adds code for no benefit.
-Otto
>
> Regards.
>
> Index: caesar.c
> ===
> RCS file: /cvs/src/games/caesar/caesar.c,v
> retrieving revision 1.20
> diff -u -p -u -p -r1.20 caesar.c
> --- caesar.c 10 Aug 2017 17:24:30 - 1.20
> +++ caesar.c 15 May 2019 14:34:49 -
> @@ -80,7 +80,7 @@ main(int argc, char *argv[])
> printit(13);
>
> if (argc > 1) {
> - i = strtonum(argv[1], 0, 25, &errstr);
> + i = strtonum(argv[1], -25, 25, &errstr);
> if (errstr)
> errx(1, "rotation is %s: %s", errstr, argv[1]);
> else
> @@ -134,7 +134,9 @@ void
> printit(int rot)
> {
> int ch;
> -
> +
> + if (rot < 0)
> + rot = rot + 26;
> while ((ch = getchar()) != EOF)
> putchar(ROTATE(ch, rot));
> exit(0);
>
caesar(6) to accept negative argument
Hi,
This little patch makes caesar(6) useful at both encrypting and
decrypting texts by allowing a negative rotation.
Example:
$ echo Ceci est un test | caesar 10
Moms ocd ex docd
$ echo Ceci est un test | caesar 10 | caesar -10
Ceci est un test
A similar patch was proposed by Dieter Rauschenberger in 2008 with
little response so perhaps explaining why this is useful will help.
Without spoiling too much: one of the puzzle in the Infocom game Leather
Goddesses of Phobos requires to decipher such a text.
I did not touch the man page as it was unclear about rotation boundaries
anyway.
Regards.
Index: caesar.c
===
RCS file: /cvs/src/games/caesar/caesar.c,v
retrieving revision 1.20
diff -u -p -u -p -r1.20 caesar.c
--- caesar.c10 Aug 2017 17:24:30 - 1.20
+++ caesar.c15 May 2019 14:34:49 -
@@ -80,7 +80,7 @@ main(int argc, char *argv[])
printit(13);
if (argc > 1) {
- i = strtonum(argv[1], 0, 25, &errstr);
+ i = strtonum(argv[1], -25, 25, &errstr);
if (errstr)
errx(1, "rotation is %s: %s", errstr, argv[1]);
else
@@ -134,7 +134,9 @@ void
printit(int rot)
{
int ch;
-
+
+ if (rot < 0)
+ rot = rot + 26;
while ((ch = getchar()) != EOF)
putchar(ROTATE(ch, rot));
exit(0);
Re: scheduler small changes
Hello Amit, On 15/05/19(Wed) 09:05, Amit Kulkarni wrote: > Hi, > > This effort is heavily based on top of Gregor's and Michal's diffs. Tried to > incorporate feedback given by different people to them in 2011/2016. Split > the new code in a ifdef, so people can do a straight comparison, tried very > hard not to delete existing code, just shifted it around. Main motivation was > to find if it is possible to do incremental improvements in scheduler. After > sometime, have still not understood completely the load avg part, > p_priority/p_usrpri calculations, the cpuset code. Looked heavily at > Dragonfly (simpler than FreeBSD) and FreeBSD code. As a newcomer, OpenBSD > code is way easier to read. Thanks for the detailed explanations given to > Michal and also in later diffs posted to tech@, they helped a lot when trying > to understand the decisions made by other devs, the commit messages help a > little but the explanations help a lot more. This invites discussions and end > users learn a lot more in the process. Why did you decide to change the data structure of the runqueue? What problem are you trying to solve? Regarding your work, if you want to continue in the scheduler area, may I suggest you start by making the global counters per-spc and export them to userland via a syscl? Add a new view to systat(1) to see what's happening. Without more visibility it's hard to confirm any theory. Regarding the choice of deriving quantum from the priority, are you sure the priorities are correct? Should we keep priorities? Or if userland needs priorities shouldn't we convert quantum into priority and not the other way around? Regarding your diff, if you find a thread in RUN state inside the sleep queue (wakeup_n()), something is really wrong there.
scheduler small changes
Hi,
This effort is heavily based on top of Gregor's and Michal's diffs. Tried to
incorporate feedback given by different people to them in 2011/2016. Split the
new code in a ifdef, so people can do a straight comparison, tried very hard
not to delete existing code, just shifted it around. Main motivation was to
find if it is possible to do incremental improvements in scheduler. After
sometime, have still not understood completely the load avg part,
p_priority/p_usrpri calculations, the cpuset code. Looked heavily at Dragonfly
(simpler than FreeBSD) and FreeBSD code. As a newcomer, OpenBSD code is way
easier to read. Thanks for the detailed explanations given to Michal and also
in later diffs posted to tech@, they helped a lot when trying to understand the
decisions made by other devs, the commit messages help a little but the
explanations help a lot more. This invites discussions and end users learn a
lot more in the process.
This diff survived multiple tens of kernel builds, a bsd.sp build, bsd.rd
build, a bsd.mp without these changes, userland/xenocara, a make regress a few
days ago all on 3 desktops on amd64. Ran under all possible scenarios listed in
previous sentence. No major invasive changes attempted, all tools should work
as is, if its broken, please let me know. This is faster than current, but not
sure by how much, placebo effect in play.
I think there is a bug in resched_proc() which is fixed in mi_switch(), a
quick enhancement in cpu_switchto(), p_slptime, and precise round robin
interval for each proc unless preempted or it yields().
Tried to fiddle with different time slices other than hz/10, not sure if it
will work on other arches, but tried to stay within MI code, so it should work.
Other than counting no idea to verify a proc is actually switched away after
its rr interval is over. Just went by what is there in the existing code. Tried
giving higher slice like 200 ms, but didn't want to exceed the rucheck() in
kern_resource 1 sec limit.
Tried to do rudimentary work on affinity without having a affinity field in
struct proc or struct schedstate_percpu (like Dragonfly/FreeBSD does). Observed
the unbalance in runqs. Affinity works most of the time under light load.
There's a problem when I try to comment out sched_steal_proc(), in kern_sched,
that is the problem with this iteration of the diff.
Not sure if the reduction from 32 queues to a single TAILQ would be accepted,
but tried it anyway, it is definitely faster. This code tries to reduce the
complexity in deciding which queue to place the proc on. There is no current
support for a real-time queue or other types of scheduling classes, so IMHO
this is a simplification.
Tried to give detailed explanation of thinking. Sent the complete diff, but
will split diff, if parts of it are found to be valid.
In any case, a request to please accept a small change in top, to display
p_priority directly.
Thanks
diff --git a/sys/conf/GENERIC b/sys/conf/GENERIC
index d6a4fdcb0e2..73995746e23 100644
--- a/sys/conf/GENERIC
+++ b/sys/conf/GENERIC
@@ -3,6 +3,7 @@
# Machine-independent option; used by all architectures for their
# GENERIC kernel
+option TSTSCHED# test scheduler
option DDB # in-kernel debugger
#optionDDBPROF # ddb(4) based profiling
#optionDDB_SAFE_CONSOLE # allow break into ddb during boot
diff --git a/sys/kern/init_main.c b/sys/kern/init_main.c
index effefd552c3..be58199d834 100644
--- a/sys/kern/init_main.c
+++ b/sys/kern/init_main.c
@@ -129,6 +129,10 @@ intncpus = 1;
intncpusfound = 1; /* number of cpus we find */
volatile int start_init_exec; /* semaphore for start_init() */
+#ifdef TSTSCHED
+int sched_start_dquantum = 0;
+#endif
+
#if !defined(NO_PROPOLICE)
long __guard_local __attribute__((section(".openbsd.randomdata")));
#endif
@@ -572,6 +576,21 @@ main(void *framep)
start_periodic_resettodr();
+ /* Inited all systems, ready to start the test scheduler now!
+*
+* I saw some instability, when rr_intvl not set to hz/10 during
+* boot, so resorting to fiddling with rr interval after we
+* have booted up fully.
+* */
+#ifdef TSTSCHED
+ printf("-\n");
+ printf("-\n");
+ printf("- TEST SCHEDULER starting -\n");
+ printf("-\n");
+ printf("-\n");
+ sched_start_dquantum = 1;
+#endif
+
/*
* proc0: nothing to do, back to sleep
*/
diff --git a/sys/kern/kern_clock.c b/sys/kern/kern_clock.c
index ee701945966..8db6611bff3 100644
--- a/sys/kern/kern_clock.c
+++ b/sys/kern/kern_clock.c
@@ -143,7 +143,6 @@ void
hardcl
ospfd: do not change router-id on reload if unspecified
When router-id is unspecified, ospfd will choose the lowest IP address of the
host. I added an area and an IP lower than the existing ones and on reload
ospfd asked me to restart and did not activate the new area.
Why would it update the router-id in such a case ?
This diff changes this behaviour. When router-id is not explicitely changed,
keep the existing setting.
Index: ospfd.c
===
RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v
retrieving revision 1.107
diff -u -p -r1.107 ospfd.c
--- ospfd.c 26 Mar 2019 20:39:33 - 1.107
+++ ospfd.c 15 May 2019 13:19:52 -
@@ -185,6 +185,8 @@ main(int argc, char *argv[])
kif_clear();
exit(1);
}
+if (ospfd_conf->rtr_id.s_addr == 0)
+ospfd_conf->rtr_id.s_addr = get_rtr_id();
if (sockname == NULL) {
if (asprintf(&sockname, "%s.%d", OSPFD_SOCKET,
@@ -641,6 +643,10 @@ ospf_reload(void)
if ((xconf = parse_config(conffile, ospfd_conf->opts)) == NULL)
return (-1);
+
+ /* No router-id was specified, keep existing value */
+if (xconf->rtr_id.s_addr == 0)
+xconf->rtr_id.s_addr = ospfd_conf->rtr_id.s_addr;
/* Abort the reload if rtr_id changed */
if (ospfd_conf->rtr_id.s_addr != xconf->rtr_id.s_addr) {
Index: ospfd.h
===
RCS file: /cvs/src/usr.sbin/ospfd/ospfd.h,v
retrieving revision 1.103
diff -u -p -r1.103 ospfd.h
--- ospfd.h 28 Dec 2018 19:25:10 - 1.103
+++ ospfd.h 15 May 2019 13:19:52 -
@@ -561,6 +561,7 @@ int carp_demote_set(char *, int);
/* parse.y */
struct ospfd_conf *parse_config(char *, int);
+u_int32_t get_rtr_id(void);
int cmdline_symset(char *);
voidconf_clear_redist_list(struct redist_list *);
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v
retrieving revision 1.96
diff -u -p -r1.96 parse.y
--- parse.y 29 Apr 2019 05:14:38 - 1.96
+++ parse.y 15 May 2019 13:19:52 -
@@ -83,7 +83,6 @@ intsymset(const char *, const char *,
char *symget(const char *);
voidclear_config(struct ospfd_conf *xconf);
-u_int32_t get_rtr_id(void);
int host(const char *, struct in_addr *, struct in_addr *);
static struct ospfd_conf *conf;
@@ -1253,9 +1252,6 @@ parse_config(char *filename, int opts)
clear_config(conf);
return (NULL);
}
-
- if (conf->rtr_id.s_addr == 0)
- conf->rtr_id.s_addr = get_rtr_id();
return (conf);
}
Fwd: Re: umsm(4) and umb(4) separate loading for the same composite USB modem device
Any progress in OpenBSD 6.5 to have umsm(4) and umb(4) recognition for the same composite USB device according to USB descriptors dumped for MC7304 and MC7455? 6.4 doesn't recognize mbim device umb(4) when umsm(4) ports enabled on one physical device simultaneously. I'm getting messages like below: umsm8: missing endpoint umsm9: missing endpoint for each umb(4) port on MC7304. Denis Forwarded Message Subject: Re: umsm(4) and umb(4) separate loading for the same composite USB modem device Date: Wed, 15 Aug 2018 16:51:23 +0300 From: Denis To: Bryan Vyhmeister CC: tech@openbsd.org umsm(4) patch is needed to recognize MC7304 by umsm(4) and to have both NMEA and AT ports enabled. Especially, I'm using MC7304's (MC7455 on another platform) NMEA port for NTP time corrections from GLO/GPS. AT port is needed for mode changes by AT commands. Very useful. umb(4) is a good one for modern fast data transmit over MBIM port. So the advantages of simultaneous running umsm(4) and umb(4) drivers for single physical device are: - having NMEA + AT serial discipline working by umsm(4); (any rather 'old' serial discipline functionality can be used like SMS, modem's mode changes by AT commands etc.) - MBIM for fast LTE Cat-3 / Cat-6 data connections by modern umb(4) driver. Denis On 8/15/2018 4:43 AM, Bryan Vyhmeister wrote: > On Tue, Aug 14, 2018 at 05:53:43PM +0300, Denis wrote: >> Most of modern modems have serial discipline ports and USB Mobile >> Broadband Interface Model (MBIM) interface in some port compositions >> simultaneously. It seems very useful to have different disciplines >> supported by umsm(4) and umb(4) drivers on the same device. >> > >> >> Does it possible to have simultaneously operated AT + NMEA ports by >> umsm(4)driver and MBIM interface by umb(4) driver on the same MC7304 >> device in 6.3? > > What is the advantage in having a device attach to both umsm(4) and > umb(4)? What are you trying to accomplish? The EM7455 worked perfectly > with umb(4) until your previous umsm(4) diff and now it only attaches as > umsm(4). Are you wanting to send SMS messages or something like that > with your devices? > > Bryan >
Re: print regress results
Hi Alexander, Alexander Bluhm wrote on Wed, May 15, 2019 at 12:01:00AM -0400: > Regress prints FAILED in the middle of the make output, this is > hard to watch. I agree this is a nuisance. I have often wondered whether the result was "PASS" or "FAIL" after doing longer regression runs in the past. > tb@ asked me to print a PASSED at the end. That would indeed be an improvement. > As the make processes cannot hold state over several targets or > directories, I create a regress log. It is placed in the top > directory where you invoke make regress. All failures are logged > there. if there are none, PASSED is printed as final output. That concept feels somewhat messy and fragile. The /^FAIL/ regular expression might misfire - at least test writers must be aware that tests must not frint FAIL at the beginning of output lines, so maybe you should document this? In principle, it would be cleaner to use the exit status of recursive make invocations. Then again, there are multiple places where make is invoked recursively, so i'm not sure that is feasible. If it isn't, or if a cleaner way would cause excessive complication, i'm not totally opposed to grepping log files, even though i don't particularly like it. Apart from the general approach, i'm not convinced the details are quite right just yet. For starters, it feels even more messy that the REGRESS_LOG variable invades bsd.subdir.mk - not sure whether that can somehow be avoided. Here are some examples of what i consider undesirable side effects: $ cd /usr/src/bin/ $ make -n obj rm -f -- /usr/src/bin/regress.log [...] I'm not convinced that "make obj" should attempt to change anything in the source tree, apart from creating "obj" symlinks. In particular, having "make obj" delete anything doesn't feel quite right. $ make -n rm -f -- /usr/src/bin/regress.log [...] While i see the point of deleting regress.log at the beginning of a regression run, it doesn't feel quite right at the beginning of a make run that has nothing to do with regression testing. Also, it doesn't feel quite right that the regress.log file is deleted from the *src* directory. Shouldn't it be deleted from the *obj* directory instead? I mean, like this: $ cd /usr/src/regress/usr.bin/mandoc/mdoc/Ad/ $ make -n regress rm -f -- /usr/src/regress/usr.bin/mandoc/mdoc/Ad/obj/regress.log [...] Then, what about this: $ make -dl regress mandoc -Ios=OpenBSD -Tascii [...] diff -au [...] [...] PASSED $ make -dl clean rm -f noarg.mandoc_ascii font.mandoc_ascii rm -f noarg.in_man noarg.mandoc_man font.in_man font.mandoc_man rm -f noarg.mandoc_lint rm -f noarg.mandoc_markdown font.mandoc_markdown $ ls obj/ regress.log $ make cleandir $ ls obj/ regress.log I would expect that both "make clean" and "make cleandir" remove the file regress.log, but they don't appear to. Evem if i remove "obj" and run "make regress" without running "make obj" before it, the file regress.log survives "make clean" and "make cleandir", this time not even below obj, but in the source tree itself. (Et ceterum censeo support for running without "make obj" esse delendam to simplify stuff, but that's a different matter.) $ cd /usr/src/regress/usr.bin/mandoc/tbl/ $ make -dl regress rm -f -- /usr/src/regress/usr.bin/mandoc/tbl/regress.log for entry in opt layout mod data macro; do [...] ===> opt diff -au [...] [...] PASSED ===> layout diff -au [...] [...] PASSED [...] ===> macro diff -au [...] [...] PASSED $ vi opt/box.out_ascii # bad guy manipulating the output $ make -dl regress rm -f -- /usr/src/regress/usr.bin/mandoc/tbl/regress.log for entry in opt layout mod data macro; do [...] ===> opt diff -au [...] *** Error 1 in opt (../../Makefile.inc:82 'box.diff_ascii') FAILED *** Error 1 in target 'regress' (ignored) diff -au [...] [...] FAIL usr.bin/mandoc/tbl/opt box ===> layout diff -au [...] [...] FAIL usr.bin/mandoc/tbl/opt box [...] ===> macro diff -au [...] [...] FAIL usr.bin/mandoc/tbl/opt box So in a recursive run, the ^FAIL lines are re-printed for each subsequent directory. Isn't the goal to print the summary once, at the end? Also, the file regress.log is created in the source tree in this case, even though i ran "make obj" before. Not sure how to prevent that because usr.bin/mandoc/tbl/ neither has nor needs an obj/ symlink because in contains only subdirs, no tests of its own. All the same, it doesn't feel quite right. These are not strong objections, you are maintaining this subsystem, but i have some doubts whether this particular patch is mature enough for commit. Yours, Ingo
Re: print regress results
On Wed, May 15, 2019 at 12:01:00AM -0400, Alexander Bluhm wrote: > Regress prints FAILED in the middle of the make output, this is > hard to watch. tb@ asked me to print a PASSED at the end. As the > make processes cannot hold state over several targets or directories, > I create a regress log. It is placed in the top directory where > you invoke make regress. All failures are logged there. if there > are none, PASSED is printed as final output. Thanks, OK kn.
