Re: missing warning in wireguard manual page

2022-07-25 Thread Philip Guenther
On Mon, Jul 25, 2022 at 7:20 AM Theo de Raadt  wrote:

> I've been watching conversation on a mailing list, and it leads me to
> wonder if we should inform the userbase better.
>

Too true.  Certification *is* the key thing that protects users, not
careful, well engineered designs.

We should be giving this warning in many other places too; for example:

 Index: stdlib/malloc.3
===
RCS file: /data/src/openbsd/src/lib/libc/stdlib/malloc.3,v
retrieving revision 1.129
diff -u -p -r1.129 malloc.3
--- stdlib/malloc.3 31 Mar 2022 17:27:16 -  1.129
+++ stdlib/malloc.3 25 Jul 2022 20:00:07 -
@@ -766,6 +766,11 @@ and
 functions appeared in
 .Ox 6.6 .
 .Sh CAVEATS
+Layout randomization in
+.Nm malloc
+uses uncertified random number generators,
+so the security properties cannot be guaranteed.
+.Pp
 When using
 .Fn malloc ,
 be wary of signed integer and


Re: echo(1): check for stdio errors

2022-07-10 Thread Philip Guenther
On Sun, Jul 10, 2022 at 1:08 PM Scott Cheloha 
wrote:

> ok?
>
> Index: echo.c
> ===
> RCS file: /cvs/src/bin/echo/echo.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 echo.c
> --- echo.c  9 Oct 2015 01:37:06 -   1.10
> +++ echo.c  10 Jul 2022 22:00:18 -
> @@ -53,12 +53,15 @@ main(int argc, char *argv[])
> nflag = 0;
>
> while (*argv) {
> -   (void)fputs(*argv, stdout);
> -   if (*++argv)
> -   putchar(' ');
> +   if (fputs(*argv, stdout) == EOF)
> +   err(1, "stdout");
> +   if (*++argv && putchar(' ') == EOF)
> +   err(1, "stdout");
> }
> if (!nflag)
> putchar('\n');
> +   if (fclose(stdout) == EOF)
> +   err(1, "stdout");
>
> return 0;
>  }
>
>
Three thoughts:
1) Since stdio errors are sticky, is there any real advantage to checking
each call instead of just checking the final fclose()?
2) It is claimed that POSIX requires *all* standard utilities to return
failure if any writes to stdout failed.  If it's important to fix this in
'echo', can we sketch out how we might fix make fixing this in the other
utilities easier?  I believe at least some chunk of the GNU utilities now
use an atexit() handler to do that; is that a reasonable idea and if not
can we come up with something better?
3) It would be no end of surprise and frustration for the external echo
utility to behave differently than the sh builtin.

Philip


Re: start unlocking kbind(2)

2022-06-15 Thread Philip Guenther
On Mon, 13 Jun 2022, Theo de Raadt wrote:
> Scott Cheloha  wrote:
> > > Am I wrong that kbind is never called twice in the same address space?
> >
> > Isn't this exactly what happened the last time we tried this?
>
> Tried what?  kbind has never been NOLOCK.

Scott's referring to rev 1.237 of kern_fork.c, where we tried to require
the first use of kbind be before __tfork(2) was called.  This blew up
because there's at least two ways to legitimately call pthread_create(3)
before doing your first lazy binding:

 1) build with -znow, then dlopen() something not linked with -znow after
calling pthread_create()

 2) pass address of pthread_create() to another function which calls
through the pointer, no special link options required (just need to
split up the _create and *funcptr enough that the compiler
won't optimize to a direct call)


I've thought about how to possibly force a lazy resolution and haven't
thought up anything that wasn't unmaintainable, and probably
unimplementable.


So, what could work reliably?

My first thought would be to have ld.so's _dl_boot() do the equivalent of
struct __kbind kb = {
.kb_addr = _syscall_in_dl_bind;
.kb_size = 0;
};
kbind(, sizeof kb, pcookie)

...after teaching the kernel to accept such a first call to kbind and set
pr->ps_kbind_* from them.  That would permit the reimposing of the "first
kbind can't be after __tfork" restriction; is it indeed enough to permit
the ps_kbind_* members without locks?


Another idea is to discard the cookie restriction and just trust the
calling address to be enough.  I mean, we trust it to be sufficient
for sigreturn and that's perhaps more powerful, no?  If we're fine with
that, then how about giving ld.so a PT_OPENBSD_KBIND segment with the
required address, so the kernel can just set it up at exec time?
Eventually, we could disable kbind if said header wasn't present on the ELF
interpreter.

(In a groteque abuse of the ELF program header, we *could* keep the cookie
by, for example, having the PT_OPENBSD_KBIND p_vaddr/p_memsz identify the
location of the random cookie in ld.so memory and pass the kbind syscall
address via p_paddr.  You are permitted to barf.)


Philip


Re: Picky, but much more efficient arc4random_uniform!

2022-05-17 Thread Philip Guenther
On Tue, May 17, 2022 at 1:10 PM Steffen Nurpmeso  wrote:

> Joerg Sonnenberger wrote in
>  :
>  |Am Fri, May 13, 2022 at 09:43:26AM -0500 schrieb Luke Small:
>  |> I made a couple new versions of a new kind of arc4random_uniform-like
>  ...
>  |If your main use case is limiting the amount of cryptography when using
>  |small bounds, there is a much simpler approach to be taken here. For
>  |boundaries below 256, use arc4random_buf to extract one byte if bound is
>  |a power of two, otherwise two. This gives most of the performance
>  |benefit without complicating the algorithm. Extracting two bytes ensures
>  |that the propability of success is > 99% and the double extracting
>  |doesn't eat up the benefits.
>
> You can use (really implemented) _buf() if you need a 8-bit or
> 16-bit etc number.
>
> I find that _uniform() often makes no difference to a simple
> modulo because like the comment in _uniform() says "p > 0.5 (worst

case, usually far better", and usually RNGs sprinkle bits nicely,
>

What does that statement mean?  You seem to be saying "module is uniform,
except when it isn't, which could be almost half the time for some cases,
but when it's uniform it's uniform, so why bother making it actually
correct and dependable".

I mean, what does that _mean_???   It's as if I said "my text handling
program handles all characters uniformly, except those with accents, but
that's less than 10% of the characters I type, so it handles all characters
uniformly."  WTF, NO!



> 0 bytes "do not occur", so a 32-bit RNG value "is" >=0x01FF in
> most cases for "my RNG" (of 10 803/759/793 NOT; 776/805/793
> NOT for Linux getrandom(2)), which is a pretty high cut off.
> Using _uniform() just because of its name seems strange thus.
>

Where do these ideas come from, that "0 bytes 'do not occur'"??  If your
rand generator doesn't provide zero bytes at the expected frequency, you
know, 1 in 256, then you're using a garbage random number generator.
Please stop making such suggestions here because THEY ARE NOT TRUE ABOUT
OPENBSD.  Do ya'll not bother to test the claims that you make?

: bleys; cat f.c
#include 
#include 

int
main(void)
{
uint32_t u;
long count;

count = 0;
while ((u = arc4random()) > 0x1ff)
count++;
printf("%08x\t%ld\n", u, count);

count = 0;
for (;;) {
u = arc4random();
if ((u & 0xff00) == 0 ||
(u & 0x00ff) == 0 ||
(u & 0xff00) == 0 ||
(u & 0x00ff) == 0)
break;
    count++;
}
printf("%08x\t%ld\n", u, count);

return 0;
}
: bleys; cc f.c
: bleys; ./a.out
00b82e5c58
ab47880036
: bleys;


Philip Guenther


Re: Picky, but much more efficient arc4random_uniform!

2022-05-15 Thread Philip Guenther
On Sun, 15 May 2022, Luke Small wrote:
> The current implementation is nothing more than a naive arc4random() %
> upper_bound which trashes initial arc4random() calls it doesn’t like, then
> transforms over a desired modulus. The whole transformation by modulus of
> perfectly decent random data seems so awkward. It’s not like it is used as
> some majestic artistry of RSA it seems like an ugly HACK to simply meet a
> demand lacking of something better.

You fail to mention correctness at all or address the fact that your 
version isn't while the current one is.  Meanwhile, you talk about getting 
only just enough random data as if there's some sort of limited supply 
when there isn't.

"My version may be wrong, but at least it doesn't look naive!"

That is utterly the wrong attitude for OpenBSD code.


Best wishes.

Philip Guenther



Re: Picky, but much more efficient arc4random_uniform!

2022-05-14 Thread Philip Guenther
On Sun, 15 May 2022, Steffen Nurpmeso wrote:
> Stuart Henderson wrote in
...
>  |what's the perceived problem you're wanting to solve? and does that
>  |problem actually exist in the first place?
> 
> The problem is that if have a low upper bound then modulo will "remove a 
> lot of randomization".  For example if you have a program which 
> generates Lotto numbers (1..49), then using _uniform() as it is will 
> generate many duplicates.

Wut.  The *WHOLE POINT* of arc4random_uniform() is that it has uniform 
distribution.  Says right so in the manpage.  If an implementation of that 
API fails to do that, it's a broken implementation.

The OpenBSD implementation achieves that.  NetBSD's implementation has the 
same core logic.  Here's my quick lotto pick demo program, doing 490 
picks, so they should all be somewhere near 10:

#include 
#include 
#define LIMIT   49
int
main(void)
{
unsigned counts[LIMIT] = { 0 };
int i;
for (i = 0; i < 10 * LIMIT; i++)
counts[arc4random_uniform(LIMIT)]++;
for (i = 0; i < LIMIT; i++)
printf("%2d\t%7u\n", i+1, counts[i]);
return 0;
}

And a sample run:

: bleys; ./a.out
   
 1   100100
 2   100639
 399965
 499729
 599641
 699650
 7   100299
 8   100164
 999791
10   100101
11   100657
12   100042
1399661
1499927
1599426
1699491
1799646
18   100133
19   100013
2099942
2199873
2299924
2399567
24   100152
25   100688
26   100011
27   100481
2899980
29   100406
3099726
3199808
3299929
33   100050
3499983
35   100048
3699771
3799906
38   100215
39   100261
40   100426
4199847
4299533
43   100368
4499695
45   100041
46   100465
4799875
48   100034
4999920
: bleys; 

Looks pretty good to me, with repeated runs showing the values bouncing around.


...
> I was a bit interested when i saw Luke's message, but i am no
> mathematician and, like i said, i in fact never needed the
> functionality.  And the question i would have is how small
> upper_bound has to be for the modulo problem to really kick in.

If the implementation isn't broken, it's not a problem, period.


> And even if, whether it matters.  For example, if you have
> a bitset of 1024 "in-flight things", and the small upper bound
> hits a used slot, you could simply search forward until you find
> a free one.  I mean, with 1024 possible values the number of
> possibilities is anyhow restricted.

Well hey, let's give Luke's a try and see how uniform it is:


#include 
#include 
#define LIMIT   49
int
main(void)
{
unsigned counts[LIMIT] = { 0 };
unsigned counts2[LIMIT] = { 0 };
int i;
for (i = 0; i < 10 * LIMIT; i++) {
counts[arc4random_uniform(LIMIT)]++;
counts2[arc4random_uniform_fast_simple(LIMIT)]++;
}
for (i = 0; i < LIMIT; i++)
printf("%2d\t%7u\t%7u\n", i+1, counts[i], counts2[i]);
return 0;
}

: bleys; ./a.out  
 1   100188   76502
 299983   76602
 3   100521   76522
 4   100416   76682
 5   100171   76387
 6   100163   76759
 7   100024   76336
 8   19   76703
 999769   76237
1099892   76532
11   100197   76730
12   100483   76398
1399769   76310
14   100075   76474
1599781   76599
1699846   76439
1799814   76430
18   100313   76648
19   100259   76813
2099885   77068
21   100302   76546
228   76698
2399491   76678
24   100340   76324
2599763  115263
2699872  153008
27   100022  152979
2899481  153793
29   100018  210714
3099617  229286
31   100167  297003
32   100270  449664
33   100468   76790
3499115   76452
3599921   76392
3699862   76140
37   100485   76607
38   100029   75885
3999577   76498
4099479   76727
41   100139   76746
42   100883   76698
43   100102   76474
4499801   76592
45   100117   76124
4699678   76417
4799770   76639
4899524   77034
49   100151   76658
: bleys; 

Wow, that last column is *bad*.  Repeated runs show 25-32 are consistently 
high, 32 being *outrageously* off, and the others are low.

Luke's implementation does not correctly implement the API.  Doesn't 
matter if it's a million times faster when it doesn't deliver the goal.


Philip Guenther



Re: stop using mquery(2) inside realloc(3)

2022-05-14 Thread Philip Guenther
On Sat, 14 May 2022, Philip Guenther wrote:
> On Sat, 14 May 2022, Theo de Raadt wrote:
> > I worry a little about having libc use an undocumented mmap(2) flag.
> > About as much as using mquery, which is non-standard.
> > 
> > Maybe __MAP_NOREPLACE should get documentation?  __MAP_NOFAULT is in the
> > same situation.  The behaviour of these flags should be documented
> > (set in stone), which may also discourage accidental behaviour changes
> > by kernel developers in the future?
> 
> To throw some spaghetti at the wall...

Fix the grammar of the __MAP_NOFAULT description and mention signals there.

Index: sys/mmap.2
===
RCS file: /data/src/openbsd/src/lib/libc/sys/mmap.2,v
retrieving revision 1.68
diff -u -p -r1.68 mmap.2
--- sys/mmap.2  31 Mar 2022 17:27:16 -  1.68
+++ sys/mmap.2  14 May 2022 19:31:28 -
@@ -58,8 +58,19 @@ The
 argument describes the address where the system should place the mapping.
 If the
 .Dv MAP_FIXED
-flag is specified, the allocation will happen at the specified address,
+flag is specified and the
+.Dv __MAP_NOREPLACE
+flag is not specified,
+the allocation will happen at the specified address,
 replacing any previously established mappings in its range.
+If both the
+.Dv MAP_FIXED
+and
+.Dv __MAP_NOREPLACE
+flags are specified,
+the allocation will happen at the specified address,
+failing with no effect if any previously established mappings are
+in its range.
 Otherwise, the mapping will be placed at the available spot at
 .Fa addr ;
 failing that it will be placed "close by".
@@ -153,6 +164,18 @@ mappings)
 must be multiples of the page size.
 Existing mappings in the address range will be replaced.
 Use of this option is discouraged.
+.It Dv __MAP_NOFAULT
+Indicate that access to pages that are not backed by the mapped
+file or device will result in zero-filled anonymous pages being
+provided instead of a signal being delivered.
+This flag must not be used with an anonymous mapping.
+.It Dv __MAP_NOREPLACE
+Indicates that a
+.Dv MAP_FIXED
+mapping should fail if it would require replacing any existing
+mappings.
+This flag must be used in combination with
+.Dv MAP_FIXED .
 .It Dv MAP_STACK
 Indicate that the mapping is used as a stack.
 This flag must be used in combination with
@@ -278,6 +301,12 @@ device does not support memory mapping.
 The allocation
 .Fa len
 was 0.
+.It Bq Er EINVAL
+.Dv __MAP_NOFAULT
+was specified for an anonymous mapping or
+.Dv __MAP_NOREPLACE
+was specified without
+.Dv MAP_FIXED .
 .It Bq Er ENOMEM
 .Dv MAP_FIXED
 was specified and the
@@ -323,6 +352,14 @@ A fully functional
 system call first appeared in SunOS 4.0
 and has been available since
 .Bx 4.3 Net/2 .
+The
+.Dv __MAP_NOFAULT
+flag appeared in
+.Ox 5.7 .
+The
+.Dv __MAP_NOREPLACE
+flag appeared in
+.Ox 5.3 .
 .Sh CAVEATS
 .St -p1003.1-2008
 specifies that references to pages beyond the end of a mapped object



Re: stop using mquery(2) inside realloc(3)

2022-05-14 Thread Philip Guenther
On Sat, 14 May 2022, Theo de Raadt wrote:
> I worry a little about having libc use an undocumented mmap(2) flag.
> About as much as using mquery, which is non-standard.
> 
> Maybe __MAP_NOREPLACE should get documentation?  __MAP_NOFAULT is in the
> same situation.  The behaviour of these flags should be documented
> (set in stone), which may also discourage accidental behaviour changes
> by kernel developers in the future?

To throw some spaghetti at the wall...

Index: sys/mmap.2
===
RCS file: /data/src/openbsd/src/lib/libc/sys/mmap.2,v
retrieving revision 1.68
diff -u -p -r1.68 mmap.2
--- sys/mmap.2  31 Mar 2022 17:27:16 -  1.68
+++ sys/mmap.2  14 May 2022 19:06:00 -
@@ -58,8 +58,19 @@ The
 argument describes the address where the system should place the mapping.
 If the
 .Dv MAP_FIXED
-flag is specified, the allocation will happen at the specified address,
+flag is specified and the
+.Dv __MAP_NOREPLACE
+flag is not specified,
+the allocation will happen at the specified address,
 replacing any previously established mappings in its range.
+If both the
+.Dv MAP_FIXED
+and
+.Dv __MAP_NOREPLACE
+flags are specified,
+the allocation will happen at the specified address,
+failing with no effect if any previously established mappings are
+in its range.
 Otherwise, the mapping will be placed at the available spot at
 .Fa addr ;
 failing that it will be placed "close by".
@@ -153,6 +164,17 @@ mappings)
 must be multiples of the page size.
 Existing mappings in the address range will be replaced.
 Use of this option is discouraged.
+.It Dv __MAP_NOFAULT
+Indicate that access to pages that are not backed by the mapped
+file or device will be replaced by zero-filled anonymous pages.
+This flag must not be used with an anonymous mapping.
+.It Dv __MAP_NOREPLACE
+Indicates that a
+.Dv MAP_FIXED
+mapping should fail if it would require replacing any existing
+mappings.
+This flag must be used in combination with
+.Dv MAP_FIXED .
 .It Dv MAP_STACK
 Indicate that the mapping is used as a stack.
 This flag must be used in combination with
@@ -278,6 +300,12 @@ device does not support memory mapping.
 The allocation
 .Fa len
 was 0.
+.It Bq Er EINVAL
+.Dv __MAP_NOFAULT
+was specified for an anonymous mapping or
+.Dv __MAP_NOREPLACE
+was specified without
+.Dv MAP_FIXED .
 .It Bq Er ENOMEM
 .Dv MAP_FIXED
 was specified and the
@@ -323,6 +351,14 @@ A fully functional
 system call first appeared in SunOS 4.0
 and has been available since
 .Bx 4.3 Net/2 .
+The
+.Dv __MAP_NOFAULT
+flag appeared in
+.Ox 5.7 .
+The
+.Dv __MAP_NOREPLACE
+flag appeared in
+.Ox 5.3 .
 .Sh CAVEATS
 .St -p1003.1-2008
 specifies that references to pages beyond the end of a mapped object



stop using mquery(2) inside realloc(3)

2022-05-13 Thread Philip Guenther
If you try to grow a 'large' (at least half a page) allocation with 
realloc(3), it'll try to extend the memory mapping for it and if that 
works it won't need to move it.

Currently, it does that by using mquery(2) to see if that area is 
available and if so then trying to mmap it, munmaping it if mmap(2) didn't 
return the desired address (perhaps due to a race with another thread).  
Instead of doing mquery/mmap/munmap, this path can use the MAP_FIXED and 
__MAP_NOREPLACE flags to directly request the specific address but not 
change anything if it's not available.


(We still use mquery in ld.so on i386 as a performance optimization, but 
that use case differs in needing to find a base address for *multiple* 
mappings, where unrolling partial hits grows to be more expensive than 
probing with mquery and only trying to do all the mapping on a successful 
set of probes: recall that on x86 munmap() is more expensive than mmap() 
due to TLB shootdowns. This case in realloc() only has a single mapping to 
probe/extend so it avoids those costs.  Indeed, this diff eliminates the 
current "munmap on failed attempt" path/cost.)


The regress/lib/libc/malloc tests continue to pass on my amd64 box, with 
ktrace confirming the change in calls.

oks?


Philip


Index: stdlib/malloc.c
===
RCS file: /data/src/openbsd/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.273
diff -u -p -r1.273 malloc.c
--- stdlib/malloc.c 26 Feb 2022 16:14:42 -  1.273
+++ stdlib/malloc.c 14 May 2022 02:35:04 -
@@ -100,9 +100,6 @@
 #define MMAPA(a,sz,f)  mmap((a), (sz), PROT_READ | PROT_WRITE, \
 MAP_ANON | MAP_PRIVATE | (f), -1, 0)
 
