Re: reorder_kernel: suport ro /usr like library_aslr does
Klemens Nanni wrote: > Reading /etc/rc I was under the impression that read-only /usr is indeed > a scenario we support, since reorder_libs() already does what I propose, > only in a more complicated way: > > revision 1.481 > date: 2016/05/26 14:59:48; author: rpe; state: Exp; lines: +32 -7; > - rename rebuildlibs() to reorder_libs() > - move the info message inside the function > - skip reordering if /usr/lib is on a nfs mounted filesystem > - temporarily remount rw if /usr/lib is on a ro ffs file-system > > OK deraadt > > Did OpenBSD's stance on read-only /usr change between then and now? The OpenBSD stance never changed. If the configuration isn't created by the install script, you are on your own. Maybe one developer put some words in about ffs, but I am sure the idea was to protect NFS diskless which cannot be mounted rw. I do not see the point behind diff after diff trying to add scripting for bizzare configurations that basically none of our users configure. Those admins are fully aware that when they make weird configuration changes, they are on their own, and they have to maintain their own diffs. I do not agree with this continual pushiness you have to find weird special cases and propose scripting changes to deal with it. OpenBSD's prime principle is SIMPLICITY, even when it means there are limited operations for operation, and you are going way out of bounds. Take a step back.
Re: reorder_kernel: suport ro /usr like library_aslr does
On Mon, Nov 14, 2022 at 03:15:53PM -0700, Theo de Raadt wrote: > Readonly /usr is not a supported or recomended configuration. > > This is adding a lot of scripting that we don't everyone to run. > > I disagree strongly with this direction of OpenBSD having undocumented > (undocumentable?) little behaviours that allow root to configure their > machine in novel non-default ways and it will still work because there > piles of of trashy shell scripts which cope with the weird situations, > which under 1% of users will use. > > I disagree with this flexiblity being a strength, I think it is very > fragile when we encourage users to do bizzare things to their machines > which they (also) will not include in future bug reports. Reading /etc/rc I was under the impression that read-only /usr is indeed a scenario we support, since reorder_libs() already does what I propose, only in a more complicated way: revision 1.481 date: 2016/05/26 14:59:48; author: rpe; state: Exp; lines: +32 -7; - rename rebuildlibs() to reorder_libs() - move the info message inside the function - skip reordering if /usr/lib is on a nfs mounted filesystem - temporarily remount rw if /usr/lib is on a ro ffs file-system OK deraadt Did OpenBSD's stance on read-only /usr change between then and now? Or does the earlier running /etc/rc take care of some read-only /usr scenario (I am not aware of) which is not relevant for reorder_kernel?
Re: reorder_kernel: suport ro /usr like library_aslr does
Readonly /usr is not a supported or recomended configuration. This is adding a lot of scripting that we don't everyone to run. I disagree strongly with this direction of OpenBSD having undocumented (undocumentable?) little behaviours that allow root to configure their machine in novel non-default ways and it will still work because there piles of of trashy shell scripts which cope with the weird situations, which under 1% of users will use. I disagree with this flexiblity being a strength, I think it is very fragile when we encourage users to do bizzare things to their machines which they (also) will not include in future bug reports. Klemens Nanni wrote: > On Tue, Nov 08, 2022 at 11:10:19AM +, Klemens Nanni wrote: > > More read-only filesystems mean less fsck during boot after crashes. > > Especially on crappy machines like the Pinebook Poop, I keep /usr > > read-only and run with this diff so I still get a relinked kernel. > > > > rc's reorder_libs() already does the same remount dance, but for > > multiple directories/filesystems. > > > > My diff works as expected with read-write and read-only /usr as well as > > interrupting /usr/libexec/reorder_kernel runs with ^C, i.e. a failed run > > will correctly mount /usr ro again if it was ro before the run. > > > > Feedback? OK? > > Ping. > > Index: libexec/reorder_kernel/reorder_kernel.sh > === > RCS file: /cvs/src/libexec/reorder_kernel/reorder_kernel.sh,v > retrieving revision 1.13 > diff -u -p -r1.13 reorder_kernel.sh > --- libexec/reorder_kernel/reorder_kernel.sh 7 Nov 2022 15:55:56 - > 1.13 > +++ libexec/reorder_kernel/reorder_kernel.sh 14 Nov 2022 19:33:25 - > @@ -27,13 +27,32 @@ LOGFILE=$KERNEL_DIR/$KERNEL/relink.log > PROGNAME=${0##*/} > SHA256=/var/db/kernel.SHA256 > > -# Silently skip if on a NFS mounted filesystem. > -df -t nonfs $KERNEL_DIR >/dev/null 2>&1 > +# Silently skip if on NFS, otherwise remount the filesystem read-write. > +DEV=$(df -t ffs $KERNEL_DIR 2>/dev/null | awk 'NR == 2 { print $1 }') > +MP=$(mount | grep ^$DEV) > +RO=false > +if [[ $MP == *read-only* ]]; then > + MP=${MP%% *} > + mount -u -w $MP > + RO=true > +fi > + > +restore_mount() { > + if $RO; then > + # Close the logfile to unbusy $MP by switching back to console. > + exec 1>/dev/console > + exec 2>&1 > + mount -u -r $MP > + fi > +} > > # Install trap handlers to inform about success or failure via syslog. > ERRMSG='failed' > -trap 'trap - EXIT; logger -st $PROGNAME "$ERRMSG" >/dev/console 2>&1' ERR > -trap 'logger -t $PROGNAME "kernel relinking done"' EXIT > +trap 'trap - EXIT > + logger -st $PROGNAME "$ERRMSG" 2>/dev/console > + restore_mount' ERR > +trap 'logger -t $PROGNAME "kernel relinking done" > + restore_mount' EXIT > > # Create kernel compile dir and redirect stdout/stderr to a logfile. > mkdir -m 700 -p $KERNEL_DIR/$KERNEL >
Re: xenodm: save ~/.xesssion to ~/.xsession.old
Klemens Nanni wrote: > On Mon, Nov 14, 2022 at 10:33:44AM -0700, Theo de Raadt wrote: > > Stuart Henderson wrote: > > > > > > Index: app/xenodm//config/Xsession.in > > > > === > > > > RCS file: /cvs/xenocara/app/xenodm/config/Xsession.in,v > > > > retrieving revision 1.2 > > > > diff -u -p -r1.2 Xsession.in > > > > --- app/xenodm//config/Xsession.in 1 Jul 2022 20:42:06 - > > > > 1.2 > > > > +++ app/xenodm//config/Xsession.in 14 Nov 2022 16:47:03 - > > > > @@ -7,6 +7,7 @@ exec_prefix="@exec_prefix@" > > > > # redirect errors to a file in user's home directory if we can > > > > > > > > errfile="$HOME/.xsession-errors" > > > > +cp -f "$errfile" "$errfile.old" 2> /dev/null > > > > > > Those files can get pretty big. mv is probably a better idea than cp. > > > > And consider the target being a symbolic link, both file and directory > > `mv -f 2>/dev/null' should be good enough for all of this, no? > It's a silent one-shot: if it works, great. If not, no .old backup and > the file gets truncated as usual. ln -s /tmp ~/.xsession-errors.old Your plan is half baked.
Re: Push kernel lock into pru_control()
> On 10 Nov 2022, at 13:54, Klemens Nanni wrote: > > Purely mechanical, then in6_control() and in_control() can be pushed > further individually. > > Feedback? OK? SS_PRIV is immutable, no reason to check it with kernel lock held. > --- > sys/kern/sys_socket.c | 2 -- > sys/netinet/in.c | 4 > sys/netinet6/in6.c| 4 > 3 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/sys/kern/sys_socket.c b/sys/kern/sys_socket.c > index b07119b71cd..e42dd948006 100644 > --- a/sys/kern/sys_socket.c > +++ b/sys/kern/sys_socket.c > @@ -134,9 +134,7 @@ soo_ioctl(struct file *fp, u_long cmd, caddr_t data, > struct proc *p) > } > if (IOCGROUP(cmd) == 'r') > return (EOPNOTSUPP); > - KERNEL_LOCK(); > error = pru_control(so, cmd, data, NULL); > - KERNEL_UNLOCK(); > break; > } > > diff --git a/sys/netinet/in.c b/sys/netinet/in.c > index cd8289d2e89..990aaf84c8a 100644 > --- a/sys/netinet/in.c > +++ b/sys/netinet/in.c > @@ -202,6 +202,8 @@ in_control(struct socket *so, u_long cmd, caddr_t data, > struct ifnet *ifp) > int privileged; > int error; > > + KERNEL_LOCK(); > + > privileged = 0; > if ((so->so_state & SS_PRIV) != 0) > privileged++; > @@ -218,6 +220,8 @@ in_control(struct socket *so, u_long cmd, caddr_t data, > struct ifnet *ifp) > break; > } > > + KERNEL_UNLOCK(); > + > return error; > } > > diff --git a/sys/netinet6/in6.c b/sys/netinet6/in6.c > index 900456fff1a..a51ca2fa5a4 100644 > --- a/sys/netinet6/in6.c > +++ b/sys/netinet6/in6.c > @@ -199,6 +199,8 @@ in6_control(struct socket *so, u_long cmd, caddr_t data, > struct ifnet *ifp) > int privileged; > int error; > > + KERNEL_LOCK(); > + > privileged = 0; > if ((so->so_state & SS_PRIV) != 0) > privileged++; > @@ -215,6 +217,8 @@ in6_control(struct socket *so, u_long cmd, caddr_t data, > struct ifnet *ifp) > break; > } > > + KERNEL_UNLOCK(); > + > return error; > } > > -- > 2.38.1 >
Re: export {b,r}ootduid as sysctl, installer/sysupgrade improvements
Mark Kettenis wrote: > > From: "Theo de Raadt" > > Date: Mon, 14 Nov 2022 10:02:40 -0700 > > > > An OpenBSD machine only has one OpenBSD install. > > I have to disagree here. Not everyone has a pile of test machines > lying around. Are you going to work on the install script? And then, after that, on the feature requests for sysupgrade? > Well, that is the real question: will this increase complexity? It will increase complexity. > We > currently have code that makes what I'd describe as an "educated > guess" at what is the OpenBSD root disk of a machine. If we can > replace that with something that finds the disk based on its DUID, > that would make things more robust and might even decrease complexity > in the installer. I don't believe either of you. I believe that is_rootdisk() will be replaced with 30 lines of kernel code, 10 lines in distrib/special/sysctl/sysctl.c, and 5 lines of shell script .. AND AFTER THAT IS DONE, a bunch of extensions will be proposed for various other system install / update features, which will then increase complexity. And I claim that will happen because this is not proposing 1 new sysctl, it is proposing 2 of them, without explaining why there needs to be 2. I think the explanation for that is "undisclosed goals".
Re: export {b,r}ootduid as sysctl, installer/sysupgrade improvements
> From: "Theo de Raadt" > Date: Mon, 14 Nov 2022 10:02:40 -0700 > > An OpenBSD machine only has one OpenBSD install. I have to disagree here. Not everyone has a pile of test machines lying around. > As soon as we leave that model, and allow other setup models, perhaps you > think there will be two or three potential configurations that people setup. > > I don't think so, I think it will keep being extended by people who do > more and more scewed up bizzare configurations, all of which (obviously, to > you) will need to be supported, and everything gets more complicated. > > I think we should say "STOP". Now. And not start down that roadmap. > > You built something for a testlab. Your conclusion is that it should > work for everyone. I simply cannot come to the same conclusion, > because it requires complexity, but instead I think we should embrace > simplicity even if it limits choice. Well, that is the real question: will this increase complexity? We currently have code that makes what I'd describe as an "educated guess" at what is the OpenBSD root disk of a machine. If we can replace that with something that finds the disk based on its DUID, that would make things more robust and might even decrease complexity in the installer. That said, I don't immediately see how this would work and how the sysctl's would help. So it would be good if Klemens showed us the complete picture. > Klemens Nanni wrote: > > > On Mon, Nov 14, 2022 at 07:49:11AM -0700, Theo de Raadt wrote: > > > Klemens Nanni wrote: > > > > > > > This is because the installer always considers the first root disk it > > > > finds as the one to upgrade, which is certainly not what I intend or > > > > expect when booting/upgrading the softraid installation on sd1-3. > > > > > > What does > > > > > > first root disk > > > > > > Mean? > > > > One machine with two phsyical disks, say one NVMe and regular SSD. > > Both disks contain a standalone OpenBSD installation. > > > > I consider each of them a root disk. > > > > > > > > There is only one root disk. The root disk is the one that actually > > > contains > > > the / that is mounted. > > > > > > It is this one: > > > > > > root on sd0a (fb786f6b01042b30.a) swap on sd0b dump on sd0b > > > > > > You cannot change this. If you use the bootloader to tell the kernel do > > > do something else, then I argue that sysupgrade and the installer should > > > punish you unless you *manually tell it that every time* > > > > I don't set anything in /etc/boot.conf or the boot> prompt. > > > > I select the disk to boot from in the UEFI boot manager. > > > > > > > > The install script knows what the root filesystem is using very simple > > > heuristics, but by creating two new sysctl, I am afraid you will enrich > > > this ability to support bizzare configurations that did not work, and > > > I argue *should never work*. > > > > > > > It is probably not that common to have multiple installations/root disks > > > > in one machine, but it isn't "weird" to me, either. > > > > > > What? It is not weird > > > > > > I think this is unsupported bullshit. > > > > > > Why do we need the install script to support this configuration you > > > created? Why do we need to encourage other people to have such > > > configurations? When they create such a configuration, and find the > > > tooling can support it, won't they go and do even stranger things, then > > > find the tooling doesn't support that even-stranger setup, and then > > > you'll come back adding support for increasingly strange setups, and > > > eventually we are going to end up with a large userbase *not using* a > > > single root filesystem? > > > > > > > Overwriting the wrong system during an upgrade because the installer > > > > makes too big of an assumption about the first disk is weird to me. > > > > > > > > > > > > I can post console logs later showing how the installer picks the wrong > > > > disk, if you want. > > > > > > There is only one possible root filesystem. > > > > > > If you created multiple root filesystems, you have created a mess and > > > why is it wrong for me to argue you need to experience pain for the > > > decisions that led you there? > > > > > > I do not think you are being honest about the reason for these extensions. > > > > > > > I'm pretty honest, but apparently not precise enough. > > > > Now that all the softraid/installboot diffs landed and the dust has > > settled, let me iterate over and test my setup again to make sure I'm > > not tripping over my own mistakes. > > > > Then I can come back with a clear update or reproducer. > > >
Re: xenodm: save ~/.xesssion to ~/.xsession.old
On Mon, Nov 14, 2022 at 10:33:44AM -0700, Theo de Raadt wrote: > Stuart Henderson wrote: > > > > Index: app/xenodm//config/Xsession.in > > > === > > > RCS file: /cvs/xenocara/app/xenodm/config/Xsession.in,v > > > retrieving revision 1.2 > > > diff -u -p -r1.2 Xsession.in > > > --- app/xenodm//config/Xsession.in1 Jul 2022 20:42:06 - > > > 1.2 > > > +++ app/xenodm//config/Xsession.in14 Nov 2022 16:47:03 - > > > @@ -7,6 +7,7 @@ exec_prefix="@exec_prefix@" > > > # redirect errors to a file in user's home directory if we can > > > > > > errfile="$HOME/.xsession-errors" > > > +cp -f "$errfile" "$errfile.old" 2> /dev/null > > > > Those files can get pretty big. mv is probably a better idea than cp. > > And consider the target being a symbolic link, both file and directory `mv -f 2>/dev/null' should be good enough for all of this, no? It's a silent one-shot: if it works, great. If not, no .old backup and the file gets truncated as usual. Index: app/xenodm/config/Xsession.in === RCS file: /cvs/xenocara/app/xenodm/config/Xsession.in,v retrieving revision 1.2 diff -u -p -r1.2 Xsession.in --- app/xenodm/config/Xsession.in 1 Jul 2022 20:42:06 - 1.2 +++ app/xenodm/config/Xsession.in 14 Nov 2022 19:39:57 - @@ -7,6 +7,7 @@ exec_prefix="@exec_prefix@" # redirect errors to a file in user's home directory if we can errfile="$HOME/.xsession-errors" +mv -f "$errfile" "$errfile.old" if ( umask 077 && cp /dev/null "$errfile" 2> /dev/null ) then exec > "$errfile" 2>&1
Re: reorder_kernel: suport ro /usr like library_aslr does
On Tue, Nov 08, 2022 at 11:10:19AM +, Klemens Nanni wrote: > More read-only filesystems mean less fsck during boot after crashes. > Especially on crappy machines like the Pinebook Poop, I keep /usr > read-only and run with this diff so I still get a relinked kernel. > > rc's reorder_libs() already does the same remount dance, but for > multiple directories/filesystems. > > My diff works as expected with read-write and read-only /usr as well as > interrupting /usr/libexec/reorder_kernel runs with ^C, i.e. a failed run > will correctly mount /usr ro again if it was ro before the run. > > Feedback? OK? Ping. Index: libexec/reorder_kernel/reorder_kernel.sh === RCS file: /cvs/src/libexec/reorder_kernel/reorder_kernel.sh,v retrieving revision 1.13 diff -u -p -r1.13 reorder_kernel.sh --- libexec/reorder_kernel/reorder_kernel.sh7 Nov 2022 15:55:56 - 1.13 +++ libexec/reorder_kernel/reorder_kernel.sh14 Nov 2022 19:33:25 - @@ -27,13 +27,32 @@ LOGFILE=$KERNEL_DIR/$KERNEL/relink.log PROGNAME=${0##*/} SHA256=/var/db/kernel.SHA256 -# Silently skip if on a NFS mounted filesystem. -df -t nonfs $KERNEL_DIR >/dev/null 2>&1 +# Silently skip if on NFS, otherwise remount the filesystem read-write. +DEV=$(df -t ffs $KERNEL_DIR 2>/dev/null | awk 'NR == 2 { print $1 }') +MP=$(mount | grep ^$DEV) +RO=false +if [[ $MP == *read-only* ]]; then + MP=${MP%% *} + mount -u -w $MP + RO=true +fi + +restore_mount() { + if $RO; then + # Close the logfile to unbusy $MP by switching back to console. + exec 1>/dev/console + exec 2>&1 + mount -u -r $MP + fi +} # Install trap handlers to inform about success or failure via syslog. ERRMSG='failed' -trap 'trap - EXIT; logger -st $PROGNAME "$ERRMSG" >/dev/console 2>&1' ERR -trap 'logger -t $PROGNAME "kernel relinking done"' EXIT +trap 'trap - EXIT + logger -st $PROGNAME "$ERRMSG" 2>/dev/console + restore_mount' ERR +trap 'logger -t $PROGNAME "kernel relinking done" + restore_mount' EXIT # Create kernel compile dir and redirect stdout/stderr to a logfile. mkdir -m 700 -p $KERNEL_DIR/$KERNEL
Re: xenodm: save ~/.xesssion to ~/.xsession.old
Stuart Henderson wrote: > > Index: app/xenodm//config/Xsession.in > > === > > RCS file: /cvs/xenocara/app/xenodm/config/Xsession.in,v > > retrieving revision 1.2 > > diff -u -p -r1.2 Xsession.in > > --- app/xenodm//config/Xsession.in 1 Jul 2022 20:42:06 - 1.2 > > +++ app/xenodm//config/Xsession.in 14 Nov 2022 16:47:03 - > > @@ -7,6 +7,7 @@ exec_prefix="@exec_prefix@" > > # redirect errors to a file in user's home directory if we can > > > > errfile="$HOME/.xsession-errors" > > +cp -f "$errfile" "$errfile.old" 2> /dev/null > > Those files can get pretty big. mv is probably a better idea than cp. And consider the target being a symbolic link, both file and directory
Re: xenodm: save ~/.xesssion to ~/.xsession.old
On 2022/11/14 16:50, Klemens Nanni wrote: > X segfaulted when I opened a window, Xorg.log.old only showed the > address without anything specific, no core dump was created and > xenodm automatically restarted. > > After I logged in I checked ~/.xsession for possible indications, but > that file gets truncated on login. > > I've restarted xenodm again, so the old Xorg.log.old containing the > generic "X segfaulted at addr 0x86..." is also gone. > > Woul it make sense to save an old copy in analogy to Xorg.log.old > so crashes like this have a higher chance of being hunted down? > > I haven't extensively tested this patch, but relogging into X now leaves > ~/.xesssion-errors.old with logs from the previous session behind. > > Index: app/xenodm//config/Xsession.in > === > RCS file: /cvs/xenocara/app/xenodm/config/Xsession.in,v > retrieving revision 1.2 > diff -u -p -r1.2 Xsession.in > --- app/xenodm//config/Xsession.in1 Jul 2022 20:42:06 - 1.2 > +++ app/xenodm//config/Xsession.in14 Nov 2022 16:47:03 - > @@ -7,6 +7,7 @@ exec_prefix="@exec_prefix@" > # redirect errors to a file in user's home directory if we can > > errfile="$HOME/.xsession-errors" > +cp -f "$errfile" "$errfile.old" 2> /dev/null Those files can get pretty big. mv is probably a better idea than cp. > if ( umask 077 && cp /dev/null "$errfile" 2> /dev/null ) > then > exec > "$errfile" 2>&1 >
Re: xenodm: save ~/.xesssion to ~/.xsession.old
Klemens Nanni writes: > X segfaulted when I opened a window, Xorg.log.old only showed the > address without anything specific, no core dump was created and > xenodm automatically restarted. > > After I logged in I checked ~/.xsession for possible indications, but > that file gets truncated on login. > > I've restarted xenodm again, so the old Xorg.log.old containing the > generic "X segfaulted at addr 0x86..." is also gone. > > Woul it make sense to save an old copy in analogy to Xorg.log.old > so crashes like this have a higher chance of being hunted down? > > I haven't extensively tested this patch, but relogging into X now leaves > ~/.xesssion-errors.old with logs from the previous session behind. > Tested, works as expected! This would have helped me semi-recently! OK abieber@ fwiw :D > Index: app/xenodm//config/Xsession.in > === > RCS file: /cvs/xenocara/app/xenodm/config/Xsession.in,v > retrieving revision 1.2 > diff -u -p -r1.2 Xsession.in > --- app/xenodm//config/Xsession.in1 Jul 2022 20:42:06 - 1.2 > +++ app/xenodm//config/Xsession.in14 Nov 2022 16:47:03 - > @@ -7,6 +7,7 @@ exec_prefix="@exec_prefix@" > # redirect errors to a file in user's home directory if we can > > errfile="$HOME/.xsession-errors" > +cp -f "$errfile" "$errfile.old" 2> /dev/null > if ( umask 077 && cp /dev/null "$errfile" 2> /dev/null ) > then > exec > "$errfile" 2>&1
Re: mips64, loongson, octeon: switch to clockintr(9)
On Sun, Nov 06, 2022 at 07:48:09PM +, Scott Cheloha wrote: > This patch switches loongson and octeon to clockintr(9). > > It has survived several release builds and upgrades from the resulting > bsd.rd images on my ER-4. The ER-4 doesn't have enough RAM to crunch a > parallel release build. It chokes on some of the larger LLVM modules. > > visa@ reports it survived a partial build on a loongson machine (he > skipped LLVM). I believe he is also testing this on a package > building machine, too. > > Testing on beefier octeon machines would help demonstrate this is > stable. My ER-4 only has USB2.0, which slows things down. So far, this patch has worked fine on the mips64 package build machines. > Notes: > > - octeon and loongson machines now have a randomized statclock(). > > - This patch merely disables the loongson glxclk. If the device has > no other use we can fully remove the driver in a separate patch. Lets keep the driver for now. The disabling is fine for the time being. > @@ -324,6 +324,10 @@ cpu_initclocks(void) > tc_init(&cp0_timecounter); > } > > + stathz = hz; > + profhz = stathz * 10; > + clockintr_init(CL_RNDSTAT); I think this clockintr_init() should be in cp0_startclock(). This would let other clock drivers do their own adjusting of the hz variables before clockintr initialization. With this fixed, OK visa@
Re: export {b,r}ootduid as sysctl, installer/sysupgrade improvements
An OpenBSD machine only has one OpenBSD install. As soon as we leave that model, and allow other setup models, perhaps you think there will be two or three potential configurations that people setup. I don't think so, I think it will keep being extended by people who do more and more scewed up bizzare configurations, all of which (obviously, to you) will need to be supported, and everything gets more complicated. I think we should say "STOP". Now. And not start down that roadmap. How many Windows configurations can I put onto a PC? Can I put two Andriod configurations onto my phone? You built something for a testlab. Your conclusion is that it should work for everyone. I simply cannot come to the same conclusion, because it requires complexity, but instead I think we should embrace simplicity even if it limits choice. Klemens Nanni wrote: > On Mon, Nov 14, 2022 at 07:49:11AM -0700, Theo de Raadt wrote: > > Klemens Nanni wrote: > > > > > This is because the installer always considers the first root disk it > > > finds as the one to upgrade, which is certainly not what I intend or > > > expect when booting/upgrading the softraid installation on sd1-3. > > > > What does > > > > first root disk > > > > Mean? > > One machine with two phsyical disks, say one NVMe and regular SSD. > Both disks contain a standalone OpenBSD installation. > > I consider each of them a root disk. > > > > > There is only one root disk. The root disk is the one that actually > > contains > > the / that is mounted. > > > > It is this one: > > > > root on sd0a (fb786f6b01042b30.a) swap on sd0b dump on sd0b > > > > You cannot change this. If you use the bootloader to tell the kernel do > > do something else, then I argue that sysupgrade and the installer should > > punish you unless you *manually tell it that every time* > > I don't set anything in /etc/boot.conf or the boot> prompt. > > I select the disk to boot from in the UEFI boot manager. > > > > > The install script knows what the root filesystem is using very simple > > heuristics, but by creating two new sysctl, I am afraid you will enrich > > this ability to support bizzare configurations that did not work, and > > I argue *should never work*. > > > > > It is probably not that common to have multiple installations/root disks > > > in one machine, but it isn't "weird" to me, either. > > > > What? It is not weird > > > > I think this is unsupported bullshit. > > > > Why do we need the install script to support this configuration you > > created? Why do we need to encourage other people to have such > > configurations? When they create such a configuration, and find the > > tooling can support it, won't they go and do even stranger things, then > > find the tooling doesn't support that even-stranger setup, and then > > you'll come back adding support for increasingly strange setups, and > > eventually we are going to end up with a large userbase *not using* a > > single root filesystem? > > > > > Overwriting the wrong system during an upgrade because the installer > > > makes too big of an assumption about the first disk is weird to me. > > > > > > > > > I can post console logs later showing how the installer picks the wrong > > > disk, if you want. > > > > There is only one possible root filesystem. > > > > If you created multiple root filesystems, you have created a mess and > > why is it wrong for me to argue you need to experience pain for the > > decisions that led you there? > > > > I do not think you are being honest about the reason for these extensions. > > > > I'm pretty honest, but apparently not precise enough. > > Now that all the softraid/installboot diffs landed and the dust has > settled, let me iterate over and test my setup again to make sure I'm > not tripping over my own mistakes. > > Then I can come back with a clear update or reproducer. >
Re: export {b,r}ootduid as sysctl, installer/sysupgrade improvements
On Mon, Nov 14, 2022 at 07:49:11AM -0700, Theo de Raadt wrote: > Klemens Nanni wrote: > > > This is because the installer always considers the first root disk it > > finds as the one to upgrade, which is certainly not what I intend or > > expect when booting/upgrading the softraid installation on sd1-3. > > What does > > first root disk > > Mean? One machine with two phsyical disks, say one NVMe and regular SSD. Both disks contain a standalone OpenBSD installation. I consider each of them a root disk. > > There is only one root disk. The root disk is the one that actually contains > the / that is mounted. > > It is this one: > > root on sd0a (fb786f6b01042b30.a) swap on sd0b dump on sd0b > > You cannot change this. If you use the bootloader to tell the kernel do > do something else, then I argue that sysupgrade and the installer should > punish you unless you *manually tell it that every time* I don't set anything in /etc/boot.conf or the boot> prompt. I select the disk to boot from in the UEFI boot manager. > > The install script knows what the root filesystem is using very simple > heuristics, but by creating two new sysctl, I am afraid you will enrich > this ability to support bizzare configurations that did not work, and > I argue *should never work*. > > > It is probably not that common to have multiple installations/root disks > > in one machine, but it isn't "weird" to me, either. > > What? It is not weird > > I think this is unsupported bullshit. > > Why do we need the install script to support this configuration you > created? Why do we need to encourage other people to have such > configurations? When they create such a configuration, and find the > tooling can support it, won't they go and do even stranger things, then > find the tooling doesn't support that even-stranger setup, and then > you'll come back adding support for increasingly strange setups, and > eventually we are going to end up with a large userbase *not using* a > single root filesystem? > > > Overwriting the wrong system during an upgrade because the installer > > makes too big of an assumption about the first disk is weird to me. > > > > > > I can post console logs later showing how the installer picks the wrong > > disk, if you want. > > There is only one possible root filesystem. > > If you created multiple root filesystems, you have created a mess and > why is it wrong for me to argue you need to experience pain for the > decisions that led you there? > > I do not think you are being honest about the reason for these extensions. > I'm pretty honest, but apparently not precise enough. Now that all the softraid/installboot diffs landed and the dust has settled, let me iterate over and test my setup again to make sure I'm not tripping over my own mistakes. Then I can come back with a clear update or reproducer.
xenodm: save ~/.xesssion to ~/.xsession.old
X segfaulted when I opened a window, Xorg.log.old only showed the address without anything specific, no core dump was created and xenodm automatically restarted. After I logged in I checked ~/.xsession for possible indications, but that file gets truncated on login. I've restarted xenodm again, so the old Xorg.log.old containing the generic "X segfaulted at addr 0x86..." is also gone. Woul it make sense to save an old copy in analogy to Xorg.log.old so crashes like this have a higher chance of being hunted down? I haven't extensively tested this patch, but relogging into X now leaves ~/.xesssion-errors.old with logs from the previous session behind. Index: app/xenodm//config/Xsession.in === RCS file: /cvs/xenocara/app/xenodm/config/Xsession.in,v retrieving revision 1.2 diff -u -p -r1.2 Xsession.in --- app/xenodm//config/Xsession.in 1 Jul 2022 20:42:06 - 1.2 +++ app/xenodm//config/Xsession.in 14 Nov 2022 16:47:03 - @@ -7,6 +7,7 @@ exec_prefix="@exec_prefix@" # redirect errors to a file in user's home directory if we can errfile="$HOME/.xsession-errors" +cp -f "$errfile" "$errfile.old" 2> /dev/null if ( umask 077 && cp /dev/null "$errfile" 2> /dev/null ) then exec > "$errfile" 2>&1
Netbooting Sparc64
Does anyone know what could cause this ... SC Alert: Host System has Reset \ Sun Fire T200, No Keyboard Copyright (c) 1998, 2010, Oracle and/or its affiliates. All rights reserved. OpenBoot 4.30.4.b, 16256 MB memory available, Serial #82196894. Ethernet address 0:14:4f:e6:39:9e, Host ID: 84e6399e. Bad magic number in disk label Can't open disk label package ERROR: boot-read fail Boot device: net File and args: 1000 Mbps full duplex Link up Requesting Internet Address for 0:14:4f:e6:39:9e Requesting Internet Address for 0:14:4f:e6:39:9e Requesting Internet Address for 0:14:4f:e6:39:9e >> OpenBSD BOOT 1.22 1000 Mbps full duplex Link up Using BOOTPARAMS protocol: ip address: 192.168.2.100bootparamd: 'whoami' call failed Using BOOTP protocol: ip address: 192.168.2.100, hostname: elk, netmask: 255.255.255.0, gateway: 192.168.2.254 root addr=192.168.2.15 path=/install/ Trying bsd... 1000 Mbps full duplex Link up Using BOOTPARAMS protocol: ip address: 192.168.2.100bootparamd: 'whoami' call failed Using BOOTP protocol: ip address: 192.168.2.100, hostname: elk, netmask: 255.255.255.0, gateway: 192.168.2.254 root addr=192.168.2.15 path=/install/ 1000 Mbps full duplex Link up Using BOOTPARAMS protocol: ip address: 192.168.2.100bootparamd: 'whoami' call failed Using BOOTP protocol: ip address: 192.168.2.100, hostname: elk, netmask: 255.255.255.0, gateway: 192.168.2.254 root addr=192.168.2.15 path=/install/ Booting /pci@780/pci@0/pci@1/network@0/bsd 8492368@0x100+2736@0x1819550+196552@0x1c0+3997752@0x1c2ffc8 symbols @ 0xfec42400 470285+165+565944+375574 start=0x100 [ using 1413000 bytes of bsd ELF symbol table ] console is /virtual-devices@100/console@1 Copyright (c) 1982, 1986, 1989, 1991, 1993 The Regents of the University of California. All rights reserved. Copyright (c) 1995-2018 OpenBSD. All rights reserved. https://www.OpenBSD.org OpenBSD 6.3 (GENERIC) #480: Sat Mar 24 22:11:46 MDT 2018 dera...@sparc64.openbsd.org:/usr/src/sys/arch/sparc64/compile/GENERIC real mem = 17045651456 (16256MB) avail mem = 16729997312 (15954MB) mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root: Sun Fire T200 cpu0 at mainbus0: SUNW,UltraSPARC-T1 (rev 0.0) @ 1000 MHz "SUNW,UltraSPARC-T1" at mainbus0 not configured [repeated for N cpus] vbus0 at mainbus0 "flashprom" at vbus0 not configured cbus0 at vbus0 vldc0 at cbus0 vldcp0 at vldc0 chan 0x0: ivec 0x0, 0x1 channel "hvctl" "ldom-primary" at vldc0 chan 0x1 not configured "fmactl" at vldc0 chan 0x3 not configured vldc1 at cbus0 "ldmfma" at vldc1 chan 0x4 not configured vldc2 at cbus0 vldcp1 at vldc2 chan 0x14: ivec 0x28, 0x29 channel "spds" "system-management" at vldc2 chan 0xd not configured vcons0 at vbus0: ivec 0x111, console vrtc0 at vbus0 "fma" at vbus0 not configured "sunvts" at vbus0 not configured "sunmc" at vbus0 not configured "explorer" at vbus0 not configured "led" at vbus0 not configured "flashupdate" at vbus0 not configured "ncp" at vbus0 not configured vpci0 at mainbus0: bus 2 to 7, dvma map 8000- pci0 at vpci0 ppb0 at pci0 dev 0 function 0 "PLX PEX 8532" rev 0xbc pci1 at ppb0 bus 3 ppb1 at pci1 dev 1 function 0 "PLX PEX 8532" rev 0xbc pci2 at ppb1 bus 4 em0 at pci2 dev 0 function 0 "Intel 82571EB" rev 0x06: ivec 0x795, address 00:14:4f:e6:39:9e em1 at pci2 dev 0 function 1 "Intel 82571EB" rev 0x06: ivec 0x796, address 00:14:4f:e6:39:9f ppb2 at pci1 dev 2 function 0 "PLX PEX 8532" rev 0xbc pci3 at ppb2 bus 5 ppb3 at pci1 dev 8 function 0 "PLX PEX 8532" rev 0xbc: msi pci4 at ppb3 bus 6 ppb4 at pci1 dev 9 function 0 "PLX PEX 8532" rev 0xbc pci5 at ppb4 bus 7 mpi0 at pci5 dev 0 function 0 "Symbios Logic SAS1064E" rev 0x02: msi mpi0: UNUSED, firmware 1.9.0.0 scsibus1 at mpi0: 63 targets sd0 at scsibus1 targ 0 lun 0: SCSI4 0/direct fixed naa.5000cca0222458c0 vpci1 at mainbus0: bus 2 to 9, dvma map 8000- pci6 at vpci1 ppb5 at pci6 dev 0 function 0 "PLX PEX 8532" rev 0xbc pci7 at ppb5 bus 3 ppb6 at pci7 dev 1 function 0 "PLX PEX 8532" rev 0xbc pci8 at ppb6 bus 4 ppb7 at pci8 dev 0 function 0 "Intel 41210 PCIE-PCIX" rev 0x09 pci9 at ppb7 bus 5 ebus0 at pci9 dev 2 function 0 "Acer Labs M1533 ISA" rev 0x00 com0 at ebus0 addr 3f8-3ff ivec 0x2: ns16550a, 16 byte fifo ohci0 at pci9 dev 5 function 0 "Acer Labs M5237 USB" rev 0x03: ivec 0x7c1, version 1.0, legacy support ohci1 at pci9 dev 6 function 0 "Acer Labs M5237 USB" rev 0x03: ivec 0x7c3, version 1.0, legacy support pciide0 at pci9 dev 8 function 0 "Acer Labs M5229 UDMA IDE" rev 0xc4: DMA, channel 0 configured to native-PCI, channel 1 configured to native-PCI pciide0: using ivec 0x7c4 for native-PCI interrupt atapiscsi0 at pciide0 channel 0 drive 0 scsibus2 at atapiscsi0: 2 targets cd0 at scsibus2 targ 0 lun 0: ATAPI 5/cdrom removable cd0(pciide0:0:0): using PIO mode 4, Ultra-DMA mode 2 pciide0: channel 1 disabled (no drives) usb0 at ohci0: USB revision 1.0 uhub0 at usb0 configuration 1 interface 0 "Acer Labs OHCI root hub" rev 1.00/1.00 addr 1 usb1 at ohci1: USB revi
Re: Turn sowriteable(), sballoc() and sbfree() macro to inline functions
On Mon, Nov 14, 2022 at 12:00:28PM +, Klemens Nanni wrote: > On Mon, Nov 14, 2022 at 02:14:27PM +0300, Vitaliy Makkoveev wrote: > > We have soreadable() already presented as inline function, but > > corresponding sowriteable() is still macro. Also it's no reason to keep > > sballoc() and sbfree() as macro. > > This reads as an improvement to me. > > sballoc() and sbfree()'s struct socket *so argument is entirely unused, > can it be removed? > It could, but I like to keep it for a while. It is useful for my per buffer locking games. > > > > Index: sys/sys/protosw.h > > === > > RCS file: /cvs/src/sys/sys/protosw.h,v > > retrieving revision 1.58 > > diff -u -p -r1.58 protosw.h > > --- sys/sys/protosw.h 17 Oct 2022 14:49:02 - 1.58 > > +++ sys/sys/protosw.h 14 Nov 2022 11:07:29 - > > @@ -53,6 +53,9 @@ > > * described below. > > */ > > > > +#ifndef _SYS_PROTOSW_H_ > > +#define _SYS_PROTOSW_H_ > > + > > struct mbuf; > > struct sockaddr; > > struct socket; > > @@ -413,3 +416,5 @@ pru_connect2(struct socket *so1, struct > > } > > > > #endif > > + > > +#endif /* _SYS_PROTOSW_H_ */ > > Index: sys/sys/socketvar.h > > === > > RCS file: /cvs/src/sys/sys/socketvar.h,v > > retrieving revision 1.111 > > diff -u -p -r1.111 socketvar.h > > --- sys/sys/socketvar.h 3 Oct 2022 16:43:52 - 1.111 > > +++ sys/sys/socketvar.h 14 Nov 2022 11:07:29 - > > @@ -160,6 +160,7 @@ struct socket { > > > > #ifdef _KERNEL > > > > +#include > > #include > > > > void soassertlocked(struct socket *); > > @@ -229,31 +230,39 @@ soreadable(struct socket *so) > > } > > > > /* can we write something to so? */ > > -#definesowriteable(so) \ > > -((sbspace((so), &(so)->so_snd) >= (so)->so_snd.sb_lowat && \ > > - (((so)->so_state & SS_ISCONNECTED) || \ > > - ((so)->so_proto->pr_flags & PR_CONNREQUIRED)==0)) || \ > > -((so)->so_state & SS_CANTSENDMORE) || (so)->so_error) > > +static inline int > > +sowriteable(struct socket *so) > > +{ > > + soassertlocked(so); > > + return ((sbspace(so, &so->so_snd) >= so->so_snd.sb_lowat && > > + ((so->so_state & SS_ISCONNECTED) || > > + (so->so_proto->pr_flags & PR_CONNREQUIRED)==0)) || > > + (so->so_state & SS_CANTSENDMORE) || so->so_error); > > +} > > > > /* adjust counters in sb reflecting allocation of m */ > > -#definesballoc(so, sb, m) do { > > \ > > - (sb)->sb_cc += (m)->m_len; \ > > - if ((m)->m_type != MT_CONTROL && (m)->m_type != MT_SONAME) \ > > - (sb)->sb_datacc += (m)->m_len; \ > > - (sb)->sb_mbcnt += MSIZE;\ > > - if ((m)->m_flags & M_EXT) \ > > - (sb)->sb_mbcnt += (m)->m_ext.ext_size; \ > > -} while (/* CONSTCOND */ 0) > > +static inline void > > +sballoc(struct socket *so, struct sockbuf *sb, struct mbuf *m) > > +{ > > + sb->sb_cc += m->m_len; > > + if (m->m_type != MT_CONTROL && m->m_type != MT_SONAME) > > + sb->sb_datacc += m->m_len; > > + sb->sb_mbcnt += MSIZE; > > + if (m->m_flags & M_EXT) > > + sb->sb_mbcnt += m->m_ext.ext_size; > > +} > > > > /* adjust counters in sb reflecting freeing of m */ > > -#definesbfree(so, sb, m) do { > > \ > > - (sb)->sb_cc -= (m)->m_len; \ > > - if ((m)->m_type != MT_CONTROL && (m)->m_type != MT_SONAME) \ > > - (sb)->sb_datacc -= (m)->m_len; \ > > - (sb)->sb_mbcnt -= MSIZE;\ > > - if ((m)->m_flags & M_EXT) \ > > - (sb)->sb_mbcnt -= (m)->m_ext.ext_size; \ > > -} while (/* CONSTCOND */ 0) > > +static inline void > > +sbfree(struct socket *so, struct sockbuf *sb, struct mbuf *m) > > +{ > > + sb->sb_cc -= m->m_len; > > + if (m->m_type != MT_CONTROL && m->m_type != MT_SONAME) > > + sb->sb_datacc -= m->m_len; > > + sb->sb_mbcnt -= MSIZE; > > + if (m->m_flags & M_EXT) > > + sb->sb_mbcnt -= m->m_ext.ext_size; > > +} > > > > /* > > * Set lock on sockbuf sb; sleep if lock is already held. > >
Re: libX11 patch for X*IfEvent() API issues, take #2
Hi Matthiew, On Nov 11 2022, Walter Alejandro Iglesias wrote: > On Nov 11 2022, Matthieu Herrb wrote: > > On Fri, Nov 11, 2022 at 03:17:21PM +0100, Walter Alejandro Iglesias wrote: > > > On Nov 11 2022, Matthieu Herrb wrote: > > > > Hi, > > > > > > > > So the patch provided by Adam Jackson upstreams is completely buggy. > > > > > > > > - the logic to setup the new locking function was busted > > > > - I've observed cases where even the XGetIfEvent() function gets > > > > re-rentered. So the flags does in fact need to be a counter. > > > > > > > > New patch which works for me with fvwm2... > > > > > > > > Testing is welcome... > > > > > > Unfortunately I still can reproduce the bug. > > > > > > This patch also makes firefox crash, (I'm not sure if it's because of > > > firefox or cwm). After reinstalling xenocara from the snapshots > > > packages I could run firefox again. > > > > > > fvwm is giving a lot of problems lately, there's also this drm bug I > > > reported: > > > > > > https://marc.info/?l=openbsd-bugs&m=166332904632232&w=2 > > > > > > Now FvwmIcons is crashing on fvwm3... I'm giving up with fvwm, I'm more > > > comfortable with cwm. > > > > Ok, I've figured out the cwm + firefox issue. There's also a > > XInternalLock that needs to be neutered in the X*IfEvent() callbacks. > > > > New diff, on which I've left my debug printfs. If you get other > > crashes it may be interesting to look in .xsession-errors... > > This patch solved the firefox cwm issue. Firefox shows this when > it starts: > > ~$ firefox > XXX _XLockDisplay 1 > XXX _XIfEventInternalLockDisplay 1 > XXX _XIfEventUnlockDisplay 0 > > fvwm2/3 still crashing. Today, after upgrading to the latest snapshot I can not reproduce this fvwm bug anymore. I ignore why. > > > > > > > Index: Makefile.bsd-wrapper > > === > > RCS file: /cvs/OpenBSD/xenocara/lib/libX11/Makefile.bsd-wrapper,v > > retrieving revision 1.29 > > diff -u -p -u -r1.29 Makefile.bsd-wrapper > > --- Makefile.bsd-wrapper3 Sep 2022 06:55:25 - 1.29 > > +++ Makefile.bsd-wrapper11 Nov 2022 15:13:59 - > > @@ -14,7 +14,6 @@ SHARED_LIBS= X11 18.0 X11_xcb 2.0 > > > > CONFIGURE_ARGS= --enable-tcp-transport --enable-unix-transport > > --enable-ipv6 \ > > --disable-composecache \ > > - --disable-thread-safety-constructor \ > > --without-xmlto --without-fop --without-xsltproc > > > > .include > > Index: include/X11/Xlibint.h > > === > > RCS file: /cvs/OpenBSD/xenocara/lib/libX11/include/X11/Xlibint.h,v > > retrieving revision 1.15 > > diff -u -p -u -r1.15 Xlibint.h > > --- include/X11/Xlibint.h 21 Feb 2022 08:01:24 - 1.15 > > +++ include/X11/Xlibint.h 11 Nov 2022 15:14:00 - > > @@ -207,6 +207,7 @@ struct _XDisplay > > > > XIOErrorExitHandler exit_handler; > > void *exit_handler_data; > > +CARD32 in_ifevent; > > }; > > > > #define XAllocIDs(dpy,ids,n) (*(dpy)->idlist_alloc)(dpy,ids,n) > > Index: src/ChkIfEv.c > > === > > RCS file: /cvs/OpenBSD/xenocara/lib/libX11/src/ChkIfEv.c,v > > retrieving revision 1.4 > > diff -u -p -u -r1.4 ChkIfEv.c > > --- src/ChkIfEv.c 30 May 2011 19:19:38 - 1.4 > > +++ src/ChkIfEv.c 11 Nov 2022 15:14:00 - > > @@ -49,6 +49,7 @@ Bool XCheckIfEvent ( > > unsigned long qe_serial = 0; > > int n; /* time through count */ > > > > +dpy->in_ifevent++; > > LockDisplay(dpy); > > prev = NULL; > > for (n = 3; --n >= 0;) { > > @@ -60,6 +61,7 @@ Bool XCheckIfEvent ( > > *event = qelt->event; > > _XDeq(dpy, prev, qelt); > > _XStoreEventCookie(dpy, event); > > +dpy->in_ifevent = False; > > UnlockDisplay(dpy); > > return True; > > } > > @@ -78,6 +80,7 @@ Bool XCheckIfEvent ( > > /* another thread has snatched this event */ > > prev = NULL; > > } > > +dpy->in_ifevent--; > > UnlockDisplay(dpy); > > return False; > > } > > Index: src/IfEvent.c > > === > > RCS file: /cvs/OpenBSD/xenocara/lib/libX11/src/IfEvent.c,v > > retrieving revision 1.4 > > diff -u -p -u -r1.4 IfEvent.c > > --- src/IfEvent.c 30 May 2011 19:19:38 - 1.4 > > +++ src/IfEvent.c 11 Nov 2022 15:14:00 - > > @@ -48,6 +48,7 @@ XIfEvent ( > > register _XQEvent *qelt, *prev; > > unsigned long qe_serial = 0; > > > > +dpy->in_ifevent++; > > LockDisplay(dpy); > > prev = NULL; > > while (1) { > > @@ -59,6 +60,7 @@ XIfEvent ( > > *event = qelt->event; > > _XDeq(dpy, prev, qelt); > > _XStoreEventCookie(dpy, event); > > +
Re: export {b,r}ootduid as sysctl, installer/sysupgrade improvements
> sd1-3 are softraid chunks hosting a separate installation for testing. > Booting into this I think this is where you went wrong. Expecting this to work is going to result in 20-40 diffs bloating all the media for a configuration which less than than 1 in a thousand people need.
Re: export {b,r}ootduid as sysctl, installer/sysupgrade improvements
Klemens Nanni wrote: > This is because the installer always considers the first root disk it > finds as the one to upgrade, which is certainly not what I intend or > expect when booting/upgrading the softraid installation on sd1-3. What does first root disk Mean? There is only one root disk. The root disk is the one that actually contains the / that is mounted. It is this one: root on sd0a (fb786f6b01042b30.a) swap on sd0b dump on sd0b You cannot change this. If you use the bootloader to tell the kernel do do something else, then I argue that sysupgrade and the installer should punish you unless you *manually tell it that every time* The install script knows what the root filesystem is using very simple heuristics, but by creating two new sysctl, I am afraid you will enrich this ability to support bizzare configurations that did not work, and I argue *should never work*. > It is probably not that common to have multiple installations/root disks > in one machine, but it isn't "weird" to me, either. What? It is not weird I think this is unsupported bullshit. Why do we need the install script to support this configuration you created? Why do we need to encourage other people to have such configurations? When they create such a configuration, and find the tooling can support it, won't they go and do even stranger things, then find the tooling doesn't support that even-stranger setup, and then you'll come back adding support for increasingly strange setups, and eventually we are going to end up with a large userbase *not using* a single root filesystem? > Overwriting the wrong system during an upgrade because the installer > makes too big of an assumption about the first disk is weird to me. > > > I can post console logs later showing how the installer picks the wrong > disk, if you want. There is only one possible root filesystem. If you created multiple root filesystems, you have created a mess and why is it wrong for me to argue you need to experience pain for the decisions that led you there? I do not think you are being honest about the reason for these extensions.
Re: mount_ntfs.8: Fix swapped -g user and -u group
ha, good catch. committed, thanks.
Re: export {b,r}ootduid as sysctl, installer/sysupgrade improvements
On Mon, Nov 14, 2022 at 05:45:33AM -0700, Theo de Raadt wrote: > Mark Kettenis wrote: > > > > > The installer considers a disk a root disk if 'a' is FFS and contains > > > > expected files. > > > > > > > > Furthermore, unattended upgrades will always install to the first root > > > > disk that is found. > > > > > > > > This works fine on machines with only one root disk, but it quickly > > > > behaves unexpectedly when having multiple disks/installations in one > > > > machine. > > > > > > > > I run such machines, esp. since fiddling with softraid and installboot. > > > I don't understand the situation. > > I suspect you are intentionally creating very weird setups, and now you > want install script to help people create such weird setups. Then it > becomes an additional weird thing we must anticipate in the future. > > Please show a valid reason why boot are different. It is very hard to > reason about this when the rest of your email shows an example where > they are the same. > > > > > The installer/sysupgrade experience can definitely be improved here, but > > > > that takes some consideration. > > > > > > > > One requirement, imho, is knowing > > > > 1. which disk we booted from, i.e. > > > >from which disk the kernel (/bsd.rd or /bsd.upgrade) was loaded > > > > 2. which disk the root filesystem is on, i.e. > > > >likely the same disk holding /home where sysupgrade put the sets > > Prove it. > Take a machine with multiple disks like this one: $ sysctl -n hw.product SolidRun CEX7 Platform $ dmesg | grep -e ^sd -e ^root sd0 at scsibus0 targ 1 lun 0: sd0: 238475MB, 512 bytes/sector, 488397168 sectors sd1 at scsibus1 targ 0 lun 0: naa.5000c5002039b4e8 sd1: 152627MB, 512 bytes/sector, 312581808 sectors sd2 at scsibus2 targ 0 lun 0: naa.5000c5002070e824 sd2: 152627MB, 512 bytes/sector, 312581808 sectors sd3 at scsibus3 targ 0 lun 0: naa.5000c500204ada8c sd3: 152627MB, 512 bytes/sector, 312581808 sectors root on sd0a (8ae6b6fd29b37541.a) swap on sd0b dump on sd0b Runnin sysupgrade(8) from sd0 upgrades sd0, as expected. sd1-3 are softraid chunks hosting a separate installation for testing. Booting into this XXX and sysupgrade'ing from there, however, upgrades sd0, not sd4. This is because the installer always considers the first root disk it finds as the one to upgrade, which is certainly not what I intend or expect when booting/upgrading the softraid installation on sd1-3. It is probably not that common to have multiple installations/root disks in one machine, but it isn't "weird" to me, either. Overwriting the wrong system during an upgrade because the installer makes too big of an assumption about the first disk is weird to me. I can post console logs later showing how the installer picks the wrong disk, if you want.
mount_ntfs.8: Fix swapped -g user and -u group
Index: mount_ntfs.8 === RCS file: /cvs/src/sbin/mount_ntfs/mount_ntfs.8,v retrieving revision 1.17 diff -u -p -r1.17 mount_ntfs.8 --- mount_ntfs.820 Aug 2022 07:03:24 - 1.17 +++ mount_ntfs.814 Nov 2022 14:06:39 - @@ -41,10 +41,10 @@ .Sh SYNOPSIS .Nm mount_ntfs .Op Fl ai -.Op Fl g Ar user +.Op Fl g Ar group .Op Fl m Ar mask .Op Fl o Ar options -.Op Fl u Ar group +.Op Fl u Ar user .Ar special .Ar node .Sh DESCRIPTION
OpenBSD Errata: November 15, 2022 (pixman)
Errata patches for X11 pixman library have been released for OpenBSD 7.1 and 7.2. Binary updates for the amd64, i386 and arm64 platform are available via the syspatch utility. Source code patches can be found on the respective errata page: https://www.openbsd.org/errata71.html https://www.openbsd.org/errata72.html
Re: non regression tests for some packaging scripts
On Mon, Nov 14 2022, Marc Espie wrote: > I'd like to add some non-regression tests for some scripts, most notably > register-plist, which is... let's say not perfect at the moment. > > I believe > ${PORTSDIR}/regress is probably the simplest least disturbing way to do that. We have /usr/ports/tests. > Since the manpages already exist under src, I could also (somehow) put them > in /usr/src/regress. > > Of course, this means requiring a checkout of the ports tree to check things. > > What do you people think ? I think those belong to the ports tree. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: export {b,r}ootduid as sysctl, installer/sysupgrade improvements
Klemens Nanni wrote: > I'd like to get on with this, can also add sysctl.2 bits to document > those before sending diffs using them. I want you to prove the use case first. > Mark asked whether CTLTYPE_QUAD would be more suited, but I still don't > understand how that is supposed to work and there was no answer in this > thread's other mail. All the consumers of this use strings. It is a string in the disklabel output, a string in the disklabel command, and they are managed as strings everywhere else. Outside the installer, the only place it is handled is as a partition name, which is either DUID.a or sd0a, either on the commandline or in fstab. Always hanled as a string, because the shell handles numbers poorly, let alone hex numbers with extra stuff afterwards.
Re: export {b,r}ootduid as sysctl, installer/sysupgrade improvements
Mark Kettenis wrote: > > > The installer considers a disk a root disk if 'a' is FFS and contains > > > expected files. > > > > > > Furthermore, unattended upgrades will always install to the first root > > > disk that is found. > > > > > > This works fine on machines with only one root disk, but it quickly > > > behaves unexpectedly when having multiple disks/installations in one > > > machine. > > > > > > I run such machines, esp. since fiddling with softraid and installboot. I don't understand the situation. I suspect you are intentionally creating very weird setups, and now you want install script to help people create such weird setups. Then it becomes an additional weird thing we must anticipate in the future. Please show a valid reason why boot are different. It is very hard to reason about this when the rest of your email shows an example where they are the same. > > > The installer/sysupgrade experience can definitely be improved here, but > > > that takes some consideration. > > > > > > One requirement, imho, is knowing > > > 1. which disk we booted from, i.e. > > >from which disk the kernel (/bsd.rd or /bsd.upgrade) was loaded > > > 2. which disk the root filesystem is on, i.e. > > >likely the same disk holding /home where sysupgrade put the sets Prove it.
Re: export {b,r}ootduid as sysctl, installer/sysupgrade improvements
> Date: Mon, 14 Nov 2022 09:22:13 + > From: Klemens Nanni > > On Tue, Sep 06, 2022 at 01:16:47AM +, Klemens Nanni wrote: > > The installer considers a disk a root disk if 'a' is FFS and contains > > expected files. > > > > Furthermore, unattended upgrades will always install to the first root > > disk that is found. > > > > This works fine on machines with only one root disk, but it quickly > > behaves unexpectedly when having multiple disks/installations in one > > machine. > > > > I run such machines, esp. since fiddling with softraid and installboot. > > > > > > The installer/sysupgrade experience can definitely be improved here, but > > that takes some consideration. > > > > One requirement, imho, is knowing > > 1. which disk we booted from, i.e. > >from which disk the kernel (/bsd.rd or /bsd.upgrade) was loaded > > 2. which disk the root filesystem is on, i.e. > >likely the same disk holding /home where sysupgrade put the sets > > > > > > The boot disk could be helpful inside installer, e.g. to check if > > /bsd.ugpraded was booted from a valid root disk -- a good indicator for > > rebooting from the same disk the user just ran sysupgrade on. > > > > The root disk is of no help inside the installer as that will always be > > the ramdisk. But it could be used by sysupgrade to perhaps prefill > > /auto_upgrade.conf to decide up-front which disk to upgrade. > > This answer to the 'Which disk is the root disk' question is currently > > answered inside the installer during unattended upgrades... and it will > > always be the first valid root disk, which is not always correct. > > > > So to make progress, here's a diff that exports readily available > > disklabel DUIDs: > > > > # disklabel sd0 | grep duid > > duid: 98c0c47c3ffddeb4 > > # sysctl hw | grep duid > > hw.bootduid=98c0c47c3ffddeb4 > > hw.rootduid=98c0c47c3ffddeb4 > > > > Having that, working out the installer/sysupgrade bits should be easier. > > > > I'm testing this on arm64 with two disks/installations. > > > > Feedback? Objection? OK? > > I'd like to get on with this, can also add sysctl.2 bits to document > those before sending diffs using them. > > Mark asked whether CTLTYPE_QUAD would be more suited, but I still don't > understand how that is supposed to work and there was no answer in this > thread's other mail. So my thinking there was that DUIDs are 64-bit integers. But it seems they are always interpreted as an array of 8 bytes. That probably means that presenting it as a 64-bit integer (which is what CTLTYPE_QUAD does) isn't a good idea. > Anyone? It is somewhat unfortunate that we are exporting binary data as a string since it means that userland code will have to parse that string again and turn it into an array of bytes. We could export it as CTLTYPE_STRUCT but then we'd need additional code to make sysctl(8) display the data. so ok kettenis@ > Index: sys/sys/sysctl.h > === > RCS file: /cvs/src/sys/sys/sysctl.h,v > retrieving revision 1.231 > diff -u -p -r1.231 sysctl.h > --- sys/sys/sysctl.h 7 Nov 2022 14:25:44 - 1.231 > +++ sys/sys/sysctl.h 9 Nov 2022 12:06:31 - > @@ -946,7 +946,9 @@ struct kinfo_file { > #define HW_SMT 24 /* int: enable SMT/HT/CMT */ > #define HW_NCPUONLINE 25 /* int: number of cpus being > used */ > #define HW_POWER26 /* int: machine has wall-power > */ > -#define HW_MAXID27 /* number of valid hw ids */ > +#define HW_BOOTDUID 27 /* string: DUID of boot disk */ > +#define HW_ROOTDUID 28 /* string: DUID of root disk */ > +#define HW_MAXID29 /* number of valid hw ids */ > > #define CTL_HW_NAMES { \ > { 0, 0 }, \ > @@ -976,6 +978,8 @@ struct kinfo_file { > { "smt", CTLTYPE_INT }, \ > { "ncpuonline", CTLTYPE_INT }, \ > { "power", CTLTYPE_INT }, \ > + { "bootduid", CTLTYPE_STRING }, \ > + { "rootduid", CTLTYPE_STRING }, \ > } > > /* > Index: sys/kern/kern_sysctl.c > === > RCS file: /cvs/src/sys/kern/kern_sysctl.c,v > retrieving revision 1.408 > diff -u -p -r1.408 kern_sysctl.c > --- sys/kern/kern_sysctl.c7 Nov 2022 14:25:44 - 1.408 > +++ sys/kern/kern_sysctl.c9 Nov 2022 12:06:30 - > @@ -770,6 +770,12 @@ hw_sysctl(int *name, u_int namelen, void > case HW_SMT: > return (sysctl_hwsmt(oldp, oldlenp, newp, newlen)); > #endif > + case HW_BOOTDUID: > + return (sysctl_rdstring(oldp, oldlenp, newp, > + duid_format(bootduid))); > + case HW_ROOTDUID: > + return (sysctl_rdstring(oldp, oldlenp, newp, > + duid_format(rootduid))); > default: > return sysctl_bounded_arr(hw_vars, nitems(hw_var
Re: Turn sowriteable(), sballoc() and sbfree() macro to inline functions
On Mon, Nov 14, 2022 at 12:00:28PM +, Klemens Nanni wrote: > On Mon, Nov 14, 2022 at 02:14:27PM +0300, Vitaliy Makkoveev wrote: > > We have soreadable() already presented as inline function, but > > corresponding sowriteable() is still macro. Also it's no reason to keep > > sballoc() and sbfree() as macro. > > This reads as an improvement to me. > > sballoc() and sbfree()'s struct socket *so argument is entirely unused, > can it be removed? This builds and runs fine on top of your diff. diff --git a/sys/kern/uipc_socket.c b/sys/kern/uipc_socket.c index 478763592b4..e0af311ae3c 100644 --- a/sys/kern/uipc_socket.c +++ b/sys/kern/uipc_socket.c @@ -931,7 +931,7 @@ dontblock: *paddr = m_copym(m, 0, m->m_len, M_NOWAIT); m = m->m_next; } else { - sbfree(so, &so->so_rcv, m); + sbfree(&so->so_rcv, m); if (paddr) { *paddr = m; so->so_rcv.sb_mb = m->m_next; @@ -955,7 +955,7 @@ dontblock: *controlp = m_copym(m, 0, m->m_len, M_NOWAIT); m = m->m_next; } else { - sbfree(so, &so->so_rcv, m); + sbfree(&so->so_rcv, m); so->so_rcv.sb_mb = m->m_next; m->m_nextpkt = m->m_next = NULL; cm = m; @@ -1057,7 +1057,7 @@ dontblock: orig_resid = 0; } else { nextrecord = m->m_nextpkt; - sbfree(so, &so->so_rcv, m); + sbfree(&so->so_rcv, m); if (mp) { *mp = m; mp = &m->m_next; @@ -1560,7 +1560,7 @@ somove(struct socket *so, int wait) * that the whole first record can be processed. */ m = so->so_rcv.sb_mb; - sbfree(so, &so->so_rcv, m); + sbfree(&so->so_rcv, m); so->so_rcv.sb_mb = m_free(m); sbsync(&so->so_rcv, nextrecord); } @@ -1570,7 +1570,7 @@ somove(struct socket *so, int wait) */ m = so->so_rcv.sb_mb; while (m && m->m_type == MT_CONTROL) { - sbfree(so, &so->so_rcv, m); + sbfree(&so->so_rcv, m); so->so_rcv.sb_mb = m_free(m); m = so->so_rcv.sb_mb; sbsync(&so->so_rcv, nextrecord); @@ -1609,7 +1609,7 @@ somove(struct socket *so, int wait) so->so_rcv.sb_datacc -= size; } else { *mp = so->so_rcv.sb_mb; - sbfree(so, &so->so_rcv, *mp); + sbfree(&so->so_rcv, *mp); so->so_rcv.sb_mb = (*mp)->m_next; sbsync(&so->so_rcv, nextrecord); } diff --git a/sys/kern/uipc_socket2.c b/sys/kern/uipc_socket2.c index d8b39e44c69..478c93a3f99 100644 --- a/sys/kern/uipc_socket2.c +++ b/sys/kern/uipc_socket2.c @@ -845,7 +845,7 @@ sbappendrecord(struct socket *so, struct sockbuf *sb, struct mbuf *m0) * Put the first mbuf on the queue. * Note this permits zero length records. */ - sballoc(so, sb, m0); + sballoc(sb, m0); SBLASTRECORDCHK(sb, "sbappendrecord 1"); SBLINKRECORD(sb, m0); m = m0->m_next; @@ -900,8 +900,8 @@ sbappendaddr(struct socket *so, struct sockbuf *sb, const struct sockaddr *asa, SBLASTRECORDCHK(sb, "sbappendaddr 1"); for (n = m; n->m_next != NULL; n = n->m_next) - sballoc(so, sb, n); - sballoc(so, sb, n); + sballoc(sb, n); + sballoc(sb, n); nlast = n; SBLINKRECORD(sb, m); @@ -937,8 +937,8 @@ sbappendcontrol(struct socket *so, struct sockbuf *sb, struct mbuf *m0, SBLASTRECORDCHK(sb, "sbappendcontrol 1"); for (m = control; m->m_next != NULL; m = m->m_next) - sballoc(so, sb, m); - sballoc(so, sb, m); + sballoc(sb, m); + sballoc(sb, m); mlast = m; SBLINKRECORD(sb, control); @@ -993,7 +993,7 @@ sbcompress(struct socket *so, struct sockbuf *sb, struct mbuf *m, else sb->sb_mb = m; sb->sb_mbtail = m; - sballoc(so, sb, m); + sballoc(sb, m); n = m; m->m_flags &= ~M_EOR; m = m->m_next; @@ -1058,12 +1058,12 @@ sbdrop(struct socket *so, struct sockbuf *sb, int len) break; } len -= m->m_len; - sbfree(so, sb, m); + sbfree(sb, m); mn = m_free(m); m = mn; } whi
Re: Turn sowriteable(), sballoc() and sbfree() macro to inline functions
On Mon, Nov 14, 2022 at 02:14:27PM +0300, Vitaliy Makkoveev wrote: > We have soreadable() already presented as inline function, but > corresponding sowriteable() is still macro. Also it's no reason to keep > sballoc() and sbfree() as macro. This reads as an improvement to me. sballoc() and sbfree()'s struct socket *so argument is entirely unused, can it be removed? > > Index: sys/sys/protosw.h > === > RCS file: /cvs/src/sys/sys/protosw.h,v > retrieving revision 1.58 > diff -u -p -r1.58 protosw.h > --- sys/sys/protosw.h 17 Oct 2022 14:49:02 - 1.58 > +++ sys/sys/protosw.h 14 Nov 2022 11:07:29 - > @@ -53,6 +53,9 @@ > * described below. > */ > > +#ifndef _SYS_PROTOSW_H_ > +#define _SYS_PROTOSW_H_ > + > struct mbuf; > struct sockaddr; > struct socket; > @@ -413,3 +416,5 @@ pru_connect2(struct socket *so1, struct > } > > #endif > + > +#endif /* _SYS_PROTOSW_H_ */ > Index: sys/sys/socketvar.h > === > RCS file: /cvs/src/sys/sys/socketvar.h,v > retrieving revision 1.111 > diff -u -p -r1.111 socketvar.h > --- sys/sys/socketvar.h 3 Oct 2022 16:43:52 - 1.111 > +++ sys/sys/socketvar.h 14 Nov 2022 11:07:29 - > @@ -160,6 +160,7 @@ struct socket { > > #ifdef _KERNEL > > +#include > #include > > void soassertlocked(struct socket *); > @@ -229,31 +230,39 @@ soreadable(struct socket *so) > } > > /* can we write something to so? */ > -#define sowriteable(so) \ > -((sbspace((so), &(so)->so_snd) >= (so)->so_snd.sb_lowat && \ > - (((so)->so_state & SS_ISCONNECTED) || \ > - ((so)->so_proto->pr_flags & PR_CONNREQUIRED)==0)) || \ > -((so)->so_state & SS_CANTSENDMORE) || (so)->so_error) > +static inline int > +sowriteable(struct socket *so) > +{ > + soassertlocked(so); > + return ((sbspace(so, &so->so_snd) >= so->so_snd.sb_lowat && > + ((so->so_state & SS_ISCONNECTED) || > + (so->so_proto->pr_flags & PR_CONNREQUIRED)==0)) || > + (so->so_state & SS_CANTSENDMORE) || so->so_error); > +} > > /* adjust counters in sb reflecting allocation of m */ > -#define sballoc(so, sb, m) do { > \ > - (sb)->sb_cc += (m)->m_len; \ > - if ((m)->m_type != MT_CONTROL && (m)->m_type != MT_SONAME) \ > - (sb)->sb_datacc += (m)->m_len; \ > - (sb)->sb_mbcnt += MSIZE;\ > - if ((m)->m_flags & M_EXT) \ > - (sb)->sb_mbcnt += (m)->m_ext.ext_size; \ > -} while (/* CONSTCOND */ 0) > +static inline void > +sballoc(struct socket *so, struct sockbuf *sb, struct mbuf *m) > +{ > + sb->sb_cc += m->m_len; > + if (m->m_type != MT_CONTROL && m->m_type != MT_SONAME) > + sb->sb_datacc += m->m_len; > + sb->sb_mbcnt += MSIZE; > + if (m->m_flags & M_EXT) > + sb->sb_mbcnt += m->m_ext.ext_size; > +} > > /* adjust counters in sb reflecting freeing of m */ > -#define sbfree(so, sb, m) do { > \ > - (sb)->sb_cc -= (m)->m_len; \ > - if ((m)->m_type != MT_CONTROL && (m)->m_type != MT_SONAME) \ > - (sb)->sb_datacc -= (m)->m_len; \ > - (sb)->sb_mbcnt -= MSIZE;\ > - if ((m)->m_flags & M_EXT) \ > - (sb)->sb_mbcnt -= (m)->m_ext.ext_size; \ > -} while (/* CONSTCOND */ 0) > +static inline void > +sbfree(struct socket *so, struct sockbuf *sb, struct mbuf *m) > +{ > + sb->sb_cc -= m->m_len; > + if (m->m_type != MT_CONTROL && m->m_type != MT_SONAME) > + sb->sb_datacc -= m->m_len; > + sb->sb_mbcnt -= MSIZE; > + if (m->m_flags & M_EXT) > + sb->sb_mbcnt -= m->m_ext.ext_size; > +} > > /* > * Set lock on sockbuf sb; sleep if lock is already held. >
Turn sowriteable(), sballoc() and sbfree() macro to inline functions
We have soreadable() already presented as inline function, but corresponding sowriteable() is still macro. Also it's no reason to keep sballoc() and sbfree() as macro. Index: sys/sys/protosw.h === RCS file: /cvs/src/sys/sys/protosw.h,v retrieving revision 1.58 diff -u -p -r1.58 protosw.h --- sys/sys/protosw.h 17 Oct 2022 14:49:02 - 1.58 +++ sys/sys/protosw.h 14 Nov 2022 11:07:29 - @@ -53,6 +53,9 @@ * described below. */ +#ifndef _SYS_PROTOSW_H_ +#define _SYS_PROTOSW_H_ + struct mbuf; struct sockaddr; struct socket; @@ -413,3 +416,5 @@ pru_connect2(struct socket *so1, struct } #endif + +#endif /* _SYS_PROTOSW_H_ */ Index: sys/sys/socketvar.h === RCS file: /cvs/src/sys/sys/socketvar.h,v retrieving revision 1.111 diff -u -p -r1.111 socketvar.h --- sys/sys/socketvar.h 3 Oct 2022 16:43:52 - 1.111 +++ sys/sys/socketvar.h 14 Nov 2022 11:07:29 - @@ -160,6 +160,7 @@ struct socket { #ifdef _KERNEL +#include #include void soassertlocked(struct socket *); @@ -229,31 +230,39 @@ soreadable(struct socket *so) } /* can we write something to so? */ -#definesowriteable(so) \ -((sbspace((so), &(so)->so_snd) >= (so)->so_snd.sb_lowat && \ - (((so)->so_state & SS_ISCONNECTED) || \ - ((so)->so_proto->pr_flags & PR_CONNREQUIRED)==0)) || \ -((so)->so_state & SS_CANTSENDMORE) || (so)->so_error) +static inline int +sowriteable(struct socket *so) +{ + soassertlocked(so); + return ((sbspace(so, &so->so_snd) >= so->so_snd.sb_lowat && + ((so->so_state & SS_ISCONNECTED) || + (so->so_proto->pr_flags & PR_CONNREQUIRED)==0)) || + (so->so_state & SS_CANTSENDMORE) || so->so_error); +} /* adjust counters in sb reflecting allocation of m */ -#definesballoc(so, sb, m) do { \ - (sb)->sb_cc += (m)->m_len; \ - if ((m)->m_type != MT_CONTROL && (m)->m_type != MT_SONAME) \ - (sb)->sb_datacc += (m)->m_len; \ - (sb)->sb_mbcnt += MSIZE;\ - if ((m)->m_flags & M_EXT) \ - (sb)->sb_mbcnt += (m)->m_ext.ext_size; \ -} while (/* CONSTCOND */ 0) +static inline void +sballoc(struct socket *so, struct sockbuf *sb, struct mbuf *m) +{ + sb->sb_cc += m->m_len; + if (m->m_type != MT_CONTROL && m->m_type != MT_SONAME) + sb->sb_datacc += m->m_len; + sb->sb_mbcnt += MSIZE; + if (m->m_flags & M_EXT) + sb->sb_mbcnt += m->m_ext.ext_size; +} /* adjust counters in sb reflecting freeing of m */ -#definesbfree(so, sb, m) do { \ - (sb)->sb_cc -= (m)->m_len; \ - if ((m)->m_type != MT_CONTROL && (m)->m_type != MT_SONAME) \ - (sb)->sb_datacc -= (m)->m_len; \ - (sb)->sb_mbcnt -= MSIZE;\ - if ((m)->m_flags & M_EXT) \ - (sb)->sb_mbcnt -= (m)->m_ext.ext_size; \ -} while (/* CONSTCOND */ 0) +static inline void +sbfree(struct socket *so, struct sockbuf *sb, struct mbuf *m) +{ + sb->sb_cc -= m->m_len; + if (m->m_type != MT_CONTROL && m->m_type != MT_SONAME) + sb->sb_datacc -= m->m_len; + sb->sb_mbcnt -= MSIZE; + if (m->m_flags & M_EXT) + sb->sb_mbcnt -= m->m_ext.ext_size; +} /* * Set lock on sockbuf sb; sleep if lock is already held.
Re: special case mpe(4) in in6_ifattach()
On Thu, Nov 10, 2022 at 04:13:33PM +, Klemens Nanni wrote: > On Fri, Nov 04, 2022 at 03:40:04PM +0100, Claudio Jeker wrote: > > So mpe(4) is a special device. It is a point-to-multipoint interface that > > does not do multicast. So setting IFF_MULTICAST on the interface is not > > correct but IPv6 depends on it because neighbor discovery. > > > > Now there is no neighbor discovery on mpe(4) the neighbors are handled via > > BGP. So lets adjust in6_ifattach() to succeed for mpe(4) interfaces and > > with that BGP IPv6 L3VPN should work. > > > > It may be an idea to move the IFF_MULTCAST check down into the > > ifp->if_type switch but right now this is good enough. I wonder if we have > > other interfaces that need similar treatment (e.g. mgre). > > Good enough for now, I also have a few minues for nd6.c but prefer > simpler changes first, then the cleanup come later. Seeing this thread again, my OK was meant for the later "I think this is a better diff." mail, of course, just to be clear.
Re: Merge uipc_bind() with unp_bind()
On Mon, Nov 14, 2022 at 09:28:34AM +, Klemens Nanni wrote: > On Mon, Nov 14, 2022 at 12:11:46PM +0300, Vitaliy Makkoveev wrote: > > uipc_bind() only calls unp_bind(). Also it is the only caller of > > unp_bind(). > > For *_bind() alone this looks like zapping a useless indirection, but > the rest of uipc_*() seems to consistently use unp_*() still, so I'm not > convinced this one-off merge is an improvement. > You talk about unp_{,dis}connect(). Such unp_*() contains the code called from multiple places, not only from corresponding uipc_*(). The uipc_bind() and unp_bind() are the different case. In the uipc_usrreq() times it made sense to have unp_bind() for prevent code mess within big switch(), but since uipc_usrreq() was splitted to multiple handlers there is no reason to keep both uipc_bind() and unp_bind(). > > > > Index: sys/kern/uipc_usrreq.c > > === > > RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v > > retrieving revision 1.192 > > diff -u -p -r1.192 uipc_usrreq.c > > --- sys/kern/uipc_usrreq.c 13 Nov 2022 16:01:32 - 1.192 > > +++ sys/kern/uipc_usrreq.c 14 Nov 2022 09:07:43 - > > @@ -322,8 +322,93 @@ int > > uipc_bind(struct socket *so, struct mbuf *nam, struct proc *p) > > { > > struct unpcb *unp = sotounpcb(so); > > + struct sockaddr_un *soun; > > + struct mbuf *nam2; > > + struct vnode *vp; > > + struct vattr vattr; > > + int error; > > + struct nameidata nd; > > + size_t pathlen; > > > > - return unp_bind(unp, nam, p); > > + if (unp->unp_flags & (UNP_BINDING | UNP_CONNECTING)) > > + return (EINVAL); > > + if (unp->unp_vnode != NULL) > > + return (EINVAL); > > + if ((error = unp_nam2sun(nam, &soun, &pathlen))) > > + return (error); > > + > > + unp->unp_flags |= UNP_BINDING; > > + > > + /* > > +* Enforce `i_lock' -> `unplock' because fifo subsystem > > +* requires it. The socket can't be closed concurrently > > +* because the file descriptor reference is still held. > > +*/ > > + > > + sounlock(unp->unp_socket); > > + > > + nam2 = m_getclr(M_WAITOK, MT_SONAME); > > + nam2->m_len = sizeof(struct sockaddr_un); > > + memcpy(mtod(nam2, struct sockaddr_un *), soun, > > + offsetof(struct sockaddr_un, sun_path) + pathlen); > > + /* No need to NUL terminate: m_getclr() returns zero'd mbufs. */ > > + > > + soun = mtod(nam2, struct sockaddr_un *); > > + > > + /* Fixup sun_len to keep it in sync with m_len. */ > > + soun->sun_len = nam2->m_len; > > + > > + NDINIT(&nd, CREATE, NOFOLLOW | LOCKPARENT, UIO_SYSSPACE, > > + soun->sun_path, p); > > + nd.ni_pledge = PLEDGE_UNIX; > > + nd.ni_unveil = UNVEIL_CREATE; > > + > > + KERNEL_LOCK(); > > +/* SHOULD BE ABLE TO ADOPT EXISTING AND wakeup() ALA FIFO's */ > > + error = namei(&nd); > > + if (error != 0) { > > + m_freem(nam2); > > + solock(unp->unp_socket); > > + goto out; > > + } > > + vp = nd.ni_vp; > > + if (vp != NULL) { > > + VOP_ABORTOP(nd.ni_dvp, &nd.ni_cnd); > > + if (nd.ni_dvp == vp) > > + vrele(nd.ni_dvp); > > + else > > + vput(nd.ni_dvp); > > + vrele(vp); > > + m_freem(nam2); > > + error = EADDRINUSE; > > + solock(unp->unp_socket); > > + goto out; > > + } > > + VATTR_NULL(&vattr); > > + vattr.va_type = VSOCK; > > + vattr.va_mode = ACCESSPERMS &~ p->p_fd->fd_cmask; > > + error = VOP_CREATE(nd.ni_dvp, &nd.ni_vp, &nd.ni_cnd, &vattr); > > + vput(nd.ni_dvp); > > + if (error) { > > + m_freem(nam2); > > + solock(unp->unp_socket); > > + goto out; > > + } > > + solock(unp->unp_socket); > > + unp->unp_addr = nam2; > > + vp = nd.ni_vp; > > + vp->v_socket = unp->unp_socket; > > + unp->unp_vnode = vp; > > + unp->unp_connid.uid = p->p_ucred->cr_uid; > > + unp->unp_connid.gid = p->p_ucred->cr_gid; > > + unp->unp_connid.pid = p->p_p->ps_pid; > > + unp->unp_flags |= UNP_FEIDSBIND; > > + VOP_UNLOCK(vp); > > +out: > > + KERNEL_UNLOCK(); > > + unp->unp_flags &= ~UNP_BINDING; > > + > > + return (error); > > } > > > > int > > @@ -724,98 +809,6 @@ unp_detach(struct unpcb *unp) > > } > > > > int > > -unp_bind(struct unpcb *unp, struct mbuf *nam, struct proc *p) > > -{ > > - struct sockaddr_un *soun; > > - struct mbuf *nam2; > > - struct vnode *vp; > > - struct vattr vattr; > > - int error; > > - struct nameidata nd; > > - size_t pathlen; > > - > > - if (unp->unp_flags & (UNP_BINDING | UNP_CONNECTING)) > > - return (EINVAL); > > - if (unp->unp_vnode != NULL) > > - return (EINVAL); > > - if ((error = unp_nam2sun(nam, &soun, &pathlen))) > > - return (error); > > - > > - unp->unp_flags |= UNP_BINDING; > > - > > - /* > > -* Enforce `i_lock' -> `unplock' because fifo subsystem > > -* requires
Re: Merge uipc_bind() with unp_bind()
On Mon, Nov 14, 2022 at 12:11:46PM +0300, Vitaliy Makkoveev wrote: > uipc_bind() only calls unp_bind(). Also it is the only caller of > unp_bind(). For *_bind() alone this looks like zapping a useless indirection, but the rest of uipc_*() seems to consistently use unp_*() still, so I'm not convinced this one-off merge is an improvement. > > Index: sys/kern/uipc_usrreq.c > === > RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v > retrieving revision 1.192 > diff -u -p -r1.192 uipc_usrreq.c > --- sys/kern/uipc_usrreq.c13 Nov 2022 16:01:32 - 1.192 > +++ sys/kern/uipc_usrreq.c14 Nov 2022 09:07:43 - > @@ -322,8 +322,93 @@ int > uipc_bind(struct socket *so, struct mbuf *nam, struct proc *p) > { > struct unpcb *unp = sotounpcb(so); > + struct sockaddr_un *soun; > + struct mbuf *nam2; > + struct vnode *vp; > + struct vattr vattr; > + int error; > + struct nameidata nd; > + size_t pathlen; > > - return unp_bind(unp, nam, p); > + if (unp->unp_flags & (UNP_BINDING | UNP_CONNECTING)) > + return (EINVAL); > + if (unp->unp_vnode != NULL) > + return (EINVAL); > + if ((error = unp_nam2sun(nam, &soun, &pathlen))) > + return (error); > + > + unp->unp_flags |= UNP_BINDING; > + > + /* > + * Enforce `i_lock' -> `unplock' because fifo subsystem > + * requires it. The socket can't be closed concurrently > + * because the file descriptor reference is still held. > + */ > + > + sounlock(unp->unp_socket); > + > + nam2 = m_getclr(M_WAITOK, MT_SONAME); > + nam2->m_len = sizeof(struct sockaddr_un); > + memcpy(mtod(nam2, struct sockaddr_un *), soun, > + offsetof(struct sockaddr_un, sun_path) + pathlen); > + /* No need to NUL terminate: m_getclr() returns zero'd mbufs. */ > + > + soun = mtod(nam2, struct sockaddr_un *); > + > + /* Fixup sun_len to keep it in sync with m_len. */ > + soun->sun_len = nam2->m_len; > + > + NDINIT(&nd, CREATE, NOFOLLOW | LOCKPARENT, UIO_SYSSPACE, > + soun->sun_path, p); > + nd.ni_pledge = PLEDGE_UNIX; > + nd.ni_unveil = UNVEIL_CREATE; > + > + KERNEL_LOCK(); > +/* SHOULD BE ABLE TO ADOPT EXISTING AND wakeup() ALA FIFO's */ > + error = namei(&nd); > + if (error != 0) { > + m_freem(nam2); > + solock(unp->unp_socket); > + goto out; > + } > + vp = nd.ni_vp; > + if (vp != NULL) { > + VOP_ABORTOP(nd.ni_dvp, &nd.ni_cnd); > + if (nd.ni_dvp == vp) > + vrele(nd.ni_dvp); > + else > + vput(nd.ni_dvp); > + vrele(vp); > + m_freem(nam2); > + error = EADDRINUSE; > + solock(unp->unp_socket); > + goto out; > + } > + VATTR_NULL(&vattr); > + vattr.va_type = VSOCK; > + vattr.va_mode = ACCESSPERMS &~ p->p_fd->fd_cmask; > + error = VOP_CREATE(nd.ni_dvp, &nd.ni_vp, &nd.ni_cnd, &vattr); > + vput(nd.ni_dvp); > + if (error) { > + m_freem(nam2); > + solock(unp->unp_socket); > + goto out; > + } > + solock(unp->unp_socket); > + unp->unp_addr = nam2; > + vp = nd.ni_vp; > + vp->v_socket = unp->unp_socket; > + unp->unp_vnode = vp; > + unp->unp_connid.uid = p->p_ucred->cr_uid; > + unp->unp_connid.gid = p->p_ucred->cr_gid; > + unp->unp_connid.pid = p->p_p->ps_pid; > + unp->unp_flags |= UNP_FEIDSBIND; > + VOP_UNLOCK(vp); > +out: > + KERNEL_UNLOCK(); > + unp->unp_flags &= ~UNP_BINDING; > + > + return (error); > } > > int > @@ -724,98 +809,6 @@ unp_detach(struct unpcb *unp) > } > > int > -unp_bind(struct unpcb *unp, struct mbuf *nam, struct proc *p) > -{ > - struct sockaddr_un *soun; > - struct mbuf *nam2; > - struct vnode *vp; > - struct vattr vattr; > - int error; > - struct nameidata nd; > - size_t pathlen; > - > - if (unp->unp_flags & (UNP_BINDING | UNP_CONNECTING)) > - return (EINVAL); > - if (unp->unp_vnode != NULL) > - return (EINVAL); > - if ((error = unp_nam2sun(nam, &soun, &pathlen))) > - return (error); > - > - unp->unp_flags |= UNP_BINDING; > - > - /* > - * Enforce `i_lock' -> `unplock' because fifo subsystem > - * requires it. The socket can't be closed concurrently > - * because the file descriptor reference is still held. > - */ > - > - sounlock(unp->unp_socket); > - > - nam2 = m_getclr(M_WAITOK, MT_SONAME); > - nam2->m_len = sizeof(struct sockaddr_un); > - memcpy(mtod(nam2, struct sockaddr_un *), soun, > - offsetof(struct sockaddr_un, sun_path) + pathlen); > - /* No need to NUL terminate: m_getclr() returns zero'd mbufs. */ > - > - soun = mtod(nam2, struct sockaddr_un *); > - > - /* Fixup sun_len to keep it in sync with m
Re: export {b,r}ootduid as sysctl, installer/sysupgrade improvements
On Tue, Sep 06, 2022 at 01:16:47AM +, Klemens Nanni wrote: > The installer considers a disk a root disk if 'a' is FFS and contains > expected files. > > Furthermore, unattended upgrades will always install to the first root > disk that is found. > > This works fine on machines with only one root disk, but it quickly > behaves unexpectedly when having multiple disks/installations in one > machine. > > I run such machines, esp. since fiddling with softraid and installboot. > > > The installer/sysupgrade experience can definitely be improved here, but > that takes some consideration. > > One requirement, imho, is knowing > 1. which disk we booted from, i.e. >from which disk the kernel (/bsd.rd or /bsd.upgrade) was loaded > 2. which disk the root filesystem is on, i.e. >likely the same disk holding /home where sysupgrade put the sets > > > The boot disk could be helpful inside installer, e.g. to check if > /bsd.ugpraded was booted from a valid root disk -- a good indicator for > rebooting from the same disk the user just ran sysupgrade on. > > The root disk is of no help inside the installer as that will always be > the ramdisk. But it could be used by sysupgrade to perhaps prefill > /auto_upgrade.conf to decide up-front which disk to upgrade. > This answer to the 'Which disk is the root disk' question is currently > answered inside the installer during unattended upgrades... and it will > always be the first valid root disk, which is not always correct. > > So to make progress, here's a diff that exports readily available > disklabel DUIDs: > > # disklabel sd0 | grep duid > duid: 98c0c47c3ffddeb4 > # sysctl hw | grep duid > hw.bootduid=98c0c47c3ffddeb4 > hw.rootduid=98c0c47c3ffddeb4 > > Having that, working out the installer/sysupgrade bits should be easier. > > I'm testing this on arm64 with two disks/installations. > > Feedback? Objection? OK? I'd like to get on with this, can also add sysctl.2 bits to document those before sending diffs using them. Mark asked whether CTLTYPE_QUAD would be more suited, but I still don't understand how that is supposed to work and there was no answer in this thread's other mail. Anyone? Index: sys/sys/sysctl.h === RCS file: /cvs/src/sys/sys/sysctl.h,v retrieving revision 1.231 diff -u -p -r1.231 sysctl.h --- sys/sys/sysctl.h7 Nov 2022 14:25:44 - 1.231 +++ sys/sys/sysctl.h9 Nov 2022 12:06:31 - @@ -946,7 +946,9 @@ struct kinfo_file { #defineHW_SMT 24 /* int: enable SMT/HT/CMT */ #defineHW_NCPUONLINE 25 /* int: number of cpus being used */ #defineHW_POWER26 /* int: machine has wall-power */ -#defineHW_MAXID27 /* number of valid hw ids */ +#defineHW_BOOTDUID 27 /* string: DUID of boot disk */ +#defineHW_ROOTDUID 28 /* string: DUID of root disk */ +#defineHW_MAXID29 /* number of valid hw ids */ #defineCTL_HW_NAMES { \ { 0, 0 }, \ @@ -976,6 +978,8 @@ struct kinfo_file { { "smt", CTLTYPE_INT }, \ { "ncpuonline", CTLTYPE_INT }, \ { "power", CTLTYPE_INT }, \ + { "bootduid", CTLTYPE_STRING }, \ + { "rootduid", CTLTYPE_STRING }, \ } /* Index: sys/kern/kern_sysctl.c === RCS file: /cvs/src/sys/kern/kern_sysctl.c,v retrieving revision 1.408 diff -u -p -r1.408 kern_sysctl.c --- sys/kern/kern_sysctl.c 7 Nov 2022 14:25:44 - 1.408 +++ sys/kern/kern_sysctl.c 9 Nov 2022 12:06:30 - @@ -770,6 +770,12 @@ hw_sysctl(int *name, u_int namelen, void case HW_SMT: return (sysctl_hwsmt(oldp, oldlenp, newp, newlen)); #endif + case HW_BOOTDUID: + return (sysctl_rdstring(oldp, oldlenp, newp, + duid_format(bootduid))); + case HW_ROOTDUID: + return (sysctl_rdstring(oldp, oldlenp, newp, + duid_format(rootduid))); default: return sysctl_bounded_arr(hw_vars, nitems(hw_vars), name, namelen, oldp, oldlenp, newp, newlen);
Merge uipc_bind() with unp_bind()
uipc_bind() only calls unp_bind(). Also it is the only caller of unp_bind(). Index: sys/kern/uipc_usrreq.c === RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v retrieving revision 1.192 diff -u -p -r1.192 uipc_usrreq.c --- sys/kern/uipc_usrreq.c 13 Nov 2022 16:01:32 - 1.192 +++ sys/kern/uipc_usrreq.c 14 Nov 2022 09:07:43 - @@ -322,8 +322,93 @@ int uipc_bind(struct socket *so, struct mbuf *nam, struct proc *p) { struct unpcb *unp = sotounpcb(so); + struct sockaddr_un *soun; + struct mbuf *nam2; + struct vnode *vp; + struct vattr vattr; + int error; + struct nameidata nd; + size_t pathlen; - return unp_bind(unp, nam, p); + if (unp->unp_flags & (UNP_BINDING | UNP_CONNECTING)) + return (EINVAL); + if (unp->unp_vnode != NULL) + return (EINVAL); + if ((error = unp_nam2sun(nam, &soun, &pathlen))) + return (error); + + unp->unp_flags |= UNP_BINDING; + + /* +* Enforce `i_lock' -> `unplock' because fifo subsystem +* requires it. The socket can't be closed concurrently +* because the file descriptor reference is still held. +*/ + + sounlock(unp->unp_socket); + + nam2 = m_getclr(M_WAITOK, MT_SONAME); + nam2->m_len = sizeof(struct sockaddr_un); + memcpy(mtod(nam2, struct sockaddr_un *), soun, + offsetof(struct sockaddr_un, sun_path) + pathlen); + /* No need to NUL terminate: m_getclr() returns zero'd mbufs. */ + + soun = mtod(nam2, struct sockaddr_un *); + + /* Fixup sun_len to keep it in sync with m_len. */ + soun->sun_len = nam2->m_len; + + NDINIT(&nd, CREATE, NOFOLLOW | LOCKPARENT, UIO_SYSSPACE, + soun->sun_path, p); + nd.ni_pledge = PLEDGE_UNIX; + nd.ni_unveil = UNVEIL_CREATE; + + KERNEL_LOCK(); +/* SHOULD BE ABLE TO ADOPT EXISTING AND wakeup() ALA FIFO's */ + error = namei(&nd); + if (error != 0) { + m_freem(nam2); + solock(unp->unp_socket); + goto out; + } + vp = nd.ni_vp; + if (vp != NULL) { + VOP_ABORTOP(nd.ni_dvp, &nd.ni_cnd); + if (nd.ni_dvp == vp) + vrele(nd.ni_dvp); + else + vput(nd.ni_dvp); + vrele(vp); + m_freem(nam2); + error = EADDRINUSE; + solock(unp->unp_socket); + goto out; + } + VATTR_NULL(&vattr); + vattr.va_type = VSOCK; + vattr.va_mode = ACCESSPERMS &~ p->p_fd->fd_cmask; + error = VOP_CREATE(nd.ni_dvp, &nd.ni_vp, &nd.ni_cnd, &vattr); + vput(nd.ni_dvp); + if (error) { + m_freem(nam2); + solock(unp->unp_socket); + goto out; + } + solock(unp->unp_socket); + unp->unp_addr = nam2; + vp = nd.ni_vp; + vp->v_socket = unp->unp_socket; + unp->unp_vnode = vp; + unp->unp_connid.uid = p->p_ucred->cr_uid; + unp->unp_connid.gid = p->p_ucred->cr_gid; + unp->unp_connid.pid = p->p_p->ps_pid; + unp->unp_flags |= UNP_FEIDSBIND; + VOP_UNLOCK(vp); +out: + KERNEL_UNLOCK(); + unp->unp_flags &= ~UNP_BINDING; + + return (error); } int @@ -724,98 +809,6 @@ unp_detach(struct unpcb *unp) } int -unp_bind(struct unpcb *unp, struct mbuf *nam, struct proc *p) -{ - struct sockaddr_un *soun; - struct mbuf *nam2; - struct vnode *vp; - struct vattr vattr; - int error; - struct nameidata nd; - size_t pathlen; - - if (unp->unp_flags & (UNP_BINDING | UNP_CONNECTING)) - return (EINVAL); - if (unp->unp_vnode != NULL) - return (EINVAL); - if ((error = unp_nam2sun(nam, &soun, &pathlen))) - return (error); - - unp->unp_flags |= UNP_BINDING; - - /* -* Enforce `i_lock' -> `unplock' because fifo subsystem -* requires it. The socket can't be closed concurrently -* because the file descriptor reference is still held. -*/ - - sounlock(unp->unp_socket); - - nam2 = m_getclr(M_WAITOK, MT_SONAME); - nam2->m_len = sizeof(struct sockaddr_un); - memcpy(mtod(nam2, struct sockaddr_un *), soun, - offsetof(struct sockaddr_un, sun_path) + pathlen); - /* No need to NUL terminate: m_getclr() returns zero'd mbufs. */ - - soun = mtod(nam2, struct sockaddr_un *); - - /* Fixup sun_len to keep it in sync with m_len. */ - soun->sun_len = nam2->m_len; - - NDINIT(&nd, CREATE, NOFOLLOW | LOCKPARENT, UIO_SYSSPACE, - soun->sun_path, p); - nd.ni_pledge = PLEDGE_UNIX; - nd.ni_unveil = UNVEIL_CREATE; - - KERNEL_LOCK(); -/* SHOULD BE ABLE TO ADOPT EXISTING AND wakeup() ALA FIFO's */ - error = namei(&nd); -
non regression tests for some packaging scripts
I'd like to add some non-regression tests for some scripts, most notably register-plist, which is... let's say not perfect at the moment. I believe ${PORTSDIR}/regress is probably the simplest least disturbing way to do that. Since the manpages already exist under src, I could also (somehow) put them in /usr/src/regress. Of course, this means requiring a checkout of the ports tree to check things. What do you people think ?