Re: remove games from PATHs provided by /etc/skel
On Thu, Aug 04, 2022 at 08:39:46PM -0600, Theo de Raadt wrote: > Bryan Steele wrote: > > > On Fri, Aug 05, 2022 at 03:37:41AM +0200, Theo Buehler wrote: > > > On Fri, Aug 05, 2022 at 03:34:57AM +0200, Theo Buehler wrote: > > > > If you want games, opt into it. They are very old, full of bugs and not > > > > really maintained. It's very easy to get a PATH containing games via > > > > /etc/skel. I think this is a poor default. > > > > > > Dropped a } by accident. > > > > If they are really unmaintained, then they should be removed from the > > tree, no? Otherwise I don't really see the point in this. > > Want to talk about something that is unmaintained? > > vi. > > Sorry, but I don't think you are in a position to push policy. I responded too quickly, sorry. It was less so my intention to suggest that thing be removed, but the opposite. Sorry for the noise..
Re: remove games from PATHs provided by /etc/skel
Bryan Steele wrote: > On Fri, Aug 05, 2022 at 03:37:41AM +0200, Theo Buehler wrote: > > On Fri, Aug 05, 2022 at 03:34:57AM +0200, Theo Buehler wrote: > > > If you want games, opt into it. They are very old, full of bugs and not > > > really maintained. It's very easy to get a PATH containing games via > > > /etc/skel. I think this is a poor default. > > > > Dropped a } by accident. > > If they are really unmaintained, then they should be removed from the > tree, no? Otherwise I don't really see the point in this. Want to talk about something that is unmaintained? vi. Sorry, but I don't think you are in a position to push policy.
Re: remove games from PATHs provided by /etc/skel
Bryan Steele wrote: > On Fri, Aug 05, 2022 at 03:37:41AM +0200, Theo Buehler wrote: > > On Fri, Aug 05, 2022 at 03:34:57AM +0200, Theo Buehler wrote: > > > If you want games, opt into it. They are very old, full of bugs and not > > > really maintained. It's very easy to get a PATH containing games via > > > /etc/skel. I think this is a poor default. > > > > Dropped a } by accident. > > If they are really unmaintained, then they should be removed from the > tree, no? Otherwise I don't really see the point in this. There were 3847 lines changed in games in 1 year, 45157 in 10 years, etc
Re: remove games from PATHs provided by /etc/skel
On Fri, Aug 05, 2022 at 03:37:41AM +0200, Theo Buehler wrote:
> On Fri, Aug 05, 2022 at 03:34:57AM +0200, Theo Buehler wrote:
> > If you want games, opt into it. They are very old, full of bugs and not
> > really maintained. It's very easy to get a PATH containing games via
> > /etc/skel. I think this is a poor default.
>
> Dropped a } by accident.
If they are really unmaintained, then they should be removed from the
tree, no? Otherwise I don't really see the point in this.
-Bryan.
> Index: dot.cshrc
> ===
> RCS file: /cvs/src/etc/skel/dot.cshrc,v
> retrieving revision 1.10
> diff -u -p -r1.10 dot.cshrc
> --- dot.cshrc 24 Jan 2020 02:09:51 - 1.10
> +++ dot.cshrc 5 Aug 2022 01:37:02 -
> @@ -13,7 +13,7 @@ alias llls -lsA
> alias tset 'set noglob histchars=""; eval `\tset -s \!*`; unset noglob
> histchars'
> alias z suspend
>
> -set path = (~/bin /bin /sbin
> /usr/{bin,sbin,X11R6/bin,local/bin,local/sbin,games})
> +set path = (~/bin /bin /sbin /usr/{bin,sbin,X11R6/bin,local/bin,local/sbin})
>
> if ($?prompt) then
> # An interactive shell -- set some stuff up
> Index: dot.profile
> ===
> RCS file: /cvs/src/etc/skel/dot.profile,v
> retrieving revision 1.7
> diff -u -p -r1.7 dot.profile
> --- dot.profile 24 Jan 2020 02:09:51 - 1.7
> +++ dot.profile 5 Aug 2022 01:30:52 -
> @@ -2,5 +2,5 @@
> #
> # sh/ksh initialization
>
> -PATH=$HOME/bin:/bin:/sbin:/usr/bin:/usr/sbin:/usr/X11R6/bin:/usr/local/bin:/usr/local/sbin:/usr/games
> +PATH=$HOME/bin:/bin:/sbin:/usr/bin:/usr/sbin:/usr/X11R6/bin:/usr/local/bin:/usr/local/sbin
> export PATH HOME TERM
>
>
Re: remove games from PATHs provided by /etc/skel
On Fri, Aug 05, 2022 at 03:34:57AM +0200, Theo Buehler wrote:
> If you want games, opt into it. They are very old, full of bugs and not
> really maintained. It's very easy to get a PATH containing games via
> /etc/skel. I think this is a poor default.
Dropped a } by accident.
Index: dot.cshrc
===
RCS file: /cvs/src/etc/skel/dot.cshrc,v
retrieving revision 1.10
diff -u -p -r1.10 dot.cshrc
--- dot.cshrc 24 Jan 2020 02:09:51 - 1.10
+++ dot.cshrc 5 Aug 2022 01:37:02 -
@@ -13,7 +13,7 @@ alias ll ls -lsA
alias tset 'set noglob histchars=""; eval `\tset -s \!*`; unset noglob
histchars'
alias zsuspend
-set path = (~/bin /bin /sbin
/usr/{bin,sbin,X11R6/bin,local/bin,local/sbin,games})
+set path = (~/bin /bin /sbin /usr/{bin,sbin,X11R6/bin,local/bin,local/sbin})
if ($?prompt) then
# An interactive shell -- set some stuff up
Index: dot.profile
===
RCS file: /cvs/src/etc/skel/dot.profile,v
retrieving revision 1.7
diff -u -p -r1.7 dot.profile
--- dot.profile 24 Jan 2020 02:09:51 - 1.7
+++ dot.profile 5 Aug 2022 01:30:52 -
@@ -2,5 +2,5 @@
#
# sh/ksh initialization
-PATH=$HOME/bin:/bin:/sbin:/usr/bin:/usr/sbin:/usr/X11R6/bin:/usr/local/bin:/usr/local/sbin:/usr/games
+PATH=$HOME/bin:/bin:/sbin:/usr/bin:/usr/sbin:/usr/X11R6/bin:/usr/local/bin:/usr/local/sbin
export PATH HOME TERM
remove games from PATHs provided by /etc/skel
If you want games, opt into it. They are very old, full of bugs and not
really maintained. It's very easy to get a PATH containing games via
/etc/skel. I think this is a poor default.
Index: dot.cshrc
===
RCS file: /cvs/src/etc/skel/dot.cshrc,v
retrieving revision 1.10
diff -u -p -r1.10 dot.cshrc
--- dot.cshrc 24 Jan 2020 02:09:51 - 1.10
+++ dot.cshrc 5 Aug 2022 01:31:01 -
@@ -13,7 +13,7 @@ alias ll ls -lsA
alias tset 'set noglob histchars=""; eval `\tset -s \!*`; unset noglob
histchars'
alias zsuspend
-set path = (~/bin /bin /sbin
/usr/{bin,sbin,X11R6/bin,local/bin,local/sbin,games})
+set path = (~/bin /bin /sbin /usr/{bin,sbin,X11R6/bin,local/bin,local/sbin)
if ($?prompt) then
# An interactive shell -- set some stuff up
Index: dot.profile
===
RCS file: /cvs/src/etc/skel/dot.profile,v
retrieving revision 1.7
diff -u -p -r1.7 dot.profile
--- dot.profile 24 Jan 2020 02:09:51 - 1.7
+++ dot.profile 5 Aug 2022 01:30:52 -
@@ -2,5 +2,5 @@
#
# sh/ksh initialization
-PATH=$HOME/bin:/bin:/sbin:/usr/bin:/usr/sbin:/usr/X11R6/bin:/usr/local/bin:/usr/local/sbin:/usr/games
+PATH=$HOME/bin:/bin:/sbin:/usr/bin:/usr/sbin:/usr/X11R6/bin:/usr/local/bin:/usr/local/sbin
export PATH HOME TERM
Re: random(6): undefined cast and error checking
Theo Buehler wrote: > On Thu, Aug 04, 2022 at 07:11:40PM -0600, Theo de Raadt wrote: > > And anyways, this directory is not in $PATH by default, so there is no > > risk. > > Unless you create a user during install in which case /etc/skel will > give you a $PATH containing /usr/games via dot.profile (I assume for > csh users via dot.cshrc). Then fix that...
Re: random(6): undefined cast and error checking
On Thu, Aug 04, 2022 at 07:11:40PM -0600, Theo de Raadt wrote: > And anyways, this directory is not in $PATH by default, so there is no > risk. Unless you create a user during install in which case /etc/skel will give you a $PATH containing /usr/games via dot.profile (I assume for csh users via dot.cshrc).
Re: random(6): undefined cast and error checking
luci...@bronze.ctrl-c.club wrote: > t...@theobuehler.org wrote: > >I have no strong opinion. I'm fine with either approach. It's such a > >silly program... > > > >As an aside, random -e has been completely broken (it's non-uniform) > >since forever. To fix -e, we should clamp denom to an integer between > >1 and 256, otherwise the truncation of the exit exit code to an 8-bit > >int introduces bias for numbers larger than 256 (that aren't powers of > >2). > > The program is broken in multiple ways: return value clamping, casting > from double to uint32_t, wrong error checking for putchar, lack of > warnings when compiling. > > What I don't understand is why such a wrong program has its place in > OpenBSD. Maybe it's historical reasons, who knows. But the fact that it > exists and it's badly written bothers me. the games directory is a playground. It is low-risk place in our tree where someone can find some very old code and try to raise it to the standard of non-games code, and learn lessons. If they make a mistake, that's OK. If they learn from the mistakes and find some great idea, that's even better. A bunch of pledge and unveil experimentation happened in games, and some privdrop and privsep has also happened there. All which are incomplete. There is more which can be done there. It kind of demonstrates that very old software CAN adopt the security-focus changes we've been making. The ideas are pretty universal, and can even be applied to terribly old software. If all the software in our ecosystem was modern, there would be no place to practice and learn. And anyways, this directory is not in $PATH by default, so there is no risk.
Re: riscv64: trigger deferred timer interrupts from splx(9)
On Fri, Aug 05, 2022 at 12:34:59AM +0200, Jeremie Courreges-Anglas wrote:
> >> [...]
> >>
> >> You're adding the timer reset to plic_setipl() but the latter is called
> >> after softintr processing in plic_splx().
> >>
> >>/* Pending software intr is handled here */
> >>if (ci->ci_ipending & riscv_smask[new])
> >>riscv_do_pending_intr(new);
> >>
> >>plic_setipl(new);
> >
> > Yes, but plic_setipl() is also called from the softintr loop in
> > riscv_do_pending_intr() (riscv64/intr.c) right *before* we dispatch
> > any pending soft interrupts:
> >
> >594 void
> >595 riscv_do_pending_intr(int pcpl)
> >596 {
> >597 struct cpu_info *ci = curcpu();
> >598 u_long sie;
> >599
> >600 sie = intr_disable();
> >601
> >602 #define DO_SOFTINT(si, ipl) \
> >603 if ((ci->ci_ipending & riscv_smask[pcpl]) & \
> >604 SI_TO_IRQBIT(si)) { \
> >605 ci->ci_ipending &= ~SI_TO_IRQBIT(si); \
> > * 606 riscv_intr_func.setipl(ipl);\
> >607 intr_restore(sie); \
> >608 softintr_dispatch(si); \
> >609 sie = intr_disable(); \
> >610 }
> >611
> >612 do {
> >613 DO_SOFTINT(SIR_TTY, IPL_SOFTTTY);
> >614 DO_SOFTINT(SIR_NET, IPL_SOFTNET);
> >615 DO_SOFTINT(SIR_CLOCK, IPL_SOFTCLOCK);
> >616 DO_SOFTINT(SIR_SOFT, IPL_SOFT);
> >617 } while (ci->ci_ipending & riscv_smask[pcpl]);
> >
> > We might be fine doing it just once in plic_splx() before we do any
> > soft interrupt stuff. That's closer to what we're doing on other
> > platforms.
> >
> > I just figured it'd be safer to do it in plic_setipl() because we're
> > already disabling interrupts there. It seems I guessed correctly
> > because the patch didn't hang your machine.
>
> Ugh, I had missed that setipl call, thanks for pointing it out.
Np.
> Since I don't wander into this code on a casual basis I won't object,
> but this looks very unobvious to me. :)
I kind of agree.
I think it would be cleaner -- logically cleaner, not necessarily
cleaner in the code -- to mask timer interrupts when we raise the IPL
to or beyond IPL_CLOCK and unmask timer interrupts when we drop the
IPL below IPL_CLOCK.
... but doing it this way is a lot faster than taking the time to read
and understand the RISC-V privileged architecture spec and how the SBI
interacts with it.
At a glance I see that there are separate Interrupt-Enable bits for
External, Timer, and Software interrupts at the supervisor level. So
what I'm imagining might be possible. I just don't know how to get
the current code to do what I've described.
Re: riscv64: trigger deferred timer interrupts from splx(9)
On Thu, Aug 04 2022, Scott Cheloha wrote:
> On Thu, Aug 04, 2022 at 09:39:13AM +0200, Jeremie Courreges-Anglas wrote:
>> On Mon, Aug 01 2022, Scott Cheloha wrote:
>> > On Mon, Aug 01, 2022 at 07:15:33PM +0200, Jeremie Courreges-Anglas wrote:
>> >> On Sun, Jul 31 2022, Scott Cheloha wrote:
>> >> > Hi,
>> >> >
>> >> > I am unsure how to properly mask RISC-V timer interrupts in hardware
>> >> > at or above IPL_CLOCK. I think that would be cleaner than doing
>> >> > another timer interrupt deferral thing.
>> >> >
>> >> > But, just to get the ball rolling, here a first attempt at the timer
>> >> > interrupt deferral thing for riscv64. The motivation, as with every
>> >> > other platform, is to eventually make it unnecessary for the machine
>> >> > dependent code to know anything about the clock interrupt schedule.
>> >> >
>> >> > The thing I'm most unsure about is where to retrigger the timer in the
>> >> > PLIC code. It seems right to do it from plic_setipl() because I want
>> >> > to retrigger it before doing soft interrupt work, but I'm not sure.
>>
>> You're adding the timer reset to plic_setipl() but the latter is called
>> after softintr processing in plic_splx().
>>
>> /* Pending software intr is handled here */
>> if (ci->ci_ipending & riscv_smask[new])
>> riscv_do_pending_intr(new);
>>
>> plic_setipl(new);
>
> Yes, but plic_setipl() is also called from the softintr loop in
> riscv_do_pending_intr() (riscv64/intr.c) right *before* we dispatch
> any pending soft interrupts:
>
>594 void
>595 riscv_do_pending_intr(int pcpl)
>596 {
>597 struct cpu_info *ci = curcpu();
>598 u_long sie;
>599
>600 sie = intr_disable();
>601
>602 #define DO_SOFTINT(si, ipl) \
>603 if ((ci->ci_ipending & riscv_smask[pcpl]) & \
>604 SI_TO_IRQBIT(si)) { \
>605 ci->ci_ipending &= ~SI_TO_IRQBIT(si); \
> * 606 riscv_intr_func.setipl(ipl);\
>607 intr_restore(sie); \
>608 softintr_dispatch(si); \
>609 sie = intr_disable(); \
>610 }
>611
>612 do {
>613 DO_SOFTINT(SIR_TTY, IPL_SOFTTTY);
>614 DO_SOFTINT(SIR_NET, IPL_SOFTNET);
>615 DO_SOFTINT(SIR_CLOCK, IPL_SOFTCLOCK);
>616 DO_SOFTINT(SIR_SOFT, IPL_SOFT);
>617 } while (ci->ci_ipending & riscv_smask[pcpl]);
>
> We might be fine doing it just once in plic_splx() before we do any
> soft interrupt stuff. That's closer to what we're doing on other
> platforms.
>
> I just figured it'd be safer to do it in plic_setipl() because we're
> already disabling interrupts there. It seems I guessed correctly
> because the patch didn't hang your machine.
Ugh, I had missed that setipl call, thanks for pointing it out.
Since I don't wander into this code on a casual basis I won't object,
but this looks very unobvious to me. :)
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: parallel divert packet soreceive
> On 4 Aug 2022, at 18:05, Vitaliy Makkoveev wrote: > > On Thu, Aug 04, 2022 at 01:42:48PM +0200, Alexander Bluhm wrote: >> On Thu, Aug 04, 2022 at 02:18:49AM +0300, Vitaliy Makkoveev wrote: >>> Also, I like to have exclusive layer locks like `tcp_lock???, >>> `udp_lock???, etc.. And take them with shared netlock held as the >>> first step of inet sockets unlocking. >> >> With PRU_LOCK and PRU_UNLOCK each layer can decide itself which >> kind is appropriate. For divert packet per PCB lock is trivial. >> For loops delivering to all sockets like UDP multicast or RAW >> sockets, a mutex per layer an easier start. >> sblock()/sbunlock() relie on exclusive socket or layer lock, which protects `sb_flags??? modification. This diff breaks them, because nothing stops concurrent soreceive() threads to set SB_LOCK flag and and be sure that socket buffer is locked. >> >> Either sb_flags is protected by the exclusice netlock or by a shared >> netlock and PCB mutex. So only one thread can access them. Right? >> > > Yes, you are right, I missed the PRU_LOCK/PRU_UNLOCK calls. > > With your diff, sbwait() releases only netlock and leaves `inp_mtx' > locked. So when we enter sbwait() in the soreceive() path, the > concurrent sbappendaddr() thread will spin instead of append mbut to > `so_rcv'. > > It seems sbwait() should release `inp_mtx' and keep shared netlock > locked. Both of netlock and `inp_mtx’ should be released by sbwait(). This is fully identical to current behaviour.
Re: random(6): undefined cast and error checking
t...@theobuehler.org wrote: >I have no strong opinion. I'm fine with either approach. It's such a >silly program... > >As an aside, random -e has been completely broken (it's non-uniform) >since forever. To fix -e, we should clamp denom to an integer between >1 and 256, otherwise the truncation of the exit exit code to an 8-bit >int introduces bias for numbers larger than 256 (that aren't powers of >2). The program is broken in multiple ways: return value clamping, casting from double to uint32_t, wrong error checking for putchar, lack of warnings when compiling. What I don't understand is why such a wrong program has its place in OpenBSD. Maybe it's historical reasons, who knows. But the fact that it exists and it's badly written bothers me.
Re: riscv64: trigger deferred timer interrupts from splx(9)
> Date: Thu, 4 Aug 2022 10:36:22 -0500
> From: Scott Cheloha
>
> On Thu, Aug 04, 2022 at 09:39:13AM +0200, Jeremie Courreges-Anglas wrote:
> > On Mon, Aug 01 2022, Scott Cheloha wrote:
> > > On Mon, Aug 01, 2022 at 07:15:33PM +0200, Jeremie Courreges-Anglas wrote:
> > >> On Sun, Jul 31 2022, Scott Cheloha wrote:
> > >> > Hi,
> > >> >
> > >> > I am unsure how to properly mask RISC-V timer interrupts in hardware
> > >> > at or above IPL_CLOCK. I think that would be cleaner than doing
> > >> > another timer interrupt deferral thing.
> > >> >
> > >> > But, just to get the ball rolling, here a first attempt at the timer
> > >> > interrupt deferral thing for riscv64. The motivation, as with every
> > >> > other platform, is to eventually make it unnecessary for the machine
> > >> > dependent code to know anything about the clock interrupt schedule.
> > >> >
> > >> > The thing I'm most unsure about is where to retrigger the timer in the
> > >> > PLIC code. It seems right to do it from plic_setipl() because I want
> > >> > to retrigger it before doing soft interrupt work, but I'm not sure.
> >
> > You're adding the timer reset to plic_setipl() but the latter is called
> > after softintr processing in plic_splx().
> >
> > /* Pending software intr is handled here */
> > if (ci->ci_ipending & riscv_smask[new])
> > riscv_do_pending_intr(new);
> >
> > plic_setipl(new);
>
> Yes, but plic_setipl() is also called from the softintr loop in
> riscv_do_pending_intr() (riscv64/intr.c) right *before* we dispatch
> any pending soft interrupts:
>
>594 void
>595 riscv_do_pending_intr(int pcpl)
>596 {
>597 struct cpu_info *ci = curcpu();
>598 u_long sie;
>599
>600 sie = intr_disable();
>601
>602 #define DO_SOFTINT(si, ipl) \
>603 if ((ci->ci_ipending & riscv_smask[pcpl]) & \
>604 SI_TO_IRQBIT(si)) { \
>605 ci->ci_ipending &= ~SI_TO_IRQBIT(si); \
> * 606 riscv_intr_func.setipl(ipl);\
>607 intr_restore(sie); \
>608 softintr_dispatch(si); \
>609 sie = intr_disable(); \
>610 }
>611
>612 do {
>613 DO_SOFTINT(SIR_TTY, IPL_SOFTTTY);
>614 DO_SOFTINT(SIR_NET, IPL_SOFTNET);
>615 DO_SOFTINT(SIR_CLOCK, IPL_SOFTCLOCK);
>616 DO_SOFTINT(SIR_SOFT, IPL_SOFT);
>617 } while (ci->ci_ipending & riscv_smask[pcpl]);
>
> We might be fine doing it just once in plic_splx() before we do any
> soft interrupt stuff. That's closer to what we're doing on other
> platforms.
>
> I just figured it'd be safer to do it in plic_setipl() because we're
> already disabling interrupts there. It seems I guessed correctly
> because the patch didn't hang your machine.
>
> > >> > Unless I'm missing something, I don't think I need to do anything in
> > >> > the default interrupt handler code, i.e. riscv64_dflt_setipl(), right?
> > >>
> > >> No idea about about the items above, but...
> > >>
> > >> > I have no riscv64 machine, so this is untested. Would appreciate
> > >> > tests and feedback.
> > >>
> > >> There's an #include missing in plic.c,
> > >
> > > Whoops, corrected patch attached below.
> > >
> > >> with that added your diff builds and GENERIC.MP seems to behave
> > >> (currently running make -j4 build), but I don't know exactly which
> > >> problems I should look for.
> > >
> > > Thank you for trying it out.
> > >
> > > The patch changes how clock interrupt work is deferred on riscv64.
> > >
> > > If the code is wrong, the hardclock and statclock should eventually
> > > die on every CPU. The death of the hardclock in particular would
> > > manifest to the user as livelock. The scheduler would stop preempting
> > > userspace and it would be impossible to use the machine interactively.
> > >
> > > There isn't really a direct way to exercise this code change.
> > >
> > > The best we can do is make the machine busy. If the machine is busy
> > > we can expect more spl(9) calls and more deferred clock interrupt
> > > work, which leaves more opportunities for the bug to surface.
> > >
> > > So, a parallel `make build` is fine. It's our gold standard for
> > > making the machine really busy.
> >
> > The diff survived three make -j4 build/release in a row, the clock seems
> > stable.
>
> Awesome! Thank you for hammering on it.
>
> kettenis, mlarkin, drahn:
>
> Is this code fine or do you want to go about this in a different way?
Ideally we'd handle the deferred timer in generic code, but that is a
refactoring that can be done later when a SoC shows up that doesn't
use plic(4).
ok kettenis@
> Index: dev/plic.c
> ===
> RCS file:
Re: ip input fragment mutex
I’m surprised by ISSET() macro.
ok mvs@
> On 4 Aug 2022, at 20:06, Alexander Bluhm wrote:
>
> On Thu, Jul 28, 2022 at 04:41:54PM +0200, Alexander Bluhm wrote:
>> -mff = (ip->ip_off & htons(IP_MF)) != 0;
>> +mff = ISSET(ip->ip_off, htons(IP_MF));
>
> This part breaks big endian machines.
> New mff is 0x0020 on little and 0x2000 on big endian.
> Later it is assigned to u_int8_t ipqe_mff. This strips higher bits.
>
> I think the best way is to have 16 bit variables everywhere.
>
> ok?
>
> bluhm
>
> Index: netinet/ip_input.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_input.c,v
> retrieving revision 1.375
> diff -u -p -r1.375 ip_input.c
> --- netinet/ip_input.c28 Jul 2022 22:05:39 - 1.375
> +++ netinet/ip_input.c4 Aug 2022 17:01:55 -
> @@ -577,7 +577,8 @@ ip_fragcheck(struct mbuf **mp, int *offp
> struct ip *ip;
> struct ipq *fp;
> struct ipqent *ipqe;
> - int mff, hlen;
> + int hlen;
> + uint16_t mff;
>
> ip = mtod(*mp, struct ip *);
> hlen = ip->ip_hl << 2;
> Index: netinet/ip_var.h
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_var.h,v
> retrieving revision 1.94
> diff -u -p -r1.94 ip_var.h
> --- netinet/ip_var.h 25 Jul 2022 23:19:34 - 1.94
> +++ netinet/ip_var.h 4 Aug 2022 17:01:55 -
> @@ -174,7 +174,7 @@ struct ipqent {
> LIST_ENTRY(ipqent) ipqe_q;
> struct ip *ipqe_ip;
> struct mbuf *ipqe_m;/* mbuf contains packet */
> - u_int8_tipqe_mff; /* for IP fragmentation */
> + uint16_t ipqe_mff; /* for IP fragmentation */
> };
>
> /*
>
softraid(4) RAID 1C boot support on amd64
This adds support for booting from RAID 1C volumes on amd64.
Only boot-loader changes are needed. Both installboot(8) and
the kernel already do what is required to make this work.
I have tested with biosboot in vmm. The changes involved are trivial,
and I am modifying copies of the same code across all bootloaders.
So I would assume that all boot methods work with this patch.
Additional testing to verify this would be welcome, of course.
In my testing, a fresh install onto a RAID 1C volume succeeds and
the resulting system can be booted as usual. I have tried this on
a volume locked with a keydisk, and a volume locked with a passphrase.
I have also tested booting with one of two disks missing and as
expected the system came up with a degraded volume:
softraid0: not all chunks were provided; attempting to bring volume 0 online
softraid0: trying to bring up sd3 degraded
sd2 at scsibus4 targ 1 lun 0:
sd2: 8191MB, 512 bytes/sector, 16776624 sectors
softraid0: volume sd2 is roaming, it used to be sd3, updating metadata
softraid0: roaming device sd1a -> sd0a
root on sd2a (3ee0bf348da5ea75.a) swap on sd2b dump on sd2b
The volume could successfully be rebuilt with bioctl -R, once the
missing disk was added back in.
ok?
diff 43cbe5bff6629c463463325cebb7e87b291b4a73
20d3307dd4ffcb7fd9baece3386a59b24bbcc8e0
commit - 43cbe5bff6629c463463325cebb7e87b291b4a73
commit + 20d3307dd4ffcb7fd9baece3386a59b24bbcc8e0
blob - c00d17a6628822b649ad4391a3af7c69801631f0
blob + e320f3d143c2d2023f83743be4ef31e82eabe2af
--- share/man/man4/softraid.4
+++ share/man/man4/softraid.4
@@ -118,7 +118,8 @@ may be used to install
in the boot storage area of the
.Nm
volume.
-Boot support is currently limited to the CRYPTO and RAID 1 disciplines
+Boot support is currently limited to the RAID 1C discipline on the
+amd64 platform, and the CRYPTO and RAID 1 disciplines
on amd64, arm64, i386, and sparc64 platforms.
On sparc64, bootable chunks must be RAID partitions using the letter
.Sq a .
blob - 543d9b404ed39512ed693e96d60958d8d467c3e8
blob + bda7e562558bc1cfad1800dac3f93e03439b48f7
--- sys/arch/amd64/stand/boot/conf.c
+++ sys/arch/amd64/stand/boot/conf.c
@@ -41,7 +41,7 @@
#include
#include
-const char version[] = "3.54";
+const char version[] = "3.55";
intdebug = 1;
blob - 5b4e78c86c4ea1a1d3491ca2a8f973588a38e7ba
blob + a53be08f763dd8a2069f5646bf266d747f3346a7
--- sys/arch/amd64/stand/cdboot/conf.c
+++ sys/arch/amd64/stand/cdboot/conf.c
@@ -42,7 +42,7 @@
#include
#include
-const char version[] = "3.54";
+const char version[] = "3.55";
intdebug = 1;
blob - dcf19e147de5c7c2e8b2e75fc0c47bf97421dd97
blob + ca83d39d7e22d24f86fc328e1a8703c4de8ce056
--- sys/arch/amd64/stand/efi32/conf.c
+++ sys/arch/amd64/stand/efi32/conf.c
@@ -40,7 +40,7 @@
#include "efidev.h"
#include "efipxe.h"
-const char version[] = "3.53";
+const char version[] = "3.54";
#ifdef EFI_DEBUG
intdebug = 0;
blob - e55f63529858ee9b98014f1cb1e0105db8e5d7a9
blob + 33b15ed229b1895c04047026193f36deb649866a
--- sys/arch/amd64/stand/efi32/efidev.c
+++ sys/arch/amd64/stand/efi32/efidev.c
@@ -599,9 +599,11 @@ efiopen(struct open_file *f, ...)
return EADAPT;
}
- if (bv->sbv_level == 'C' && bv->sbv_keys == NULL)
+ if ((bv->sbv_level == 'C' || bv->sbv_level == 0x1C) &&
+ bv->sbv_keys == NULL) {
if (sr_crypto_unlock_volume(bv) != 0)
return EPERM;
+ }
if (bv->sbv_diskinfo == NULL) {
dip = alloc(sizeof(struct diskinfo));
blob - 4eac75be43a98df370691f543dd172b8885dc0a4
blob + 8ef9e04e890c237994cdcacc98b8d78de43d4e37
--- sys/arch/amd64/stand/efi64/conf.c
+++ sys/arch/amd64/stand/efi64/conf.c
@@ -40,7 +40,7 @@
#include "efidev.h"
#include "efipxe.h"
-const char version[] = "3.53";
+const char version[] = "3.54";
#ifdef EFI_DEBUG
intdebug = 0;
blob - e55f63529858ee9b98014f1cb1e0105db8e5d7a9
blob + 33b15ed229b1895c04047026193f36deb649866a
--- sys/arch/amd64/stand/efi64/efidev.c
+++ sys/arch/amd64/stand/efi64/efidev.c
@@ -599,9 +599,11 @@ efiopen(struct open_file *f, ...)
return EADAPT;
}
- if (bv->sbv_level == 'C' && bv->sbv_keys == NULL)
+ if ((bv->sbv_level == 'C' || bv->sbv_level == 0x1C) &&
+ bv->sbv_keys == NULL) {
if (sr_crypto_unlock_volume(bv) != 0)
return EPERM;
+ }
if (bv->sbv_diskinfo == NULL) {
dip = alloc(sizeof(struct diskinfo));
blob - 2096ee66f3d2908bf2b462211f8ba7aa5b6a83a0
blob + b202cb05372802682f84d370c1e33dee21259938
--- sys/arch/amd64/stand/efiboot/conf.c
+++ sys/arch/amd64/stand/efiboot/conf.c
@@ -40,7 +40,7 @@
#include "efidev.h"
#include "efipxe.h"
-const char version[] = "3.61";
+const char version[] = "3.62";
Re: random(6): undefined cast and error checking
On Wed, Aug 03, 2022 at 04:21:43PM -0500, luci...@bronze.ctrl-c.club wrote:
> >On Wed, Aug 03, 2022 at 08:26:20AM -0600, Theo de Raadt wrote:
> >> luci...@bronze.ctrl-c.club wrote:
> >>
> >> > Another way to solve this problem would be to trim the numbers with
> >> > something like this: if (denom > UINT32_MAX) denom = UINT32_MAX.
> >>
> >> And then document that the program returns incorrect results?
>
> t...@theobuehler.org wrote:
>
> >See this. There's also linked BSD-licensed code that should not be hard
> >to adapt.
> >
> >https://mumble.net/~campbell/2014/04/28/uniform-random-float?
> >
> >Then pick a line if the random number drawn is < 1/denom.
>
> So now we have two options. (1) We use the trim trick and document
> the range of values that the denominator can be in. (2) We use the
> code linked by tb@ to generate uniform doubles. (see patch below)
>
> (2) seems to work but I need to spend some more time to understand
> the algorithm behind it.
>
> (1) seems more robust cosidering that the man page will also contain
> information about the range of denom.
Thanks.
I have no strong opinion. I'm fine with either approach. It's such a
silly program...
Since you've already done 90% of the work for (2), we can push it over
the line. This will need adding Taylor R. Campbell's license since a
significant chunk of code is included verbatim. Add the license below
the existing one.
As an aside, random -e has been completely broken (it's non-uniform)
since forever. To fix -e, we should clamp denom to an integer between 1
and 256, otherwise the truncation of the exit exit code to an 8-bit int
introduces bias for numbers larger than 256 (that aren't powers of 2).
Restricting denom >= 1 would seem necessary for all modes of this
program given that 1/denominator is documented to be a probability.
The patch doesn't compile due to missing clz64. Instead of using a
__builtin (which I'm unsure if it's available on all our ancient gccs),
I think we can implement this naively. Something along these lines
should be good enough (only very lightly tested):
int
clz64(uint64_t x)
{
static const uint64_t mask[] = {
0x, 0x, 0xff00, 0xf0, 0xc, 0x2,
};
static int zeroes[] = {
32, 16, 8, 4, 2, 1,
};
int clz = 0;
int i;
if (x == 0)
return 64;
for (i = 0; i < 6; i++) {
if ((x & mask[i]) == 0)
clz += zeroes[i];
else
x >>= zeroes[i];
}
return clz;
}
Some more comments inline.
>
> Index: random.c
> ===
> RCS file: /cvs/src/games/random/random.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 random.c
> --- random.c 7 Mar 2016 12:07:56 - 1.20
> +++ random.c 3 Aug 2022 20:28:08 -
> @@ -38,6 +38,75 @@
> #include
> #include
> #include
> +#include
> +#include
style: keep these in alphabetical order
> +
> +uint64_t random64(void)
style: tab should be a newline:
uint64_t
random64(void)
> +{
> + uint64_t r;
style: add empty line
> + arc4random_buf(, sizeof(uint64_t));
and here
> + return r;
> +}
> +
> +/*
> + * random_real: Generate a stream of bits uniformly at random and
> + * interpret it as the fractional part of the binary expansion of a
> + * number in [0, 1], 0.101001010100...; then round it.
> + */
> +double
> +random_real(void)
> +{
> + int exponent = -64;
> + uint64_t significand;
> + unsigned shift;
> +
> + /*
> + * Read zeros into the exponent until we hit a one; the rest
> + * will go into the significand.
> + */
> + while (__predict_false((significand = random64()) == 0)) {
> + exponent -= 64;
> + /*
> + * If the exponent falls below -1074 = emin + 1 - p,
> + * the exponent of the smallest subnormal, we are
> + * guaranteed the result will be rounded to zero. This
> + * case is so unlikely it will happen in realistic
> + * terms only if random64 is broken.
> + */
> + if (__predict_false(exponent < -1074))
> + return 0;
> + }
> +
> + /*
> + * There is a 1 somewhere in significand, not necessarily in
> + * the most significant position. If there are leading zeros,
> + * shift them into the exponent and refill the less-significant
> + * bits of the significand. Can't predict one way or another
> + * whether there are leading zeros: there's a fifty-fifty
> + * chance, if random64 is uniformly distributed.
> + */
> + shift = clz64(significand);
> + if (shift != 0) {
> + exponent -= shift;
> + significand <<= shift;
> + significand |= (random64() >> (64 - shift));
> + }
> +
> + /*
> + * Set the
Re: ip input fragment mutex
On Thu, Jul 28, 2022 at 04:41:54PM +0200, Alexander Bluhm wrote:
> - mff = (ip->ip_off & htons(IP_MF)) != 0;
> + mff = ISSET(ip->ip_off, htons(IP_MF));
This part breaks big endian machines.
New mff is 0x0020 on little and 0x2000 on big endian.
Later it is assigned to u_int8_t ipqe_mff. This strips higher bits.
I think the best way is to have 16 bit variables everywhere.
ok?
bluhm
Index: netinet/ip_input.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_input.c,v
retrieving revision 1.375
diff -u -p -r1.375 ip_input.c
--- netinet/ip_input.c 28 Jul 2022 22:05:39 - 1.375
+++ netinet/ip_input.c 4 Aug 2022 17:01:55 -
@@ -577,7 +577,8 @@ ip_fragcheck(struct mbuf **mp, int *offp
struct ip *ip;
struct ipq *fp;
struct ipqent *ipqe;
- int mff, hlen;
+ int hlen;
+ uint16_t mff;
ip = mtod(*mp, struct ip *);
hlen = ip->ip_hl << 2;
Index: netinet/ip_var.h
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_var.h,v
retrieving revision 1.94
diff -u -p -r1.94 ip_var.h
--- netinet/ip_var.h25 Jul 2022 23:19:34 - 1.94
+++ netinet/ip_var.h4 Aug 2022 17:01:55 -
@@ -174,7 +174,7 @@ struct ipqent {
LIST_ENTRY(ipqent) ipqe_q;
struct ip *ipqe_ip;
struct mbuf *ipqe_m;/* mbuf contains packet */
- u_int8_tipqe_mff; /* for IP fragmentation */
+ uint16_t ipqe_mff; /* for IP fragmentation */
};
/*
Re: nd6: Rename is_newentry to newentry
On Thu, Aug 04, 2022 at 05:37:54PM +0200, Florian Obser wrote: > On 2022-08-04 14:21 UTC, Klemens Nanni wrote: > > This matches the extensive comments and schema for related variables. > > No functional change. > > are you planning to work on ND, or is this just shuffing of deck chairs? Yes, I am. > > When I rewrote source address selection it was worthwhile that blame > worked to figure out when stuff was added. You are adding noise to > that. Seeing that both Net- and FreeBSD still stick with this, I might as well drop this change (other changes ought to be synced/adapted). It was merely annoying to not find a variable with the name "newentry" when using the editor to search for the word under the cursor.
Re: nd6: Rename is_newentry to newentry
On 2022-08-04 14:21 UTC, Klemens Nanni wrote:
> This matches the extensive comments and schema for related variables.
> No functional change.
are you planning to work on ND, or is this just shuffing of deck chairs?
When I rewrote source address selection it was worthwhile that blame
worked to figure out when stuff was added. You are adding noise to
that.
(I do not plan to work on this any time soon.)
>
> OK?
>
> diff --git a/sys/netinet6/nd6.c b/sys/netinet6/nd6.c
> index c0b92f059c9..d793a0b07f4 100644
> --- a/sys/netinet6/nd6.c
> +++ b/sys/netinet6/nd6.c
> @@ -1084,7 +1084,7 @@ nd6_cache_lladdr(struct ifnet *ifp, struct in6_addr
> *from, char *lladdr,
> {
> struct rtentry *rt = NULL;
> struct llinfo_nd6 *ln = NULL;
> - int is_newentry;
> + int newentry;
> struct sockaddr_dl *sdl = NULL;
> int do_update;
> int olladdr;
> @@ -1113,14 +1113,14 @@ nd6_cache_lladdr(struct ifnet *ifp, struct in6_addr
> *from, char *lladdr,
> rt = nd6_lookup(from, 0, ifp, ifp->if_rdomain);
> if (rt == NULL) {
> rt = nd6_lookup(from, 1, ifp, ifp->if_rdomain);
> - is_newentry = 1;
> + newentry = 1;
> } else {
> /* do not overwrite local or static entry */
> if (ISSET(rt->rt_flags, RTF_STATIC|RTF_LOCAL)) {
> rtfree(rt);
> return;
> }
> - is_newentry = 0;
> + newentry = 0;
> }
>
> if (!rt)
> @@ -1175,7 +1175,7 @@ fail:
> bcopy(lladdr, LLADDR(sdl), ifp->if_addrlen);
> }
>
> - if (!is_newentry) {
> + if (!newentry) {
> if ((!olladdr && lladdr) || /* (3) */
> (olladdr && lladdr && llchange)) { /* (5) */
> do_update = 1;
> @@ -1259,7 +1259,7 @@ fail:
> /*
>* New entry must have is_router flag cleared.
>*/
> - if (is_newentry)/* (6-7) */
> + if (newentry) /* (6-7) */
> ln->ln_router = 0;
> break;
> case ND_REDIRECT:
> @@ -1270,7 +1270,7 @@ fail:
>*/
> if (code == ND_REDIRECT_ROUTER)
> ln->ln_router = 1;
> - else if (is_newentry) /* (6-7) */
> + else if (newentry) /* (6-7) */
> ln->ln_router = 0;
> break;
> case ND_ROUTER_SOLICIT:
> @@ -1283,8 +1283,8 @@ fail:
> /*
>* Mark an entry with lladdr as a router.
>*/
> - if ((!is_newentry && (olladdr || lladdr)) ||/* (2-5) */
> - (is_newentry && lladdr)) { /* (7) */
> + if ((!newentry && (olladdr || lladdr)) || /* (2-5) */
> + (newentry && lladdr)) { /* (7) */
> ln->ln_router = 1;
> }
> break;
>
--
I'm not entirely sure you are real.
Re: riscv64: trigger deferred timer interrupts from splx(9)
On Thu, Aug 04, 2022 at 09:39:13AM +0200, Jeremie Courreges-Anglas wrote:
> On Mon, Aug 01 2022, Scott Cheloha wrote:
> > On Mon, Aug 01, 2022 at 07:15:33PM +0200, Jeremie Courreges-Anglas wrote:
> >> On Sun, Jul 31 2022, Scott Cheloha wrote:
> >> > Hi,
> >> >
> >> > I am unsure how to properly mask RISC-V timer interrupts in hardware
> >> > at or above IPL_CLOCK. I think that would be cleaner than doing
> >> > another timer interrupt deferral thing.
> >> >
> >> > But, just to get the ball rolling, here a first attempt at the timer
> >> > interrupt deferral thing for riscv64. The motivation, as with every
> >> > other platform, is to eventually make it unnecessary for the machine
> >> > dependent code to know anything about the clock interrupt schedule.
> >> >
> >> > The thing I'm most unsure about is where to retrigger the timer in the
> >> > PLIC code. It seems right to do it from plic_setipl() because I want
> >> > to retrigger it before doing soft interrupt work, but I'm not sure.
>
> You're adding the timer reset to plic_setipl() but the latter is called
> after softintr processing in plic_splx().
>
> /* Pending software intr is handled here */
> if (ci->ci_ipending & riscv_smask[new])
> riscv_do_pending_intr(new);
>
> plic_setipl(new);
Yes, but plic_setipl() is also called from the softintr loop in
riscv_do_pending_intr() (riscv64/intr.c) right *before* we dispatch
any pending soft interrupts:
594 void
595 riscv_do_pending_intr(int pcpl)
596 {
597 struct cpu_info *ci = curcpu();
598 u_long sie;
599
600 sie = intr_disable();
601
602 #define DO_SOFTINT(si, ipl) \
603 if ((ci->ci_ipending & riscv_smask[pcpl]) & \
604 SI_TO_IRQBIT(si)) { \
605 ci->ci_ipending &= ~SI_TO_IRQBIT(si); \
* 606 riscv_intr_func.setipl(ipl);\
607 intr_restore(sie); \
608 softintr_dispatch(si); \
609 sie = intr_disable(); \
610 }
611
612 do {
613 DO_SOFTINT(SIR_TTY, IPL_SOFTTTY);
614 DO_SOFTINT(SIR_NET, IPL_SOFTNET);
615 DO_SOFTINT(SIR_CLOCK, IPL_SOFTCLOCK);
616 DO_SOFTINT(SIR_SOFT, IPL_SOFT);
617 } while (ci->ci_ipending & riscv_smask[pcpl]);
We might be fine doing it just once in plic_splx() before we do any
soft interrupt stuff. That's closer to what we're doing on other
platforms.
I just figured it'd be safer to do it in plic_setipl() because we're
already disabling interrupts there. It seems I guessed correctly
because the patch didn't hang your machine.
> >> > Unless I'm missing something, I don't think I need to do anything in
> >> > the default interrupt handler code, i.e. riscv64_dflt_setipl(), right?
> >>
> >> No idea about about the items above, but...
> >>
> >> > I have no riscv64 machine, so this is untested. Would appreciate
> >> > tests and feedback.
> >>
> >> There's an #include missing in plic.c,
> >
> > Whoops, corrected patch attached below.
> >
> >> with that added your diff builds and GENERIC.MP seems to behave
> >> (currently running make -j4 build), but I don't know exactly which
> >> problems I should look for.
> >
> > Thank you for trying it out.
> >
> > The patch changes how clock interrupt work is deferred on riscv64.
> >
> > If the code is wrong, the hardclock and statclock should eventually
> > die on every CPU. The death of the hardclock in particular would
> > manifest to the user as livelock. The scheduler would stop preempting
> > userspace and it would be impossible to use the machine interactively.
> >
> > There isn't really a direct way to exercise this code change.
> >
> > The best we can do is make the machine busy. If the machine is busy
> > we can expect more spl(9) calls and more deferred clock interrupt
> > work, which leaves more opportunities for the bug to surface.
> >
> > So, a parallel `make build` is fine. It's our gold standard for
> > making the machine really busy.
>
> The diff survived three make -j4 build/release in a row, the clock seems
> stable.
Awesome! Thank you for hammering on it.
kettenis, mlarkin, drahn:
Is this code fine or do you want to go about this in a different way?
Index: dev/plic.c
===
RCS file: /cvs/src/sys/arch/riscv64/dev/plic.c,v
retrieving revision 1.10
diff -u -p -r1.10 plic.c
--- dev/plic.c 6 Apr 2022 18:59:27 - 1.10
+++ dev/plic.c 4 Aug 2022 15:32:01 -
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "riscv64/dev/riscv_cpu_intc.h"
#include
@@ -556,6 +557,10 @@ plic_setipl(int new)
/* higher values are higher priority */
plic_set_threshold(ci->ci_cpuid, new);
+
+ /*
Re: parallel divert packet soreceive
On Thu, Aug 04, 2022 at 02:14:40PM +0200, Claudio Jeker wrote: > On Thu, Aug 04, 2022 at 01:42:48PM +0200, Alexander Bluhm wrote: > > On Thu, Aug 04, 2022 at 02:18:49AM +0300, Vitaliy Makkoveev wrote: > > > Also, I like to have exclusive layer locks like `tcp_lock???, > > > `udp_lock???, etc.. And take them with shared netlock held as the > > > first step of inet sockets unlocking. > > > > With PRU_LOCK and PRU_UNLOCK each layer can decide itself which > > kind is appropriate. For divert packet per PCB lock is trivial. > > For loops delivering to all sockets like UDP multicast or RAW > > sockets, a mutex per layer an easier start. > > Can we please not use PRU_LOCK/UNLOCK but instead implement them as > function pointers. I prefer simpler usrreq handlers. Especially since in > some handlers other locks are grabbed in various places. > Yes please. Also I like (*pr_usrreq)() be splitted to multiple handlers. > -- > :wq Claudio >
Re: parallel divert packet soreceive
On Thu, Aug 04, 2022 at 01:42:48PM +0200, Alexander Bluhm wrote:
> On Thu, Aug 04, 2022 at 02:18:49AM +0300, Vitaliy Makkoveev wrote:
> > Also, I like to have exclusive layer locks like `tcp_lock???,
> > `udp_lock???, etc.. And take them with shared netlock held as the
> > first step of inet sockets unlocking.
>
> With PRU_LOCK and PRU_UNLOCK each layer can decide itself which
> kind is appropriate. For divert packet per PCB lock is trivial.
> For loops delivering to all sockets like UDP multicast or RAW
> sockets, a mutex per layer an easier start.
>
> > > sblock()/sbunlock() relie on exclusive socket or layer lock,
> > > which protects `sb_flags??? modification. This diff breaks them,
> > > because nothing stops concurrent soreceive() threads to set
> > > SB_LOCK flag and and be sure that socket buffer is locked.
>
> Either sb_flags is protected by the exclusice netlock or by a shared
> netlock and PCB mutex. So only one thread can access them. Right?
>
Yes, you are right, I missed the PRU_LOCK/PRU_UNLOCK calls.
With your diff, sbwait() releases only netlock and leaves `inp_mtx'
locked. So when we enter sbwait() in the soreceive() path, the
concurrent sbappendaddr() thread will spin instead of append mbut to
`so_rcv'.
It seems sbwait() should release `inp_mtx' and keep shared netlock
locked.
@@ -907,7 +907,7 @@ restart:
sbunlock(so, >so_rcv);
error = sbwait(so, >so_rcv);
if (error) {
- sounlock(so);
+ sounlock_shared(so);
return (error);
}
@@ -1140,7 +1140,7 @@ dontblock:
error = sbwait(so, >so_rcv);
if (error) {
sbunlock(so, >so_rcv);
- sounlock(so);
+ sounlock_shared(so);
return (0);
}
@@ -221,22 +221,15 @@ divert_packet(struct mbuf *m, int dir, u
if_put(ifp);
}
+ mtx_enter(>inp_mtx);
so = inp->inp_socket;
- /*
-* XXXSMP sbappendaddr() is not MP safe and this function is called
-* from pf with shared netlock. To call only one sbappendaddr() from
-* divert_packet(), protect it with kernel lock. All other places
-* call sbappendaddr() with exclusive net lock. This blocks
-* divert_packet() as we have the shared lock.
-*/
- KERNEL_LOCK();
if (sbappendaddr(so, >so_rcv, sintosa(), m, NULL) == 0) {
- KERNEL_UNLOCK();
+ mtx_leave(>inp_mtx);
divstat_inc(divs_fullsock);
goto bad;
}
+ mtx_leave(>inp_mtx);
nd6: Rename is_newentry to newentry
This matches the extensive comments and schema for related variables.
No functional change.
OK?
diff --git a/sys/netinet6/nd6.c b/sys/netinet6/nd6.c
index c0b92f059c9..d793a0b07f4 100644
--- a/sys/netinet6/nd6.c
+++ b/sys/netinet6/nd6.c
@@ -1084,7 +1084,7 @@ nd6_cache_lladdr(struct ifnet *ifp, struct in6_addr
*from, char *lladdr,
{
struct rtentry *rt = NULL;
struct llinfo_nd6 *ln = NULL;
- int is_newentry;
+ int newentry;
struct sockaddr_dl *sdl = NULL;
int do_update;
int olladdr;
@@ -1113,14 +1113,14 @@ nd6_cache_lladdr(struct ifnet *ifp, struct in6_addr
*from, char *lladdr,
rt = nd6_lookup(from, 0, ifp, ifp->if_rdomain);
if (rt == NULL) {
rt = nd6_lookup(from, 1, ifp, ifp->if_rdomain);
- is_newentry = 1;
+ newentry = 1;
} else {
/* do not overwrite local or static entry */
if (ISSET(rt->rt_flags, RTF_STATIC|RTF_LOCAL)) {
rtfree(rt);
return;
}
- is_newentry = 0;
+ newentry = 0;
}
if (!rt)
@@ -1175,7 +1175,7 @@ fail:
bcopy(lladdr, LLADDR(sdl), ifp->if_addrlen);
}
- if (!is_newentry) {
+ if (!newentry) {
if ((!olladdr && lladdr) || /* (3) */
(olladdr && lladdr && llchange)) { /* (5) */
do_update = 1;
@@ -1259,7 +1259,7 @@ fail:
/*
* New entry must have is_router flag cleared.
*/
- if (is_newentry)/* (6-7) */
+ if (newentry) /* (6-7) */
ln->ln_router = 0;
break;
case ND_REDIRECT:
@@ -1270,7 +1270,7 @@ fail:
*/
if (code == ND_REDIRECT_ROUTER)
ln->ln_router = 1;
- else if (is_newentry) /* (6-7) */
+ else if (newentry) /* (6-7) */
ln->ln_router = 0;
break;
case ND_ROUTER_SOLICIT:
@@ -1283,8 +1283,8 @@ fail:
/*
* Mark an entry with lladdr as a router.
*/
- if ((!is_newentry && (olladdr || lladdr)) ||/* (2-5) */
- (is_newentry && lladdr)) { /* (7) */
+ if ((!newentry && (olladdr || lladdr)) || /* (2-5) */
+ (newentry && lladdr)) { /* (7) */
ln->ln_router = 1;
}
break;
netinet6: Call getuptime() just once per function
IPv6 pendant to bluhm's sys/netinet/if_ether.c r1.249:
Instead of calling getuptime() all the time in ARP code, do it only
once per function. This gives a more consistent time value.
OK claudio@ miod@ mvs@
Feedback? OK?
diff --git a/sys/netinet6/ip6_forward.c b/sys/netinet6/ip6_forward.c
index 099f77aee24..a8eefd6c121 100644
--- a/sys/netinet6/ip6_forward.c
+++ b/sys/netinet6/ip6_forward.c
@@ -102,9 +102,13 @@ ip6_forward(struct mbuf *m, struct rtentry *rt, int srcrt)
if ((m->m_flags & (M_BCAST|M_MCAST)) != 0 ||
IN6_IS_ADDR_MULTICAST(>ip6_dst) ||
IN6_IS_ADDR_UNSPECIFIED(>ip6_src)) {
+ time_t uptime;
+
ip6stat_inc(ip6s_cantforward);
- if (ip6_log_time + ip6_log_interval < getuptime()) {
- ip6_log_time = getuptime();
+ uptime = getuptime();
+
+ if (ip6_log_time + ip6_log_interval < uptime) {
+ ip6_log_time = uptime;
inet_ntop(AF_INET6, >ip6_src, src6, sizeof(src6));
inet_ntop(AF_INET6, >ip6_dst, dst6, sizeof(dst6));
log(LOG_DEBUG,
@@ -189,11 +193,14 @@ reroute:
*/
if (in6_addr2scopeid(m->m_pkthdr.ph_ifidx, >ip6_src) !=
in6_addr2scopeid(rt->rt_ifidx, >ip6_src)) {
+ time_t uptime;
+
ip6stat_inc(ip6s_cantforward);
ip6stat_inc(ip6s_badscope);
+ uptime = getuptime();
- if (ip6_log_time + ip6_log_interval < getuptime()) {
- ip6_log_time = getuptime();
+ if (ip6_log_time + ip6_log_interval < uptime) {
+ ip6_log_time = uptime;
inet_ntop(AF_INET6, >ip6_src, src6, sizeof(src6));
inet_ntop(AF_INET6, >ip6_dst, dst6, sizeof(dst6));
log(LOG_DEBUG,
diff --git a/sys/netinet6/ip6_mroute.c b/sys/netinet6/ip6_mroute.c
index 006f6ad964a..f281376064a 100644
--- a/sys/netinet6/ip6_mroute.c
+++ b/sys/netinet6/ip6_mroute.c
@@ -897,11 +897,15 @@ ip6_mforward(struct ip6_hdr *ip6, struct ifnet *ifp,
struct mbuf *m)
* (although such packets must normally set 1 to the hop limit field).
*/
if (IN6_IS_ADDR_UNSPECIFIED(>ip6_src)) {
+ time_t uptime;
+
ip6stat_inc(ip6s_cantforward);
- if (ip6_log_time + ip6_log_interval < getuptime()) {
+ uptime = getuptime();
+
+ if (ip6_log_time + ip6_log_interval < uptime) {
char src[INET6_ADDRSTRLEN], dst[INET6_ADDRSTRLEN];
- ip6_log_time = getuptime();
+ ip6_log_time = uptime;
inet_ntop(AF_INET6, >ip6_src, src, sizeof(src));
inet_ntop(AF_INET6, >ip6_dst, dst, sizeof(dst));
diff --git a/sys/netinet6/nd6.c b/sys/netinet6/nd6.c
index 2dbfa1ab866..c0b92f059c9 100644
--- a/sys/netinet6/nd6.c
+++ b/sys/netinet6/nd6.c
@@ -319,14 +319,16 @@ void
nd6_timer(void *unused)
{
struct llinfo_nd6 *ln, *nln;
- time_t expire = getuptime() + nd6_gctimer;
- int secs;
+ time_t uptime, expire;
+
+ uptime = getuptime();
+ expire = uptime + nd6_gctimer;
NET_LOCK();
TAILQ_FOREACH_SAFE(ln, _list, ln_list, nln) {
struct rtentry *rt = ln->ln_rt;
- if (rt->rt_expire && rt->rt_expire <= getuptime())
+ if (rt->rt_expire && rt->rt_expire <= uptime)
if (nd6_llinfo_timer(rt))
continue;
@@ -334,12 +336,9 @@ nd6_timer(void *unused)
expire = rt->rt_expire;
}
- secs = expire - getuptime();
- if (secs < 0)
- secs = 0;
if (!TAILQ_EMPTY(_list)) {
- nd6_timer_next = getuptime() + secs;
- timeout_add_sec(_timer_to, secs);
+ nd6_timer_next = expire;
+ timeout_add_sec(_timer_to, nd6_gctimer);
}
NET_UNLOCK();
@@ -444,7 +443,6 @@ void
nd6_expire_timer_update(struct in6_ifaddr *ia6)
{
time_t expire_time = INT64_MAX;
- int secs;
KERNEL_ASSERT_LOCKED();
@@ -469,6 +467,8 @@ nd6_expire_timer_update(struct in6_ifaddr *ia6)
if (!timeout_pending(_expire_timeout) ||
nd6_expire_next > expire_time) {
+ int secs;
+
secs = expire_time - getuptime();
if (secs < 0)
secs = 0;
Re: rpki-client unveil main process
sure you'll get EPERM and you can call it a day :)
but if you no longer need to call unveil again and pledge is in place just
remove its promise and if you try to call it your program will nicely abort
instead :D
On 08:59 Thu 04 Aug , Bryan Steele wrote:
> On Thu, Aug 04, 2022 at 12:47:36PM +0100, Ricardo Mestre wrote:
> > We are using pledge so if you don't remove the unveil permission it will be
> > allowed throughtout the entire process, so please just change unveil(NULL,
> > NULL)
> > to old previous pledge("stdio rpath wpath cpath fattr sendfd").
> >
> > Thank you :)
>
> Stylistically I agree.
>
> It's equivalent. unveil(2) will return EPERM once locked, even if the
> process hasn't dropped the unveil promise.
>
> > On 12:29 Thu 04 Aug , Claudio Jeker wrote:
> > > On Thu, Aug 04, 2022 at 12:24:03PM +0200, Theo Buehler wrote:
> > > > On Thu, Aug 04, 2022 at 12:11:45PM +0200, Claudio Jeker wrote:
> > > > > This diff adds unveil to the main process. This is done after all
> > > > > files
> > > > > from the command line have been read. Both for regular and -f mode.
> > > > > Once the args have been read the process can limit the access to the
> > > > > cachedir and the output dir. In -f mode only read access to the
> > > > > cachdir is
> > > > > required. In regular both cachedir and outputdir need rwc rights.
> > > >
> > > >
> > > > >
> > > > > --
> > > > > :wq Claudio
> > > > >
> > > > > Index: main.c
> > > > > ===
> > > > > RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> > > > > retrieving revision 1.208
> > > > > diff -u -p -r1.208 main.c
> > > > > --- main.c27 Jun 2022 10:18:27 - 1.208
> > > > > +++ main.c28 Jul 2022 16:57:16 -
> > > > > @@ -1006,8 +1006,7 @@ main(int argc, char *argv[])
> > > > > signal(SIGALRM, suicide);
> > > > > }
> > > > >
> > > > > - /* TODO unveil cachedir and outputdir, no other access allowed
> > > > > */
> > > > > - if (pledge("stdio rpath wpath cpath fattr sendfd", NULL) == -1)
> > > > > + if (pledge("stdio rpath wpath cpath fattr sendfd unveil", NULL)
> > > > > == -1)
> > > > > err(1, "pledge");
> > > > >
> > > > > msgbuf_init();
> > > > > @@ -1049,6 +1048,18 @@ main(int argc, char *argv[])
> > > > > while (*argv != NULL)
> > > > > queue_add_file(*argv++, RTYPE_FILE, 0);
> > > > > }
> > > >
> > > > This brace ends an if (filemode) block. I'm wondering if this would not
> > > > be cleaner:
> > > >
> > > > if (filemode) {
> > > > while (*argv != NULL)
> > > > queue_add_file(*argv++, RTYPE_FILE, 0);
> > > >
> > > > if (unveil(cachedir, "r") == -1)
> > > > err(1, "unveil cachedir");
> > > > } else {
> > > > if (unveil(outputdir, "rwc") == -1)
> > > > err(1, "unveil outputdir");
> > > > if (unveil(cachedir, "rwc") == -1)
> > > > err(1, "unveil cachedir");
> > > > }
> > > > if (unveil(NULL, NULL) == -1)
> > > > err(1, "unveil");
> > > >
> > > > Either way ok
> > >
> > > Sure, good suggestion. Will commit that version.
> > >
> > > --
> > > :wq Claudio
> > >
> >
> >
>
Re: rpki-client unveil main process
On Thu, Aug 04, 2022 at 12:47:36PM +0100, Ricardo Mestre wrote:
> We are using pledge so if you don't remove the unveil permission it will be
> allowed throughtout the entire process, so please just change unveil(NULL,
> NULL)
> to old previous pledge("stdio rpath wpath cpath fattr sendfd").
>
> Thank you :)
Stylistically I agree.
It's equivalent. unveil(2) will return EPERM once locked, even if the
process hasn't dropped the unveil promise.
> On 12:29 Thu 04 Aug , Claudio Jeker wrote:
> > On Thu, Aug 04, 2022 at 12:24:03PM +0200, Theo Buehler wrote:
> > > On Thu, Aug 04, 2022 at 12:11:45PM +0200, Claudio Jeker wrote:
> > > > This diff adds unveil to the main process. This is done after all files
> > > > from the command line have been read. Both for regular and -f mode.
> > > > Once the args have been read the process can limit the access to the
> > > > cachedir and the output dir. In -f mode only read access to the cachdir
> > > > is
> > > > required. In regular both cachedir and outputdir need rwc rights.
> > >
> > >
> > > >
> > > > --
> > > > :wq Claudio
> > > >
> > > > Index: main.c
> > > > ===
> > > > RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> > > > retrieving revision 1.208
> > > > diff -u -p -r1.208 main.c
> > > > --- main.c 27 Jun 2022 10:18:27 - 1.208
> > > > +++ main.c 28 Jul 2022 16:57:16 -
> > > > @@ -1006,8 +1006,7 @@ main(int argc, char *argv[])
> > > > signal(SIGALRM, suicide);
> > > > }
> > > >
> > > > - /* TODO unveil cachedir and outputdir, no other access allowed
> > > > */
> > > > - if (pledge("stdio rpath wpath cpath fattr sendfd", NULL) == -1)
> > > > + if (pledge("stdio rpath wpath cpath fattr sendfd unveil", NULL)
> > > > == -1)
> > > > err(1, "pledge");
> > > >
> > > > msgbuf_init();
> > > > @@ -1049,6 +1048,18 @@ main(int argc, char *argv[])
> > > > while (*argv != NULL)
> > > > queue_add_file(*argv++, RTYPE_FILE, 0);
> > > > }
> > >
> > > This brace ends an if (filemode) block. I'm wondering if this would not
> > > be cleaner:
> > >
> > > if (filemode) {
> > > while (*argv != NULL)
> > > queue_add_file(*argv++, RTYPE_FILE, 0);
> > >
> > > if (unveil(cachedir, "r") == -1)
> > > err(1, "unveil cachedir");
> > > } else {
> > > if (unveil(outputdir, "rwc") == -1)
> > > err(1, "unveil outputdir");
> > > if (unveil(cachedir, "rwc") == -1)
> > > err(1, "unveil cachedir");
> > > }
> > > if (unveil(NULL, NULL) == -1)
> > > err(1, "unveil");
> > >
> > > Either way ok
> >
> > Sure, good suggestion. Will commit that version.
> >
> > --
> > :wq Claudio
> >
>
>
Re: parallel divert packet soreceive
On Thu, Aug 04, 2022 at 01:42:48PM +0200, Alexander Bluhm wrote: > On Thu, Aug 04, 2022 at 02:18:49AM +0300, Vitaliy Makkoveev wrote: > > Also, I like to have exclusive layer locks like `tcp_lock???, > > `udp_lock???, etc.. And take them with shared netlock held as the > > first step of inet sockets unlocking. > > With PRU_LOCK and PRU_UNLOCK each layer can decide itself which > kind is appropriate. For divert packet per PCB lock is trivial. > For loops delivering to all sockets like UDP multicast or RAW > sockets, a mutex per layer an easier start. Can we please not use PRU_LOCK/UNLOCK but instead implement them as function pointers. I prefer simpler usrreq handlers. Especially since in some handlers other locks are grabbed in various places. -- :wq Claudio
Re: rpki-client unveil main process
We are using pledge so if you don't remove the unveil permission it will be
allowed throughtout the entire process, so please just change unveil(NULL, NULL)
to old previous pledge("stdio rpath wpath cpath fattr sendfd").
Thank you :)
On 12:29 Thu 04 Aug , Claudio Jeker wrote:
> On Thu, Aug 04, 2022 at 12:24:03PM +0200, Theo Buehler wrote:
> > On Thu, Aug 04, 2022 at 12:11:45PM +0200, Claudio Jeker wrote:
> > > This diff adds unveil to the main process. This is done after all files
> > > from the command line have been read. Both for regular and -f mode.
> > > Once the args have been read the process can limit the access to the
> > > cachedir and the output dir. In -f mode only read access to the cachdir is
> > > required. In regular both cachedir and outputdir need rwc rights.
> >
> >
> > >
> > > --
> > > :wq Claudio
> > >
> > > Index: main.c
> > > ===
> > > RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> > > retrieving revision 1.208
> > > diff -u -p -r1.208 main.c
> > > --- main.c27 Jun 2022 10:18:27 - 1.208
> > > +++ main.c28 Jul 2022 16:57:16 -
> > > @@ -1006,8 +1006,7 @@ main(int argc, char *argv[])
> > > signal(SIGALRM, suicide);
> > > }
> > >
> > > - /* TODO unveil cachedir and outputdir, no other access allowed */
> > > - if (pledge("stdio rpath wpath cpath fattr sendfd", NULL) == -1)
> > > + if (pledge("stdio rpath wpath cpath fattr sendfd unveil", NULL) == -1)
> > > err(1, "pledge");
> > >
> > > msgbuf_init();
> > > @@ -1049,6 +1048,18 @@ main(int argc, char *argv[])
> > > while (*argv != NULL)
> > > queue_add_file(*argv++, RTYPE_FILE, 0);
> > > }
> >
> > This brace ends an if (filemode) block. I'm wondering if this would not
> > be cleaner:
> >
> > if (filemode) {
> > while (*argv != NULL)
> > queue_add_file(*argv++, RTYPE_FILE, 0);
> >
> > if (unveil(cachedir, "r") == -1)
> > err(1, "unveil cachedir");
> > } else {
> > if (unveil(outputdir, "rwc") == -1)
> > err(1, "unveil outputdir");
> > if (unveil(cachedir, "rwc") == -1)
> > err(1, "unveil cachedir");
> > }
> > if (unveil(NULL, NULL) == -1)
> > err(1, "unveil");
> >
> > Either way ok
>
> Sure, good suggestion. Will commit that version.
>
> --
> :wq Claudio
>
Re: parallel divert packet soreceive
On Thu, Aug 04, 2022 at 02:18:49AM +0300, Vitaliy Makkoveev wrote: > Also, I like to have exclusive layer locks like `tcp_lock???, > `udp_lock???, etc.. And take them with shared netlock held as the > first step of inet sockets unlocking. With PRU_LOCK and PRU_UNLOCK each layer can decide itself which kind is appropriate. For divert packet per PCB lock is trivial. For loops delivering to all sockets like UDP multicast or RAW sockets, a mutex per layer an easier start. > > sblock()/sbunlock() relie on exclusive socket or layer lock, > > which protects `sb_flags??? modification. This diff breaks them, > > because nothing stops concurrent soreceive() threads to set > > SB_LOCK flag and and be sure that socket buffer is locked. Either sb_flags is protected by the exclusice netlock or by a shared netlock and PCB mutex. So only one thread can access them. Right? > > I started the work to allow socket buffer locking without socket > > lock held, but the work is far from the diff expose. Good to hear. I am advancing from the other direction. In protocol layer is much room for MP improvement. bluhm
rmt(8): add unveil and remove limitation on slashes/symlinks
Hello,
I'm using rmt in combination with the -d option for remote dumps from
multiple machines. It works fine, but the limitation on forward slashes
fills my backup directory with hundreds of dumpfiles in only a month.
I'd like to keep this a bit more organized in subdirectories based on
device or level.
So I added unveil and removed the limitation on forward slashes as well
as symbolic links when using the -d option. I believe (please correct me
if I'm wrong) these limitations were added to prevent breaking out of
the directory while keeping the code simple. With unveil this isn't
needed anymore.
While there I also made some additional changes to the -d option logic,
see separate diff at the bottom:
- Join if condition lines and fix indent for statements inside the block
- Always remove leading slashes from filenames, not just when stripping
a valid directory prefix
I've tested this with files but not an actual tape device.
Regards,
Andre
--- usr.sbin/rmt/rmt.8
+++ usr.sbin/rmt/rmt.8
@@ -57,7 +57,6 @@ The options are as follows:
.It Fl d Ar directory
Confine file access to
.Ar directory .
-Forward slashes in filenames are disallowed and symlinks are not followed.
.It Fl r
Read-only mode, suitable for use with
.Xr rrestore 8 .
--- usr.sbin/rmt/rmt.c
+++ usr.sbin/rmt/rmt.c
@@ -83,7 +83,7 @@ main(int argc, char *argv[])
char *devp;
size_t dirlen;
- if (pledge("stdio rpath wpath cpath inet", NULL) == -1)
+ if (pledge("stdio rpath wpath cpath inet unveil", NULL) == -1)
err(1, "pledge");
while ((ch = getopt(argc, argv, "d:rw")) != -1) {
@@ -118,10 +118,27 @@ main(int argc, char *argv[])
}
if (dir) {
+ if (rflag) {
+ if (unveil(dir, "r") == -1)
+ err(1, "unveil %s", dir);
+ } else {
+ if (unveil(dir, "rwc") == -1)
+ err(1, "unveil %s", dir);
+ }
if (chdir(dir) != 0)
err(1, "chdir");
dirlen = strlen(dir);
+ } else {
+ if (rflag) {
+ if (unveil("/", "r") == -1)
+ err(1, "unveil /");
+ } else {
+ if (unveil("/", "rwc") == -1)
+ err(1, "unveil /");
+ }
}
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
top:
errno = 0;
@@ -150,12 +167,6 @@ top:
while (*devp == '/')
devp++;
}
- /* Don't allow directory traversal. */
- if (strchr(devp, '/')) {
- errno = EACCES;
- goto ioerror;
- }
- f |= O_NOFOLLOW;
}
if (rflag) {
/*
--- usr.sbin/rmt/rmt.c
+++ usr.sbin/rmt/rmt.c
@@ -161,12 +161,10 @@ top:
if (dir) {
/* Strip away valid directory prefix. */
if (strncmp(dir, devp, dirlen) == 0 &&
- (devp[dirlen - 1] == '/' ||
-devp[dirlen] == '/')) {
-devp += dirlen;
-while (*devp == '/')
+ (devp[dirlen - 1] == '/' || devp[dirlen] == '/'))
+ devp += dirlen;
+ while (*devp == '/')
devp++;
- }
}
if (rflag) {
/*
Re: rpki-client unveil main process
On Thu, Aug 04, 2022 at 12:24:03PM +0200, Theo Buehler wrote:
> On Thu, Aug 04, 2022 at 12:11:45PM +0200, Claudio Jeker wrote:
> > This diff adds unveil to the main process. This is done after all files
> > from the command line have been read. Both for regular and -f mode.
> > Once the args have been read the process can limit the access to the
> > cachedir and the output dir. In -f mode only read access to the cachdir is
> > required. In regular both cachedir and outputdir need rwc rights.
>
>
> >
> > --
> > :wq Claudio
> >
> > Index: main.c
> > ===
> > RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> > retrieving revision 1.208
> > diff -u -p -r1.208 main.c
> > --- main.c 27 Jun 2022 10:18:27 - 1.208
> > +++ main.c 28 Jul 2022 16:57:16 -
> > @@ -1006,8 +1006,7 @@ main(int argc, char *argv[])
> > signal(SIGALRM, suicide);
> > }
> >
> > - /* TODO unveil cachedir and outputdir, no other access allowed */
> > - if (pledge("stdio rpath wpath cpath fattr sendfd", NULL) == -1)
> > + if (pledge("stdio rpath wpath cpath fattr sendfd unveil", NULL) == -1)
> > err(1, "pledge");
> >
> > msgbuf_init();
> > @@ -1049,6 +1048,18 @@ main(int argc, char *argv[])
> > while (*argv != NULL)
> > queue_add_file(*argv++, RTYPE_FILE, 0);
> > }
>
> This brace ends an if (filemode) block. I'm wondering if this would not
> be cleaner:
>
> if (filemode) {
> while (*argv != NULL)
> queue_add_file(*argv++, RTYPE_FILE, 0);
>
> if (unveil(cachedir, "r") == -1)
> err(1, "unveil cachedir");
> } else {
> if (unveil(outputdir, "rwc") == -1)
> err(1, "unveil outputdir");
> if (unveil(cachedir, "rwc") == -1)
> err(1, "unveil cachedir");
> }
> if (unveil(NULL, NULL) == -1)
> err(1, "unveil");
>
> Either way ok
Sure, good suggestion. Will commit that version.
--
:wq Claudio
Re: rpki-client unveil main process
On Thu, Aug 04, 2022 at 12:11:45PM +0200, Claudio Jeker wrote:
> This diff adds unveil to the main process. This is done after all files
> from the command line have been read. Both for regular and -f mode.
> Once the args have been read the process can limit the access to the
> cachedir and the output dir. In -f mode only read access to the cachdir is
> required. In regular both cachedir and outputdir need rwc rights.
>
> --
> :wq Claudio
>
> Index: main.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.208
> diff -u -p -r1.208 main.c
> --- main.c27 Jun 2022 10:18:27 - 1.208
> +++ main.c28 Jul 2022 16:57:16 -
> @@ -1006,8 +1006,7 @@ main(int argc, char *argv[])
> signal(SIGALRM, suicide);
> }
>
> - /* TODO unveil cachedir and outputdir, no other access allowed */
> - if (pledge("stdio rpath wpath cpath fattr sendfd", NULL) == -1)
> + if (pledge("stdio rpath wpath cpath fattr sendfd unveil", NULL) == -1)
> err(1, "pledge");
>
> msgbuf_init();
> @@ -1049,6 +1048,18 @@ main(int argc, char *argv[])
> while (*argv != NULL)
> queue_add_file(*argv++, RTYPE_FILE, 0);
> }
This brace ends an if (filemode) block. I'm wondering if this would not
be cleaner:
if (filemode) {
while (*argv != NULL)
queue_add_file(*argv++, RTYPE_FILE, 0);
if (unveil(cachedir, "r") == -1)
err(1, "unveil cachedir");
} else {
if (unveil(outputdir, "rwc") == -1)
err(1, "unveil outputdir");
if (unveil(cachedir, "rwc") == -1)
err(1, "unveil cachedir");
}
if (unveil(NULL, NULL) == -1)
err(1, "unveil");
Either way ok
> +
> + /* from here on only cachedir and outputdir are accessed */
> + if (!filemode) {
> + if (unveil(outputdir, "rwc") == -1)
> + err(1, "unveil outputdir");
> + if (unveil(cachedir, "rwc") == -1)
> + err(1, "unveil cachedir");
> + } else
> + if (unveil(cachedir, "r") == -1)
> + err(1, "unveil cachedir");
> + if (unveil(NULL, NULL) == -1)
> + err(1, "unveil");
>
> /* change working directory to the cache directory */
> if (fchdir(cachefd) == -1)
>
rpki-client unveil main process
This diff adds unveil to the main process. This is done after all files
from the command line have been read. Both for regular and -f mode.
Once the args have been read the process can limit the access to the
cachedir and the output dir. In -f mode only read access to the cachdir is
required. In regular both cachedir and outputdir need rwc rights.
--
:wq Claudio
Index: main.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
retrieving revision 1.208
diff -u -p -r1.208 main.c
--- main.c 27 Jun 2022 10:18:27 - 1.208
+++ main.c 28 Jul 2022 16:57:16 -
@@ -1006,8 +1006,7 @@ main(int argc, char *argv[])
signal(SIGALRM, suicide);
}
- /* TODO unveil cachedir and outputdir, no other access allowed */
- if (pledge("stdio rpath wpath cpath fattr sendfd", NULL) == -1)
+ if (pledge("stdio rpath wpath cpath fattr sendfd unveil", NULL) == -1)
err(1, "pledge");
msgbuf_init();
@@ -1049,6 +1048,18 @@ main(int argc, char *argv[])
while (*argv != NULL)
queue_add_file(*argv++, RTYPE_FILE, 0);
}
+
+ /* from here on only cachedir and outputdir are accessed */
+ if (!filemode) {
+ if (unveil(outputdir, "rwc") == -1)
+ err(1, "unveil outputdir");
+ if (unveil(cachedir, "rwc") == -1)
+ err(1, "unveil cachedir");
+ } else
+ if (unveil(cachedir, "r") == -1)
+ err(1, "unveil cachedir");
+ if (unveil(NULL, NULL) == -1)
+ err(1, "unveil");
/* change working directory to the cache directory */
if (fchdir(cachefd) == -1)
Re: riscv64: trigger deferred timer interrupts from splx(9)
On Mon, Aug 01 2022, Scott Cheloha wrote:
> On Mon, Aug 01, 2022 at 07:15:33PM +0200, Jeremie Courreges-Anglas wrote:
>> On Sun, Jul 31 2022, Scott Cheloha wrote:
>> > Hi,
>> >
>> > I am unsure how to properly mask RISC-V timer interrupts in hardware
>> > at or above IPL_CLOCK. I think that would be cleaner than doing
>> > another timer interrupt deferral thing.
>> >
>> > But, just to get the ball rolling, here a first attempt at the timer
>> > interrupt deferral thing for riscv64. The motivation, as with every
>> > other platform, is to eventually make it unnecessary for the machine
>> > dependent code to know anything about the clock interrupt schedule.
>> >
>> > The thing I'm most unsure about is where to retrigger the timer in the
>> > PLIC code. It seems right to do it from plic_setipl() because I want
>> > to retrigger it before doing soft interrupt work, but I'm not sure.
You're adding the timer reset to plic_setipl() but the latter is called
after softintr processing in plic_splx().
/* Pending software intr is handled here */
if (ci->ci_ipending & riscv_smask[new])
riscv_do_pending_intr(new);
plic_setipl(new);
>> > Unless I'm missing something, I don't think I need to do anything in
>> > the default interrupt handler code, i.e. riscv64_dflt_setipl(), right?
>>
>> No idea about about the items above, but...
>>
>> > I have no riscv64 machine, so this is untested. Would appreciate
>> > tests and feedback.
>>
>> There's an #include missing in plic.c,
>
> Whoops, corrected patch attached below.
>
>> with that added your diff builds and GENERIC.MP seems to behave
>> (currently running make -j4 build), but I don't know exactly which
>> problems I should look for.
>
> Thank you for trying it out.
>
> The patch changes how clock interrupt work is deferred on riscv64.
>
> If the code is wrong, the hardclock and statclock should eventually
> die on every CPU. The death of the hardclock in particular would
> manifest to the user as livelock. The scheduler would stop preempting
> userspace and it would be impossible to use the machine interactively.
>
> There isn't really a direct way to exercise this code change.
>
> The best we can do is make the machine busy. If the machine is busy
> we can expect more spl(9) calls and more deferred clock interrupt
> work, which leaves more opportunities for the bug to surface.
>
> So, a parallel `make build` is fine. It's our gold standard for
> making the machine really busy.
The diff survived three make -j4 build/release in a row, the clock seems
stable.
> Index: include/cpu.h
> ===
> RCS file: /cvs/src/sys/arch/riscv64/include/cpu.h,v
> retrieving revision 1.12
> diff -u -p -r1.12 cpu.h
> --- include/cpu.h 10 Jun 2022 21:34:15 - 1.12
> +++ include/cpu.h 1 Aug 2022 17:36:41 -
> @@ -92,7 +92,7 @@ struct cpu_info {
> uint64_tci_lasttb;
> uint64_tci_nexttimerevent;
> uint64_tci_nextstatevent;
> - int ci_statspending;
> + volatile intci_timer_deferred;
>
> uint32_tci_cpl;
> uint32_tci_ipending;
> Index: riscv64/clock.c
> ===
> RCS file: /cvs/src/sys/arch/riscv64/riscv64/clock.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 clock.c
> --- riscv64/clock.c 24 Jul 2021 22:41:09 - 1.3
> +++ riscv64/clock.c 1 Aug 2022 17:36:41 -
> @@ -21,6 +21,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> @@ -106,6 +107,17 @@ clock_intr(void *frame)
> int s;
>
> /*
> + * If the clock interrupt is masked, defer all clock interrupt
> + * work until the clock interrupt is unmasked from splx(9).
> + */
> + if (ci->ci_cpl >= IPL_CLOCK) {
> + ci->ci_timer_deferred = 1;
> + sbi_set_timer(UINT64_MAX);
> + return 0;
> + }
> + ci->ci_timer_deferred = 0;
> +
> + /*
>* Based on the actual time delay since the last clock interrupt,
>* we arrange for earlier interrupt next time.
>*/
> @@ -132,30 +144,23 @@ clock_intr(void *frame)
>
> sbi_set_timer(nextevent);
>
> - if (ci->ci_cpl >= IPL_CLOCK) {
> - ci->ci_statspending += nstats;
> - } else {
> - nstats += ci->ci_statspending;
> - ci->ci_statspending = 0;
> -
> - s = splclock();
> - intr_enable();
> -
> - /*
> - * Do standard timer interrupt stuff.
> - */
> - while (ci->ci_lasttb < prevtb) {
> - ci->ci_lasttb += tick_increment;
> - clock_count.ec_count++;
> - hardclock((struct clockframe *)frame);
> - }
> + s = splclock();
> + intr_enable();
