Re: uvm_pagelookup(): moar sanity checks
> Date: Fri, 11 Aug 2023 21:34:45 +0200 > From: Martin Pieuchot > > On 11/08/23(Fri) 20:41, Mark Kettenis wrote: > > > Date: Fri, 11 Aug 2023 20:12:19 +0200 > > > From: Martin Pieuchot > > > > > > Here's a simple diff to add some more sanity checks in uvm_pagelookup(). > > > > > > Nothing fancy, it helps documenting the flags and reduce the difference > > > with NetBSD. This is part of my on-going work on UVM. > > > > > > ok? > > > > NetBSD really has that extra blank line after the return? > > No. It's my mistake. ok kettenis@ > > > Index: uvm/uvm_page.c > > > === > > > RCS file: /cvs/src/sys/uvm/uvm_page.c,v > > > retrieving revision 1.172 > > > diff -u -p -r1.172 uvm_page.c > > > --- uvm/uvm_page.c13 May 2023 09:24:59 - 1.172 > > > +++ uvm/uvm_page.c11 Aug 2023 17:55:43 - > > > @@ -1219,10 +1219,16 @@ struct vm_page * > > > uvm_pagelookup(struct uvm_object *obj, voff_t off) > > > { > > > /* XXX if stack is too much, handroll */ > > > - struct vm_page pg; > > > + struct vm_page p, *pg; > > > + > > > + p.offset = off; > > > + pg = RBT_FIND(uvm_objtree, &obj->memt, &p); > > > + > > > + KASSERT(pg == NULL || obj->uo_npages != 0); > > > + KASSERT(pg == NULL || (pg->pg_flags & PG_RELEASED) == 0 || > > > + (pg->pg_flags & PG_BUSY) != 0); > > > + return (pg); > > > > > > - pg.offset = off; > > > - return RBT_FIND(uvm_objtree, &obj->memt, &pg); > > > } > > > > > > /* > > > > > > >
smr_grace_wait(): Skip halted CPUs
When stopping a machine, with "halt -p" for example, secondary CPUs are removed from the scheduler before smr_flush() is called. So there's no need for the SMR thread to peg itself to such CPUs. This currently isn't a problem because we use per-CPU runqueues but it doesn't work with a global one. So the diff below skip halted CPUs. It should also speed up rebooting/halting on machine with a huge number of CPUs. ok? Index: kern/kern_smr.c === RCS file: /cvs/src/sys/kern/kern_smr.c,v retrieving revision 1.16 diff -u -p -r1.16 kern_smr.c --- kern/kern_smr.c 14 Aug 2022 01:58:27 - 1.16 +++ kern/kern_smr.c 11 Aug 2023 19:43:54 - @@ -158,6 +158,8 @@ smr_grace_wait(void) CPU_INFO_FOREACH(cii, ci) { if (!CPU_IS_RUNNING(ci)) continue; + if (ci->ci_schedstate.spc_schedflags & SPCF_HALTED) + continue; if (READ_ONCE(ci->ci_schedstate.spc_smrgp) == smrgp) continue; sched_peg_curproc(ci);
Re: uvm_pagelookup(): moar sanity checks
On 11/08/23(Fri) 20:41, Mark Kettenis wrote: > > Date: Fri, 11 Aug 2023 20:12:19 +0200 > > From: Martin Pieuchot > > > > Here's a simple diff to add some more sanity checks in uvm_pagelookup(). > > > > Nothing fancy, it helps documenting the flags and reduce the difference > > with NetBSD. This is part of my on-going work on UVM. > > > > ok? > > NetBSD really has that extra blank line after the return? No. It's my mistake. > > Index: uvm/uvm_page.c > > === > > RCS file: /cvs/src/sys/uvm/uvm_page.c,v > > retrieving revision 1.172 > > diff -u -p -r1.172 uvm_page.c > > --- uvm/uvm_page.c 13 May 2023 09:24:59 - 1.172 > > +++ uvm/uvm_page.c 11 Aug 2023 17:55:43 - > > @@ -1219,10 +1219,16 @@ struct vm_page * > > uvm_pagelookup(struct uvm_object *obj, voff_t off) > > { > > /* XXX if stack is too much, handroll */ > > - struct vm_page pg; > > + struct vm_page p, *pg; > > + > > + p.offset = off; > > + pg = RBT_FIND(uvm_objtree, &obj->memt, &p); > > + > > + KASSERT(pg == NULL || obj->uo_npages != 0); > > + KASSERT(pg == NULL || (pg->pg_flags & PG_RELEASED) == 0 || > > + (pg->pg_flags & PG_BUSY) != 0); > > + return (pg); > > > > - pg.offset = off; > > - return RBT_FIND(uvm_objtree, &obj->memt, &pg); > > } > > > > /* > > > >
Re: uvm_pagelookup(): moar sanity checks
> Date: Fri, 11 Aug 2023 20:12:19 +0200 > From: Martin Pieuchot > > Here's a simple diff to add some more sanity checks in uvm_pagelookup(). > > Nothing fancy, it helps documenting the flags and reduce the difference > with NetBSD. This is part of my on-going work on UVM. > > ok? NetBSD really has that extra blank line after the return? > Index: uvm/uvm_page.c > === > RCS file: /cvs/src/sys/uvm/uvm_page.c,v > retrieving revision 1.172 > diff -u -p -r1.172 uvm_page.c > --- uvm/uvm_page.c13 May 2023 09:24:59 - 1.172 > +++ uvm/uvm_page.c11 Aug 2023 17:55:43 - > @@ -1219,10 +1219,16 @@ struct vm_page * > uvm_pagelookup(struct uvm_object *obj, voff_t off) > { > /* XXX if stack is too much, handroll */ > - struct vm_page pg; > + struct vm_page p, *pg; > + > + p.offset = off; > + pg = RBT_FIND(uvm_objtree, &obj->memt, &p); > + > + KASSERT(pg == NULL || obj->uo_npages != 0); > + KASSERT(pg == NULL || (pg->pg_flags & PG_RELEASED) == 0 || > + (pg->pg_flags & PG_BUSY) != 0); > + return (pg); > > - pg.offset = off; > - return RBT_FIND(uvm_objtree, &obj->memt, &pg); > } > > /* > >
uvm_pagelookup(): moar sanity checks
Here's a simple diff to add some more sanity checks in uvm_pagelookup(). Nothing fancy, it helps documenting the flags and reduce the difference with NetBSD. This is part of my on-going work on UVM. ok? Index: uvm/uvm_page.c === RCS file: /cvs/src/sys/uvm/uvm_page.c,v retrieving revision 1.172 diff -u -p -r1.172 uvm_page.c --- uvm/uvm_page.c 13 May 2023 09:24:59 - 1.172 +++ uvm/uvm_page.c 11 Aug 2023 17:55:43 - @@ -1219,10 +1219,16 @@ struct vm_page * uvm_pagelookup(struct uvm_object *obj, voff_t off) { /* XXX if stack is too much, handroll */ - struct vm_page pg; + struct vm_page p, *pg; + + p.offset = off; + pg = RBT_FIND(uvm_objtree, &obj->memt, &p); + + KASSERT(pg == NULL || obj->uo_npages != 0); + KASSERT(pg == NULL || (pg->pg_flags & PG_RELEASED) == 0 || + (pg->pg_flags & PG_BUSY) != 0); + return (pg); - pg.offset = off; - return RBT_FIND(uvm_objtree, &obj->memt, &pg); } /*
Re: bioctl: do not confirm new passphrases on stdin
On Wed, Aug 02, 2023 at 10:37:36AM +, Klemens Nanni wrote: > Creating new volumes prompts > Passphrase: > Re-type passphrase: > which is sane for interative usage, but -s (which omits prompts) to read > from stdin also prompts twice. > > I think that's neither intuitive nor ergonomical and as intended for > non-interactive scripts, -s should take a new passphase just once. > > Until a month ago, the manual errorneously said -s could not be used during > initial creation anyway, so I worry little about existing scripts like > > printf '%s\n%s\n' "$pw" "$pw" | bioctl -s -cC -lsd0a softraid0 > > Diff below makes -s create new volumes with a single passphase: > > # echo secret | bioctl -s -Cforce -cC -lvnd0a softraid0 > bioctl: Passphrases did not match > # echo secret | ./obj/bioctl -s -Cforce -cC -lvnd0a softraid0 > softraid0: CRYPTO volume attached as sd3 > > Feedback? Objection? OK? Ping. Index: bioctl.c === RCS file: /cvs/src/sbin/bioctl/bioctl.c,v retrieving revision 1.151 diff -u -p -r1.151 bioctl.c --- bioctl.c18 Oct 2022 07:04:20 - 1.151 +++ bioctl.c11 Aug 2023 15:42:41 - @@ -989,7 +989,7 @@ bio_kdf_generate(struct sr_crypto_kdfinf derive_key(kdfinfo->pbkdf.generic.type, kdfinfo->pbkdf.rounds, kdfinfo->maskkey, sizeof(kdfinfo->maskkey), kdfinfo->pbkdf.salt, sizeof(kdfinfo->pbkdf.salt), - "New passphrase: ", 1); + "New passphrase: ", rpp_flag == RPP_REQUIRE_TTY ? 1 : 0); } int
Re: installer: disk crypto: crank KDF rounds to hardware based default
> From: "Theo de Raadt" > Date: Fri, 11 Aug 2023 08:50:32 -0600 > > Mark Kettenis wrote: > > > > Date: Fri, 11 Aug 2023 11:13:23 + > > > From: Klemens Nanni > > > > > > On Mon, May 08, 2023 at 11:00:27AM +, Klemens Nanni wrote: > > > > On Sun, Apr 23, 2023 at 05:07:30PM +, Klemens Nanni wrote: > > > > > For new installs, it seems adequate to base the number on the actual > > > > > hardware, > > > > > assuming the CRYPTO volume will stay in that hardware for a while. > > > > > > > > > > The current default of 16 is from old PKCS5 PBKDF2 times and changing > > > > > it in > > > > > bioctl(8) is a more invasive change (for later, perhaps). > > > > > > > > > > Thoughts? Feedback from the crypto folks appreciated. > > > > > > > > > > On X230 and T14, 16 feels pretty instant, whereas 'auto' takes about > > > > > a second > > > > > on a T14. > > > > > > > > Ping. > > > > > > Anyone? > > > > > > I consider a hardware based value a saner default for new installations > > > (root disk volumes are most likely to stick around on the same machine) > > > than a decade old constant. > > > > See the recent discussion about _bcrypt_autorounds() in libc. > > > > System performance varies, and even on modern hardware it can provide > > varying results. The ramdisk environment is considerably different > > from the mult-user environment in this respect. > > > > Using a fixed value is way more predictable. If 16 no longer is a > > safe default it should be raised. > > I think this case is different, because the ramdisk has no process > contention. > > The code still sticks to minimum 16: > > if (r < 16) > r = 16; > > On faster machines, it will increase the rounds. For that machine, for > that disk configuration. This is processed early on to bring the disk up, > when there is little or no contention. So it will not have a regressive > performance impact. So the man page just lies?
Re: installer: disk crypto: crank KDF rounds to hardware based default
On Fri, Aug 11, 2023 at 03:51:38PM +0100, Stuart Henderson wrote: > Agreed. (Re bcrypt, I usually completely ignore auto rounds, I had just > forgotten to set that up on the machine where I noticed the problem..) > > Also, am I right in thinking that this only affects the time when > entering the passphrase when mounting or creating the device, i.e. once > per boot? Yes, correct. > > If so, there's nowhere near as much a downside to that being slow > as there is for user login. (anyone actually wanting to crack these > passphrases would be doing it on a fast system rather than whatever > the device is normally used with, so there are valid reasons for > picking something that might be a bit slow if it doesn't cause too > much system impact). The minimum of 16 can be cranked independently whilst still defaulting to a hardware based default.
Re: installer: disk crypto: crank KDF rounds to hardware based default
On 2023/08/11 16:43, Mark Kettenis wrote: > > Date: Fri, 11 Aug 2023 11:13:23 + > > From: Klemens Nanni > > > > On Mon, May 08, 2023 at 11:00:27AM +, Klemens Nanni wrote: > > > On Sun, Apr 23, 2023 at 05:07:30PM +, Klemens Nanni wrote: > > > > For new installs, it seems adequate to base the number on the actual > > > > hardware, > > > > assuming the CRYPTO volume will stay in that hardware for a while. > > > > > > > > The current default of 16 is from old PKCS5 PBKDF2 times and changing > > > > it in > > > > bioctl(8) is a more invasive change (for later, perhaps). > > > > > > > > Thoughts? Feedback from the crypto folks appreciated. > > > > > > > > On X230 and T14, 16 feels pretty instant, whereas 'auto' takes about a > > > > second > > > > on a T14. > > > > > > Ping. > > > > Anyone? > > > > I consider a hardware based value a saner default for new installations > > (root disk volumes are most likely to stick around on the same machine) > > than a decade old constant. > > See the recent discussion about _bcrypt_autorounds() in libc. > > System performance varies, and even on modern hardware it can provide > varying results. The ramdisk environment is considerably different > from the mult-user environment in this respect. > > Using a fixed value is way more predictable. If 16 no longer is a > safe default it should be raised. Agreed. (Re bcrypt, I usually completely ignore auto rounds, I had just forgotten to set that up on the machine where I noticed the problem..) Also, am I right in thinking that this only affects the time when entering the passphrase when mounting or creating the device, i.e. once per boot? If so, there's nowhere near as much a downside to that being slow as there is for user login. (anyone actually wanting to crack these passphrases would be doing it on a fast system rather than whatever the device is normally used with, so there are valid reasons for picking something that might be a bit slow if it doesn't cause too much system impact). > > >From bioctl(8): > > -r rounds > > The number of iterations for the KDF algorithm to use when > > converting a passphrase into a key, in order to create a new > > encrypted volume or change the passphrase of an existing > > encrypted volume. A larger number of iterations takes more > > time, > > but offers increased resistance against passphrase guessing > > attacks. If rounds is specified as auto, the number of rounds > > will be automatically determined based on system performance. > > Otherwise the minimum is 4 rounds and the default is 16. > > > > Rebased diff. > > > > Index: install.sub > > === > > RCS file: /cvs/src/distrib/miniroot/install.sub,v > > retrieving revision 1.1253 > > diff -u -p -r1.1253 install.sub > > --- install.sub 10 Aug 2023 17:09:34 - 1.1253 > > +++ install.sub 11 Aug 2023 11:02:19 - > > @@ -3097,7 +3097,7 @@ encrypt_root() { > > md_prep_fdisk $_chunk > > echo 'RAID *' | disklabel -w -A -T- $_chunk > > > > - until bioctl -Cforce -cC -l${_chunk}a softraid0 >/dev/null; do > > + until bioctl -Cforce -cC -rauto -l${_chunk}a softraid0 >/dev/null; do > > # Most likely botched passphrases, silently retry twice. > > ((++_tries < 3)) || exit > > done > > > > >
Re: installer: disk crypto: crank KDF rounds to hardware based default
Mark Kettenis wrote: > > Date: Fri, 11 Aug 2023 11:13:23 + > > From: Klemens Nanni > > > > On Mon, May 08, 2023 at 11:00:27AM +, Klemens Nanni wrote: > > > On Sun, Apr 23, 2023 at 05:07:30PM +, Klemens Nanni wrote: > > > > For new installs, it seems adequate to base the number on the actual > > > > hardware, > > > > assuming the CRYPTO volume will stay in that hardware for a while. > > > > > > > > The current default of 16 is from old PKCS5 PBKDF2 times and changing > > > > it in > > > > bioctl(8) is a more invasive change (for later, perhaps). > > > > > > > > Thoughts? Feedback from the crypto folks appreciated. > > > > > > > > On X230 and T14, 16 feels pretty instant, whereas 'auto' takes about a > > > > second > > > > on a T14. > > > > > > Ping. > > > > Anyone? > > > > I consider a hardware based value a saner default for new installations > > (root disk volumes are most likely to stick around on the same machine) > > than a decade old constant. > > See the recent discussion about _bcrypt_autorounds() in libc. > > System performance varies, and even on modern hardware it can provide > varying results. The ramdisk environment is considerably different > from the mult-user environment in this respect. > > Using a fixed value is way more predictable. If 16 no longer is a > safe default it should be raised. I think this case is different, because the ramdisk has no process contention. The code still sticks to minimum 16: if (r < 16) r = 16; On faster machines, it will increase the rounds. For that machine, for that disk configuration. This is processed early on to bring the disk up, when there is little or no contention. So it will not have a regressive performance impact. It will be very rare to use this in a non-ramdisk circumstance. It is different then the user 'password change' operation.
Re: installer: disk crypto: crank KDF rounds to hardware based default
> Date: Fri, 11 Aug 2023 11:13:23 + > From: Klemens Nanni > > On Mon, May 08, 2023 at 11:00:27AM +, Klemens Nanni wrote: > > On Sun, Apr 23, 2023 at 05:07:30PM +, Klemens Nanni wrote: > > > For new installs, it seems adequate to base the number on the actual > > > hardware, > > > assuming the CRYPTO volume will stay in that hardware for a while. > > > > > > The current default of 16 is from old PKCS5 PBKDF2 times and changing it > > > in > > > bioctl(8) is a more invasive change (for later, perhaps). > > > > > > Thoughts? Feedback from the crypto folks appreciated. > > > > > > On X230 and T14, 16 feels pretty instant, whereas 'auto' takes about a > > > second > > > on a T14. > > > > Ping. > > Anyone? > > I consider a hardware based value a saner default for new installations > (root disk volumes are most likely to stick around on the same machine) > than a decade old constant. See the recent discussion about _bcrypt_autorounds() in libc. System performance varies, and even on modern hardware it can provide varying results. The ramdisk environment is considerably different from the mult-user environment in this respect. Using a fixed value is way more predictable. If 16 no longer is a safe default it should be raised. > >From bioctl(8): > -r rounds > The number of iterations for the KDF algorithm to use when > converting a passphrase into a key, in order to create a new > encrypted volume or change the passphrase of an existing > encrypted volume. A larger number of iterations takes more time, > but offers increased resistance against passphrase guessing > attacks. If rounds is specified as auto, the number of rounds > will be automatically determined based on system performance. > Otherwise the minimum is 4 rounds and the default is 16. > > Rebased diff. > > Index: install.sub > === > RCS file: /cvs/src/distrib/miniroot/install.sub,v > retrieving revision 1.1253 > diff -u -p -r1.1253 install.sub > --- install.sub 10 Aug 2023 17:09:34 - 1.1253 > +++ install.sub 11 Aug 2023 11:02:19 - > @@ -3097,7 +3097,7 @@ encrypt_root() { > md_prep_fdisk $_chunk > echo 'RAID *' | disklabel -w -A -T- $_chunk > > - until bioctl -Cforce -cC -l${_chunk}a softraid0 >/dev/null; do > + until bioctl -Cforce -cC -rauto -l${_chunk}a softraid0 >/dev/null; do > # Most likely botched passphrases, silently retry twice. > ((++_tries < 3)) || exit > done > >
bioctl: default KDF rounds to hardware based value
Joel encouraged me to switch to '-r auto' by default sooner than later. The alternative installer diff on tech@ would thus be obsolete/a NOOP. If you do encrypted disk installs on one machine, but use them on another, or you want a specific number of rounds, just use '-r N' during creation or passphrase updates to change existing volumes (might be recommended to increase rounds on older volumes?). Diff below only flips the default so '-r' value behaviour does not change. Do we want a warning on '-r auto' usage to deprecate explicitly specifying the new default? Feedback? Objection? OK? Index: bioctl.8 === RCS file: /cvs/src/sbin/bioctl/bioctl.8,v retrieving revision 1.111 diff -u -p -r1.111 bioctl.8 --- bioctl.86 Jul 2023 21:08:50 - 1.111 +++ bioctl.811 Aug 2023 13:49:09 - @@ -282,11 +282,13 @@ passphrase into a key, in order to creat passphrase of an existing encrypted volume. A larger number of iterations takes more time, but offers increased resistance against passphrase guessing attacks. -If +By default, or if .Ar rounds -is specified as "auto", the number of rounds will be automatically determined +is specified as +.Cm auto , +the number of rounds will be automatically determined based on system performance. -Otherwise the minimum is 4 rounds and the default is 16. +The minimum is 4 rounds. .It Fl s Read the passphrase for the selected crypto volume from .Pa /dev/stdin Index: bioctl.c === RCS file: /cvs/src/sbin/bioctl/bioctl.c,v retrieving revision 1.151 diff -u -p -r1.151 bioctl.c --- bioctl.c18 Oct 2022 07:04:20 - 1.151 +++ bioctl.c11 Aug 2023 13:36:41 - @@ -89,7 +89,7 @@ int devh = -1; inthuman; intverbose; u_int32_t cflags = 0; -intrflag = 0; +intrflag = -1; /* auto */ char *password; void *bio_cookie;
Re: ldd: check read return value to avoid unitialized struct fields
Greg Steuck wrote: > Thanks for the patch. > > I could see some value in tightening the conditions to always check > `!= expected`. I don't see enough improvement from separating the error > case of -1 from the incomplete read case considering the otherwise > identical behavior. Like this? The int -> size_t promotion can be a different commit or left out. --- commit 7663fd702838ae390515cb9326c2706a57a2983b (ldd-read-rv) from: Lucas date: Fri Aug 11 11:43:32 2023 UTC Check for a full read. Don't use warn when errno might be unmodified. Promote size from int to size_t. M libexec/ld.so/ldd/ldd.c | 5+ 4- 1 file changed, 5 insertions(+), 4 deletions(-) diff 7d242c13afd19e56cc21befac2ce5cdc1ac4992b 7663fd702838ae390515cb9326c2706a57a2983b commit - 7d242c13afd19e56cc21befac2ce5cdc1ac4992b commit + 7663fd702838ae390515cb9326c2706a57a2983b blob - 9e8c5065cd843ff36d91efcb868b94ffd4c98365 blob + 16b59a75e63bfe894922c4195aa11cd5e75802a1 --- libexec/ld.so/ldd/ldd.c +++ libexec/ld.so/ldd/ldd.c @@ -96,7 +96,8 @@ doit(char *name) { Elf_Ehdr ehdr; Elf_Phdr *phdr; - int fd, i, size, status, interp=0; + size_t size; + int fd, i, status, interp=0; char buf[PATH_MAX]; struct stat st; void * dlhandle; @@ -118,8 +119,8 @@ doit(char *name) return 1; } - if (read(fd, &ehdr, sizeof(ehdr)) < 0) { - warn("read(%s)", name); + if (read(fd, &ehdr, sizeof(ehdr)) != sizeof(ehdr)) { + warnx("%s: incomplete ELF header", name); close(fd); return 1; } @@ -141,7 +142,7 @@ doit(char *name) size = ehdr.e_phnum * sizeof(Elf_Phdr); if (pread(fd, phdr, size, ehdr.e_phoff) != size) { - warn("read(%s)", name); + warnx("%s: incomplete program header", name); close(fd); free(phdr); return 1;
Re: installer: disk crypto: crank KDF rounds to hardware based default
On Mon, May 08, 2023 at 11:00:27AM +, Klemens Nanni wrote: > On Sun, Apr 23, 2023 at 05:07:30PM +, Klemens Nanni wrote: > > For new installs, it seems adequate to base the number on the actual > > hardware, > > assuming the CRYPTO volume will stay in that hardware for a while. > > > > The current default of 16 is from old PKCS5 PBKDF2 times and changing it in > > bioctl(8) is a more invasive change (for later, perhaps). > > > > Thoughts? Feedback from the crypto folks appreciated. > > > > On X230 and T14, 16 feels pretty instant, whereas 'auto' takes about a > > second > > on a T14. > > Ping. Anyone? I consider a hardware based value a saner default for new installations (root disk volumes are most likely to stick around on the same machine) than a decade old constant. >From bioctl(8): -r rounds The number of iterations for the KDF algorithm to use when converting a passphrase into a key, in order to create a new encrypted volume or change the passphrase of an existing encrypted volume. A larger number of iterations takes more time, but offers increased resistance against passphrase guessing attacks. If rounds is specified as auto, the number of rounds will be automatically determined based on system performance. Otherwise the minimum is 4 rounds and the default is 16. Rebased diff. Index: install.sub === RCS file: /cvs/src/distrib/miniroot/install.sub,v retrieving revision 1.1253 diff -u -p -r1.1253 install.sub --- install.sub 10 Aug 2023 17:09:34 - 1.1253 +++ install.sub 11 Aug 2023 11:02:19 - @@ -3097,7 +3097,7 @@ encrypt_root() { md_prep_fdisk $_chunk echo 'RAID *' | disklabel -w -A -T- $_chunk - until bioctl -Cforce -cC -l${_chunk}a softraid0 >/dev/null; do + until bioctl -Cforce -cC -rauto -l${_chunk}a softraid0 >/dev/null; do # Most likely botched passphrases, silently retry twice. ((++_tries < 3)) || exit done