-#define MQUERY(a,sz,f) mquery((a), (sz), PROT_READ | PROT_WRITE, \
-MAP_ANON | MAP_PRIVATE | MAP_FIXED | (f), -1, 0)
-
 struct region_info {
void *p;/* page; low bits used to mark chunks */
uintptr_t size; /* size for pages, or chunk_info pointer */
@@ -1687,11 +1684,7 @@ orealloc(struct dir_info **argpool, void
size_t needed = rnewsz - roldsz;
 
STATS_INC(pool->cheap_realloc_tries);
-   q = MQUERY(hint, needed, pool->mmap_flag);
-   if (q == hint)
-   q = MMAPA(hint, needed, 
pool->mmap_flag);
-   else
-   q = MAP_FAILED;
+   q = MMAPA(hint, needed, MAP_FIXED | 
__MAP_NOREPLACE | pool->mmap_flag);
if (q == hint) {
STATS_ADD(pool->malloc_used, needed);
if (pool->malloc_junk == 2)
@@ -1709,9 +1702,6 @@ orealloc(struct dir_info **argpool, void
STATS_INC(pool->cheap_reallocs);
ret = p;
goto done;
-   } else if (q != MAP_FAILED) {
-   if (munmap(q, needed))
-   wrterror(pool, "munmap %p", q);
}
}
} else if (rnewsz < roldsz) {



eliminate use of fseek(3) inside libc

2022-05-13 Thread Philip Guenther
fseek(3) is implemented by just calling fseeko(3), the POSIX addition 
version that takes an off_t instead of a long:

int
fseek(FILE *fp, long offset, int whence)
{
return (fseeko(fp, offset, whence));
}

...so there's no reason for libc to be using fseek() internally, as that 
just loses range on ILP32 platforms and adds that (tail)call overhead.

Diff below converts the last three such uses** and then updates the symbol 
naming for fseek to block attempts to reference it in the future so uses 
don't accidentally crawl back in.

ok?


Philip

** theoretically fixing the handling of xdr_setpos() to a position
beyond 2GB on an xdrstdio, I guess



Index: gen/getcap.c
===
RCS file: /data/src/openbsd/src/lib/libc/gen/getcap.c,v
retrieving revision 1.35
diff -u -p -r1.35 getcap.c
--- gen/getcap.c3 Jul 2019 03:24:04 -   1.35
+++ gen/getcap.c23 Dec 2019 23:21:11 -
@@ -244,7 +244,7 @@ getent(char **cap, u_int *len, char **db
 * Open database if not already open.
 */
if (fp != NULL) {
-   (void)fseek(fp, 0L, SEEK_SET);
+   fseeko(fp, 0, SEEK_SET);
myfd = 0;
opened++;
} else {
Index: rpc/xdr_stdio.c
===
RCS file: /data/src/openbsd/src/lib/libc/rpc/xdr_stdio.c,v
retrieving revision 1.16
diff -u -p -r1.16 xdr_stdio.c
--- rpc/xdr_stdio.c 14 Feb 2022 03:38:59 -  1.16
+++ rpc/xdr_stdio.c 14 May 2022 01:40:56 -
@@ -144,7 +144,7 @@ static bool_t
 xdrstdio_setpos(XDR *xdrs, u_int pos)
 { 
 
-   return ((fseek((FILE *)xdrs->x_private, (long)pos, SEEK_SET) == -1) ?
+   return ((fseeko((FILE *)xdrs->x_private, pos, SEEK_SET) == -1) ?
FALSE : TRUE);
 }
 
Index: stdio/rewind.c
===
RCS file: /data/src/openbsd/src/lib/libc/stdio/rewind.c,v
retrieving revision 1.6
diff -u -p -r1.6 rewind.c
--- stdio/rewind.c  31 Aug 2015 02:53:57 -  1.6
+++ stdio/rewind.c  23 Dec 2019 23:30:22 -
@@ -37,7 +37,7 @@
 void
 rewind(FILE *fp)
 {
-   (void) fseek(fp, 0L, SEEK_SET);
+   fseeko(fp, 0, SEEK_SET);
clearerr(fp);
errno = 0;  /* not required, but seems reasonable */
 }
Index: hidden/stdio.h
===
RCS file: /data/src/openbsd/src/lib/libc/hidden/stdio.h,v
retrieving revision 1.7
diff -u -p -r1.7 stdio.h
--- hidden/stdio.h  6 Sep 2016 19:56:36 -   1.7
+++ hidden/stdio.h  14 May 2022 02:01:51 -
@@ -65,7 +65,7 @@ PROTO_NORMAL(fputs);
 PROTO_NORMAL(fread);
 PROTO_NORMAL(freopen);
 PROTO_NORMAL(fscanf);
-PROTO_NORMAL(fseek);
+PROTO_STD_DEPRECATED(fseek);
 PROTO_NORMAL(fseeko);
 PROTO_NORMAL(fsetpos);
 PROTO_NORMAL(ftell);
Index: stdio/fseek.c
===
RCS file: /data/src/openbsd/src/lib/libc/stdio/fseek.c,v
retrieving revision 1.13
diff -u -p -r1.13 fseek.c
--- stdio/fseek.c   28 Jun 2019 13:32:42 -  1.13
+++ stdio/fseek.c   14 May 2022 02:00:03 -
@@ -250,4 +250,3 @@ fseek(FILE *fp, long offset, int whence)
 {
return (fseeko(fp, offset, whence));
 }
-DEF_STRONG(fseek);



Re: VMM avoid duplication and reduce atack surface with octboot(4)

2022-03-22 Thread Philip Guenther
On Tue, Mar 22, 2022 at 6:04 PM Alexis  wrote:
>
> english is not my native language my point is octboot good be used to
> make openbsd the bootloader on vmm therefore no need to maintaine other
> upstream stacks, therefore avoiding duplication and reducing atack surface
> like it was done on that project explained in that CCC talk.

The responses you're getting are driven by the fact that you appear to
ascribe to octboot(4/octeon) the functionality to replace an x86 BIOS.
octboot(4/octeon) is a driver (chunk of code) in the MIPS64 version of
the OpenBSD kernel which lets an *already running* kernel replace
itself with another kernel.  An x86 BIOS is the code which bootstraps
an x86 with particular memory and I/O setups, providing early services
for access to that I/O and memory such that other stuff, like OpenBSD
boot loaders, can themselves be invoked.
These are completely different functions and neither can replace the other.

I'm not sure what information led you to understand these to be
similar functions, but boot sequences are among the most...specific
pieces of system knowledge and while some people certainly start their
approach from that angle, I suspect most people dig into that only
after time spent on more portable (and frankly, usable) levels.  Not
knowing what your interests and energy are in, I can give no specific
recommendation.


Philip Guenther



Re: Provide memory barriers in refcnt_rele() and refcnt_finalize()

2022-03-14 Thread Philip Guenther
On Mon, Mar 14, 2022 at 12:47 AM Visa Hankala  wrote:

> On Sun, Mar 13, 2022 at 06:26:19PM -0700, Philip Guenther wrote:
> > On Sun, Mar 13, 2022 at 10:27 AM Visa Hankala  wrote:
> >
> > > On Sun, Mar 13, 2022 at 04:29:44PM +0100, Mark Kettenis wrote:
> > >
> > ...
> >
> > > > Under what circumstances does memory ordering matter for these
> > > > interfaces?
> > >
> > > Consider the following scenario:
> > >
> > > struct widget {
> > > struct refcnt   w_refcnt;
> > > /* more fields spanning many cache lines */
> > > ...
> > > int w_var;
> > > };
> > >
> > > First, CPU 1 executes:
> > >
> > > w->w_var = 1;
> > > refcnt_rele(>w_refcnt);  /* remains above zero */
> > >
> >
> > Having incremented the refcnt previous does not give this thread
> exclusive
> > access to 'w', so if it's writing to w->w_var then it must either
> > a) have some sort of write lock taken, which it will release after this
> and
> > which will contain the necessary member, OR
> > b) have the only access patch to this structure (i.e, it's not yet
> > 'published' into structures which can be seen by other threads), in which
> > case the operations which do that 'publishing' of the access to 'w'
> (adding
> > it to a global list, etc) must include the necessary membar.
>
> Lets change the sequence to this:
>
> local_var = atomic_load_int(>w_var);
> refcnt_rele(>w_refcnt);
>
> Without the release barrier, is the load guaranteed to happen before
> the reference count is decremented?
>

That's completely uncomparable to what you described before for "CPU 1"
because there's no write to anything but the refcnt.

If no one writes to the object that is ref counted, then there are no
visibility problems, no?


> Next, CPU 2 executes:
> > >
> > > if (refcnt_rele(>w_refcnt))  /* refcnt drops to zero */
> > > free(w);
> > >
> >
> > How did CPU 2 get what is now exclusive access to 'w' without any
> membars?
> > If that's possible then it was just accessing 'w' and possibly not seeing
> > the update to w->w_var even _before_ the refcnt_rele(), so putting a
> membar
> > in refcnt_rele() hides the incorrect code by suppressing the later crash!
> >
> > If these membars appear to help then the code is and remains broken.
> This
> > change should not be done.
>
> It is not uncommon to see something like below:
>
> Access object with read intent:
>
> mtx_enter(_lock);
> w = lookup_from_list();
> if (w != NULL)
> refcnt_take(>w_refcnt);
> mtx_leave(_lock);
> if (w == NULL)
> return;
> ...
>

No writes to *w described *OR LEGAL*.



> if (refcnt_rele(>w_refcnt))
> free(w);

Delete object:
>
> mtx_enter(_lock);
> w = lookup_from_list();
> if (w != NULL)
> remove_from_list(w);
> mtx_leave(_lock);
>

This does the 'release' on the change to w's list link(s).



> /* Release list's reference. */
> if (w != NULL && refcnt_rele(>w_refcnt))
> free(w);
>
> Above, any refcnt_rele() can release the final reference.
>
> If there is no acquire barrier after the refcnt 1->0 transition, what
> is known about the CPU's local view of the object after refcnt_rele()?
>

Okay, if refcnt is used with objects that have embedded list links, or
could be, then refcnt_rele needs acquire semantics on the 1->0 transition.
I can agree with your justification for that.

Do you have a similar example for giving it release semantics as your diff
proposed?  What's the otherwise-unprotected-by-synchronization-primitive
that has to be protected?



> The decrement operation in the  API of Linux and
> the  API of FreeBSD provide release and acquire
> barriers. Are they wrong?
>

I'm not sure what argument you're making here.

So far, I've understood this to be "here's why this API needs to provide
these semantics, as justified by the logic of acquire/release semantics and
 examples".  Per above, I think you've justified giving it acquire
semantics, but don't understand the justification for also giving it
release semantics.

Now you're giving a completely different justification: "they did so".
That's a justification under any of (at least) three lines:
 a) they're incapable of making mistakes
 b) you've read the docs/explanation behind the

Re: Provide memory barriers in refcnt_rele() and refcnt_finalize()

2022-03-13 Thread Philip Guenther
On Sun, Mar 13, 2022 at 10:27 AM Visa Hankala  wrote:

> On Sun, Mar 13, 2022 at 04:29:44PM +0100, Mark Kettenis wrote:
>
...

> > Under what circumstances does memory ordering matter for these
> > interfaces?
>
> Consider the following scenario:
>
> struct widget {
> struct refcnt   w_refcnt;
> /* more fields spanning many cache lines */
> ...
> int w_var;
> };
>
> First, CPU 1 executes:
>
> w->w_var = 1;
> refcnt_rele(>w_refcnt);  /* remains above zero */
>

Having incremented the refcnt previous does not give this thread exclusive
access to 'w', so if it's writing to w->w_var then it must either
a) have some sort of write lock taken, which it will release after this and
which will contain the necessary member, OR
b) have the only access patch to this structure (i.e, it's not yet
'published' into structures which can be seen by other threads), in which
case the operations which do that 'publishing' of the access to 'w' (adding
it to a global list, etc) must include the necessary membar.


Next, CPU 2 executes:
>
> if (refcnt_rele(>w_refcnt))  /* refcnt drops to zero */
> free(w);
>

How did CPU 2 get what is now exclusive access to 'w' without any membars?
If that's possible then it was just accessing 'w' and possibly not seeing
the update to w->w_var even _before_ the refcnt_rele(), so putting a membar
in refcnt_rele() hides the incorrect code by suppressing the later crash!

If these membars appear to help then the code is and remains broken.  This
change should not be done.


Philip Guenther


Re: ifconfig(8): always print the mtu, don't hide it on "bridges"

2022-02-22 Thread Philip Guenther
On Mon, Feb 21, 2022 at 9:47 PM David Gwynne  wrote:

> this lets ifconfig show the MTU on interfaces like nvgre, vxlan, etc.
> they currently don't show it because they also implement a bridge ioctl,
> so ifconfig thinks they're a bridge.
>
> why ifconfig hides the mtu on bridges looks to be a hold over from when
> brconfig was merged into ifconfig. if we dont want bridge(4) to report
> an mtu, then i can make bridge(4) itself hide the mtu or stop setting
> the mtu.
>
> found by jason tubnor.
>
> ok?


 Ah, this test explains one of the three times ioctl(SIOCBRDGRTS) is used
on each interface.  This will drop it to twice per interface ;)

ok guenther@


Re: Power-up cc --print-file-name for .so names

2022-02-13 Thread Philip Guenther
On Sun, Feb 13, 2022 at 11:29 PM Philip Guenther  wrote:

> On Sun, Feb 13, 2022 at 11:18 PM Mark Kettenis 
> wrote:
>
>> > From: Greg Steuck 
>> > Date: Sun, 13 Feb 2022 22:37:13 -0800
>> >
>> > To give a sense of the kind of change required to get the feature I
>> > want, see the patch at the end. The change in DriverUtils.cpp is just to
>> > show that the same function is hiding in there.
>> >
>> > If this looks like a good direction, I can cleanup the code and maybe it
>> > could be shared, though I'm not sure where this would belong. Maybe a
>> > new tiny library of such wrappers to maintain in our tree?
>> >
>> > Thanks
>> > Greg
>> >
>> > P.S. Naturally, once I install this patched cc, ghci is suddenly very
>> > happy.
>>
>> Something like this was rejected upstream.
>>
>> The solution would be to add symlinks like all the other OSes do.  But
>> Theo doesn't like that.
>>
>
> My recall was it was less that "Theo didn't like it" than "no one had
> actually worked out how the symlink update process would work across major
> and minor version bumps in base builds and what else needed to be adjusted
> to keep things from exploding during that", because if there's one thing
> Theo doesn't like it's "oh, something committed months ago and now embedded
> in our ecosystem means that our ABI change process NO LONGER WORKS".
>
> So, IMHO, what is needed is for someone to camp out and watch for a diff
> floated that will require a major bump *and that will affect stuff run
> during the base build!* and then
> 1) apply the symlink changes that *seem* fine, and build some dependent
> ports,
> 2) apply the major bump diff
> 3) build base
> 4) verify whether anything exploded in the base build and if so, what/why
> 5) so, do those ports still work, or do they explode until an update to
> them is pushed?
>

Those of long memory will recall a hackathon where dependencies on libc
were put in place, the libm vs libc deps were changed as functions were
moved from libm to libc, and base builds completely broke.  My recall is
that the diffs had to basically be unrolled to restore base builds to
operation, and then the libm to libc stuff was redone, but I may be
repressing memories.


Philip


Re: Power-up cc --print-file-name for .so names

2022-02-13 Thread Philip Guenther
On Sun, Feb 13, 2022 at 11:18 PM Mark Kettenis 
wrote:

> > From: Greg Steuck 
> > Date: Sun, 13 Feb 2022 22:37:13 -0800
> >
> > To give a sense of the kind of change required to get the feature I
> > want, see the patch at the end. The change in DriverUtils.cpp is just to
> > show that the same function is hiding in there.
> >
> > If this looks like a good direction, I can cleanup the code and maybe it
> > could be shared, though I'm not sure where this would belong. Maybe a
> > new tiny library of such wrappers to maintain in our tree?
> >
> > Thanks
> > Greg
> >
> > P.S. Naturally, once I install this patched cc, ghci is suddenly very
> > happy.
>
> Something like this was rejected upstream.
>
> The solution would be to add symlinks like all the other OSes do.  But
> Theo doesn't like that.
>

My recall was it was less that "Theo didn't like it" than "no one had
actually worked out how the symlink update process would work across major
and minor version bumps in base builds and what else needed to be adjusted
to keep things from exploding during that", because if there's one thing
Theo doesn't like it's "oh, something committed months ago and now embedded
in our ecosystem means that our ABI change process NO LONGER WORKS".

So, IMHO, what is needed is for someone to camp out and watch for a diff
floated that will require a major bump *and that will affect stuff run
during the base build!* and then
1) apply the symlink changes that *seem* fine, and build some dependent
ports,
2) apply the major bump diff
3) build base
4) verify whether anything exploded in the base build and if so, what/why
5) so, do those ports still work, or do they explode until an update to
them is pushed?


Philip


Re: [PATCH v2 3/3] script(1): fix exit status wording, use 125 for general failure

2022-01-28 Thread Philip Guenther
On Fri, Jan 28, 2022 at 5:28 AM наб 
wrote:

> This is a base-line attempt at separating errors from the child from the
> ones from script itself ‒ 125 is the general-purpose code in POSIX
> utilities that exec() (with 126 being ENOEXEC and 127 ‒ ENOENT)
>

I just checked the draft of the next revision of the POSIX spec and can
find no reference to 125 being either recommended or required as the status
for general exec failures.  For example, the spec for xargs includes this:

EXIT STATUS
The following exit values shall be returned:
 0All invocations of utility returned exit
status zero.
 1-125 A command line meeting the specified
requirements could
   not be assembled, one or more of the
invocations of utility
   returned a non-zero exit status, or some
other error occurred.
 126The utility specified by utility was found but
could not be invoked.
 127The utility specified by utility could not be
found.

I'm confident that this isn't a change from previous versions.  Where is
this proposed use of 125 documented?

Philip Guenther


Re: touch(1): don't leak descriptor if futimens(2) fails

2022-01-28 Thread Philip Guenther
On Fri, Jan 28, 2022 at 7:37 AM Scott Cheloha 
wrote:

> On Fri, Jan 28, 2022 at 07:28:40AM -0700, Todd C. Miller wrote:
> > On Thu, 27 Jan 2022 20:02:18 -0800, Philip Guenther wrote:
> >
> > > > I think futimens(2) and close(2) failures are exotic enough to
> warrant
> > > > printing the system call name.
> > >
> > > I don't understand this.  Can you give an example of an error message
> that
> > > touch currently might emit where knowing that the failed call was
> > > futimens() or close() would affect the analysis of how to deal with
> it?  I
> > > mean, it looks like the only errors that futimens() could really
> return are
> > > EROFS, EIO, and EPERM (implies a race by different users to create the
> > > file), and close() could only return EIO.  For any of those errors,
> you're
> > > going to handle them the same whether they're from open, futimens, or
> > > close, no?
> >
> > I agree.  The actual syscall in this case is pretty much irrelevant.
> > The mostly likely failure is due to an I/O error of some kind.
>
> Alright, you have both convinced me.
>
> We'll go with this patch?
>

Sure (but I think the rval assignment and warn() call should be in a
consistent order).

Philip Guenther


Re: touch(1): don't leak descriptor if futimens(2) fails

2022-01-27 Thread Philip Guenther
On Thu, Jan 27, 2022 at 4:03 PM Scott Cheloha 
wrote:

> If futimens(2) fails here then close(2) is not called and we leak the
> descriptor.
>

Good catch.


I think futimens(2) and close(2) failures are exotic enough to warrant
> printing the system call name.
>

I don't understand this.  Can you give an example of an error message that
touch currently might emit where knowing that the failed call was
futimens() or close() would affect the analysis of how to deal with it?  I
mean, it looks like the only errors that futimens() could really return are
EROFS, EIO, and EPERM (implies a race by different users to create the
file), and close() could only return EIO.  For any of those errors, you're
going to handle them the same whether they're from open, futimens, or
close, no?


Philip Guenther


Re: Properly check if ACPI devices are enabled

2022-01-24 Thread Philip Guenther
On Mon, Jan 24, 2022 at 11:41 AM Mark Kettenis 
wrote:

> > Date: Mon, 24 Jan 2022 20:19:46 +0100
> > From: Anton Lindqvist 
> >
> > On Mon, Jan 24, 2022 at 05:31:49PM +0100, Mark Kettenis wrote:
> > > Currently we attach ACPI devices that are present in a machine.
> > > However, in some cases ACPI devices can be present, but not enabled.
> > > Attaching a device driver to devices that are not enabled is not a
> > > good idea since reading and writing from/to its registers will fail
> > > and the driver will malfunction in interesting ways.  Such as a com(4)
> > > serial port that is misdetected and hangs the kernel when it is
> > > actually opened.
> > >
> > > The diff below makes sure we only enable devices that are actually
> > > enabled.  This may cause some devices to disappear in OpenBSD.
> > > However those devices should have been unusable anyway, so that isn't
> > > an issue.
> > >
> > > ok?
> >
> > According to the ACPI specification[1]:
> >
> > > A device can only decode its hardware resources if both bits 0 and 1
> are
> > > set. If the device is not present (bit [0] cleared) or not enabled (bit
> > > [1] cleared), then the device must not decode its resources.
>
> Just before that it says:
>
>   If bit [0] is cleared, then bit 1 must also be cleared (in other
>   words, a device that is not present cannot be enabled).
>
> > Should we therefore check for presence of both STA_PRESENT and
> > STA_ENABLED?
>
> So according to the ACPI specification we don't need to do that.
> Should we do it just to be safe?
>

Unless you're taking money bets about this being the one thing in the ACPI
spec that some vendor won't screw up, doing both seems "can't be worse; can
be better".

Philip


Re: usr.bin/mg: fix -Wunused-but-set-variable warnings

2022-01-19 Thread Philip Guenther
On Sun, Jan 16, 2022 at 8:10 AM Christian Weisgerber 
wrote:

> usr.bin/mg: fix -Wunused-but-set-variable warnings
>
> * strtonum() is only called to verify that a string is numerical, the
>   return value is unused.
> * inlist is no longer used after the code was refactored.
>
> OK?
>

ok guenther@


Re: usr.sbin/dvmrpctl: fix -Wunused-but-set-variable warning

2022-01-19 Thread Philip Guenther
On Sun, Jan 16, 2022 at 12:51 PM Christian Weisgerber 
wrote:

> usr.sbin/dvmrpctl: fix -Wunused-but-set-variable warning
>
> This looks like /* not yet implemented */, but the companion functions
> show_rib_detail_msg() and show_mfc_detail_msg() are equally empty.
>

ok guenther@


Re: usr.sbin/eigrpd: fix -Wunused-but-set-variable warning

2022-01-19 Thread Philip Guenther
On Sun, Jan 16, 2022 at 1:51 PM Christian Weisgerber 
wrote:

> usr.sbin/eigrpd: fix -Wunused-but-set-variable warning
>

ok guenther@


Re: usr.sbin/ospf6ctl: fix -Wunused-but-set-variable warning

2022-01-19 Thread Philip Guenther
On Mon, Jan 17, 2022 at 6:36 AM Christian Weisgerber 
wrote:

> usr.sbin/ospf6ctl: fix -Wunused-but-set-variable warning
>
> Maybe this is uncompleted code, but I think we can remove it for
> the time being.
>
> M  usr.sbin/ospf6ctl/ospf6ctl.c
>

Agreed.  ok guenther@


Re: dt: make vmm tracepoints amd64 only

2022-01-17 Thread Philip Guenther
On Mon, Jan 17, 2022 at 5:02 AM Klemens Nanni  wrote:

> These don't hurt on !VMM architectures but I was still surprised to see
> them on e.g. sparc64:
>
> # arch -s ; btrace -l | grep vmm
> sparc64
> tracepoint:vmm:guest_enter
> tracepoint:vmm:guest_exit
>
> Like some network drivers, we could use __amd64__ to limit those to
> amd64 and save a few bits in all other kernels.
>
> Is this approach feasible or should we just ignore such one-offs?
>

If there's a non-trivial number of trace points in drivers, then it would
suggest that such tracepoints should be added/registered/pick-a-term by the
driver's attach routine.

Philip


Re: sbin/isakmpd: fix -Wunused-but-set-variable warnings

2022-01-15 Thread Philip Guenther
On Sat, Jan 15, 2022 at 12:36 PM Christian Weisgerber 
wrote:

> sbin/isakmpd: fix -Wunused-but-set-variable warnings
>
> The one in pf_key_v2.c could use an extra set of eyes, but I don't
> think there are any side effects.
>

The involved variables in pf_key_v2.c were added in 2000 as part of some
sort of sync-with-upstream, weren't used then, and never used after.
Deleting them seems perfectly fine to me.

ok guenther@ on the entire diff


Re: new: lang/polyml

2022-01-11 Thread Philip Guenther
On Tue, Jan 11, 2022 at 4:09 PM Daniel Dickman  wrote:

> On Mon, Jan 10, 2022 at 8:12 PM Leo Larnack  wrote:
> >
> > i386
> >
>
>
>
> with this diff I was able to install includes, rebuild ld.so and
> ctfconv. I've not managed to build a release yet.

...

Umm, with what diff?  There was no diff in nor attached to that message.
:-/

(That was a lot of lines of output.  I don't know about ports@, but my
expectation would be there would be *zero* reviewers of anything before,
say, the last 50 lines of output before the switch to actual compilation.
Standard "make lots of noise so when a failure occurs we can see the
leadup, but we'll ignore it otherwise" style of output, like a base build.
You read the lead up to the warnings and errors only.  )


Philip Guenther


Re: new: lang/polyml

2022-01-10 Thread Philip Guenther
Yeah, it makes sense to move our base C environment to match the values
seen in the output of 'readelf' and in the broader programming environment.


Philip

On Mon, Jan 10, 2022 at 3:34 PM Mark Kettenis 
wrote:

> > Date: Sun, 09 Jan 2022 23:54:23 +0100
> > From: Leo Larnack 
> >
> > Daniel Dickman  wrote:
> >
> > > elfexport.cpp:352:56: error: use of undeclared identifier 'R_386_32'
> > > reloc.r_info = ELFXX_R_INFO(symbolNum, isFuncPtr ?
> > > HOST_DIRECT_FPTR_RELOC : HOST_DIRECT_DATA_RELOC);
> > >^
> > > elfexport.cpp:142:33: note: expanded from macro
> 'HOST_DIRECT_FPTR_RELOC'
> > > # define HOST_DIRECT_FPTR_RELOC R_386_32
> >
> > It looks like Poly/ML expects R_386_32 to be either in  of
> >  (it's located in  on Void Linux, for
> > example).  After a bit of searching, what I understand is that
> > OpenBSD/i386's (and other platforms')  contains
> > the right values, but doesn't follow the same naming convention
> > (RELOC_32), while other platforms do follow that convention.
> >
> > Hoping that I'm not doing something dumb, here is a diff that
> > standardizes the constants' names for these platforms. The new names
> > come from src/gnu/usr.bin/binutils-2.17/include/elf.
>
> Yes, I think this is a diff we want.  But it is probably best to
> review and test this on a per-architecture basis.
>
> Can you feed them to us one-by-one?
>
>
> > Index: libexec/ld.so/hppa/rtld_machine.c
> > ===
> > RCS file: /cvs/src/libexec/ld.so/hppa/rtld_machine.c,v
> > retrieving revision 1.43
> > diff -u -p -r1.43 rtld_machine.c
> > --- libexec/ld.so/hppa/rtld_machine.c 8 Jan 2022 06:49:41 -
>  1.43
> > +++ libexec/ld.so/hppa/rtld_machine.c 9 Jan 2022 22:03:27 -
> > @@ -164,7 +164,7 @@ _dl_md_reloc(elf_object_t *object, int r
> >   int type;
> >
> >   type = ELF_R_TYPE(rela->r_info);
> > - if (type == RELOC_NONE)
> > + if (type == R_TYPE(NONE))
> >   continue;
> >
> >   sym = object->dyn.symtab + ELF_R_SYM(rela->r_info);
> > @@ -191,7 +191,7 @@ _dl_md_reloc(elf_object_t *object, int r
> >  #endif
> >
> >   switch (type) {
> > - case RELOC_DIR32:
> > + case R_TYPE(DIR32):
> >   if (ELF_R_SYM(rela->r_info) && sym->st_name) {
> >   *pt = sr.obj->obj_base + sr.sym->st_value +
> >   rela->r_addend;
> > @@ -213,7 +213,7 @@ _dl_md_reloc(elf_object_t *object, int r
> >   }
> >   break;
> >
> > - case RELOC_PLABEL32:
> > + case R_TYPE(PLABEL32):
> >   if (ELF_R_SYM(rela->r_info)) {
> >   if (ELF_ST_TYPE(sr.sym->st_info) !=
> STT_FUNC) {
> >   DL_DEB(("[%x]PLABEL32: bad\n", i));
> > @@ -236,7 +236,7 @@ _dl_md_reloc(elf_object_t *object, int r
> >   }
> >   break;
> >
> > - case RELOC_IPLT:
> > + case R_TYPE(IPLT):
> >   if (ELF_R_SYM(rela->r_info)) {
> >   pt[0] = sr.obj->obj_base +
> sr.sym->st_value +
> >   rela->r_addend;
> > @@ -256,7 +256,7 @@ _dl_md_reloc(elf_object_t *object, int r
> >   }
> >   break;
> >
> > - case RELOC_COPY:
> > + case R_TYPE(COPY):
> >   {
> >   sr = _dl_find_symbol(symn,
> >   SYM_SEARCH_OTHER|SYM_WARNNOTFOUND|SYM_NOTPLT,
> > @@ -381,7 +381,7 @@ _dl_md_reloc_got(elf_object_t *object, i
> >   for (i = 0; i < numrela; i++, rela++) {
> >   Elf_Addr *r_addr = (Elf_Addr *)(ooff +
> rela->r_offset);
> >
> > - if (ELF_R_TYPE(rela->r_info) != RELOC_IPLT) {
> > + if (ELF_R_TYPE(rela->r_info) != R_TYPE(IPLT)) {
> >   _dl_printf("unexpected reloc 0x%x\n",
> >   ELF_R_TYPE(rela->r_info));
> >   return 1;
> > Index: libexec/ld.so/m88k/rtld_machine.c
> > ===
> > RCS file: /cvs/src/libexec/ld.so/m88k/rtld_machine.c,v
> > retrieving revision 1.31
> > diff -u -p -r1.31 rtld_machine.c
> > --- libexec/ld.so/m88k/rtld_machine.c 8 Jan 2022 06:49:42 -
>  1.31
> > +++ libexec/ld.so/m88k/rtld_machine.c 9 Jan 2022 22:03:27 -
> > @@ -99,17 +99,17 @@ _dl_md_reloc(elf_object_t *object, int r
> >
> >   type = ELF_R_TYPE(relas->r_info);
> >
> > - if (type == RELOC_GOTP_ENT && rel != DT_JMPREL)
> > + if (type == R_TYPE(GOTP_ENT) && rel != DT_JMPREL)
> >   continue;
> >
> > - if (type == RELOC_NONE)
> > +

Re: WTERMSIG behavior difference

2022-01-07 Thread Philip Guenther
On Thu, Jan 6, 2022 at 10:47 PM Greg Steuck  wrote:

> I started by debugging a weird test failure in ghc. I narrowed it
> down to a simple C program which behaves differently between OpenBSD and
> FreeBSD. I stared at the headers of both systems for a bit and still
> don't see why on OpenBSD the program prints:
>
> uname -a ; cc ./a.c && ./a.out
> OpenBSD home.nest.cx 7.0 GENERIC.MP#28 amd64
> x 0 pret 130
>
> Yet on FreeBSD:
> uname -a ; cc ./a.c && ./a.out
> FreeBSD ... 13.0-STABLE FreeBSD 13.0-STABLE
> x 0 pret -2
>
> Where's the signed/unsigned confusion hiding?
>

No where.  The difference in behavior is that of 'sh' when signaled.  Run
your test programs under "ktrace -i" and compare the behavioral difference
of the child 'sh' process after reception of the SIGINT.


Philip Guenther


Re: less: fix use after free bug

2021-12-31 Thread Philip Guenther
On Fri, Dec 31, 2021 at 6:22 AM Tobias Stoeckmann 
wrote:

> Hi,
>
> it is possible to trigger a use after free bug in less with huge
> files or tight memory constraints. PoC with 100 MB file:
>
> dd if=/dev/zero bs=1024 count=102400 | tr '\0' 'a' > less-poc.txt
> ulimit -d 157286
> less less-poc.txt
>
> The linebuf and attr buffers in line.c are supposed to never be freed,
> since they are used for keeping (meta) data of current line in RAM.
>
> The linebuf and attr buffers are expanded in expand_linebuf. If linebuf
> could be expanded but attr could not, then returned pointers are freed,
> but linebuf variable still points to previous location. Subsequent
> accesses will therefore trigger a use after free bug.
>
> The easiest fix is to keep reallocated linebuf. The size of both buffers
> differ, but the code still assumes that the previous (smaller) size is
> the correct one.
>
> Thoughts on this approach?
>

Analysis makes sense.

To bikeshed slightly I would be inclined to do the work progressively,
perhaps like the diff below...but your diff works too.


Philip Guenther


Index: line.c
===
RCS file: /data/src/openbsd/src/usr.bin/less/line.c,v
retrieving revision 1.34
diff -u -p -r1.34 line.c
--- line.c  25 Oct 2021 19:54:29 -  1.34
+++ line.c  1 Jan 2022 06:24:31 -
@@ -96,16 +96,16 @@ expand_linebuf(void)

/* Just realloc to expand the buffer, if we can. */
char *new_buf = recallocarray(linebuf, size_linebuf, new_size, 1);
-   char *new_attr = recallocarray(attr, size_linebuf, new_size, 1);
-   if (new_buf == NULL || new_attr == NULL) {
-   free(new_attr);
-   free(new_buf);
-   return (1);
+   if (new_buf != NULL) {
+   char *new_attr = recallocarray(attr, size_linebuf,
new_size, 1);
+   linebuf = new_buf;
+   if (new_attr != NULL) {
+   attr = new_attr;
+   size_linebuf = new_size;
+   return (0);
+   }
}
-   linebuf = new_buf;
-   attr = new_attr;
-   size_linebuf = new_size;
-   return (0);
+   return (1);
 }

 /*


Re: Fix GNUism in bsd.dep.mk

2021-12-31 Thread Philip Guenther
On Fri, Dec 31, 2021 at 7:44 AM Christian Ehrhardt 
wrote:

> Here at genua, trying to build libpcap sometimes breaks in
> libpcap with the following error message:
>
> | Using $< in a non-suffix rule context is a GNUmake idiom \
> |(/data/git/ehrhardt/genuos/os/mk/bsd.dep.mk:47)
>
> The bug is in bsd.dep.mk where ${.IMPSRC} (aka $<) is used
> in a context where it is not neccessarily defined by OpenBSD make.
>
> Below is a diff to Use ${.ALLSRC:M*.y} instead of ${.IMPSRC} as
> the input file for yacc.
>
> The issue is with the rule for the grammar.h file that is generated
> by yacc from grammar.c. You can easily reproduce the bug with the
> following steps:
> - build libpcap from scratch: cd .../src/lib/libpcap && make clean all
> - remove the generated grammar.h file: rm obj*/grammar.h
> - build libpcap again (incremental build): make
> In normal builds this does not trigger as grammar.h is implicitly
> generated by the rule for grammar.c and when make checks for
> dependencies it simply finds grammar.h uptodate. However,
> incremental or parallel builds might decide to make grammar.h
> from grammar.y.
>
> Now, why is this only a problem for grammar.h but not for
> grammar.c? The answer to this question is burried deeply in
> OpenBSD's mk files.
>
> The snippet in bsd.dep.mk that triggers the error is a single
> rule statement that generates foo.c and foo.h from foo.y with a
> call to yacc -d. The rule is generated with a loop, i.e. it is not
> a prefix rule. However, a prefix rule context is required for
> the use of ${.IMPSRC} aka $<. For the .c file such a prefix
> rule is provided by bsd.sys.mk and this rule is in scope when
> make evaluates the yacc rule. However, for .h file generation
> from a .y file there is no such prefix rule defined in any of
> the Makefiles. Even if it were the .h suffix is missing from
> .SUFFIXES and the rule would not be considered.
>
> NOTE: The obvious way to fix this would be to use $f instead of
> ${.IMPSRC}. However, this does not work as $f is then missing
> the path prefix and yacc won't find it if an obj directory is
> used. This is probably the reason for the use of ${.IMPSRC} in
> the first place.
>

Ah, nice analysis!  The suggested fix looks safe to me (can't see how a .c
could have two .y direct prerequisites, so this can't be ambiguous).

ok guenther@


Re: hangman(6): skip retguard symbols

2021-12-24 Thread Philip Guenther
Skipping all leading double-underbar symbols makes sense to me.

Tempting to skip all symbols with more than one digit (or maybe just more
than one consecutive digit?), as guessing among per-chip symbols from, say,
the ar* or dc* families is an exercise in futility.

On Fri, Dec 24, 2021 at 5:23 AM Theo Buehler  wrote:

> __retguard_ symbols are easy to recognize and no real fun to guess.
> I suggest we skip them. Perhaps we should even skip all __* symbols?
>
> Index: ksyms.c
> ===
> RCS file: /cvs/src/games/hangman/ksyms.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 ksyms.c
> --- ksyms.c 28 Jun 2019 13:32:52 -  1.12
> +++ ksyms.c 23 Dec 2021 18:28:02 -
> @@ -70,6 +70,9 @@ sym_getword(void)
> /* ignore symbols containing dots or dollar signs */
> if (strchr(sym, '.') != NULL || strchr(sym, '$') != NULL)
> continue;
> +   /* guessing retguard symbols is no fun */
> +   if (strncmp(sym, "__retguard", 10) == 0)
> +   continue;
>
> break;
> }
>
>


warning for -current builders: update your kernel before libc/ld.so!

2021-12-23 Thread Philip Guenther
For those building -current themselves, when you update past the commit
below you must be sure to build *and reboot to* a new kernel with the
change before you install a new libc or ld.so!

If you fail to do so then anything using the newer-than-kernel libc/ld.so
will coredump immediately, generally on the first mmap(2), and you'll need
to reboot to a bsd.rd or similar and put a matching kernel+libc+ld.so in
place.

This might be a good time to just install an official snapshot instead.

-- Forwarded message -
From: Philip Guenther 
Date: Thu, Dec 23, 2021 at 10:51 AM
Subject: CVS: cvs.openbsd.org: src
To: 


CVSROOT:/cvs
Module name:src
Changes by: guent...@cvs.openbsd.org2021/12/23 11:50:33

Modified files:
sys/kern   : syscalls.master vfs_syscalls.c kern_ktrace.c
 kern_pledge.c
sys/arch/sh/sh : trap.c
sys/arch/hppa/hppa: trap.c
sys/uvm: uvm_mmap.c
lib/libc/sys   : Makefile.inc
libexec/ld.so  : Makefile loader.c
libexec/ld.so/m88k: rtld_machine.c
usr.bin/kdump  : kdump.c
Added files:
libexec/ld.so  : syscall.h
Removed files:
lib/libc/sys   : ftruncate.c lseek.c mmap.c mquery.c pread.c
 preadv.c pwrite.c pwritev.c truncate.c
libexec/ld.so/m88k: syscall.h
libexec/ld.so/aarch64: syscall.h
libexec/ld.so/alpha: syscall.h
libexec/ld.so/amd64: syscall.h
libexec/ld.so/arm: syscall.h
libexec/ld.so/hppa: syscall.h
libexec/ld.so/i386: syscall.h
libexec/ld.so/mips64: syscall.h
libexec/ld.so/powerpc: syscall.h
libexec/ld.so/powerpc64: syscall.h
libexec/ld.so/riscv64: syscall.h
libexec/ld.so/sh: syscall.h
libexec/ld.so/sparc64: syscall.h

Log message:
Roll the syscalls that have an off_t argument to remove the explicit
padding.
Switch libc and ld.so to the generic stubs for these calls.
WARNING: reboot to updated kernel before installing libc or ld.so!

Time for a story...

When gcc (back in 1.x days) first implemented long long, it didn't (always)
pass 64bit arguments in 'aligned' registers/stack slots, with the result
that
argument offsets didn't match structure offsets.  This affected the nine
system
calls that pass off_t arguments:
ftruncate lseek mmap mquery pread preadv pwrite pwritev truncate

To avoid having to do custom ASM wrappers for those, BSD put an explicit pad
argument in so that the off_t argument would always start on a even slot and
thus be naturally aligned.  Thus those odd wrappers in lib/libc/sys/ that
use
__syscall() and pass an extra '0' argument.

The ABIs for different CPUs eventually settled how things should be passed
on
each and gcc 2.x followed them.  The only arch now where it helps is
landisk,
which needs to skip the last argument register if it would be the first
half of
a 64bit argument.  So: add new syscalls without the pad argument and on
landisk
do that skipping directly in the syscall handler in the kernel.  Keep compat
support for the existing syscalls long enough for the transition.

ok deraadt@


Re: moncontrol(3): remove hertz()

2021-12-06 Thread Philip Guenther
On Mon, Dec 6, 2021 at 10:30 AM Scott Cheloha 
wrote:

> In the moncontrol(3) code we have this fallback function, hertz().
> The idea is to use an undocumented side effect of setitimer(2) to
> determine the width of the hardclock(9) tick.
>
> hertz() has a number of problems:
>


> So, I want to get rid of hertz() and replace it with nothing.  This is
> an extreme edge case.  sysctl(2) has to fail.  I do not think that is
> possible here, outside of stack corruption.
>

I agree: sysctl(KERN_CLOCKRATE) isn't blocked by pledge(2), so how would
they manage to break it?

(I'm not even sure the failover from profhz to hz should be kept: the
kernel does that itself in initclocks(9) so if profhz is 0 then hz is too,
sysctl(2) failed, and your CPU is on fire.)

ok guenther@


Re: New hw.perfpolicy behavior

2021-11-09 Thread Philip Guenther
On Tue, Nov 9, 2021 at 3:29 PM Jan Stary  wrote:

> On Nov 10 00:21:59, h...@stare.cz wrote:
> > Regarding C states, this machine has
> >
> > acpicpu0 at acpi0: C3(350@96 mwait.1@0x20), C2(500@64 mwait.1@0x10),
> C1(1000@1 mwait.1), PSS
> >
> > I suppose the cpu supports C1, C2, C3, but can someone please kindly
> > explain (or point to an explanation of) what the whole line says?
>
> For comparison, my Thinkpad T400 has
> acpicpu0 at acpi0: !C3(100@57 mwait.3@0x30), !C2(500@1 mwait.1@0x10),
> C1(1000@1 mwait.1), PSS
> What does the ! mean?
>

This is generated by sys/dev/acpi/acpicpu.c:acpicpu_print_one_cst() and at
this point should either be suppressed by default or documented in the
manpage; I'm not sure which.

To enumerate it from right to left:

 !C3(100@57 mwait.3@0x30)
 ^  ^   ^   ^ ^ ^   ^
 |   ||| |  |   |
 |   ||| |  |   \-- 'hints' passed to the actual
MWAIT instruction, or address for 'io' method
 |   ||| |  \-- fixed-function level bit flags: 1="HW
coordinated" 2="bus-master avoidance required"
 |   ||| |   OR '!' if this is a faked-up fallback
to the pre-CST C1-via-HALT (bios is broken)
 |   ||| \-- sleep method, one of mwait, io, or halt
 |   ||\-- advertised latency when exiting the state
 |   |\-- advertised power-consumption
 |   \-- the C state, duh
 \-- '!' to indicate this state is disabled because the CPU's LAPIC stops
in C2 or deeper (no ARAT)


Re: [PATCH] /etc/netstart: unquoted command substitution inside arithmetic expression

2021-10-10 Thread Philip Guenther
On Sun, Oct 10, 2021 at 1:48 PM bm1les  wrote:

> Exactly my point. Even if the circumstances were contrived, I think it
> would good to fix it just for the sake of correctness.
>

Sure, knowing what circumstances could cause a problem assists in
achieving correctness.



> The issue is actually a pattern I found not only in /etc/netstart but also
> in /etc/rc. (( )) cannot deal with an empty result yet it sometimes
> includes calls to sysctl which apparently can return an empty string in
> some cases.
>
> So options are:
>
> 1. ensure that sysctl always returns a number where it is expected
> 2. work around the issue by using /bin/[ in place of or in addition to,
> the arithmetic expression (depending on the expression being tested), so
> that whatever returns empty string can be tested instead of blowing up.
>

3. use syntax with works when the expansion is empty, such as
  (( $(sysctl blah) + 0 != 0 ))

   which is unary plus when it's empty, or
   (( $(sysctl blah)0 != 0 ))

   which multiplies the value by 10 when not empty and is zero when it's
empty.


Philip Guenther


(Per POSIX rules, any arithmetic expression is effectively evaluated in
double-quotes.  If you follow the spec from there, you can work out that if
an arithmetic expression that directly** contains double-quotes parses
correctly, then it must also parse correctly with the double-quotes
removed; adding double-quotes can't fix an arithmetic expression.  The
reverse is not true: adding double-quotes can break the parsing.  ** i.e.,
not counting quotes inside a nested parameter expansion or command
substitution.)


Re: [PATCH] /etc/netstart: unquoted command substitution inside arithmetic expression

2021-10-08 Thread Philip Guenther
On Fri, Oct 8, 2021 at 8:57 AM Theo de Raadt  wrote:

> Philip Guenther  wrote:
>
> > On Thu, Oct 7, 2021 at 5:57 PM bm1les  wrote:
> >
> > > --- netstart2 Sep 2021 19:38:20 -   1.216
> > > +++ netstart8 Oct 2021 02:43:30 -
> > > @@ -365,7 +365,7 @@ ifmstart "tun tap gif etherip gre egre p
> > >  if [[ $ip6kernel == YES ]]; then
> > > # Ensure IPv6 Duplicate Address Detection (DAD) is completed.
> > > count=0
> > > -   while ((count++ < 10 && $(sysctl -n net.inet6.ip6.dad_pending)
> !=
> > > 0)); do
> > > +   while ((count++ < 10 && "$(sysctl -n
> net.inet6.ip6.dad_pending)"
> > > != 0)); do
> > > sleep 1
> > > done
> > >  fi
> > >
> >
> > I can't figure out what problem you think this could solve.  Can you
> > explain the circumstances under which those quotes could make a
> difference?
>
> Not the OP's issue, but I think a kernels compiled without option INET6
> will return an errno, and I cannot tell if sysctl prints out an error
> message
> or converts to "", the empty string, which would conceivably mis-parse.
>

AFAICT, an empty quoted string there results in the exact same error.  As I
wrote off-list to the original submitter:

Can you be clearer about how the quoting makes the result any better when
> run under bsd.rd?  Doesn't it fail in the same way?  Testing with 'echo'
> instead would seem to indicate so:
> : bleys; (( 1 < 10 && $(echo) != 0 )); echo $?
> /bin/ksh:  1 < 10 &&  != 0 : unexpected `!='
> 2
> : bleys; (( 1 < 10 && $(echo -n) != 0 )); echo $?
> /bin/ksh:  1 < 10 &&  != 0 : unexpected `!='
> 2
> : bleys; (( 1 < 10 && "$(echo)" != 0 )); echo $?
> /bin/ksh:  1 < 10 &&  != 0 : unexpected `!='
> 2
> : bleys; (( 1 < 10 && "$(echo -n)" != 0 )); echo $?
> /bin/ksh:  1 < 10 &&  != 0 : unexpected `!='
> 2
> : bleys;



Philip


Re: [PATCH] /etc/netstart: unquoted command substitution inside arithmetic expression

2021-10-07 Thread Philip Guenther
On Thu, Oct 7, 2021 at 5:57 PM bm1les  wrote:

> --- netstart2 Sep 2021 19:38:20 -   1.216
> +++ netstart8 Oct 2021 02:43:30 -
> @@ -365,7 +365,7 @@ ifmstart "tun tap gif etherip gre egre p
>  if [[ $ip6kernel == YES ]]; then
> # Ensure IPv6 Duplicate Address Detection (DAD) is completed.
> count=0
> -   while ((count++ < 10 && $(sysctl -n net.inet6.ip6.dad_pending) !=
> 0)); do
> +   while ((count++ < 10 && "$(sysctl -n net.inet6.ip6.dad_pending)"
> != 0)); do
> sleep 1
> done
>  fi
>

I can't figure out what problem you think this could solve.  Can you
explain the circumstances under which those quotes could make a difference?


Philip Guenther


Re: mi_switch() & setting `p_stat'

2021-10-02 Thread Philip Guenther
On Sat, Oct 2, 2021 at 8:57 AM Martin Pieuchot  wrote:

> When a thread running on a CPU schedules itself out, it does the following
> (pseudo_code):
>
> SCHED_LOCK()
> curproc->p_stat = SSLEEP;
> // some more operations
> mi_switch()
>
> The problem with this is that any instrumentation between setting `p_stat'
> and cpu_switchto() is incorrect because 'curproc' is still being executed
> and is not yet sleeping.  Its `p_stat' should be SONPROC and not SSLEEP.
>
...

> To fix this we should set `p_stat' as late a possible, diff below does that
> just before calling cpu_switchto().
>
...

> --- kern/kern_sig.c 28 Sep 2021 10:00:18 -  1.283
> +++ kern/kern_sig.c 2 Oct 2021 17:00:52 -
> @@ -1347,7 +1347,6 @@ proc_stop(struct proc *p, int sw)
> SCHED_ASSERT_LOCKED();
>  #endif
>
> -   p->p_stat = SSTOP;
> atomic_clearbits_int(>ps_flags, PS_WAITED);
> atomic_setbits_int(>ps_flags, PS_STOPPED);
> atomic_setbits_int(>p_flag, P_SUSPSIG);
> @@ -1357,7 +1356,7 @@ proc_stop(struct proc *p, int sw)
>  */
> softintr_schedule(proc_stop_si);
> if (sw)
> -   mi_switch();
> +   mi_switch(SSTOP);
>

This pair of chunks is wrong, as then the proc_stop(p, 0) call in
ptsignal() doesn't change the process from SSLEEP to SSTOP.  Sending a stop
signal to a blocked process needs to change its state.


Philip Guenther


Re: rsync fix symlink discovery

2021-07-05 Thread Philip Guenther
Based on the fts_open(3) manpage and other base source usage, shouldn't
this use fts_accpath instead of fts_name?

The use of fts_statp in this code seems a bit loose vs ftp_info: instead of
using S_ISLNK() on fts_statp I would expect this code to check for fts_info
== FTS_SL: according the manpage fts_statp's value is undefined for various
values of fts_info.


Philip Guenther

On Fri, Jul 2, 2021 at 4:46 AM Claudio Jeker 
wrote:

> Hit this today while doing some tests. symlink_read() needs to use just
> the filename and not the full path because fts_read(3) does chdir
> internally.
>
> Without this I got:
> openrsync: error: ./obj/openrsync.1: readlink: No such file or directory
> openrsync: error: symlink_read
> openrsync: error: flist_gen_dirent
> openrsync: error: flist_gen
> openrsync: error: rsync_sender
>
> --
> :wq Claudio
>
> Index: flist.c
> ===
> RCS file: /cvs/src/usr.bin/rsync/flist.c,v
> retrieving revision 1.32
> diff -u -p -r1.32 flist.c
> --- flist.c 30 Jun 2021 13:10:04 -  1.32
> +++ flist.c 2 Jul 2021 13:14:01 -
> @@ -972,7 +972,7 @@ flist_gen_dirent(struct sess *sess, char
> /* Optionally copy link information. */
>
> if (S_ISLNK(ent->fts_statp->st_mode)) {
> -   f->link = symlink_read(f->path);
> +   f->link = symlink_read(ent->fts_name);
> if (f->link == NULL) {
> ERRX1("symlink_read");
> goto out;
>
>


Re: ld.so: program headers: do not rely on DYNAMIC coming before GNU_RELRO

2021-05-25 Thread Philip Guenther
On Mon, May 24, 2021 at 4:59 AM Klemens Nanni  wrote:

> When tinkering with ld.so crashes due to file corruption the other day
> I tested a few changes but did not want to replace /usr/libexec/ld.so
> and since recompiling executable to change their interpreter is not
> always an option, I went for https://github.com/NixOS/patchelf which
> allows me to manipulate executables in place.
>
> The tool works just fine and as a byproduct rearanges program headers;
> that in itself is fine except our run-time link-editor is not happy with
> such valid executables:
>
>
>  ELF mandates nothing but the file header be at a fixed location, hence
>  ld.so(1) must not assume any specific order for headers, segments, etc.
>

(Not quite: it does place ordering requirements in some specific cases,
such as that PT_INTERP and PT_PHDR, if present, must appear before any
PT_LOAD segments in the program header, and that PT_LOAD segments must
appear in p_vaddr order.)


 Looping over the program header table to parse segment headers,
>  _dl_boot() creates the executable object upon DYNAMIC and expects it to
>  be set upon GNU_RELRO, resulting in a NULL dereference iff that order is
>  reversed.
>
>  Store relocation bits in temporary variables and update the executable
>  object once all segment headers are parsed to lift this dependency.
>
>  Under __mips__ _dl_boot() later on uses the same temporary variable, so
>  move nothing but the declaration out of MI code so as to not alter the
>  MD code's logic/behaviour.
>
>
> This fix is needed for the following work on OpenBSD:
>
> $ patchelf --set-interpreter $PWD/obj/ld.so ./my-ldso-test
> $ readelf -l ./my-ldso-test | grep interpreter
>   [Requesting program interpreter: /usr/src/libexec/
> ld.so/obj/ld.so]
> $ ./my-ldso-test
> it works!
>
> amd64 and arm64 regress is happy and all my patched executables work
> with this.
>
> Feedback? Objections? OK?
>
> diff d7231fb4fb547dd287a884c56ae7c8b10f9145fe
> f023dbe355bef379d55eb93eddbb2702559d5bdb
> blob - 18bd30af759bffbc4e3fbfee9ffc29906f0d1bb0
> blob + b66dbb169aad9afffa1283d480ad9276aff9072a
> --- libexec/ld.so/loader.c
> +++ libexec/ld.so/loader.c
> @@ -464,6 +464,7 @@ _dl_boot(const char **argv, char **envp, const long dy
> int failed;
> struct dep_node *n;
> Elf_Addr minva, maxva, exe_loff, exec_end, cur_exec_end;
> +   Elf_Addr relro_addr = 0, relro_size = 0;
> Elf_Phdr *ptls = NULL;
> int align;
>
> @@ -552,8 +553,8 @@ _dl_boot(const char **argv, char **envp, const long dy
> ptls = phdp;
> break;
> case PT_GNU_RELRO:
> -   exe_obj->relro_addr = phdp->p_vaddr + exe_loff;
> -   exe_obj->relro_size = phdp->p_memsz;
> +   relro_addr = phdp->p_vaddr + exe_loff;
> +   relro_size = phdp->p_memsz;
> break;
> }
> phdp++;
> @@ -561,6 +562,8 @@ _dl_boot(const char **argv, char **envp, const long dy
> exe_obj->load_list = load_list;
> exe_obj->obj_flags |= DF_1_GLOBAL;
> exe_obj->load_size = maxva - minva;
> +   exe_obj->relro_addr = relro_addr;
> +   exe_obj->relro_size = relro_size;
> _dl_set_sod(exe_obj->load_name, _obj->sod);
>
>  #ifdef __i386__
> @@ -638,7 +641,7 @@ _dl_boot(const char **argv, char **envp, const long dy
> debug_map->r_ldbase = dyn_loff;
> _dl_debug_map = debug_map;
>  #ifdef __mips__
> -   Elf_Addr relro_addr = exe_obj->relro_addr;
> +   relro_addr = exe_obj->relro_addr;
> if (dynp->d_tag == DT_DEBUG &&
> ((Elf_Addr)map_link + sizeof(*map_link) <= relro_addr
> ||
>  (Elf_Addr)map_link >= relro_addr +
> exe_obj->relro_size)) {
>
>
ok guenther@

(This still assumes PT_GNU_RELRO comes after PT_PHDR, for exe_loff, but I
suspect even patchelf wouldn't screw with that.)


Re: Use atomic op for UVM map refcount

2021-05-21 Thread Philip Guenther
On Wed, May 19, 2021 at 11:29 PM Martin Pieuchot  wrote:

> On 19/05/21(Wed) 16:17, Mark Kettenis wrote:
>
...

> > There are the READ_ONCE() and WRITE_ONCE() macros.  I'm not a big fan
> > of those (since they add clutter) but they do take care of dependency
> > ordering issues that exist in the alpha memory model.  Must admit that
> > I only vaguely understand that issue, but I think it involves ordered
> > access to two atomic variables which doesn't seem to be the case.
>
> These macros are used in places where declaring the field as "volatile"
> could also work, no?  We can look at __mp_lock and SMR implementations.
> So could we agree one way to do things?
>
> Visa, David, why did you pick READ_ONCE() in SMR and veb(4)?  Anything
> we overlooked regarding the use of "volatile"?
>

If _all_ references to a member/variable use READ/WRITE_ONCE, then
declaring it volatile should be equivalent, but if there are any uses
which have a lock for protection instead than making the member volatile
will result in worse object code for the protected sequences where *_ONCE
are not needed.  Also, initialization of structs with volatile members can
be pessimal: the compiler has to assume this could be in uncached memory
mapped from a device where writes to the member have to be of the size of
the member: no paving with larger writes or deferring initializations.

volatile is a *really* blunt hammer.  READ/WRITE_ONCE use it carefully to
build a sharper tool.  Unifying on "just plain volatile" when the work has
already been done to use a sharper tool correctly..well, if that's a good
idea then why have SMR at all when locks would be easier for everyone to
think about, despite being a blunter hammer?  /s


Philip Guenther


Re: Use atomic op for UVM map refcount

2021-05-21 Thread Philip Guenther
On Wed, May 19, 2021 at 5:19 AM Mark Kettenis 
wrote:

> > Date: Tue, 18 May 2021 13:24:42 +0200
> > From: Martin Pieuchot 
>
...

> > There's only a couple of 'volatile' usages in sys/sys.  These annotations
> > do not explicitly indicate which piece of code requires it.  Maybe it
> would
> > be clearer to use a cast or a macro where necessary.  This might help us
> > understand why and where "volatile" is needed.
>
> There are the READ_ONCE() and WRITE_ONCE() macros.  I'm not a big fan
> of those (since they add clutter) but they do take care of dependency
> ordering issues that exist in the alpha memory model.  Must admit that
> I only vaguely understand that issue, but I think it involves ordered
> access to two atomic variables which doesn't seem to be the case.
>

> On non-alpha systems, READ_ONCE() and WRITE_ONCE() just do a volatile
> pointer cast.
>

READ/WRITE_ONCE() are 99% about keeping the compiler from deciding to
rearrange code such that the indicated variable is read/written more than
once.  To point to Linus posts from 2008/2009, when it was still
ACCESS_ONCE():
 https://yarchive.net/comp/linux/ACCESS_ONCE.html

ISTR a paper by Ousterhout describing this same problem to the C standard
committee(?) in the early 90's, which kinda opened the "memory model"
rathole.

If the variable is actually being protected by a lock then these are indeed
noise/pessimization; it's the lock-less accesses where the compiler can
pull a rabbit from its hat and stab you with it.


Philip Guenther


Re: ld.so: NULL dereference on corrupt library

2021-05-05 Thread Philip Guenther
On Tue, May 4, 2021 at 12:43 PM Klemens Nanni  wrote:
...

> I compared my corrupted shared library with an intact copy from ports
> and it showed that the corrupted one was simply zeroed out at some point
> (not truncated) until the end, e.g. readelf(1)'s `-h' or `-l' report
> "Error: no .dynamic section in the dynamic segment".
>
> So this isn't a case of some badly linked library or one that has a few
> bits flipped, it's simply a partial one... seems like bad luck?
>

IMHO, the benefit of adding this check is almost zero: it gives a slightly
better experience for a small set of possible data corruption cases, when
similar corruptions that affect other pages aren't helped at all as it'll
crash when it executes zeroed text, or accesses zeroed data, or fails to
find a required symbol because the symbol table was zeroed out.

If we want to protect against that sort of hardware lossage, then a
filesystem which does so is the way to go, not an alarm on one window of a
glass house.


Philip Guenther


Re: printf(1) Fix hex numbers

2021-04-18 Thread Philip Guenther
On Sun, Apr 18, 2021 at 12:04 PM Martijn van Duren <
openbsd+t...@list.imperialat.at> wrote:

> On Sun, 2021-04-18 at 11:17 -0900, Philip Guenther wrote:
> >
> > I'll just throw in a note that the current POSIX spec does not include
> support for \x in the printf(1) format or in the argument to the %b format
> specifier.  At least for \x in the format string it
> > appears to require that it _not_ be interpreted, such that
> > printf '\x61\n'
> > must output
> > \x61
> >  and not
> > a
>
> Could you point out where it states that an escape-sequence not
> specified must output the original text? The way I read it[0][1]:
>
> In addition to the escape sequences shown in XBD File Format Notation (
> '\\', '\a', '\b', '\f', '\n', '\r', '\t', '\v' ), "\ddd", where ddd is a
> one, two, or three-digit octal number, shall be written as a byte with
> the numeric value specified by the octal number.
>
> and
>
> Escape Sequences and Associated Actions lists escape sequences and
> associated actions on display devices capable of the action.
>

Before that statement in XBD 5 is this:
-
The format is a character string that contains three types of objects
defined below:

   1. Characters that are not "escape sequences" or "conversion
specifications", as described
 below, shall be copied to the output.

   2. Escape Sequences represent non-graphic characters and the escape
character ().

   3. Conversion Specifications specify the output format of each argument;
see below.
-

So, per (1) above, if it's not an escape sequence or conversion
specification, it SHALL be copied to the output.  Unfortunately, the list
of escape sequences is just a table, and while the printf(1) specification
has the clause you cited that adds \ddd to the list, there's nothing to
state that, for formats,  followed by anything else results in
unspecified behavior.  Ergo \x isn't an escape sequence under (2) and,
since it's not a converspecification, it must follow (1) and be copied to
the output.


Anyway, POSIX wording will probably change, it's just a place to note in
the manual that handling \xXX is an extension to the POSIX standard and
that compliant (or portable) applications must use \ddd instead.


Philip Guenther


Re: printf(1) Fix hex numbers

2021-04-18 Thread Philip Guenther
I'll just throw in a note that the current POSIX spec does not include
support for \x in the printf(1) format or in the argument to the %b format
specifier.  At least for \x in the format string it appears to require that
it _not_ be interpreted, such that
printf '\x61\n'
must output
\x61
 and not
a

FreeBSD 12's /usr/bin/printf does this, for example, however at least some
on the list seemed to feel the spec should be changed to make it
unspecified behavior.

The behavior of
printf '%b\n' '\x61'

is clearly unspecified already.

Philip Guenther


On Sun, Apr 18, 2021 at 7:02 AM Martijn van Duren <
openbsd+t...@list.imperialat.at> wrote:

> On Thu, 2021-04-15 at 19:48 -0600, Todd C. Miller wrote:
> > On Fri, 16 Apr 2021 03:34:04 +0200, Martijn van Duren wrote:
> >
> > > We currently have NetBSD's behaviour when it comes to to no characters,
> > > so no need to change there imho, but the 2 character limit seems like
> > > a good place to stop, especially since there's no way to break out of
> > > this hungry hungry hippo.
> > >
> > > limiter inspiration taken from octal notation.
> > >
> > > While here, also document \x notation.
> >
> > No objection from me but I'm not sure it needs to make 6.9.
> >
> >  - todd
> I found a (different) bug in NetBSD's implementation, which got fixed by
> christos. But he also added a check that at least 1 isxdigit(3) needs to
> be present. From the commit message:
> Also add a warning if the conversion fails (like the gnu printf does)
>
> Do we want something similar now that we are the odd ones out, or keep
> the diff as is? I still think that the current diff is fine, but since
> the landscape changed I wanted to throw it out there.
>
> martijn@
>
>


Re: rw_lock_held() & friends

2020-12-07 Thread Philip Guenther
On Mon, Dec 7, 2020 at 11:16 AM Alexandr Nedvedicky <
alexandr.nedvedi...@oracle.com> wrote:

> What's the plan for rw_write_held()? Currently macro is true, if whoever
> holds
> the lock.
>
> >
> > +#define  rw_write_held(rwl)  (rw_status(rwl) == RW_WRITE)
>

Nope.  rw_status() returns RW_WRITE_OTHER if a different thread holds the
lock.


Philip


Re: Use SMR_TAILQ for `ps_threads'

2020-12-02 Thread Philip Guenther
On Tue, Dec 1, 2020 at 5:48 AM Martin Pieuchot  wrote:
...

> exit1() for a multi-threaded process does the following:
>
> atomic_setbits_int(>p_flag, P_WEXIT);
> single_thread_check(p, 0);  // directly or via single_thread_set()
> SMR_TAILQ_REMOVE_LOCKED(>ps_threads, p, p_thr_link);
>
> Note that exit1() can't be called in parallel.
>

Well, multiple threads in a process can easily hit exit1() practically
concurrently, under the limitations of the kernel lock.  If anything in
exit1() sleeps, it has to be ready for some other thread to start into
exit1()!


If exit1() is called with EXIT_NORMAL, the current thread will start by
> setting `ps_single' then iterate over `ps_thread'.  This is done before
> updating `ps_thread' so there's no race in that case.
>
> If exit1() is called with EXIT_THREAD, the current thread might end up
> removing itself from `ps_thread' while another one is iterating over it.
> Since we're currently concerned about the iteration in single_thread_set()
> notice that `P_WEXIT' is checked first.  So if my understanding is correct
> is should be enough to do the following:
>
> SMR_TAILQ_REMOVE_LOCKED(>ps_threads, p, p_thr_link);
> smr_barrier();
>
> That would delays exit1() a bit but doesn't involve callback which is
> IMHO simpler.
>
> Did I miss anything?
>

What does it take to be sure that an "is empty" (or "only contains one")
test is _really_ true with SMR?

Philip


Re: stdio: fclose(3), fopen(3): intended locking hierarchy?

2020-11-30 Thread Philip Guenther
On Mon, Nov 30, 2020 at 6:10 PM Scott Cheloha 
wrote:

> On Sat, Nov 28, 2020 at 01:41:50PM -0800, Philip Guenther wrote:
>
...

> > After thinking through states more, #4 isn't safe: _fwalk() can't take
> > sfp_mutex because it's called from __srefill() which has its FILE locked,
> > which would reverse the order: two concurrent calls to __srefill() from
> > line-buffered FILEs could have one in _fwalk() blocking on locking the
> > other, while the other blocks on the sfp_mutex for _fwalk().
>
> This part in __srefill():
>
> /*
>  * Before reading from a line buffered or unbuffered file,
>  * flush all line buffered output files, per the ANSI C
>  * standard.
>  */
>
...

> Where in the standard(s) is this behavior required?  I'm not even sure
> how to look for it.
>

Pick a version of the C standard and search for "buffered" until you find
something like this, which is part of 7.19.3p3 in the draft I'm looking at:

   <...>  When a stream is line buffered, characters are intended to be
   transmitted to or from the host environment as a block when a new-line
character is
   encountered. Furthermore, characters are intended to be transmitted as a
block to the host
   environment when a buffer is filled, when input is requested on an
unbuffered stream, or
   when input is requested on a line buffered stream that requires the
transmission of
   characters from the host environment. Support for these characteristics
is
   implementation-defined, and may be affected via the setbuf and setvbuf
functions.

Effectively the same text appears in the POSIX standard in XSH 2.5p6.

Basically, you're allowed to stop doing that by instead printing out your
cell-phone number so that everyone who wants to complain that their program
failed to output its prompt before blocking for input can call and scream
at you.  :D


Philip Guenther


Re: kvm_getfiles and KERN_FILE_BYFILE

2020-11-29 Thread Philip Guenther
On Sun, Nov 29, 2020 at 12:14 PM Martijn van Duren <
openbsd+t...@list.imperialat.at> wrote:

> On Sat, 2020-11-28 at 16:23 -0800, Philip Guenther wrote:
> > On Thu, Nov 26, 2020 at 1:08 PM Martijn van Duren <
> openbsd+t...@list.imperialat.at> wrote:
> > > I'm currently playing around a bit with kvm_getfiles and found that I
> > > couldn't use KERN_FILE_BYFILE with DTYPE_SOCKET.
> > > According to kvm_getfiles(3):
> > >  For KERN_FILE_BYFILE the recognized file types are defined in
> > >  :
> > >
> > >DTYPE_VNODE   files and devices
> > >DTYPE_SOCKET  sockets, regardless of domain
> > >DTYPE_PIPEpipes and FIFOs
> > >DTYPE_KQUEUE  kqueues
> > >
> > > But these defines are under ifdef _KERNEL.
> > >
> > > So is the manpage lying here, or should the defines be hoisted out
> > > of the ifdef?
> > >
> >
> >
> > Let's go ahead and hoist them: FreeBSD and NetBSD already have.  If
> possible, the diff to do that should also simplify the #include bits in
> these files:
> > usr.bin/netstat/inet.c
> > usr.bin/fstat/fstat.c
> > usr.bin/fstat/fuser.c
> > usr.bin/systat/netstat.c
> >
> >
> > Philip Guenther
> >
>
> The others have the #endif/#ifdef break rather low in the file.
> Personally I reckon it's better reading if the common code is more
> towards the top.
>
> OK?
>

ok guenther@

How do the userland clean up bits look?


Philip Guenther


Re: kvm_getfiles and KERN_FILE_BYFILE

2020-11-28 Thread Philip Guenther
On Thu, Nov 26, 2020 at 1:08 PM Martijn van Duren <
openbsd+t...@list.imperialat.at> wrote:

> I'm currently playing around a bit with kvm_getfiles and found that I
> couldn't use KERN_FILE_BYFILE with DTYPE_SOCKET.
> According to kvm_getfiles(3):
>  For KERN_FILE_BYFILE the recognized file types are defined in
>  :
>
>DTYPE_VNODE   files and devices
>DTYPE_SOCKET  sockets, regardless of domain
>DTYPE_PIPEpipes and FIFOs
>DTYPE_KQUEUE  kqueues
>
> But these defines are under ifdef _KERNEL.
>
> So is the manpage lying here, or should the defines be hoisted out
> of the ifdef?
>

Let's go ahead and hoist them: FreeBSD and NetBSD already have.  If
possible, the diff to do that should also simplify the #include bits in
these files:
usr.bin/netstat/inet.c
usr.bin/fstat/fstat.c
    usr.bin/fstat/fuser.c
usr.bin/systat/netstat.c


Philip Guenther


Re: stdio: fclose(3), fopen(3): intended locking hierarchy?

2020-11-28 Thread Philip Guenther
On Fri, Nov 27, 2020 at 10:35 PM Philip Guenther  wrote:
...

> So yeah, maybe it does work to:
> 1) make __sfp() FLOCKFILE() the allocated FILE before unlocking sfp_mutex
> 2) update f{,d,mem,un}open() and open_*memstream() to match (1), unlocking
>in all paths when the FILE is safe to be accessed by _fwalk(), and
> locking
>sfp_mutex around the zeroing of _flags.
> 3) make fclose() and freopen() also lock sfp_mutex around the zeroing of
> _flags
>(should add an _frelease() to findfp.c that does this dance for (2) and
> (3))
> 4) make _fwalk() take sfp_mutex, and maybe also a FILE* so the setting of
>__SIGN can be done under the lock?
>
5) decide how/whether to handle adjust the FLOCKFILE placement in the
> _fwalk()
>tree: is the testing of the "is line-buffered" flag in lflush() safe
> without
>a lock?  Mumble...
>

After thinking through states more, #4 isn't safe: _fwalk() can't take
sfp_mutex because it's called from __srefill() which has its FILE locked,
which would reverse the order: two concurrent calls to __srefill() from
line-buffered FILEs could have one in _fwalk() blocking on locking the
other, while the other blocks on the sfp_mutex for _fwalk().

Hmm, there's currently a loop between two __srefill() calls like that, as
there's nothing to force visibility of the __SIGN flag between CPUs so they
could try to lock each other.  Grrr.

Time to check other BSDs and see if they have a good solution to this...


Philip





>
> Now, back to that first assumption: if you're not willing to assume
> it then the "is allocated" test needs to not use _flags but be some
> other state change which can be relied on to not have false-positives,
> but otherwise the other bits above still apply, I believe.  Would
> be a major ABI change...and if we do that there's like 3 other
> things we should do at the same time (merge __sfileext into FILE,
> grow _file to an int, and maybe move the recursive mutex for
> f{,un}lockfile() into FILE?)
>
>
> Philip Guenther
>
>


Re: stdio: fclose(3), fopen(3): intended locking hierarchy?

2020-11-27 Thread Philip Guenther
ted" test needs to not use _flags but be some
other state change which can be relied on to not have false-positives,
but otherwise the other bits above still apply, I believe.  Would
be a major ABI change...and if we do that there's like 3 other
things we should do at the same time (merge __sfileext into FILE,
grow _file to an int, and maybe move the recursive mutex for
f{,un}lockfile() into FILE?)


Philip Guenther


Re: Fix ilogb(3)

2020-11-07 Thread Philip Guenther
On Fri, Nov 6, 2020 at 4:51 PM George Koehler  wrote:

> Your ilogb fix is ok gkoehler@
>

It's annoying that C and/or ieee754 and the original hardware
implementation in the x87 instructions diverged in their definitions, but
the former is what matters and libm needs to follow that.  ok guenther@



> On Sat, 31 Oct 2020 16:09:07 +0100 (CET)
> Mark Kettenis  wrote:
>
> > - Dropping the amd64 and i386 versions.  Fixing the corner cases in
> >   assembly is hard, and the C implementation should be fast enough for
> >   regular floating-point values.
>
> The amd64 and i386 assembly uses the x87 fxtract instruction.  I feel
> that x87 instructions are obsolete on amd64.  <...>


Umm, no?  The amd64 ABI defines "long double" as matching the format used
by the x87 instructions; a function returning that type returns it in the
x87 %st(0) register and a review of libm finds many of the functions are
naturally implemented by the native x87 instructions.

I believe the main issue in this case is that the standard evolved away
from what Kahan originally did as a consultant with Intel.  For a note on
how the ieee754 standard originated in (large) part from what Kahan did
with Intel, see
  https://people.eecs.berkeley.edu/~wkahan/ieee754status/754story.html

I'm not nearly enough of a numerical analyst to judge the decision of the
standard.


Philip Guenther


Re: powerpc ld.lld fix

2020-11-01 Thread Philip Guenther
Makes sense.  This code is just the space reservation, the relocation
generation or whatever fills them in is suppressed already, yes?  Assuming
so, r+

On Sat, Oct 31, 2020 at 2:46 PM Mark Kettenis 
wrote:

> > Date: Sat, 10 Oct 2020 23:19:19 +0200 (CEST)
> > From: Mark Kettenis 
> >
> > On powerpc with the secure-plt ABI we need a .got section, even if the
> > _GLOBAL_OFFSET_TABLE_ symbol isn't referenced.  This is needed because
> > the first three entries of the GOT are used by the dynamic linker.
> >
> > With this fix I can build executables of all flavours (including
> > -static/-nopie).
>
> Turns out that adding these GOT entries when using "ld -r" is a bad
> idea.  Dif below fixes that.
>
> ok?
>
>
> Index: gnu/llvm/lld/ELF/SyntheticSections.cpp
> ===
> RCS file: /cvs/src/gnu/llvm/lld/ELF/SyntheticSections.cpp,v
> retrieving revision 1.2
> diff -u -p -r1.2 SyntheticSections.cpp
> --- gnu/llvm/lld/ELF/SyntheticSections.cpp  11 Oct 2020 13:10:13
> -  1.2
> +++ gnu/llvm/lld/ELF/SyntheticSections.cpp  31 Oct 2020 23:37:11 -
> @@ -604,7 +604,7 @@ GotSection::GotSection()
>// ElfSym::globalOffsetTable.
>if (ElfSym::globalOffsetTable && !target->gotBaseSymInGotPlt)
>  numEntries += target->gotHeaderEntriesNum;
> -  else if (config->emachine == EM_PPC)
> +  else if (config->emachine == EM_PPC && !config->relocatable)
>  numEntries += target->gotHeaderEntriesNum;
>  }
>
>
>


Re: Diff: Introductory Clause Comma Crap

2020-10-31 Thread Philip Guenther
On Sat, Oct 31, 2020 at 10:19 AM VARIK VALEFOR  wrote:

> This message contains even more grammatical fixes for the OpenBSD
> manual pages.  To ensure that the changes which are proposed in this
> message can be justified, this message primarily fixes only a single
> type of error: the absence of commas after introductory clauses.
>

As a procedural side-note, I would like to suggest that before going on a
quest to make a change that touches so many files cross OpenBSD's code
base, that it would be wise to send out a diff with just a couple examples
and verify that the particular item of concern and the proposed fix is
agreed upon before spending the time to search and edit many other pages.

This is true not just for documentation changes but code changes, of
course: doing lots of work before there's "buy-in" is risking your time.


Philip Guenther


Re: [PATCH] ifconfig: keep track of allowed ips pointer correctly

2020-10-27 Thread Philip Guenther
On Tue, Oct 27, 2020 at 9:18 AM Jason A. Donenfeld  wrote:
...

> @@ -5721,16 +5720,18 @@ growwgdata(size_t by)
> if (wg_aip != NULL)
> wg_aip = (void *)wg_interface + aip_offset;
>
> -   ret = (void *)wg_interface + wgdata.wgd_size - by;
> -   bzero(ret, by);
> -
> -   return ret;
> +   bzero((void *)wg_interface + wgdata.wgd_size - by, by);
>  }
>

Total side note, but this caught my eye as relying on the gcc extension to
do pointer arithmetic on "void *" pointers.  At least historically we've
avoided relying on that, which is easy in this case by just casting to
"char *" instead.

Philip Guenther


Re: [patch] const cclasses reference in ksh

2020-10-26 Thread Philip Guenther
On Mon, Oct 26, 2020 at 8:35 AM Matthew Martin  wrote:

> Recently cclasses in lib/libc/gen/charclass.h was made const.[1]
> Mark the pointer used to walk the array in ksh const as well.
>
> 1: https://marc.info/?l=openbsd-cvs=160256416506433=2
>

Ugh, I totally missed that reach-around.

ok guenther@



> diff --git misc.c misc.c
> index 9e6e9db5e76..7226f74eccf 100644
> --- misc.c
> +++ misc.c
> @@ -713,7 +713,7 @@ do_gmatch(const unsigned char *s, const unsigned char
> *se,
>  static int
>  posix_cclass(const unsigned char *pattern, int test, const unsigned char
> **ep)
>  {
> -   struct cclass *cc;
> +   const struct cclass *cc;
> const unsigned char *colon;
> size_t len;
> int rval = 0;
>
>


Re: Infinite loop in uvm protection mapping

2020-10-19 Thread Philip Guenther
On Mon, Oct 19, 2020 at 3:13 PM Tom Rollet  wrote:

> Hi,
>
> I'm starting to help in the development of the dt device.
>
> I'm stuck on permission handling of memory. I'm trying to allocate a
> page in kernel with read/write protections, fill the allocated page
> with data then change the permissions to  read/exec.
>
> Snippet of my code:
>
>   addr = uvm_km_alloc(kernel_map, PAGE_SIZE);
>
>  [...] (memcpy data in allocated page)
>
>   uvm_map_protect(kernel_map, addr, addr + PAGE_SIZE, PROT_READ
>  | PROT_EXEC, FALSE)))
>

This is same usage as seen in the 'sti' driver...which is on hppa only, so
while it's presumably the correct usage of uvm_km_alloc() and
uvm_map_protect(), I don't think uvm_map_protect() has been used on
kernel-space on amd64 (or possibly all non-hppa archs) before in OpenBSD.
Whee?


It triggers the following error at boot time when executing
> the uvm_map_protect function.
>
> uvm_fault(0x81fb2c90, 0x7ffec0008000, 0, 2) -> e kernel: page fault
> trap, code=0 Stopped atpmap_write_protect+0x1f5:  lock andq
> $-0x3,0(%rdi)
>
> Trace:
>
> pmap_write_protect(82187b28,80002255b000,80002255c000,
>  5,50e8b70481f4f622,fd81b6567e70) at pmap_write_protect+0x212
> uvm_map_protect(82129ae0,80002255b000,80002255c000
>  ,5,0,82129ae0) at uvm_map_protect+0x501
> dt_alloc_kprobe(815560e0,80173900,e7ef01a2855152cc,
>  82395c98,0,815560e0) at dt_alloc_kprobe+0x1ff
> dt_prov_kprobe_init(2333e28db00d3edd,0,82121150,0,0,
>  824d9008) at dt_prov_kprobe_init+0x1d9
> dtattach(1,821fb384,f,1,c2ee1c3f472154e,2dda28) at dtattach+0x5d
> main(0,0,0,0,0,1) at main+0x419
>
> The problem comes from the loop in pmap_write_protect
> (sys/arch/amd64/amd64/pmap.c:2108) that is executed
> infinity in my case.
>
> Entry of function pmap_write_protect:
>  sva:  80002250A000
>  eva:  80002250B000
>
> After &= PG_FRAME (line 2098-2099)
>  sva= F80002250A000
>  eva= F80002250B000
>
>   loop:  (ligne 2108)
>
>   first iteration:
>  va   = F80002250A000
>  eva = F80002250B000
>  blockend = 080012240
>
...

> Does anyone have an idea how to fix this issue?


So, blockend is clearly wrong for va and eva.  I suspect the use of
L2_FRAME here:
   blockend = (va & L2_FRAME) + NBPD_L2;

is wrong here and it should be
   blockend = (va & VA_SIGN_NEG(L2_FRAME)) + NBPD_L2;

or some equivalent expression to keep all the bits above the frame.


Philip Guenther


Re: incorrect result from getppid for ptraced processes

2020-09-04 Thread Philip Guenther
On Fri, Sep 4, 2020 at 2:59 PM Mateusz Guzik  wrote:

> On 9/5/20, Philip Guenther  wrote:
> > On Fri, Sep 4, 2020 at 1:06 PM Mateusz Guzik  wrote:
> >
> >> On 9/4/20, Vitaliy Makkoveev  wrote:
> >> > On Fri, Sep 04, 2020 at 05:24:42PM +0200, Mateusz Guzik wrote:
> >> >> getppid blindly follows the parent pointer and reads the pid.
> >> >>
> >> >> The problem is that ptrace reparents the traced process, so in
> >> >> particular if you gdb -p $something, the target proc will start
> seeing
> >> >> gdb instead of its actual parent.
> >> >>
> >> >> There is a lot to say about the entire reparenting business or
> storing
> >> >> the original pid in ps_oppid (instead of some form of a reference to
> >> >> the process).
> >> >>
> >> >> However, I think the most feasible fix for now is the same thing
> >> >> FreeBSD did: *always* store the actual parent pid in ps_oppid. This
> >> >> means all repareting will keep updating it (most notably when
> >> >> abandoning children on exit), while ptrace will skip that part.
> >> >>
> >> >> Side effect of such a change be that getppid will stop requiring the
> >> >> kernel lock.
> >> >>
> >> >
> >> > Thanks for report. But we are in beta stage now so such modification
> is
> >> > impossible until next iteration.
> >> >
> >> > Since original parent identifier is stored as `ps_oppid' while process
> >> > is traced we just return it to userland for this case. This is the way
> >> > I
> >> > propose to fix this bug for now.
> >> >
> >> > Comments? OKs?
> >> >
> >> > Index: sys/kern/kern_prot.c
> >> > ===
> >> > RCS file: /cvs/src/sys/kern/kern_prot.c,v
> >> > retrieving revision 1.76
> >> > diff -u -p -r1.76 kern_prot.c
> >> > --- sys/kern/kern_prot.c  9 Jul 2019 12:23:25 -   1.76
> >> > +++ sys/kern/kern_prot.c  4 Sep 2020 21:12:15 -
> >> > @@ -84,7 +84,11 @@ int
> >> >  sys_getppid(struct proc *p, void *v, register_t *retval)
> >> >  {
> >> >
> >> > - *retval = p->p_p->ps_pptr->ps_pid;
> >> > + if (p->p_p->ps_flags & PS_TRACED)
> >> > + *retval = p->p_p->ps_oppid;
> >> > + else
> >> > + *retval = p->p_p->ps_pptr->ps_pid;
> >> > +
> >> >   return (0);
> >> >  }
> >>
> >> This is definitely a bare minimum fix, but it does the job.
> >>
> >
> > ptrace() has behaved like this for the life of OpenBSD and an indefinite
> > number of years previous in the BSD releases.  What has happened that a
> > definitely incomplete fix is needed Right Now?
>
> I don't see how this reads as a demand this is fixed Right Now.
>

I didn't call it a demand, but the point stands: what has changed?


I don't see how the fix is incomplete either. It can be done better
> with more effort, but AFAICS the above results in correct behavior.
>

There are at least 2 other uses of ps_pptr->ps_pid that should also change,
unless you like coredumps and ps disagreeing with getppid(), and someone
needs to think how it affects doas.

Philip Guenther


Re: incorrect result from getppid for ptraced processes

2020-09-04 Thread Philip Guenther
On Fri, Sep 4, 2020 at 1:06 PM Mateusz Guzik  wrote:

> On 9/4/20, Vitaliy Makkoveev  wrote:
> > On Fri, Sep 04, 2020 at 05:24:42PM +0200, Mateusz Guzik wrote:
> >> getppid blindly follows the parent pointer and reads the pid.
> >>
> >> The problem is that ptrace reparents the traced process, so in
> >> particular if you gdb -p $something, the target proc will start seeing
> >> gdb instead of its actual parent.
> >>
> >> There is a lot to say about the entire reparenting business or storing
> >> the original pid in ps_oppid (instead of some form of a reference to
> >> the process).
> >>
> >> However, I think the most feasible fix for now is the same thing
> >> FreeBSD did: *always* store the actual parent pid in ps_oppid. This
> >> means all repareting will keep updating it (most notably when
> >> abandoning children on exit), while ptrace will skip that part.
> >>
> >> Side effect of such a change be that getppid will stop requiring the
> >> kernel lock.
> >>
> >
> > Thanks for report. But we are in beta stage now so such modification is
> > impossible until next iteration.
> >
> > Since original parent identifier is stored as `ps_oppid' while process
> > is traced we just return it to userland for this case. This is the way I
> > propose to fix this bug for now.
> >
> > Comments? OKs?
> >
> > Index: sys/kern/kern_prot.c
> > ===
> > RCS file: /cvs/src/sys/kern/kern_prot.c,v
> > retrieving revision 1.76
> > diff -u -p -r1.76 kern_prot.c
> > --- sys/kern/kern_prot.c  9 Jul 2019 12:23:25 -   1.76
> > +++ sys/kern/kern_prot.c  4 Sep 2020 21:12:15 -
> > @@ -84,7 +84,11 @@ int
> >  sys_getppid(struct proc *p, void *v, register_t *retval)
> >  {
> >
> > - *retval = p->p_p->ps_pptr->ps_pid;
> > + if (p->p_p->ps_flags & PS_TRACED)
> > + *retval = p->p_p->ps_oppid;
> > + else
> > +     *retval = p->p_p->ps_pptr->ps_pid;
> > +
> >   return (0);
> >  }
>
> This is definitely a bare minimum fix, but it does the job.
>

ptrace() has behaved like this for the life of OpenBSD and an indefinite
number of years previous in the BSD releases.  What has happened that a
definitely incomplete fix is needed Right Now?


Philip Guenther


Re: timekeep: fixing large skews on amd64 with RDTSCP

2020-07-17 Thread Philip Guenther
On Thu, Jul 16, 2020 at 4:55 PM Scott Cheloha 
wrote:

> > On Jul 16, 2020, at 19:36, Theo de Raadt  wrote:
> >
> >> Note the third sentence.
> >>
> >> Given that, I reason that a serializing instruction before *and* after
> >> the RDTSC should freeze it in place.
> >
> > I haven't seen anyone read it that way.
>
> They say that instructions after RDTSC can run before it because
> it isn't a serializing instruction.
>
> Do we want that?
>
> And then, consider this bit of programming advice.  Also from the
> ISA reference (Vol. 2B 4-547):
>
> > If software requires RDTSC to be executed prior to execution of any
> > subsequent instruction (including any memory accesses), it can
> > execute the sequence LFENCE immediately after RDTSC.
>
> What other reading is possible given this documentation?
>
> What is your interpretation?  Am I missing something?
>

If you're trying to time a sequence of instructions via rdtsc, then you
have to put an lfence after the first rdtsc (so that the sequence being
timed doesn't get lifted to before it completes) and an lfence before the
second rdtsc (so that the sequence is actually complete before the second
one).  In this case, is it a problem if instructions after the rdtsc that
are not dependent on its result may be started before it's actually
complete?  If not, then there's no reason to bracket that side.


Philip Guenther


Re: disable libc sys wrappers?

2020-07-09 Thread Philip Guenther
On Wed, Jul 8, 2020 at 8:06 AM Theo de Raadt  wrote:

> Mark Kettenis  wrote:
>
> > > From: "Theo de Raadt" 
> > > Date: Wed, 08 Jul 2020 09:42:41 -0600
> > >
> > > I think we need something like this.
> > >
> > > Documenting it will be a challenge.
> > >
> > > I really don't like the name as is too generic, when the control is
> only
> > > for a narrow set of "current time" system calls.
> >
> > I'm not sure we should be using getenv() in this early initialization
> > function though.
>
> Ah, you worry about the static "#ifndef PIC / early_static_init" versus
> "PIC ld.so" environ setup, and this very early getenv() call might not be
> looking at the environ global.
>

It's late enough in the process (after a possible call
to early_static_init(), and definitely after any fixup by ld.so) that it
should work Just Fine.

I would flip the test to check the environment before running issetugid(2)
because the syscall is more expensive and it's nice not to clutter the
kdump output.  ;-)


Philip Guenther


Re: SSE in kernel?

2020-06-23 Thread Philip Guenther
On Mon, Jun 22, 2020 at 9:12 PM  wrote:

> Are SSE instructions allowed in the AMD64 kernel?  Is #ifdef __SSE__
> a sufficient guard?
>
> I have a rasops32 putchar with SSE that is 2x faster.
>

As Bryan and Patrick noted: it's possible, but there are restrictions and
costs.

The main restriction is that the code must not permit a context-switch
between the fpu_kernel_enter() and fpu_kernel_exit() calls.  No taking an
rwlock or calling any of the sleep functions, for example.

If you're using more than the minimal level of SSE which is already
required by the kernel (for lfence, etc) then you should also check whether
the necessary extension bits are present in curcpu()->ci_feature_* and fall
back to the current code if not present.

The cost is that if the thread doing this isn't a system thread, then the
first fpu_kernel_enter() call after the userspace->kernel transition has to
save and reset the FPU registers (XSAVEOPT + XRSTOR on newish CPUs).  Every
fpu_kernel_exit(), regardless of thread type, resets them again (XRSTOR).

If the restriction isn't a problem and the cost of those is worth the gain,
then sure, go for it.  We already do it for AES stuff in the kernel, for
example.  c.f. /usr/src/sys/arch/amd64/amd64/aesni.c


Philip Guenther


Re: EV_SET(2) shadows variable

2020-04-05 Thread Philip Guenther
On Sat, 4 Apr 2020, Theo de Raadt wrote:
> Philip Guenther  wrote:
> 
> > On Fri, 3 Apr 2020, Martin Pieuchot wrote:
> > > Thanks, here it is, ok?
> > 
> > ok guenther@
> 
> Should we do the same to all other macros, just in case?

Checking /usr/include/{,sys/}*.h, the diff below fixes the only ones I 
found to be potential problems

/usr/include/net* and some others have not-completely-safe macros, like 
IP6_EXTHDR_GET()

Index: include/bitstring.h
===
RCS file: /data/src/openbsd/src/include/bitstring.h,v
retrieving revision 1.5
diff -u -p -r1.5 bitstring.h
--- include/bitstring.h 2 Jun 2003 19:34:12 -   1.5
+++ include/bitstring.h 6 Apr 2020 00:37:52 -
@@ -83,46 +83,46 @@ typedef unsigned char bitstr_t;
 
/* clear bits start ... stop in bitstring */
 #definebit_nclear(name, start, stop) do { \
-   register bitstr_t *_name = name; \
-   register int _start = start, _stop = stop; \
-   while (_start <= _stop) { \
-   bit_clear(_name, _start); \
-   _start++; \
+   register bitstr_t *__name = (name); \
+   register int ___start = (start), __stop = (stop); \
+   while (__start <= __stop) { \
+   bit_clear(__name, __start); \
+   __start++; \
} \
 } while(0)
 
/* set bits start ... stop in bitstring */
 #definebit_nset(name, start, stop) do { \
-   register bitstr_t *_name = name; \
-   register int _start = start, _stop = stop; \
-   while (_start <= _stop) { \
-   bit_set(_name, _start); \
-   _start++; \
+   register bitstr_t *__name = (name); \
+   register int __start = (start), __stop = (stop); \
+   while (__start <= __stop) { \
+   bit_set(__name, __start); \
+   __start++; \
} \
 } while(0)
 
/* find first bit clear in name */
 #definebit_ffc(name, nbits, value) do { \
-   register bitstr_t *_name = name; \
-   register int _bit, _nbits = nbits, _value = -1; \
-   for (_bit = 0; _bit < _nbits; ++_bit) \
-   if (!bit_test(_name, _bit)) { \
-   _value = _bit; \
+   register bitstr_t *__name = (name); \
+   register int __bit, __nbits = (nbits), __value = -1; \
+   for (__bit = 0; __bit < __nbits; ++__bit) \
+   if (!bit_test(__name, __bit)) { \
+   __value = __bit; \
break; \
} \
-   *(value) = _value; \
+   *(value) = __value; \
 } while(0)
 
/* find first bit set in name */
 #definebit_ffs(name, nbits, value) do { \
-   register bitstr_t *_name = name; \
-   register int _bit, _nbits = nbits, _value = -1; \
-   for (_bit = 0; _bit < _nbits; ++_bit) \
-   if (bit_test(_name, _bit)) { \
-   _value = _bit; \
+   register bitstr_t *__name = (name); \
+   register int __bit, __nbits = (nbits), __value = -1; \
+   for (__bit = 0; __bit < __nbits; ++__bit) \
+   if (bit_test(__name, __bit)) { \
+   __value = __bit; \
break; \
} \
-   *(value) = _value; \
+   *(value) = __value; \
 } while(0)
 
 #endif /* !_BITSTRING_H_ */
Index: sys/sys/disklabel.h
===
RCS file: /data/src/openbsd/src/sys/sys/disklabel.h,v
retrieving revision 1.75
diff -u -p -r1.75 disklabel.h
--- sys/sys/disklabel.h 24 Oct 2017 09:36:13 -  1.75
+++ sys/sys/disklabel.h 6 Apr 2020 00:52:08 -
@@ -156,37 +156,37 @@ struct__partitionv0 { /* old (v0) part
 
 #define DL_GETPSIZE(p) (((u_int64_t)(p)->p_sizeh << 32) + (p)->p_size)
 #define DL_SETPSIZE(p, n)  do { \
-   u_int64_t x = (n); \
-   (p)->p_sizeh = x >> 32; \
-   (p)->p_size = x; \
+   u_int64_t __x = (n); \
+   (p)->p_sizeh = __x >> 32; \
+   (p)->p_size = __x; \
} while (0)
 #define DL_GETPOFFSET(p)   (((u_int64_t)(p)->p_offseth << 32) + 
(p)->p_offset)
 #define DL_SETPOFFSET(p, n)do { \
-   u_int64_t x = (n); \
-   (p)->p_offseth = x >> 32; \
-   (p)->p_offset = x; \
+   u_int64_t __x = (n); \
+   (p)->p_offseth = __x >> 32; \
+   

Re: split futex into three

2020-04-05 Thread Philip Guenther
On Sun, 5 Apr 2020, Stuart Henderson wrote:
> On 2020/04/05 10:28, Martin Pieuchot wrote:
> > Another way to proceed would be to do a port grep for futex and see what
> > the ecosystem is using.
> 
> Sorry it's not filtered, but :
> 
> https://junkpile.org/grep.futex.gz

Sure looks like the only occurence of futex() used with FUTEX_REQUEUE (== 
3) is the linux kernel test program.  Everything else, including rust, is 
using FUTEX_CMP_REQUEUE or one of the PI operations 
(FUTEX_WAIT_REQUEUE_PI, FUTEX_CMP_REQUEUE_PI).


Philip



Re: kdump futex fix

2020-04-04 Thread Philip Guenther
On Sat, 4 Apr 2020, Martin Pieuchot wrote:
> On 03/04/20(Fri) 19:26, Philip Guenther wrote:
> > On Fri, 3 Apr 2020, Martin Pieuchot wrote:
> > > Depending on the operation requested futex(2) might return the number of 
> > > woken threads or an error.  That means the following...
> > > 
> > > mpv  CALL  
> > > futex(0xa58935899b0,0x82,1,0,0)
> > > mpv  RET   futex -1 errno 1 Operation not permitted
> > > 
> > > ...is not an error but it indicates that 1 thread has been awoken.
> > > 
> > > Diff below would have saved me quite some time looking in the wrong 
> > > direction.
> > > 
> > > ok?
> > 
> > Isn't that just a complicated way to get the same effect as deleting the 
> > "case SYS_futex:" line shortly above there?
> 
> Thanks!  Updated below :)

ok guenther@



Re: split futex into three

2020-04-04 Thread Philip Guenther
On Sat, 4 Apr 2020, Paul Irofti wrote:
> Here is a proper diff (both sys and libc into one).

Okay, bunch o' comments and thoughts of varying strength of opinion.

> diff --git lib/libc/Symbols.list lib/libc/Symbols.list
> index f9aa62ab6e8..4fa37a835aa 100644
> --- lib/libc/Symbols.list
> +++ lib/libc/Symbols.list
> @@ -86,7 +86,10 @@ _thread_sys_fstatat
>  _thread_sys_fstatfs
>  _thread_sys_fsync
>  _thread_sys_ftruncate
> -_thread_sys_futex
> +_thread_sys_ofutex

The ofutex syscall should not be built (or exported) by libc. The kernel 
needs to continue to provide it for the normal period to support older 
libc's where futex(2) is a direct syscall,but no application does, would, 
or should invoke ofutex() under that name.


> +_thread_sys_futex_wait
> +_thread_sys_futex_wake
> +_thread_sys_futex_requeue

glibc has internal inline functions futex_wait() and futex_wake() and 
there has been at least discussion about exporting some version of them.  
If our signatures matched the last-best-proposal over there (which was 
dropped, mind you) then I would be tempted to use those names.  If not, 
then maybe go with _futex_wait() and _futex_wake()?

FUTEX_REQUEUE is the old bad one, with no val3 argument that's checked 
before the operation.  Our libc/libpthread don't actually use them, and in 
the Linux world glibc switched completely to FUTEX_CMP_REQUEUE.  Perhaps 
we should drop support for FUTEX_REQUEUE (major bump, yah) and add 
_futex_cmp_requeue(2) when we need it?


> --- lib/libc/gen/sigwait.c
> +++ lib/libc/gen/sigwait.c

This is unrelated to futex stuff.  :)


> --- lib/libc/hidden/sys/futex.h
> +++ lib/libc/hidden/sys/futex.h
> @@ -20,6 +20,8 @@
>  
>  #include_next 
>  
> -PROTO_NORMAL(futex);

You need to keep this as long as futex() is used internally.  Once futex() 
is no longer used in libc then this should change to 
"PROTO_DEPRECATED(futex);" and the "DEF_STRONG(futex);" line should be 
deleted.

(If you have DEF_STRONG(foo) or DEF_WEAK(foo), then you must have 
PROTO_NORMAL(foo).  c.f libc/include/README II.B.2)


> --- lib/libc/thread/synch.h
> +++ lib/libc/thread/synch.h
> @@ -19,10 +19,33 @@
>  #include 
>  #include 
>  
> +static inline int
> +_futex(volatile uint32_t *p, int op, int val, const struct timespec 
> *timeout, uint32_t *g)
> +{
> + int flags = 0;
> +
> + if (op & FUTEX_PRIVATE_FLAG)
> + flags |= FT_PRIVATE;
> +
> + switch (op) {
> + case FUTEX_WAIT:
> + case FUTEX_WAIT_PRIVATE:
> + return futex_wait(p, val, timeout, flags);
> + case FUTEX_WAKE:
> + case FUTEX_WAKE_PRIVATE:
> + return futex_wake(p, val, flags);
> + case FUTEX_REQUEUE:
> + case FUTEX_REQUEUE_PRIVATE:
> + return futex_requeue(p, val, g, timeout, flags);
> + }
> +
> + return ENOSYS;
> +}

I would put this logic directly into futex()'s definition, and then...


>  static inline int
>  _wake(volatile uint32_t *p, int n)
>  {
> - return futex(p, FUTEX_WAKE_PRIVATE, n, NULL, NULL);
> + return _futex(p, FUTEX_WAKE_PRIVATE, n, NULL, NULL);
>  }

have this just be
return futex_wake(p, n, FUTEX_WAKE_PRIVATE);

and similar for _twait() below.


(I would be inclined to use FUTEX_WAKE_PRIVATE as the flag for the new 
syscalls instead of exposing FT_PRIVATE into the userspace API.)


> --- sys/kern/sys_futex.c
> +++ sys/kern/sys_futex.c
> @@ -83,9 +83,74 @@ futex_init(void)
>  }
>  
>  int
> -sys_futex(struct proc *p, void *v, register_t *retval)
> +sys_futex_wait(struct proc *p, void *v, register_t *retval)
>  {
> - struct sys_futex_args /* {
> + struct sys_futex_wait_args /* {
> + syscallarg(uint32_t *) f;
> + syscallarg(inr) val;
> + syscallarg(const struct timespec *) timeout;
> + syscallarg(int) flags;
> + } */ *uap = v;
> + uint32_t *uaddr = SCARG(uap, f);
> + uint32_t val = SCARG(uap, val);
> + const struct timespec *timeout = SCARG(uap, timeout);
> + int flags = SCARG(uap, flags);
> +
> + KERNEL_LOCK();
> + rw_enter_write();
> + *retval = futex_wait(uaddr, val, timeout, flags);
> + rw_exit_write();
> + KERNEL_UNLOCK();
> +
> + return 0;
> +}
> +
> +int
> +sys_futex_wake(struct proc *p, void *v, register_t *retval)
> +{
> + struct sys_futex_wake_args /* {
> + syscallarg(uint32_t *) f;
> + syscallarg(int) val;
> + syscallarg(int) flags;
> + } */ *uap = v;
> + uint32_t *uaddr = SCARG(uap, f);
> + uint32_t val = SCARG(uap, val);
> + int flags = SCARG(uap, flags);
> +
> + rw_enter_write();
> + *retval = futex_wake(uaddr, val, flags);
> + rw_exit_write();
> +
> + return 0;
> +}
> +
> +int
> +sys_futex_requeue(struct proc *p, void *v, register_t *retval)
> +{
> + struct sys_futex_requeue_args /* {
> + syscallarg(uint32_t *) f;
> + syscallarg(int) val;
> + syscallarg(uint32_t *) g;
> + 

Re: kdump futex fix

2020-04-03 Thread Philip Guenther
On Fri, 3 Apr 2020, Martin Pieuchot wrote:
> Depending on the operation requested futex(2) might return the number of 
> woken threads or an error.  That means the following...
> 
> mpv  CALL  futex(0xa58935899b0,0x82,1,0,0)
> mpv  RET   futex -1 errno 1 Operation not permitted
> 
> ...is not an error but it indicates that 1 thread has been awoken.
> 
> Diff below would have saved me quite some time looking in the wrong 
> direction.
> 
> ok?

Isn't that just a complicated way to get the same effect as deleting the 
"case SYS_futex:" line shortly above there?

The real problem is that futex(2) is actually 3 different syscalls wrapped 
into one.  It was split into three then kdump could properly report 
futex_wake(2) and futex_requeue(2) as returning a count, while 
futex_wait(2) returns an errno.  The existing 'switch' in sys_futex() 
would just move to userspace's futex(3), provided for linux compat.

(And our syscalls would have proper prototypes and futex_wait() could take 
a clockid_t, and)



Re: EV_SET(2) shadows variable

2020-04-03 Thread Philip Guenther
On Fri, 3 Apr 2020, Martin Pieuchot wrote:
> Thanks, here it is, ok?

ok guenther@



Re: EV_SET(2) shadows variable

2020-04-02 Thread Philip Guenther
On Tue, Mar 31, 2020 at 11:24 PM Martin Pieuchot  wrote:

> The current form of EV_SET(2) declares a `kevp' variable.  This can
> cause to subtle bugs hard to discover if one uses a variable of the
> same to retrieve events.
>
> Diff below prefixes the locally declared variable by an underscore,
> like it it done in FD_ZERO(), which should be good enough to prevent
> surprises.
>
> Is it the right way to correct such issue?  How many underscores are
> enough?  Can the standards gurus tell me?
>

tl;dr: this should use a variable that starts with either two underbars
(__kevp) or an underbar and a capital (_Kevp).  The same is true of
FD_ZERO().

The namespace of identifiers that begin with a single underbar is only
"reserved for use as identifiers with file scope in both the ordinary and
tag name spaces".  That means it's perfectly legal for an application to
declare a *local* variable with such a name.  So, this should be legal:

#include 
#include 
#include 
#include 
int main(void)
{
struct fd_set _p;
struct kevent _kevp;
FD_ZERO(&_p):
EV_SET(&_kevp, 0, 0, 0, 0, 0, 0);
return 0;
}


...but it currently blows up on the FD_ZERO() and would blow up in the
EV_SET() with your proposed diff.


Philip Guenther


Re: ktrwriteraw & vget

2020-03-17 Thread Philip Guenther
On Tue, Mar 17, 2020 at 5:18 AM Martin Pieuchot  wrote:

> On 17/03/20(Tue) 04:02, Philip Guenther wrote:
> > On Tue, Mar 17, 2020 at 1:07 AM Martin Pieuchot  wrote:
> > [...]
> > > @@ -663,8 +668,6 @@ ktrwriteraw(struct proc *curp, struct vn
> > > LIST_FOREACH(pr, , ps_list)
> > > if (pr->ps_tracevp == vp && pr->ps_tracecred == cred)
> > > ktrcleartrace(pr);
> > > -
> > > -   vput(vp);
> > > return (error);
> > >  }
> > >
> >
> > This looks unsafe to me: isn't ktrcleartrace() only safe if the caller
> > holds a reference to the vnode?  Once ktrcleartrace() clears the
> reference
> > from the current thread's process and it goes on the freelist, can't the
> > vnode vp points to be invalidated and reused?
>
> As long as a process holds a reference to the vnode, via `ps_tracevp',
> it wont be recycle.  Only the last call of ktrcleartrace() will release
> the vnode via vrele(9).
>

...and after that last reference is released this code will continue to
walk the allprocess list, comparing a possibly-recycled pointer to
ps_tracevp pointers in the remaining processes.  Good thing that vrele(9)
is guaranteed to never sleep and thereby let another process start
ktracing, reusing that vnode.??


Re: ktrwriteraw & vget

2020-03-17 Thread Philip Guenther
On Tue, Mar 17, 2020 at 1:07 AM Martin Pieuchot  wrote:

> On 16/03/20(Mon) 14:01, Martin Pieuchot wrote:
> > vget(9) might fail, stop right away if that happens.
> >
> > CID 1453020 Unchecked return value.
>
> Updated diff that stops tracing if vget(9) fails, similar to what's
> currently done if VOP_WRITE(9) fails, suggested by visa@.
>
> Code shuffling is there to avoid calling vput(9) if vget(9) doesn't
> succeed.
>
> ok?
>
> Index: kern/kern_ktrace.c
> ===
> RCS file: /cvs/src/sys/kern/kern_ktrace.c,v
> retrieving revision 1.100
> diff -u -p -r1.100 kern_ktrace.c
> --- kern/kern_ktrace.c  6 Oct 2019 16:24:14 -   1.100
> +++ kern/kern_ktrace.c  17 Mar 2020 09:46:03 -
> @@ -649,12 +649,17 @@ ktrwriteraw(struct proc *curp, struct vn
> auio.uio_iovcnt++;
> auio.uio_resid += kth->ktr_len;
> }
> -   vget(vp, LK_EXCLUSIVE | LK_RETRY);
> +   error = vget(vp, LK_EXCLUSIVE | LK_RETRY);
> +   if (error)
> +   goto bad;
> error = VOP_WRITE(vp, , IO_UNIT|IO_APPEND, cred);
> -   if (!error) {
> -   vput(vp);
> -   return (0);
> -   }
> +   vput(vp);
> +   if (error)
> +   goto bad;
> +
> +   return (0);
> +
> +bad:
> /*
>  * If error encountered, give up tracing on this vnode.
>  */
> @@ -663,8 +668,6 @@ ktrwriteraw(struct proc *curp, struct vn
> LIST_FOREACH(pr, , ps_list)
> if (pr->ps_tracevp == vp && pr->ps_tracecred == cred)
> ktrcleartrace(pr);
> -
> -   vput(vp);
> return (error);
>  }
>

This looks unsafe to me: isn't ktrcleartrace() only safe if the caller
holds a reference to the vnode?  Once ktrcleartrace() clears the reference
from the current thread's process and it goes on the freelist, can't the
vnode vp points to be invalidated and reused?

Maybe this vget() should be split into vref() + vn_lock(), and the vput()
similarly split into VOP_UNLOCK() + vrele(), so the vrele() can be left
after this LIST_FOREACH() while the VOP_UNLOCK() is hoisted to the
"vn_lock() succeeded" path.


Philip Guenther


Re: posix_openpt: allow O_CLOEXEC

2020-02-06 Thread Philip Guenther
On Thu, Feb 6, 2020 at 11:38 AM joshua stein  wrote:

> On Thu, 06 Feb 2020 at 11:21:11 -0700, Todd C. Miller wrote:
> > On Thu, 06 Feb 2020 10:45:44 -0700, "Theo de Raadt" wrote:
> >
> > > That feels better, and will be more atomic.
> >
> > Unfortunately, we can't do this in ptmioctl() since we don't have
> > the index of the open ptm device to use to check for the presence
> > of UF_EXCLOSE.  So it has to be done in libc instead.
>
> Alright, so like this then?
>

I'm sorry, but this is only superficially correct: the otherwise
unobtainable value of O_CLOEXEC (and SOCK_CLOEXEC) is that it's atomic with
the creation of the fd: there is no window where another thread calling
execve() or fork() will catch the fd without the flag set.  Setting the
flag in the libc code doesn't provide that guarantee.

I see two ways to do this in a way that provides the guarantee without
breaking the existing ioctl(PTMGET) ABI:

1) rename PTMGET to PTMGET_O66; add PTMGET which *always* sets FD_CLOEXEC
on both fds and change libc to instead _clear_ FD_CLOEXEC when O_CLOEXEC is
not passed.  Downside: extra syscalls in the common case

2) rename PTMGET to PTMGET_O66 and struct ptmget to struct ptmget_o66;
define a new struct ptmget which includes an "int flags" member which can
be used to pass O_* flags that should be applied by the kernel to both
control and slave fd before returning; change libc to set that from its
flags argument.

2b) bonus: do (2) but also provide a flag which tells the kernel to *not
open* the slave, and use that in posix_openpt(), fixing a subtle bug in our
posix_openpt()'s behavior

I guess I meant three ways.


Side note: posix_openpt() should, like openpty(), unconditionally pass
O_CLOEXEC when opening /dev/ptm.


Philip Guenther


Re: waitpid vs ptrace

2020-01-30 Thread Philip Guenther
On Thu, 30 Jan 2020, Marc Espie wrote:
> So, basically, if we start arbitrary commands, then
> the classic loop 
> /* Wait for the child to exit.  */
> while (waitpid(cpid, , 0) == -1 && errno == EINTR)
>   continue;
> 
> is not quite enough.
> 
> See the small note in manpages (not only us, but everyone):
>  WIFSTOPPED(status)
>  True if the process has not terminated, but has stopped and can
>  be restarted.  This macro can be true only if the wait call
>  specified the WUNTRACED option or if the child process is being
>  traced (see ptrace(2)).
> 
> this means that somebody could run a command that forks, and then its child
> (our grand-child) could use ptrace on its parent, and then we would get
> a notification with WIFSTOPPED (assuming the current kernel bug is fixed).

No, what you describe would not cause waitpid(2) to return in the 'make' 
process: when ptrace(PT_ATTACH) is used, then target is effectively 
reparented to the tracing process such that the tracing process is the one 
which sees the wait() status changes, including the "stopped for tracing" 
state.  Only when it is detached (and thus either continued or killed!) 
will the make process see any wait statuses; at that point they will be 
normal wait statuses.


The case you describe _can_ happen if make's direct child simply calls 
ptrace(PT_TRACE_ME) and then takes a signal or execs, but a process doing 
that without providing a tracer process as well is simply buggy and 
misdesigned.


(The kernel bug is that wait calls in the "original but not currently the 
parent" process do not act like the traced process is still there and 
running: the fact that the child has been attached to and is being stopped 
and started by its tracer should be completely invisible to the original 
parent.)


Philip Guenther



Re: TIMESPEC_TO_NSEC(): futex(2), __thrsigdivert(2) & __thrsleep(2)

2020-01-12 Thread Philip Guenther
On Sun, Jan 12, 2020 at 4:44 AM Martin Pieuchot  wrote:

> The consensus is now to switch syscall doing sleeps to use tsleep_nsec(9).
> Our direct goal is to stop expressing time as ticks, more might come
> later.
>

The API futex(2) has a bug: it doesn't take a clockid_t and doesn't have a
way to take an absolute timeout, which means userspace has to do non-ideal
conversions to work around those limitations.  While it might(?) make sense
to keep futex(2) the way it is to support ports that use that exact API**,
it feels like we should support a richer API to make libc/libpthread
happier...and perhaps not have the insane argument unuse/reuse/overloading
that futex(2) has (e.g., "pass uint32_t val2 as the fourth argument instead
of timeout").

Let's say a diff magically appeared splitting out three syscalls for the
three ops, with correct types and args and where timeouts were specified
with timespec+clockid_t+"is absolute" flag, can that be slotted into all
this without wailing and gnashing of teeth?

** I have no idea if any ports actually do that

Philip Guenther


Re: PATCH: fix race condition in binutils-2.17 build

2020-01-02 Thread Philip Guenther
On Thu, Jan 2, 2020 at 2:26 AM Marc Espie  wrote:

> We don't normally hit this thanks to the "expensive job" heuristics
> (meaning that we don't start the second target while the first is
> building), but still there is a race, and if for whatever reason we
> remove that heuristics we hit it fairly hard.
>
> The patch is fairly trivial, just run both targets separately.
> Example of a crash and analysis follows.
>
> Note that the problem only occurs in binutils-2.17. The former versions
> did have one less level of indirection in their Makefiles.
>

ok guenther@


Re: sparc64: iommu: unbreak DEBUG build: %lx and time_t

2019-12-23 Thread Philip Guenther
On Mon, Dec 23, 2019 at 12:42 PM Klemens Nanni  wrote:

> `option DEBUG' builds are broken on sparc64, here's the first step in
> fixing it.
>
> /sys/arch/sparc64/dev/iommu.c: In function 'iommu_strbuf_flush_done':
> /sys/arch/sparc64/dev/iommu.c:582: warning: format '%lx' expects type
> 'long unsigned int', but argument 4 has type 'time_t'
> /sys/arch/sparc64/dev/iommu.c:582: warning: format '%lx' expects type
> 'long unsigned int', but argument 6 has type 'time_t'
>
> time_t is a long unsinged integer, that is
>
> /usr/include/time.h:typedef __time_ttime_t;
> /usr/include/sys/_types.h:typedef   __int64_t   __time_t;   /*
> epoch time */
> /usr/include/amd64/_types.h:typedef long long   __int64_t;
>
> OK?  Any better idea?


In general, for time_t we've case to (long long) and used %ll, so
that it's consistent across the system.


Philip Guenther


Re: dmesg alloc

2019-12-23 Thread Philip Guenther
On Mon, 23 Dec 2019, Alexander Bluhm wrote:
> dmesg(8) allocates a bit too much memory.
> 
> sysctl KERN_MSGBUFSIZE returns msg_bufs.
> sysctl KERN_MSGBUF copies out msg_bufs + offsetof(struct msgbuf,
> msg_bufc) bytes.
> dmesg allocates msg_bufs + sizeof(struct msgbuf) - 1 bytes.
> 
> This is not the same value as struct msgbuf is padded.
> struct msgbuf {
>   ...
> longmsg_bufd;   /* number of dropped bytes */
> charmsg_bufc[1];/* buffer */
> };
> 
> Better use the same size algorithm in kernel and userland.
> 
> ok?

The first chunk should be replaced with this one:

@@ -39,6 +39,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 

to just pick up the offsetof() supplied for the last 30 years by the C 
standard.  Otherwise, ok.



Re: fix mcount.po target in libc/gmon/Makefile.inc

2019-12-19 Thread Philip Guenther
On Thu, 19 Dec 2019, Theo Buehler wrote:
> According to the comment in lib/libc/gmon/Makefile.inc, mcount needs
> special treatment since it cannot be compiled with profiling or pie.
> 
> When depend was removed as an independent target, this special case was
> missed.  Align it with the inference rules in bsd.lib.mk. This also
> makes 'make clean' work correctly (it currently leaves mcount.po.d
> behind).

ok guenther@



Re: amd64 SMEP SMAP trap panic print

2019-12-19 Thread Philip Guenther
On Fri, 20 Dec 2019, Alexander Bluhm wrote:
> Can we use the regular trap panic for SMEP and SMAP?  pageflttrap() 
> returns 0 to print a nice reason in kerntrap().  Especially if ddb is 
> disabled, additional information is printed.
> 
> attempt to access user address 0xe27539f1000 in supervisor mode
> fatal page fault in supervisor mode
> trap type 6 code 3 rip 819c2665 cs 8 rflags 10202 cr2 e27539f1000 cpl 
> 0 rsp 80001ffbfe28
> gsbase 0x80001fb63ff0  kgsbase 0x0
> panic: trap type 6, code=3, pc=819c2665
...
> Index: arch/amd64/amd64/trap.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/arch/amd64/amd64/trap.c,v
> retrieving revision 1.77
> diff -u -p -r1.77 trap.c
> --- arch/amd64/amd64/trap.c   6 Sep 2019 12:22:01 -   1.77
> +++ arch/amd64/amd64/trap.c   19 Dec 2019 23:46:40 -
> @@ -163,14 +163,18 @@ pageflttrap(struct trapframe *frame, int
>   extern struct vm_map *kernel_map;
> 
>   /* This will only trigger if SMEP is enabled */
> - if (cr2 <= VM_MAXUSER_ADDRESS && frame->tf_err & PGEX_I)
> - panic("attempt to execute user address %p "
> - "in supervisor mode", (void *)cr2);
> + if (cr2 <= VM_MAXUSER_ADDRESS && frame->tf_err & PGEX_I) {
> + printf("attempt to execute user address %p "
> + "in supervisor mode\n", (void *)cr2);
> + return 0;
> + }
>   /* This will only trigger if SMAP is enabled */
>   if (pcb->pcb_onfault == NULL && cr2 <= VM_MAXUSER_ADDRESS &&
> - frame->tf_err & PGEX_P)
> - panic("attempt to access user address %p "
> - "in supervisor mode", (void *)cr2);
> + frame->tf_err & PGEX_P) {
> + printf("attempt to access user address %p "
> + "in supervisor mode\n", (void *)cr2);
> + return 0;
> + }
> 
>   /*
>* It is only a kernel address space fault iff:

For this part, should we reuse the 'faultstr' logic seen later to set the 
panic string and do something like, say...

Index: amd64/trap.c
===
RCS file: /data/src/openbsd/src/sys/arch/amd64/amd64/trap.c,v
retrieving revision 1.77
diff -u -p -r1.77 trap.c
--- amd64/trap.c6 Sep 2019 12:22:01 -   1.77
+++ amd64/trap.c20 Dec 2019 02:21:03 -
@@ -77,6 +77,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -132,6 +133,24 @@ static inline void verify_smap(const cha
 static inline void debug_trap(struct trapframe *_frame, struct proc *_p,
 long _type);
 
+static inline int
+fault(const char *format, ...)
+{
+   static char faultbuf[512];
+   va_list ap;
+
+   /*
+* Save the fault info for DDB and retain the kernel lock to keep
+* faultbuf from being overwritten by another CPU.
+*/
+   va_start(ap, format);
+   vsnprintf(faultbuf, sizeof faultbuf, format, ap);
+   va_end(ap);
+   printf("%s\n", faultbuf);
+   faultstr = faultbuf;
+   return 0;
+}
+
 /*
  * pageflttrap(frame, usermode): page fault handler
  * Returns non-zero if the fault was handled (possibly by generating
@@ -164,12 +183,12 @@ pageflttrap(struct trapframe *frame, int
 
/* This will only trigger if SMEP is enabled */
if (cr2 <= VM_MAXUSER_ADDRESS && frame->tf_err & PGEX_I)
-   panic("attempt to execute user address %p "
+   return fault("attempt to execute user address %p "
"in supervisor mode", (void *)cr2);
/* This will only trigger if SMAP is enabled */
if (pcb->pcb_onfault == NULL && cr2 <= VM_MAXUSER_ADDRESS &&
frame->tf_err & PGEX_P)
-   panic("attempt to access user address %p "
+   return fault("attempt to access user address %p "
"in supervisor mode", (void *)cr2);
 
/*
@@ -211,18 +230,9 @@ pageflttrap(struct trapframe *frame, int
frame->tf_rip = (u_int64_t)pcb->pcb_onfault;
return 1;
} else {
-   /*
-* Bad memory access in the kernel; save the fault
-* info for DDB and retain the kernel lock to keep
-* faultbuf from being overwritten by another CPU.
-*/
-   static char faultbuf[512];
-   snprintf(faultbuf, sizeof faultbuf,
-   "uvm_fault(%p, 0x%llx, 0, %d) -> %x",
+   /* bad memory access in the kernel */
+   return 

Re: inteldrm(4) fix

2019-12-17 Thread Philip Guenther
On Tue, Dec 17, 2019 at 5:02 AM Mark Kettenis 
wrote:

> Hit the
>
>   KASSERT(curcpu()->ci_inatomic == 0);
>
> in pagefault_disable().  Analysis of the code in i915_gem_execbuffer.c
> shows that pagefault_disable() may be called when page faults are
> already disabled, i.e. from eb_relocate_slow() where eb_relocate_vma()
> is called after pagefault_disable().
>
> Diff below fixes this.
>
> ok?
>

Like a making a lock recursive, it suggests confusion in the code...but if
upstream supports it then doing otherwise is a losing proposition.

ok guenther@


Re: Sync KVE_ET_* with UVM_ET_*

2019-12-05 Thread Philip Guenther
On Thu, Dec 5, 2019 at 9:15 AM Martin Pieuchot  wrote:

> Sync with reality, will help KERN_PROC_VMMAP consumers.  Ok?
>

ok guenther@


Re: fwide() does not unlock if error was occurred

2019-12-02 Thread Philip Guenther
On Mon, Dec 2, 2019 at 7:48 PM Masato Asou  wrote:

> fwide() does not unlock if error was occurred.
>
> ok?
>

ok guenther@


Re: iwm: support 9260 devices

2019-11-16 Thread Philip Guenther
On Sat, Nov 16, 2019 at 8:19 AM Stefan Sperling  wrote:

> On Sat, Nov 16, 2019 at 04:51:44PM +0100, Stefan Sperling wrote:
> > This diff adds support for iwm(4) 9260 devices and hopefully 9560
> > devices as well but I have not yet had time to test those.
> >
> > Joint work with patrick@. Some parts were lifted from FreeBSD.
> >
> > If you have the followng device in pcidump it should at least get
> > an IP address from DHCP and be able to ping:
> >  4:0:0: Intel Dual Band Wireless-AC 9260
> >  0x: Vendor ID: 8086, Product ID: 2526
> >
> > The firmware is not in fw_update yet.
> > In the meantime firmware can be fetched from here:
> >
> https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/
> >
> > Copy these files to /etc/firmware as indicated:
> > for 9260: iwlwifi-9260-th-b0-jf-b0-34.ucode -> /etc/firmware/iwm-9260-34
> > for 9560: iwlwifi-9000-pu-b0-jf-b0-34.ucode -> /etc/firmware/iwm-9000-34
> >
> > Checks for regressions on already supported devices are also welcome,
> > in which case the firmware isn't needed.
>
> Better diff which fixes an Rx throughput issue which was present in
> the previous diff.


Looking good so far on my X1 extreme once I
added PCI_PRODUCT_INTEL_WL_9560_2 next to the _1 lines in if_iwm.c.
Awesome!

Philip Guenther


Re: Fix cpio -Hustar -o

2019-11-14 Thread Philip Guenther
On Thu, Nov 14, 2019 at 1:36 PM Christian Weisgerber 
wrote:

> I found this discrepancy surprising:
>
> $ find /bin -print | cpio -o -Hustar >foo
> $ file foo
> foo: POSIX tar archive
> $ find /bin -print | cpio -Hustar -o >foo
> $ file foo
> foo: ASCII cpio archive (SVR4 with CRC)
>
> The argument order should not matter here.
>
> Fix: When processing the -o switch, only set the archive format if
> not already set.
>
> ok?
>
> Index: bin/pax/options.c
> ===
> RCS file: /cvs/src/bin/pax/options.c,v
> retrieving revision 1.102
> diff -u -p -r1.102 options.c
> --- bin/pax/options.c   13 Sep 2018 12:33:43 -  1.102
> +++ bin/pax/options.c   14 Nov 2019 20:59:04 -
> @@ -1226,7 +1226,8 @@ cpio_options(int argc, char **argv)
>  * create an archive
>  */
> act = ARCHIVE;
> -   frmt = &(fsub[F_CPIO]);
> +   if (frmt == NULL)
> +   frmt = &(fsub[F_CPIO]);
> break;
> case 'p':
> /*
>

ok guenther@


Re: fix amd64 pmap comment

2019-11-02 Thread Philip Guenther
On Sat, Nov 2, 2019 at 4:34 AM Martin Pieuchot  wrote:

> uvm_km_alloc(9) has never been used in amd64's pmap.  pool_get(9) is the
> thing since its import by mickey@.
>
> ok?
>

ok guenther@


ftp: utime->utimensat: use UTIME_OMIT instead of calling time()

2019-10-13 Thread Philip Guenther


ftp currently uses utime(3) to set the modification time on the retrieved 
file.  It set the access time on the file to the current time when doing 
so.  By converting this to use utimensat(), we can avoid the silly call 
to time(3) to get the current time for the atime by instead passing 
UTIME_NOW.

However, I _think_ ftp does this just because utime(3) has no way to set 
the modification time without also setting the access time and calling 
stat(2) just to get the current atime (and dealing with the possible error 
cases) wasn't worth it, so instead of setting the atime to the current 
time with UTIME_NOW, I propose leaving the atime unchanged with 
UTIME_OMIT.

Thoughts?  oks?


Philip Guenther


Index: ftp.c
===
RCS file: /data/src/openbsd/src/usr.bin/ftp/ftp.c,v
retrieving revision 1.105
diff -u -p -r1.105 ftp.c
--- ftp.c   28 Jun 2019 13:35:01 -  1.105
+++ ftp.c   14 Oct 2019 01:32:56 -
@@ -72,6 +72,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -79,7 +80,6 @@
 #include 
 #include 
 #include 
-#include 
 
 #include "ftp_var.h"
 
@@ -1211,11 +1211,12 @@ break2:
if (preserve && (closefunc == fclose)) {
mtime = remotemodtime(remote, 0);
if (mtime != -1) {
-   struct utimbuf ut;
+   struct timespec times[2];
 
-   ut.actime = time(NULL);
-   ut.modtime = mtime;
-   if (utime(local, ) == -1)
+   times[0].tv_nsec = UTIME_OMIT;
+   times[1].tv_sec = mtime;
+   times[1].tv_nsec = 0;
+   if (utimensat(AT_FDCWD, local, times, 0) == -1)
fprintf(ttyout,
"Can't change modification time on %s to %s",
local, asctime(localtime()));



Re: Remove some sigvec Xr's

2019-07-25 Thread Philip Guenther
On Thu, Jul 25, 2019 at 1:04 PM Todd C. Miller  wrote:

> We should only cross-reference the obsolete sigvec(3) function from
> the signal compat manuals and sigaction(2).
>
> This also syncs the SEE ALSO section in ualarm(3) match that of
> alarm(3).
>

ok guenther@


We could reference signal(3) in csh instead of sigaction(2) if
> that's what people prefer.


I see no reason to prefer signal(3) over sigaction(2) for pages that aren't
about C-standard functions.

Hmm: sh(1) and ksh(1) have *nothing* from sections 2 or 3 in their SEE
ALSO.  That doesn't seem like a wrong choice, albeit inconsistent with
csh(1).  Part of me feels like _if_ they're going to mention umask(2),
setrlimit(2), and sigaction(2), then they should mention chdir(2), as the
other classic "must be in the shell" syscall.  

Philip Guenther


Re: remove duplicate definitions of LAPIC_ID_MASK and LAPIC_ID_SHIFT

2019-07-25 Thread Philip Guenther
On Thu, Jul 25, 2019 at 4:44 PM Kevin Lo  wrote:

> ok?
>

Yes, please.  guenther@


Re: Comment fix: No mcontext

2019-07-13 Thread Philip Guenther
On Thu, Jul 11, 2019 at 10:12 PM Benjamin Baier 
wrote:

> there is no mcontext, and since V1.2 of process_machdep.c struct reg
> and struct trapframe don't have to be closely synced.
> process_machdep.c no longer memcpy's one to the other.
>

Yep.  Committed.  Thanks!


Philip Guenther


logger(1): add -c option for LOG_CONS

2019-06-16 Thread Philip Guenther


Testing something else I needed to call syslog(3) with LOG_CONS.  Diff 
below adds support to logger(1) for doing that.  Option choice is 
compatible with NetBSD.

ok?

Philip Guenther

Index: logger.1
===
RCS file: /data/src/openbsd/src/usr.bin/logger/logger.1,v
retrieving revision 1.21
diff -u -p -r1.21 logger.1
--- logger.15 Jun 2013 17:35:12 -   1.21
+++ logger.116 Jun 2019 19:19:26 -
@@ -38,7 +38,7 @@
 .Nd make entries in the system log
 .Sh SYNOPSIS
 .Nm logger
-.Op Fl is
+.Op Fl cis
 .Op Fl f Ar file
 .Op Fl p Ar pri
 .Op Fl t Ar tag
@@ -52,6 +52,10 @@ system log module.
 .Pp
 The options are as follows:
 .Bl -tag -width "-f file"
+.It Fl c
+If unable to pass the message to
+.Xr syslogd 8 ,
+attempt to write the message to the console.
 .It Fl f Ar file
 Read from the specified
 .Ar file .
@@ -102,5 +106,5 @@ utility is compliant with the
 specification.
 .Pp
 The flags
-.Op Fl ifpst
+.Op Fl cfipst
 are extensions to that specification.
Index: logger.c
===
RCS file: /data/src/openbsd/src/usr.bin/logger/logger.c,v
retrieving revision 1.17
diff -u -p -r1.17 logger.c
--- logger.c28 Mar 2016 18:18:52 -  1.17
+++ logger.c16 Jun 2019 19:18:33 -
@@ -61,8 +61,11 @@ main(int argc, char *argv[])
tag = NULL;
pri = LOG_NOTICE;
logflags = 0;
-   while ((ch = getopt(argc, argv, "f:ip:st:")) != -1)
+   while ((ch = getopt(argc, argv, "cf:ip:st:")) != -1)
switch(ch) {
+   case 'c':   /* log to console */
+   logflags |= LOG_CONS;
+   break;
case 'f':   /* file to log */
if (freopen(optarg, "r", stdin) == NULL) {
(void)fprintf(stderr, "logger: %s: %s.\n",
@@ -180,6 +183,6 @@ void
 usage(void)
 {
(void)fprintf(stderr,
-   "usage: logger [-is] [-f file] [-p pri] [-t tag] [message ...]\n");
+   "usage: logger [-cis] [-f file] [-p pri] [-t tag] [message ...]\n");
exit(1);
 }



Re: usr.bin/getconf: Add reporting LONG_BIT

2019-05-27 Thread Philip Guenther
On Mon, May 27, 2019 at 3:43 PM Brian Callahan  wrote:

> Below is a small diff in response to some configuration attempts
> found in ports land.
> lang/ponyc uses $(shell getconf LONG_BIT) in its Makefile to
> determine whether or not we're on a 64-bit platform.
>

It needs to know that at the scripting level, outside of the C code itself
where it can directly test LONG_BIT (or perhaps better, _LP64)?  Huh.



> However, our getconf(1) doesn't support reporting LONG_BIT.
> This diff enables it. GNU/FreeBSD/DragonFly/MacOS getconf reports
> LONG_BIT, but NetBSD getconf does not.
> Desirable? OK?
>

That's the way to do it, I just have this vague "what tempting lunacy has
led them to this?" lurking in my mind.


Philip Guenther


Re: Make pthread_np.h standalone

2019-05-27 Thread Philip Guenther
On Mon, May 27, 2019 at 7:36 AM Jeremie Courreges-Anglas 
wrote:

>
> A bunch of our ports expect pthread_np.h to be standalone, alas our
> version doesn't include pthread.h.  The diff below should help us get
> rid of some patches in (at least) mongodb, mono, gnustep and webkitgtk4.
>
> ok?
>

ok guenther@


Re: wall(1) unveil for non-root users

2019-05-16 Thread Philip Guenther
On Wed, May 15, 2019 at 10:54 PM Theo de Raadt  wrote:

> Martijn van Duren  wrote:
>
> > I don't see much point in the check.
> >
> > If we don't have write permissions open(2) will fail.
> > If we open it based on S_IWOTH permissions than checking for S_IWGRP
> > without considering who is in that group seems really absurd to me.
> >
> > So I'd be OK with patch 1
>
> There are 3 write permission checks which could permit the open,
> but then it checks the specific one.
>
> I believe this is similar to comsat / biff, which in those programs
> are used to indicate "I am ok with messages to my tty".
>
> That is an old behaviour, but I bet that is the history of this S_IWGRP
> check.
>

Yep.  mesg(1) manipulates your tty's group-write bit so that wall(1),
talk(1), and write(1) can check it.

Philip Guenther


Re: ld.so speedup (part 2)

2019-05-11 Thread Philip Guenther
On Tue, 7 May 2019, Jeremie Courreges-Anglas wrote:
> On Sat, Apr 27 2019, Nathanael Rensen  
> wrote:
> > The diff below speeds up ld.so library intialisation where the 
> > dependency tree is broad and deep, such as samba's smbd which links 
> > over 100 libraries.
...
> As I told mpi@ earlier today, I think your changes are correct as is,
> and are good to be committed.  So this counts as an ok jca@.  But I'd
> expect other developers to chime in soon, maybe they'll spot something
> that I didn't.

drahn@ and I pulled on our ld.so waders and agreed it's good, so I've 
committed it with some tweaking to the #defines to make them 
self-explanatory and have contiguous bit-assignments.

Thank you for identifying this badly inefficient algorithm and spotting 
how easy it was to fix!


Philip Guenther



Re: Switch powerpc to big PIC

2019-02-05 Thread Philip Guenther
On Tue, Feb 5, 2019 at 12:59 PM Mark Kettenis 
wrote:

> The architecture already has big PIE.  The issue is that clang doesn't
> support secure-plt for small pic.  I haven't entirely figured out
> what's going on here and we probably need some further fixes to clang
> here.  On the other hand I think it is probably time to recognize
> there is more and more bloat in the world.
>
> Thoughts?
>

libexec/ld.so/powerpc/Makefile.inc needs updating to match, no?
---
CFLAGS += -fpic -msoft-float
---


Also, does clang support secure-plt for small pie?  If not, then
lib/csu/Makefile needs updating too:
---
# Override powerpc default of -fPIE
# XXX if this is safe, why not override CFLAGS for alpha and sparc64 too?
# Does it work because the csu bits come first and get the first few GOT
# entries?
.if ${MACHINE_ARCH} == "powerpc"
CFLAGS+=-fpie
.endif
---


Philip Guenther


Re: trunk shouldnt care if it's stacked

2019-01-09 Thread Philip Guenther
On Wed, Jan 9, 2019 at 1:11 AM Klemens Nanni  wrote:

> On Wed, Jan 09, 2019 at 01:12:31PM +1000, David Gwynne wrote:
> > -#define TRUNK_MAX_STACKING   4   /* maximum number of stacked
> trunks */
> Is this an arbitrary limit or does it conceal other limitiations?
>

If stacking is handled via possibly recursive calls then there's an
effective limit imposed by size of the kernel stack.

Imposing a depth limit also prevents looping a stack by adding an upper
trunk to a bottom one.


Philip Guenther


Re: ksh: quote empties

2018-12-30 Thread Philip Guenther
On Sun, 15 Apr 2018, Martijn Dekker wrote:
> Op 15-04-18 om 03:41 schreef Philip Guenther:
> > On Sun, 15 Apr 2018, Klemens Nanni wrote:
> > > It also badly effects non-empty cases:
> > ...
> > >   $ ./obj/ksh -c alias
> > >   autoload=''
> > >   functions=''
> > 
> > Hah!  The original diff i actually broken (it tests the wrong variable)
> > but I fixed that by accident when I manually made the diff in my tree!
> > 
> > So, uh, I'm no longer fine with the original diff...
> 
> D'oh!
> 
> That's embarrassing. Sorry about that. :-/
...
> --- misc.c9 Apr 2018 17:53:36 -   1.70
> +++ misc.c15 Apr 2018 02:06:55 -
> @@ -966,6 +966,12 @@ print_value_quoted(const char *s)
>   const char *p;
>   int inquote = 0;
> 
> + /* Check for empty */
> + if (!*s) {
> + shprintf("''");
> + return;
> + }
> +

This thread was never resolved/committed.  Looking again at the diffs, I 
still think I prefer that we _not_ touch print_value_quoted(), as the 
other callers all use the 'key=value' format and don't need special 
handling of empty values, leaving a diff like the one below.

ok?

Philip


Index: bin/ksh/c_sh.c
===
RCS file: /data/src/openbsd/src/bin/ksh/c_sh.c,v
retrieving revision 1.63
diff -u -p -r1.63 c_sh.c
--- bin/ksh/c_sh.c  9 Apr 2018 17:53:36 -   1.63
+++ bin/ksh/c_sh.c  30 Dec 2018 22:24:19 -
@@ -478,7 +478,10 @@ c_trap(char **wp)
for (p = sigtraps, i = NSIG+1; --i >= 0; p++) {
if (p->trap != NULL) {
shprintf("trap -- ");
-   print_value_quoted(p->trap);
+   if (p->trap[0])
+   print_value_quoted(p->trap);
+   else
+   shprintf("''");
shprintf(" %s\n", p->name);
}
}
Index: regress/bin/ksh/obsd-regress.t
===
RCS file: /data/src/openbsd/src/regress/bin/ksh/obsd-regress.t,v
retrieving revision 1.10
diff -u -p -r1.10 obsd-regress.t
--- regress/bin/ksh/obsd-regress.t  8 Dec 2018 21:03:51 -   1.10
+++ regress/bin/ksh/obsd-regress.t  30 Dec 2018 22:41:27 -
@@ -503,3 +503,20 @@ description:
 stdin:
kill -s SIGINFO $$
 ---
+
+name: trap-ignore-output
+description:
+   trap output shows ignored signals correctly
+stdin:
+   trap '' SIGUSR1 SIGUSR2
+   trap "" SIGPIPE
+   trap exit SIGINT
+   trap exit\ 1 SIGTERM
+   trap
+expected-stdout:
+   trap -- exit INT
+   trap -- '' PIPE
+   trap -- 'exit 1' TERM
+   trap -- '' USR1
+   trap -- '' USR2
+---



reduce libgen.h usage

2018-12-30 Thread Philip Guenther


 only prototypes dirname() and basename().  There are a bunch of 
source files that #include it but don't need it; the diff below deletes 
those pointless includes.

ok?

Philip

Index: bin/ksh/edit.c
===
RCS file: /data/src/openbsd/src/bin/ksh/edit.c,v
retrieving revision 1.66
diff -u -p -r1.66 edit.c
--- bin/ksh/edit.c  18 Jun 2018 17:03:58 -  1.66
+++ bin/ksh/edit.c  21 Oct 2018 03:47:03 -
@@ -12,7 +12,6 @@
 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
Index: usr.bin/cvs/commit.c
===
RCS file: /data/src/openbsd/src/usr.bin/cvs/commit.c,v
retrieving revision 1.158
diff -u -p -r1.158 commit.c
--- usr.bin/cvs/commit.c1 Jun 2017 08:08:24 -   1.158
+++ usr.bin/cvs/commit.c21 Oct 2018 03:39:46 -
@@ -20,7 +20,6 @@
 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
Index: usr.bin/less/less.h
===
RCS file: /data/src/openbsd/src/usr.bin/less/less.h,v
retrieving revision 1.27
diff -u -p -r1.27 less.h
--- usr.bin/less/less.h 26 Mar 2016 08:59:29 -  1.27
+++ usr.bin/less/less.h 21 Oct 2018 03:41:31 -
@@ -19,7 +19,6 @@
 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
Index: usr.bin/mg/dired.c
===
RCS file: /data/src/openbsd/src/usr.bin/mg/dired.c,v
retrieving revision 1.83
diff -u -p -r1.83 dired.c
--- usr.bin/mg/dired.c  7 Oct 2016 00:17:20 -   1.83
+++ usr.bin/mg/dired.c  21 Oct 2018 03:39:48 -
@@ -16,7 +16,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
Index: usr.bin/mg/grep.c
===
RCS file: /data/src/openbsd/src/usr.bin/mg/grep.c,v
retrieving revision 1.46
diff -u -p -r1.46 grep.c
--- usr.bin/mg/grep.c   9 Jan 2018 17:59:29 -   1.46
+++ usr.bin/mg/grep.c   21 Oct 2018 03:39:50 -
@@ -7,7 +7,6 @@
 #include 
 
 #include 
-#include 
 #include 
 #include 
 #include 
Index: usr.bin/patch/util.c
===
RCS file: /data/src/openbsd/src/usr.bin/patch/util.c,v
retrieving revision 1.41
diff -u -p -r1.41 util.c
--- usr.bin/patch/util.c7 Apr 2018 14:55:13 -   1.41
+++ usr.bin/patch/util.c20 Apr 2018 02:22:30 -
@@ -31,7 +31,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
Index: usr.bin/rcs/rcs.c
===
RCS file: /data/src/openbsd/src/usr.bin/rcs/rcs.c,v
retrieving revision 1.85
diff -u -p -r1.85 rcs.c
--- usr.bin/rcs/rcs.c   9 May 2016 13:03:55 -   1.85
+++ usr.bin/rcs/rcs.c   21 Oct 2018 03:39:52 -
@@ -30,7 +30,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
Index: usr.sbin/npppd/npppd/npppd.c
===
RCS file: /data/src/openbsd/src/usr.sbin/npppd/npppd/npppd.c,v
retrieving revision 1.48
diff -u -p -r1.48 npppd.c
--- usr.sbin/npppd/npppd/npppd.c25 Jul 2018 02:18:36 -  1.48
+++ usr.sbin/npppd/npppd/npppd.c21 Oct 2018 03:35:48 -
@@ -50,7 +50,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
Index: usr.sbin/smtpd/envelope.c
===
RCS file: /data/src/openbsd/src/usr.sbin/smtpd/envelope.c,v
retrieving revision 1.41
diff -u -p -r1.41 envelope.c
--- usr.sbin/smtpd/envelope.c   27 Dec 2018 15:41:50 -  1.41
+++ usr.sbin/smtpd/envelope.c   30 Dec 2018 18:39:47 -
@@ -33,7 +33,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
Index: usr.sbin/smtpd/queue.c
===
RCS file: /data/src/openbsd/src/usr.sbin/smtpd/queue.c,v
retrieving revision 1.188
diff -u -p -r1.188 queue.c
--- usr.sbin/smtpd/queue.c  8 Dec 2018 08:01:15 -   1.188
+++ usr.sbin/smtpd/queue.c  9 Dec 2018 21:05:57 -
@@ -28,7 +28,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
Index: usr.sbin/smtpd/queue_backend.c
===
RCS file: /data/src/openbsd/src/usr.sbin/smtpd/queue_backend.c,v
retrieving revision 1.64
diff -u -p -r1.64 queue_backend.c
--- usr.sbin/smtpd/queue_backend.c  31 May 2018 21:06:12 -  1.64
+++ usr.sbin/smtpd/queue_backend.c  21 Oct 2018 03:36:06 -
@@ -31,7 +31,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
Index: usr.sbin/smtpd/queue_fs.c
===
RCS file: /data/src/openbsd/src/usr.sbin/smtpd/queue_fs.c,v
retrieving revision 1.17
diff -u -p 

  1   2   3   4   5   6   7   8   9   10   >