random toeplitz seeds

2020-06-25 Thread Theo Buehler
This adds an stoeplitz_random_seed() function that generates a random
Toeplitz key seed with an invertible matrix T. This is necessary and
sufficient for the hash to spread out over all 65536 possible values.

While it is clear from T * (-1) == 0 that seeds with parity 0 are bad,
I don't have a neat and clean proof for the fact that a seed with
parity 1 always generates an invertible Toeplitz matrix. It's not hard
to check, but rather tedious.

I'm unsure how to hook it up. I enabled random seeds by using the
function in stoeplitz_init(), but that's just for illustration.

Index: sys/net/toeplitz.c
===
RCS file: /var/cvs/src/sys/net/toeplitz.c,v
retrieving revision 1.7
diff -u -p -r1.7 toeplitz.c
--- sys/net/toeplitz.c  19 Jun 2020 08:48:15 -  1.7
+++ sys/net/toeplitz.c  25 Jun 2020 18:43:02 -
@@ -69,9 +69,38 @@ static struct stoeplitz_cachestoeplitz_
 const struct stoeplitz_cache *const
stoeplitz_cache = _syskey_cache; 
 
+/* parity of n16: count (mod 2) of ones in the binary representation. */
+int
+parity(uint16_t n16)
+{
+   n16 = ((n16 & 0x) >> 1) ^ (n16 & 0x);
+   n16 = ((n16 & 0x) >> 2) ^ (n16 & 0x);
+   n16 = ((n16 & 0xf0f0) >> 4) ^ (n16 & 0x0f0f);
+   n16 = ((n16 & 0xff00) >> 8) ^ (n16 & 0x00ff);
+
+   return (n16);
+}
+
+/*
+ * The Toeplitz matrix obtained from a seed is invertible if and only if the
+ * parity of the seed is 1. Generate such a seed uniformly at random.
+ */
+stoeplitz_key
+stoeplitz_random_seed(void)
+{
+   stoeplitz_key seed;
+   
+   seed = arc4random() & UINT16_MAX;
+   if (parity(seed) == 0)
+   seed ^= 1;
+
+   return (seed);
+}
+
 void
 stoeplitz_init(void)
 {
+   stoeplitz_keyseed = stoeplitz_random_seed();
stoeplitz_cache_init(_syskey_cache, stoeplitz_keyseed);
 }
 



[PATCH] fast conditional console scrolling

2020-06-25 Thread johnc
This causes the write-only framebuffer console to only redraw the
chars that differ between the start and end positions.

'time ls -R /usr/src/sys' is 3x faster with this, because most of
the characters stay the same after a scroll.

If this looks good, I can do the same thing for clear rows and copy/
clear columns, although I will need to make a test case for them.

It would probably be a good idea to change the rasops interface to
have generic block copy and clear oeprations, versus the current
full-column / full-row interface, so tmux and friends could get the
full acceleration.

Index: rasops.c
===
RCS file: /cvs/src/sys/dev/rasops/rasops.c,v
retrieving revision 1.61
diff -u -p -r1.61 rasops.c
--- rasops.c25 May 2020 09:55:49 -  1.61
+++ rasops.c26 Jun 2020 04:14:13 -
@@ -1627,28 +1627,42 @@ rasops_vcons_copyrows(void *cookie, int 
struct rasops_info *ri = scr->rs_ri;
int cols = ri->ri_cols;
int row, col, rc;
+   int srcofs;
+   int move;
 
+   /* update the scrollback buffer if the entire screen is moving */
if (dst == 0 && (src + num == ri->ri_rows) && scr->rs_sbscreens > 0)
memmove(>rs_bs[dst], >rs_bs[src * cols],
-   ((ri->ri_rows * (scr->rs_sbscreens + 1) * cols) -
-   (src * cols)) * sizeof(struct wsdisplay_charcell));
-   else
+   ri->ri_rows * scr->rs_sbscreens * cols
+   * sizeof(struct wsdisplay_charcell));
+
+   /* copy everything */
+   if ((ri->ri_flg & RI_WRONLY) == 0 || !scr->rs_visible) {
memmove(>rs_bs[dst * cols + scr->rs_dispoffset],
-   >rs_bs[src * cols + scr->rs_dispoffset],
-   num * cols * sizeof(struct wsdisplay_charcell));
+   >rs_bs[src * cols + scr->rs_dispoffset],
+   num * cols * sizeof(struct wsdisplay_charcell));
 
-   if (!scr->rs_visible)
-   return 0;
+   if (!scr->rs_visible)
+   return 0;
 
-   if ((ri->ri_flg & RI_WRONLY) == 0)
return ri->ri_copyrows(ri, src, dst, num);
+   }
 
-   for (row = dst; row < dst + num; row++) {
+   /* smart update, only redraw characters that are different */
+   srcofs = (src - dst) * cols;
+
+   for (move = 0 ; move < num ; move++) {
+   row = srcofs > 0 ? dst + move : dst + num - 1 - move;
for (col = 0; col < cols; col++) {
int off = row * cols + col + scr->rs_dispoffset;
-
-   rc = ri->ri_putchar(ri, row, col,
-   scr->rs_bs[off].uc, scr->rs_bs[off].attr);
+   int newc = scr->rs_bs[off+srcofs].uc;
+   int newa = scr->rs_bs[off+srcofs].attr;
+   if ( scr->rs_bs[off].uc == newc 
+   && scr->rs_bs[off].attr == newa )
+   continue;
+   scr->rs_bs[off].uc = newc;
+   scr->rs_bs[off].attr = newa;
+   rc = ri->ri_putchar(ri, row, col, newc, newa);
if (rc != 0)
return rc;
}




Re: userland clock_gettime proof of concept

2020-06-25 Thread George Koehler
On Mon, 22 Jun 2020 19:12:22 +0300
Paul Irofti  wrote:

> New iteration:
> 
>   - ps_timekeep should not coredump, pointed by deraadt@
>   - set ps_timekeep to 0 before user uvm_map for randomization
>   - map timekeep before fixup. confirmed by naddy@ that it fixes NULL init
>   - initialize va. clarified by kettenis@

Here's macppc again.  My macppc isn't using your newest diff but does
now need to define TC_TB in .

The /sys/arch/powerpc/include/timetc.h in your diff never gets used,
because there is no #include .  On macppc,
  uname -m => macppc and
  uname -p => powerpcare different,
and #include  is /sys/arch/macppc/include/timetc.h.
I suspect that  is /sys/arch/$i/include/timetc.h
if and only if /sys/arch/$i/compile exists.

10 days ago, naddy said, "You only need the lower register."  That is
correct, so this diff also stops using mftbu (the higher register).

--- lib/libc/arch/powerpc/gen/usertc.c.before   Wed Jun 24 16:42:36 2020
+++ lib/libc/arch/powerpc/gen/usertc.c  Wed Jun 24 16:46:00 2020
@@ -18,4 +18,17 @@
 #include 
 #include 
 
-int (*const _tc_get_timecount)(struct timekeep *, u_int *) = NULL;
+int
+tc_get_timecount(struct timekeep *tk, u_int *tc)
+{
+   u_int tb;
+
+   if (tk->tk_user != TC_TB)
+   return -1;
+
+   asm volatile("mftb %0" : "=r"(tb));
+   *tc = tb;
+   return 0;
+}
+int (*const _tc_get_timecount)(struct timekeep *tk, u_int *tc)
+   = tc_get_timecount;
--- sys/arch/macppc/include/timetc.h.before Wed Jun 24 16:36:03 2020
+++ sys/arch/macppc/include/timetc.hWed Jun 24 16:37:47 2020
@@ -18,6 +18,7 @@
 #ifndef _MACHINE_TIMETC_H_
 #define _MACHINE_TIMETC_H_
 
-#defineTC_LAST 0
+#defineTC_TB   1
+#defineTC_LAST 2
 
 #endif /* _MACHINE_TIMETC_H_ */
--- sys/arch/macppc/macppc/clock.c.before   Wed Jun 24 16:39:58 2020
+++ sys/arch/macppc/macppc/clock.c  Wed Jun 24 16:40:08 2020
@@ -57,7 +57,7 @@
 static int32_t ticks_per_intr;
 
 static struct timecounter tb_timecounter = {
-   tb_get_timecount, NULL, 0x7fff, 0, "tb", 0, NULL, 0
+   tb_get_timecount, NULL, 0x7fff, 0, "tb", 0, NULL, TC_TB
 };
 
 /* calibrate the timecounter frequency for the listed models */



Re: Destructive Install Process (fdisk)

2020-06-25 Thread Solene Rapenne
On Thu, 25 Jun 2020 19:27:22 -0400
David Blevins :

> OpenBSD Community,
> 
> I've been experimenting with the OpenBSD 6.7 install process (though
> this "issue" is likely present in earlier version) and have noticed
> that the fdisk program in the installation program will destructively
> edit the hard drive upon selecting either MBR or GPT partitioning.
> I have repeatedly wiped a test hard drive's partitioning scheme as a
> consequence of this, in spite of only desiring to examine the
> partitioning scheme.
> 

This is explained in the INSTALL.amd64 file (I guess you use amd64)
https://ftp.fr.openbsd.org/pub/OpenBSD/6.7/amd64/INSTALL.amd64

The installation program will ask you if you want to use the
whole disk for OpenBSD.  If you don't need to or don't intend
to share the disk with other operating systems, answer "w"
here to use "MBR" partitioning or "g" to use "GPT"
partitioning. The installation program will then create a single
partition spanning the whole disk, dedicated to OpenBSD.

> Additionally, I have noticed that (per warnings in the installation
> program) GPT partitioning does not work on my system.  Does anyone
> have insight into this issue, and if so is there any interest in
> a fix for GPT partitioning?
> 
> -David Blevins

You shoud provide information about your system. I never had issue
with GPT.



Re: pipex(4): use reference counters for `ifnet'

2020-06-25 Thread Vitaliy Makkoveev



> On 25 Jun 2020, at 20:13, Christiano F. Haesbaert  
> wrote:
> 
> You can.
> 
> The idea is that ifindex is always monotonically increased, so to actually
> get a new interface you would have to have "overflowed" 65k interfaces,
> which is unreal.
> 
> So if your interface is gone, you can be sure if_get will give you NULL.

No I can’t :) It’s *not* the operational infinity you talks about.

You can run:
while true ; do ifconfig tun0 create ; ifconfig tun0 destroy ; done

And after couple of minutes you will see is very real to have *counter* be
overflowed. Not the interface count.

> 
> On Thu, Jun 25, 2020, 18:55 Vitaliy Makkoveev 
> wrote:
> 
>> Updated diff.
>> 
>> OpenBSD uses 16 bit counter for allocate interface indexes. So we can't
>> store index in session and be sure if_get(9) returned `ifnet' is our
>> original `ifnet'.
>> 
>> Now each pipex(4) session has it's own reference to `ifnet'. Also pppoe
>> related sessions has the reference to it's ethernet interface.
>> 
>> All `ifnet's are obtained by if_get(9) and we use `if_index' from stored
>> reference to `ifnet'.
>> 
>> Index: net/if_pppx.c
>> ===
>> RCS file: /cvs/src/sys/net/if_pppx.c,v
>> retrieving revision 1.90
>> diff -u -p -r1.90 if_pppx.c
>> --- net/if_pppx.c   24 Jun 2020 08:52:53 -  1.90
>> +++ net/if_pppx.c   25 Jun 2020 16:41:25 -
>> @@ -714,15 +714,15 @@ pppx_add_session(struct pppx_dev *pxd, s
>>ifp->if_softc = pxi;
>>/* ifp->if_rdomain = req->pr_rdomain; */
>> 
>> -   error = pipex_link_session(session, >pxi_ifcontext);
>> -   if (error)
>> -   goto remove;
>> -
>>/* XXXSMP breaks atomicity */
>>NET_UNLOCK();
>>if_attach(ifp);
>>NET_LOCK();
>> 
>> +   error = pipex_link_session(session, >pxi_ifcontext);
>> +   if (error)
>> +   goto detach;
>> +
>>if_addgroup(ifp, "pppx");
>>if_alloc_sadl(ifp);
>> 
>> @@ -771,7 +771,12 @@ pppx_add_session(struct pppx_dev *pxd, s
>> 
>>return (error);
>> 
>> -remove:
>> +detach:
>> +   /* XXXSMP breaks atomicity */
>> +   NET_UNLOCK();
>> +   if_detach(ifp);
>> +   NET_LOCK();
>> +
>>if (RBT_REMOVE(pppx_ifs, _ifs, pxi) == NULL)
>>panic("%s: inconsistent RB tree", __func__);
>>LIST_REMOVE(pxi, pxi_list);
>> @@ -860,13 +865,13 @@ pppx_if_destroy(struct pppx_dev *pxd, st
>>CLR(ifp->if_flags, IFF_RUNNING);
>> 
>>pipex_unlink_session(session);
>> +   pipex_rele_session(session);
>> 
>>/* XXXSMP breaks atomicity */
>>NET_UNLOCK();
>>if_detach(ifp);
>>NET_LOCK();
>> 
>> -   pipex_rele_session(session);
>>if (RBT_REMOVE(pppx_ifs, _ifs, pxi) == NULL)
>>panic("%s: inconsistent RB tree", __func__);
>>LIST_REMOVE(pxi, pxi_list);
>> Index: net/pipex.c
>> ===
>> RCS file: /cvs/src/sys/net/pipex.c,v
>> retrieving revision 1.116
>> diff -u -p -r1.116 pipex.c
>> --- net/pipex.c 22 Jun 2020 09:38:15 -  1.116
>> +++ net/pipex.c 25 Jun 2020 16:41:25 -
>> @@ -148,7 +148,7 @@ pipex_iface_init(struct pipex_iface_cont
>>struct pipex_session *session;
>> 
>>pipex_iface->pipexmode = 0;
>> -   pipex_iface->ifnet_this = ifp;
>> +   pipex_iface->ifnet_this = if_get(ifp->if_index);
>> 
>>if (pipex_rd_head4 == NULL) {
>>if (!rn_inithead((void **)_rd_head4,
>> @@ -165,6 +165,7 @@ pipex_iface_init(struct pipex_iface_cont
>>session = pool_get(_session_pool, PR_WAITOK | PR_ZERO);
>>session->is_multicast = 1;
>>session->pipex_iface = pipex_iface;
>> +   session->ifnet_this = if_get(ifp->if_index);
>>pipex_iface->multicast_session = session;
>> }
>> 
>> @@ -194,10 +195,11 @@ pipex_iface_stop(struct pipex_iface_cont
>> void
>> pipex_iface_fini(struct pipex_iface_context *pipex_iface)
>> {
>> -   pool_put(_session_pool, pipex_iface->multicast_session);
>>NET_LOCK();
>>pipex_iface_stop(pipex_iface);
>>NET_UNLOCK();
>> +   pipex_rele_session(pipex_iface->multicast_session);
>> +   if_put(pipex_iface->ifnet_this);
>> }
>> 
>> int
>> @@ -358,7 +360,7 @@ pipex_init_session(struct pipex_session
>>MIN(req->pr_local_address.ss_len,
>> sizeof(session->local)));
>> #ifdef PIPEX_PPPOE
>>if (req->pr_protocol == PIPEX_PROTO_PPPOE)
>> -   session->proto.pppoe.over_ifidx = over_ifp->if_index;
>> +   session->proto.pppoe.over_iface =
>> if_get(over_ifp->if_index);
>> #endif
>> #ifdef PIPEX_PPTP
>>if (req->pr_protocol == PIPEX_PROTO_PPTP) {
>> @@ -421,6 +423,13 @@ pipex_init_session(struct pipex_session
>> void
>> pipex_rele_session(struct pipex_session *session)
>> {
>> +#ifdef PIPEX_PPPOE
>> +   if (session->protocol == PIPEX_PROTO_PPPOE)
>> +   

Re: ffs(3): libc arch versions, regress

2020-06-25 Thread Christian Weisgerber
Trying again, this time with powerpc64 added:

This adds the optimized ffs(3) versions on aarch64, powerpc, and
powerpc64 to libc.  Also add a brief regression test.

Index: lib/libc/arch/aarch64/string/Makefile.inc
===
RCS file: /cvs/src/lib/libc/arch/aarch64/string/Makefile.inc,v
retrieving revision 1.1
diff -u -p -r1.1 Makefile.inc
--- lib/libc/arch/aarch64/string/Makefile.inc   11 Jan 2017 18:09:24 -  
1.1
+++ lib/libc/arch/aarch64/string/Makefile.inc   11 Jun 2020 20:30:34 -
@@ -2,7 +2,7 @@
 
 SRCS+= bcopy.c memcpy.c memmove.c \
strchr.c strrchr.c \
-   bcmp.c bzero.c ffs.c memchr.c memcmp.c memset.c \
+   bcmp.c bzero.c ffs.S memchr.c memcmp.c memset.c \
strcmp.c strncmp.c \
strcat.c strcpy.c strcspn.c strlen.c strlcat.c strlcpy.c \
strncat.c  strncpy.c strpbrk.c strsep.c \
Index: lib/libc/arch/aarch64/string/ffs.S
===
RCS file: lib/libc/arch/aarch64/string/ffs.S
diff -N lib/libc/arch/aarch64/string/ffs.S
--- /dev/null   1 Jan 1970 00:00:00 -
+++ lib/libc/arch/aarch64/string/ffs.S  11 Jun 2020 20:31:19 -
@@ -0,0 +1,18 @@
+/* $OpenBSD$ */
+/*
+ * Written by Christian Weisgerber .
+ * Public domain.
+ */
+ 
+#include "DEFS.h"
+
+ENTRY(ffs)
+   RETGUARD_SETUP(ffs, x15)
+   rbitw1, w0
+   clz w1, w1
+   cmp w0, wzr
+   csinc   w0, wzr, w1, eq
+   RETGUARD_CHECK(ffs, x15)
+   ret
+END(ffs)
+.protected
Index: lib/libc/arch/powerpc/string/Makefile.inc
===
RCS file: /cvs/src/lib/libc/arch/powerpc/string/Makefile.inc,v
retrieving revision 1.6
diff -u -p -r1.6 Makefile.inc
--- lib/libc/arch/powerpc/string/Makefile.inc   15 May 2015 22:29:37 -  
1.6
+++ lib/libc/arch/powerpc/string/Makefile.inc   11 Jun 2020 20:33:04 -
@@ -2,7 +2,7 @@
 
 SRCS+= memcpy.c memmove.S \
strchr.c strrchr.c \
-   bcmp.c bzero.c ffs.c memchr.c memcmp.c memset.c strcat.c \
+   bcmp.c bzero.c ffs.S memchr.c memcmp.c memset.c strcat.c \
strcmp.c strcpy.c strcspn.c strlen.c strlcat.c strlcpy.c \
strncat.c strncmp.c strncpy.c strpbrk.c strsep.c \
strspn.c strstr.c swab.c
Index: lib/libc/arch/powerpc/string/ffs.S
===
RCS file: lib/libc/arch/powerpc/string/ffs.S
diff -N lib/libc/arch/powerpc/string/ffs.S
--- /dev/null   1 Jan 1970 00:00:00 -
+++ lib/libc/arch/powerpc/string/ffs.S  11 Jun 2020 20:33:19 -
@@ -0,0 +1,16 @@
+/* $OpenBSD$ */
+/*
+ * Written by Christian Weisgerber .
+ * Public domain.
+ */
+ 
+#include "SYS.h"
+
+ENTRY(ffs)
+   neg %r4, %r3
+   and %r3, %r3, %r4
+   cntlzw  %r3, %r3
+   subfic  %r3, %r3, 32
+   blr
+END(ffs)
+.protected
Index: lib/libc/arch/powerpc64/string/Makefile.inc
===
RCS file: /cvs/src/lib/libc/arch/powerpc64/string/Makefile.inc,v
retrieving revision 1.1
diff -u -p -r1.1 Makefile.inc
--- lib/libc/arch/powerpc64/string/Makefile.inc 25 Jun 2020 02:34:22 -  
1.1
+++ lib/libc/arch/powerpc64/string/Makefile.inc 25 Jun 2020 20:53:42 -
@@ -2,7 +2,7 @@
 
 SRCS+= memcpy.c memmove.S \
strchr.c strrchr.c \
-   bcmp.c bzero.c ffs.c memchr.c memcmp.c memset.c strcat.c \
+   bcmp.c bzero.c ffs.S memchr.c memcmp.c memset.c strcat.c \
strcmp.c strcpy.c strcspn.c strlen.c strlcat.c strlcpy.c \
strncat.c strncmp.c strncpy.c strpbrk.c strsep.c \
strspn.c strstr.c swab.c
Index: lib/libc/arch/powerpc64/string/ffs.S
===
RCS file: lib/libc/arch/powerpc64/string/ffs.S
diff -N lib/libc/arch/powerpc64/string/ffs.S
--- /dev/null   1 Jan 1970 00:00:00 -
+++ lib/libc/arch/powerpc64/string/ffs.S25 Jun 2020 20:57:16 -
@@ -0,0 +1,15 @@
+/* $OpenBSD$ */
+/*
+ * Written by Christian Weisgerber .
+ * Public domain.
+ */
+ 
+#include "DEFS.h"
+
+ENTRY(ffs)
+   neg %r4, %r3
+   and %r3, %r3, %r4
+   cntlzw  %r3, %r3
+   subfic  %r3, %r3, 32
+   blr
+END_BUILTIN(ffs)
Index: regress/lib/libc/ffs/Makefile
===
RCS file: regress/lib/libc/ffs/Makefile
diff -N regress/lib/libc/ffs/Makefile
--- /dev/null   1 Jan 1970 00:00:00 -
+++ regress/lib/libc/ffs/Makefile   20 Jun 2020 15:26:51 -
@@ -0,0 +1,6 @@
+PROG=  ffs_test
+
+# prevent constant folding and inlining of __builtin_ffs()
+CFLAGS+=   -ffreestanding
+
+.include 
Index: regress/lib/libc/ffs/ffs_test.c
===
RCS file: regress/lib/libc/ffs/ffs_test.c
diff -N regress/lib/libc/ffs/ffs_test.c
--- /dev/null   1 Jan 1970 00:00:00 -
+++ regress/lib/libc/ffs/ffs_test.c 12 Jun 2020 

Re: pipex(4): use reference counters for `ifnet'

2020-06-25 Thread Vitaliy Makkoveev
Updated diff. 

OpenBSD uses 16 bit counter for allocate interface indexes. So we can't
store index in session and be sure if_get(9) returned `ifnet' is our
original `ifnet'.

Now each pipex(4) session has it's own reference to `ifnet'. Also pppoe
related sessions has the reference to it's ethernet interface.

All `ifnet's are obtained by if_get(9) and we use `if_index' from stored
reference to `ifnet'. 

Index: net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.90
diff -u -p -r1.90 if_pppx.c
--- net/if_pppx.c   24 Jun 2020 08:52:53 -  1.90
+++ net/if_pppx.c   25 Jun 2020 16:41:25 -
@@ -714,15 +714,15 @@ pppx_add_session(struct pppx_dev *pxd, s
ifp->if_softc = pxi;
/* ifp->if_rdomain = req->pr_rdomain; */
 
-   error = pipex_link_session(session, >pxi_ifcontext);
-   if (error)
-   goto remove;
-
/* XXXSMP breaks atomicity */
NET_UNLOCK();
if_attach(ifp);
NET_LOCK();
 
+   error = pipex_link_session(session, >pxi_ifcontext);
+   if (error)
+   goto detach;
+
if_addgroup(ifp, "pppx");
if_alloc_sadl(ifp);
 
@@ -771,7 +771,12 @@ pppx_add_session(struct pppx_dev *pxd, s
 
return (error);
 
-remove:
+detach:
+   /* XXXSMP breaks atomicity */
+   NET_UNLOCK();
+   if_detach(ifp);
+   NET_LOCK();
+
if (RBT_REMOVE(pppx_ifs, _ifs, pxi) == NULL)
panic("%s: inconsistent RB tree", __func__);
LIST_REMOVE(pxi, pxi_list);
@@ -860,13 +865,13 @@ pppx_if_destroy(struct pppx_dev *pxd, st
CLR(ifp->if_flags, IFF_RUNNING);
 
pipex_unlink_session(session);
+   pipex_rele_session(session);
 
/* XXXSMP breaks atomicity */
NET_UNLOCK();
if_detach(ifp);
NET_LOCK();
 
-   pipex_rele_session(session);
if (RBT_REMOVE(pppx_ifs, _ifs, pxi) == NULL)
panic("%s: inconsistent RB tree", __func__);
LIST_REMOVE(pxi, pxi_list);
Index: net/pipex.c
===
RCS file: /cvs/src/sys/net/pipex.c,v
retrieving revision 1.116
diff -u -p -r1.116 pipex.c
--- net/pipex.c 22 Jun 2020 09:38:15 -  1.116
+++ net/pipex.c 25 Jun 2020 16:41:25 -
@@ -148,7 +148,7 @@ pipex_iface_init(struct pipex_iface_cont
struct pipex_session *session;
 
pipex_iface->pipexmode = 0;
-   pipex_iface->ifnet_this = ifp;
+   pipex_iface->ifnet_this = if_get(ifp->if_index);
 
if (pipex_rd_head4 == NULL) {
if (!rn_inithead((void **)_rd_head4,
@@ -165,6 +165,7 @@ pipex_iface_init(struct pipex_iface_cont
session = pool_get(_session_pool, PR_WAITOK | PR_ZERO);
session->is_multicast = 1;
session->pipex_iface = pipex_iface;
+   session->ifnet_this = if_get(ifp->if_index);
pipex_iface->multicast_session = session;
 }
 
@@ -194,10 +195,11 @@ pipex_iface_stop(struct pipex_iface_cont
 void
 pipex_iface_fini(struct pipex_iface_context *pipex_iface)
 {
-   pool_put(_session_pool, pipex_iface->multicast_session);
NET_LOCK();
pipex_iface_stop(pipex_iface);
NET_UNLOCK();
+   pipex_rele_session(pipex_iface->multicast_session);
+   if_put(pipex_iface->ifnet_this);
 }
 
 int
@@ -358,7 +360,7 @@ pipex_init_session(struct pipex_session 
MIN(req->pr_local_address.ss_len, sizeof(session->local)));
 #ifdef PIPEX_PPPOE
if (req->pr_protocol == PIPEX_PROTO_PPPOE)
-   session->proto.pppoe.over_ifidx = over_ifp->if_index;
+   session->proto.pppoe.over_iface = if_get(over_ifp->if_index);
 #endif
 #ifdef PIPEX_PPTP
if (req->pr_protocol == PIPEX_PROTO_PPTP) {
@@ -421,6 +423,13 @@ pipex_init_session(struct pipex_session 
 void
 pipex_rele_session(struct pipex_session *session)
 {
+#ifdef PIPEX_PPPOE
+   if (session->protocol == PIPEX_PROTO_PPPOE)
+   if_put(session->proto.pppoe.over_iface);
+#endif
+   if (session->ifnet_this) {
+   if_put(session->ifnet_this);
+   }
if (session->mppe_recv.old_session_keys)
pool_put(_key_pool, session->mppe_recv.old_session_keys);
pool_put(_session_pool, session);
@@ -439,6 +448,7 @@ pipex_link_session(struct pipex_session 
return (EEXIST);
 
session->pipex_iface = iface;
+   session->ifnet_this = if_get(iface->ifnet_this->if_index);
 
LIST_INSERT_HEAD(_session_list, session, session_list);
chain = PIPEX_ID_HASHTABLE(session->session_id);
@@ -655,7 +665,7 @@ pipex_destroy_session(struct pipex_sessi
}
 
pipex_unlink_session(session);
-   pool_put(_session_pool, session);
+   pipex_rele_session(session);
 
return (0);
 }
@@ -919,7 +929,7 @@ pipex_ip_output(struct mbuf *m0, struct 
struct ifnet *ifp;
 
/* output succeed here as a 

Re: Destructive Install Process (fdisk)

2020-06-25 Thread David Blevins
List,

Apologies, I left HTML mode enabled on my web client.
I understand that a Linux dmesg is probably not very useful.
I can provide a different set of system information (or an
OpenBSD dmesg from an MBR boot) but I'm not sure what
set of information would be most useful in determining my
issue regarding GPT boot on this particular system.



ssh-keygen(1): -a is missing in SYNOPSIS and small wording change

2020-06-25 Thread Solene Rapenne
I found that ssh-keygen(1) missed mention of -a flag in SYNOPSIS.

The following patch adds mention to [-a rounds] with default (no
flag), -p, -c, -K and -A

All the functions triggered by these flags use the rounds variable
defined with -a parameter (default 0)


I also propose a small wording change, in the sentence:
"After a key is generated, instructions below detail [...]"

I thought below refered to the list of options after that sentence,
but it may be a mistake of mine here.


Index: ssh-keygen.1
===
RCS file: /cvs/src/usr.bin/ssh/ssh-keygen.1,v
retrieving revision 1.203
diff -u -p -r1.203 ssh-keygen.1
--- ssh-keygen.13 Apr 2020 02:26:56 -   1.203
+++ ssh-keygen.125 Jun 2020 16:28:54 -
@@ -44,6 +44,7 @@
 .Sh SYNOPSIS
 .Nm ssh-keygen
 .Op Fl q
+.Op Fl a Ar rounds
 .Op Fl b Ar bits
 .Op Fl C Ar comment
 .Op Fl f Ar output_keyfile
@@ -54,6 +55,7 @@
 .Op Fl w Ar provider
 .Nm ssh-keygen
 .Fl p
+.Op Fl a Ar rounds
 .Op Fl f Ar keyfile
 .Op Fl m Ar format
 .Op Fl N Ar new_passphrase
@@ -71,6 +73,7 @@
 .Op Fl f Ar input_keyfile
 .Nm ssh-keygen
 .Fl c
+.Op Fl a Ar rounds
 .Op Fl C Ar comment
 .Op Fl f Ar keyfile
 .Op Fl P Ar passphrase
@@ -93,6 +96,7 @@
 .Op Fl f Ar known_hosts_file
 .Nm ssh-keygen
 .Fl K
+.Op Fl a Ar rounds
 .Op Fl w Ar provider
 .Nm ssh-keygen
 .Fl R Ar hostname
@@ -125,6 +129,7 @@
 .Op Fl f Ar input_keyfile
 .Nm ssh-keygen
 .Fl A
+.Op Fl a Ar rounds
 .Op Fl f Ar prefix_path
 .Nm ssh-keygen
 .Fl k
@@ -248,7 +253,9 @@ keys may be converted using this option 
 .Fl p
 (change passphrase) flag.
 .Pp
-After a key is generated, instructions below detail where the keys
+After a key is generated,
+.Nm
+will ask where the keys
 should be placed to be activated.
 .Pp
 The options are as follows:



Re: Destructive Install Process (fdisk)

2020-06-25 Thread David Blevins
> Personally, I don't think we should provide that information.  But
>someone is providing it, I guess, in the FAQ.

I'm sure it's just my system, but I've tried the instructions (even
checked I was using capital M instead of lowercase m) and
they don't work in Ubuntu.  I would love to get corrected here
but I actually have an easier time creating the install media
in Windows. I've followed the recipe 30 times this week.

I would love to contribute to the GRUB boot information but
I have no clue who works on that.  I would prefer contacting
them directly rather than hitting the mailing list with
irrelevant information.

Any insight regarding the GPT boot would be appreciated.
As I said, MBR boot works fine.

Thanks for the feedback, looking forward to any potential
insight.

On Thu, Jun 25, 2020 at 4:16 PM Theo de Raadt  wrote:

> David Blevins  wrote:
>
> > Theo et al.,
> >
> > I understand that it's an installer, but I do have an interest in
> > having an OpenBSD installing along side a GNU Linux installation.
> > That said, the basic "OpenBSD takes over the machine" install does
> > in fact work just fine.  If that's the only intended use case why
> > provide instructions for enabling multiboot through GRUB? I've been
> > attempting multiboot through GRUB 2 but the issues I'm encountering
> > seem fairly common, and I haven't gotten into the fine details yet.
>
> Personally, I don't think we should provide that information.  But
> someone is providing it, I guess, in the FAQ.
>
> > I'd say that I simply don't see why the installer destructively
> > re-arranges the disk's scheme prior to officially choosing to write
> > the new partitioning scheme to the disk.  Howver, I must admit that
> > the procedure would not work if this were deferred to the next step
> > in DISKLABEL.
>
> I haven't checked the FAQ.  You probably went off the recipe.  And
> you reap what you sow.
>


Re: Destructive Install Process (fdisk)

2020-06-25 Thread Theo de Raadt
David Blevins  wrote:

> Theo et al.,
> 
> I understand that it's an installer, but I do have an interest in 
> having an OpenBSD installing along side a GNU Linux installation.
> That said, the basic "OpenBSD takes over the machine" install does
> in fact work just fine.  If that's the only intended use case why
> provide instructions for enabling multiboot through GRUB? I've been
> attempting multiboot through GRUB 2 but the issues I'm encountering
> seem fairly common, and I haven't gotten into the fine details yet.

Personally, I don't think we should provide that information.  But
someone is providing it, I guess, in the FAQ.

> I'd say that I simply don't see why the installer destructively 
> re-arranges the disk's scheme prior to officially choosing to write
> the new partitioning scheme to the disk.  Howver, I must admit that
> the procedure would not work if this were deferred to the next step
> in DISKLABEL.

I haven't checked the FAQ.  You probably went off the recipe.  And
you reap what you sow.



Re: Destructive Install Process (fdisk)

2020-06-25 Thread David Blevins
Theo et al.,

I understand that it's an installer, but I do have an interest in
having an OpenBSD installing along side a GNU Linux installation.
That said, the basic "OpenBSD takes over the machine" install does
in fact work just fine.  If that's the only intended use case why
provide instructions for enabling multiboot through GRUB? I've been
attempting multiboot through GRUB 2 but the issues I'm encountering
seem fairly common, and I haven't gotten into the fine details yet.
I'd say that I simply don't see why the installer destructively
re-arranges the disk's scheme prior to officially choosing to write
the new partitioning scheme to the disk.  Howver, I must admit that
the procedure would not work if this were deferred to the next step
in DISKLABEL.

Regarding AMD64 (yes, it's an AMD system, and I'm writing this from
a fresh Ubuntu install for convenience).  I've only managed to get
the system to work from an MBR arrangement thus far. I have included
a DMESG from the fresh Ubuntu install as an email attachment.

On Thu, Jun 25, 2020 at 3:56 PM Theo de Raadt  wrote:

> David Blevins  wrote:
>
> > OpenBSD Community,
> >
> > I've been experimenting with the OpenBSD 6.7 install process (though
> > this "issue" is likely present in earlier version) and have noticed
> > that the fdisk program in the installation program will destructively
> > edit the hard drive upon selecting either MBR or GPT partitioning.
>
> Yes, because it is an installer of OpenBSD.
>
> The default usage pattern is "OpenBSD takes over the machine".  OpenBSD
> runs on may kinds of platforms, and the dominant pattern is "take over
> the machine".
>
> Perhaps you are thinking of putting multiple operating systems on the
> same disk?
>
> We put minimal effort into sharing a machine between multiple operating
> systems, because it would SUBSTANTIALLY complicate the install
> procedure.
>
> And want to know the other reason we don't have built-in support for
> multiple OS on the disk?  None of us (developers) put multiple operating
> systems onto a single machine.  We tend to develop code we use.
> Meaning, we tend to not develop code we won't use ourselves, because if
> we don't use it ourselves all the time then we won't continually test it
> and take care of it in the future and you can bet it is the first thing
> that breaks, badly.
>
> > I have repeatedly wiped a test hard drive's partitioning scheme as a
> > consequence of this, in spite of only desiring to examine the
> > partitioning scheme.
>
> Oh come on.  You ran the installer.  You are about 10 questions in
> at this point, and you know you are in the installer, and you are
> the first person I've heard in a decade to complain that it should
> not be destructive.
>
> > Is anyone receptive to a less-commital partitioning process for
> > installation?  It seems to me that the drive's partitioning scheme
> > should not be re-written until the user has affirmed that the
> > particular scheme and layout is what they want.
>
> BTW, wait until you try to adapt that to other architectures...
> our code, as written now, makes those easy, but what you propose
> makes the code very difficult.
>
[0.00] Linux version 5.3.0-61-generic (buildd@lcy01-amd64-014) (gcc 
version 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04)) #55~18.04.1-Ubuntu SMP Mon Jun 22 
16:40:20 UTC 2020 (Ubuntu 5.3.0-61.55~18.04.1-generic 5.3.18)
[0.00] Command line: BOOT_IMAGE=/boot/vmlinuz-5.3.0-61-generic 
root=UUID=b6ae5d12-6509-4ba8-9a1c-74440f2fa573 ro quiet splash vt.handoff=1
[0.00] KERNEL supported cpus:
[0.00]   Intel GenuineIntel
[0.00]   AMD AuthenticAMD
[0.00]   Hygon HygonGenuine
[0.00]   Centaur CentaurHauls
[0.00]   zhaoxin   Shanghai  
[0.00] random: get_random_u32 called from bsp_init_amd+0x207/0x2c0 with 
crng_init=0
[0.00] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point 
registers'
[0.00] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers'
[0.00] x86/fpu: Supporting XSAVE feature 0x004: 'AVX registers'
[0.00] x86/fpu: xstate_offset[2]:  576, xstate_sizes[2]:  256
[0.00] x86/fpu: Enabled xstate features 0x7, context size is 832 bytes, 
using 'standard' format.
[0.00] BIOS-provided physical RAM map:
[0.00] BIOS-e820: [mem 0x-0x0009] usable
[0.00] BIOS-e820: [mem 0x0010-0xaa9b] usable
[0.00] BIOS-e820: [mem 0xaa9c-0xaab43fff] reserved
[0.00] BIOS-e820: [mem 0xaab44000-0xaab53fff] ACPI data
[0.00] BIOS-e820: [mem 0xaab54000-0xab957fff] ACPI NVS
[0.00] BIOS-e820: [mem 0xab958000-0xaca34fff] reserved
[0.00] BIOS-e820: [mem 0xaca35000-0xaca35fff] usable
[0.00] BIOS-e820: [mem 0xaca36000-0xacc3bfff] ACPI NVS
[0.00] BIOS-e820: [mem 

Re: Destructive Install Process (fdisk)

2020-06-25 Thread Theo de Raadt
David Blevins  wrote:

> OpenBSD Community,
> 
> I've been experimenting with the OpenBSD 6.7 install process (though
> this "issue" is likely present in earlier version) and have noticed
> that the fdisk program in the installation program will destructively
> edit the hard drive upon selecting either MBR or GPT partitioning.

Yes, because it is an installer of OpenBSD.

The default usage pattern is "OpenBSD takes over the machine".  OpenBSD
runs on may kinds of platforms, and the dominant pattern is "take over
the machine".

Perhaps you are thinking of putting multiple operating systems on the
same disk?

We put minimal effort into sharing a machine between multiple operating
systems, because it would SUBSTANTIALLY complicate the install
procedure.

And want to know the other reason we don't have built-in support for
multiple OS on the disk?  None of us (developers) put multiple operating
systems onto a single machine.  We tend to develop code we use.
Meaning, we tend to not develop code we won't use ourselves, because if
we don't use it ourselves all the time then we won't continually test it
and take care of it in the future and you can bet it is the first thing
that breaks, badly.

> I have repeatedly wiped a test hard drive's partitioning scheme as a
> consequence of this, in spite of only desiring to examine the
> partitioning scheme.

Oh come on.  You ran the installer.  You are about 10 questions in
at this point, and you know you are in the installer, and you are
the first person I've heard in a decade to complain that it should
not be destructive.

> Is anyone receptive to a less-commital partitioning process for
> installation?  It seems to me that the drive's partitioning scheme
> should not be re-written until the user has affirmed that the
> particular scheme and layout is what they want.

BTW, wait until you try to adapt that to other architectures...
our code, as written now, makes those easy, but what you propose
makes the code very difficult.



Destructive Install Process (fdisk)

2020-06-25 Thread David Blevins
OpenBSD Community,

I've been experimenting with the OpenBSD 6.7 install process (though
this "issue" is likely present in earlier version) and have noticed
that the fdisk program in the installation program will destructively
edit the hard drive upon selecting either MBR or GPT partitioning.
I have repeatedly wiped a test hard drive's partitioning scheme as a
consequence of this, in spite of only desiring to examine the
partitioning scheme.

Is anyone receptive to a less-commital partitioning process for
installation?  It seems to me that the drive's partitioning scheme
should not be re-written until the user has affirmed that the
particular scheme and layout is what they want.

Additionally, I have noticed that (per warnings in the installation
program) GPT partitioning does not work on my system.  Does anyone
have insight into this issue, and if so is there any interest in
a fix for GPT partitioning?

-David Blevins


Re: pipex(4): use reference counters for `ifnet'

2020-06-25 Thread Vitaliy Makkoveev
> On 25 Jun 2020, at 16:35, Christiano F. Haesbaert  
> wrote:
> 
> On Thu, 25 Jun 2020 at 14:06, Vitaliy Makkoveev
>  wrote:
>> 
>> 
>> 
>>> On 25 Jun 2020, at 11:55, Martin Pieuchot  wrote:
>>> 
>>> On 24/06/20(Wed) 17:10, Vitaliy Makkoveev wrote:
 While `mbuf' enqueued to `pipexinq' or `pipexoutq' it has the reference
 to corresponding pipex(4) session as `ph_cookie'. `ph_cookie' is
 accessed by pipexintr() and it's always defferent context from context
 where we destroy session. `ph_cookie' is protected only while we destroy
 session by pipex_timer() but this protection is not effective. While we
 destroy session related to pppx(4) `ph_cookie' is not potected. While we
 destroy session related to pppac(4) by `PIPEXSMODE' ioctl() or by
 closing pppac(4) device node `ph_cookie' is also not protected.
 
 I'am going to use reference counters to protect `ph_cookie' but some
 additional steps required to be done.
>>> 
>>> Please no.  Store an ifidx in session instead of a pointer.  Such
>>> index are guaranteed to be unique and can be used with if_get(9).
>> 
>> This means I should do if_get(9) before each `ifnet’ usage. Also I
>> should do checks and introduce error path. It's ugly.
>> 
> 
> My 2 cents,
> 
> Not really, this is actually easier to reason with, it produces less
> dependencies between data structures and the protocol is kinda clear,
> you have to account for the loss of atomicity.
> Reference counting is pretty much the worst thing you can do in MP 
> development.
> Very often it is actually desirable to pay for the cost of a lookup
> (which is not the case here, since it's a silly O(1)) than to pay the
> human/debug cost of interlinking data structures.
> Also, in an ideal world if_get() would be RCUed (or SMRed) and would
> not require bumping a reference count which implies an atomic
> operation, it would also remove the need for if_put() since the epoch
> is "until we context switch", as the kernel is not preemptive this is
> easy to do without introducing new bugs.
> 

But I can’t store interface index with session instead of pointer.
Session can be alive after corresponding `ifnet’ was destroyed and
replaced by new `ifnet’.

> 
>> Alternatively, I can pass `ifnet->if_index’ instead of `ifnet’ to
>> sessinon being linked and store obtained reference up to session
>> destruction. I should add one error path to pipex_link_session().
>> It’s ugly but a little bit.
>> 
>>> 
>>> We deliberately kept if_ref() private to keep the code base coherent.
>> 
>> Could you explain please why if_ref() and if_get() are incoherent?



Re: pipex(4): use reference counters for `ifnet'

2020-06-25 Thread Christiano F. Haesbaert
You can.

The idea is that ifindex is always monotonically increased, so to actually
get a new interface you would have to have "overflowed" 65k interfaces,
which is unreal.

So if your interface is gone, you can be sure if_get will give you NULL.

On Thu, Jun 25, 2020, 18:55 Vitaliy Makkoveev 
wrote:

> Updated diff.
>
> OpenBSD uses 16 bit counter for allocate interface indexes. So we can't
> store index in session and be sure if_get(9) returned `ifnet' is our
> original `ifnet'.
>
> Now each pipex(4) session has it's own reference to `ifnet'. Also pppoe
> related sessions has the reference to it's ethernet interface.
>
> All `ifnet's are obtained by if_get(9) and we use `if_index' from stored
> reference to `ifnet'.
>
> Index: net/if_pppx.c
> ===
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.90
> diff -u -p -r1.90 if_pppx.c
> --- net/if_pppx.c   24 Jun 2020 08:52:53 -  1.90
> +++ net/if_pppx.c   25 Jun 2020 16:41:25 -
> @@ -714,15 +714,15 @@ pppx_add_session(struct pppx_dev *pxd, s
> ifp->if_softc = pxi;
> /* ifp->if_rdomain = req->pr_rdomain; */
>
> -   error = pipex_link_session(session, >pxi_ifcontext);
> -   if (error)
> -   goto remove;
> -
> /* XXXSMP breaks atomicity */
> NET_UNLOCK();
> if_attach(ifp);
> NET_LOCK();
>
> +   error = pipex_link_session(session, >pxi_ifcontext);
> +   if (error)
> +   goto detach;
> +
> if_addgroup(ifp, "pppx");
> if_alloc_sadl(ifp);
>
> @@ -771,7 +771,12 @@ pppx_add_session(struct pppx_dev *pxd, s
>
> return (error);
>
> -remove:
> +detach:
> +   /* XXXSMP breaks atomicity */
> +   NET_UNLOCK();
> +   if_detach(ifp);
> +   NET_LOCK();
> +
> if (RBT_REMOVE(pppx_ifs, _ifs, pxi) == NULL)
> panic("%s: inconsistent RB tree", __func__);
> LIST_REMOVE(pxi, pxi_list);
> @@ -860,13 +865,13 @@ pppx_if_destroy(struct pppx_dev *pxd, st
> CLR(ifp->if_flags, IFF_RUNNING);
>
> pipex_unlink_session(session);
> +   pipex_rele_session(session);
>
> /* XXXSMP breaks atomicity */
> NET_UNLOCK();
> if_detach(ifp);
> NET_LOCK();
>
> -   pipex_rele_session(session);
> if (RBT_REMOVE(pppx_ifs, _ifs, pxi) == NULL)
> panic("%s: inconsistent RB tree", __func__);
> LIST_REMOVE(pxi, pxi_list);
> Index: net/pipex.c
> ===
> RCS file: /cvs/src/sys/net/pipex.c,v
> retrieving revision 1.116
> diff -u -p -r1.116 pipex.c
> --- net/pipex.c 22 Jun 2020 09:38:15 -  1.116
> +++ net/pipex.c 25 Jun 2020 16:41:25 -
> @@ -148,7 +148,7 @@ pipex_iface_init(struct pipex_iface_cont
> struct pipex_session *session;
>
> pipex_iface->pipexmode = 0;
> -   pipex_iface->ifnet_this = ifp;
> +   pipex_iface->ifnet_this = if_get(ifp->if_index);
>
> if (pipex_rd_head4 == NULL) {
> if (!rn_inithead((void **)_rd_head4,
> @@ -165,6 +165,7 @@ pipex_iface_init(struct pipex_iface_cont
> session = pool_get(_session_pool, PR_WAITOK | PR_ZERO);
> session->is_multicast = 1;
> session->pipex_iface = pipex_iface;
> +   session->ifnet_this = if_get(ifp->if_index);
> pipex_iface->multicast_session = session;
>  }
>
> @@ -194,10 +195,11 @@ pipex_iface_stop(struct pipex_iface_cont
>  void
>  pipex_iface_fini(struct pipex_iface_context *pipex_iface)
>  {
> -   pool_put(_session_pool, pipex_iface->multicast_session);
> NET_LOCK();
> pipex_iface_stop(pipex_iface);
> NET_UNLOCK();
> +   pipex_rele_session(pipex_iface->multicast_session);
> +   if_put(pipex_iface->ifnet_this);
>  }
>
>  int
> @@ -358,7 +360,7 @@ pipex_init_session(struct pipex_session
> MIN(req->pr_local_address.ss_len,
> sizeof(session->local)));
>  #ifdef PIPEX_PPPOE
> if (req->pr_protocol == PIPEX_PROTO_PPPOE)
> -   session->proto.pppoe.over_ifidx = over_ifp->if_index;
> +   session->proto.pppoe.over_iface =
> if_get(over_ifp->if_index);
>  #endif
>  #ifdef PIPEX_PPTP
> if (req->pr_protocol == PIPEX_PROTO_PPTP) {
> @@ -421,6 +423,13 @@ pipex_init_session(struct pipex_session
>  void
>  pipex_rele_session(struct pipex_session *session)
>  {
> +#ifdef PIPEX_PPPOE
> +   if (session->protocol == PIPEX_PROTO_PPPOE)
> +   if_put(session->proto.pppoe.over_iface);
> +#endif
> +   if (session->ifnet_this) {
> +   if_put(session->ifnet_this);
> +   }
> if (session->mppe_recv.old_session_keys)
> pool_put(_key_pool,
> session->mppe_recv.old_session_keys);
> pool_put(_session_pool, session);
> @@ -439,6 +448,7 @@ pipex_link_session(struct pipex_session
> return (EEXIST);
>
> session->pipex_iface = 

Re: ssh-keygen(1): -a is missing in SYNOPSIS and small wording change

2020-06-25 Thread Jason McIntyre
On Thu, Jun 25, 2020 at 06:40:36PM +0200, Solene Rapenne wrote:
> I found that ssh-keygen(1) missed mention of -a flag in SYNOPSIS.
> 

i think this got accidently removed in -r1.184:

remove single letter flags for moduli operations

> The following patch adds mention to [-a rounds] with default (no
> flag), -p, -c, -K and -A
> 

i'm definitely not the right person to ok that

> All the functions triggered by these flags use the rounds variable
> defined with -a parameter (default 0)
> 
> 
> I also propose a small wording change, in the sentence:
> "After a key is generated, instructions below detail [...]"
> 
> I thought below refered to the list of options after that sentence,
> but it may be a mistake of mine here.
> 

i wanted to ask about this text too. it's unclear to me. after a key is
generated, ssh-keygen asks where to save it. i wonder why we use the
wording "should be placed to be activated"? it seems odd, but maybe
there's a reason.

jmc

> 
> Index: ssh-keygen.1
> ===
> RCS file: /cvs/src/usr.bin/ssh/ssh-keygen.1,v
> retrieving revision 1.203
> diff -u -p -r1.203 ssh-keygen.1
> --- ssh-keygen.1  3 Apr 2020 02:26:56 -   1.203
> +++ ssh-keygen.1  25 Jun 2020 16:28:54 -
> @@ -44,6 +44,7 @@
>  .Sh SYNOPSIS
>  .Nm ssh-keygen
>  .Op Fl q
> +.Op Fl a Ar rounds
>  .Op Fl b Ar bits
>  .Op Fl C Ar comment
>  .Op Fl f Ar output_keyfile
> @@ -54,6 +55,7 @@
>  .Op Fl w Ar provider
>  .Nm ssh-keygen
>  .Fl p
> +.Op Fl a Ar rounds
>  .Op Fl f Ar keyfile
>  .Op Fl m Ar format
>  .Op Fl N Ar new_passphrase
> @@ -71,6 +73,7 @@
>  .Op Fl f Ar input_keyfile
>  .Nm ssh-keygen
>  .Fl c
> +.Op Fl a Ar rounds
>  .Op Fl C Ar comment
>  .Op Fl f Ar keyfile
>  .Op Fl P Ar passphrase
> @@ -93,6 +96,7 @@
>  .Op Fl f Ar known_hosts_file
>  .Nm ssh-keygen
>  .Fl K
> +.Op Fl a Ar rounds
>  .Op Fl w Ar provider
>  .Nm ssh-keygen
>  .Fl R Ar hostname
> @@ -125,6 +129,7 @@
>  .Op Fl f Ar input_keyfile
>  .Nm ssh-keygen
>  .Fl A
> +.Op Fl a Ar rounds
>  .Op Fl f Ar prefix_path
>  .Nm ssh-keygen
>  .Fl k
> @@ -248,7 +253,9 @@ keys may be converted using this option 
>  .Fl p
>  (change passphrase) flag.
>  .Pp
> -After a key is generated, instructions below detail where the keys
> +After a key is generated,
> +.Nm
> +will ask where the keys
>  should be placed to be activated.
>  .Pp
>  The options are as follows:
> 



Re: pipex(4): use reference counters for `ifnet'

2020-06-25 Thread Vitaliy Makkoveev



> On 25 Jun 2020, at 11:55, Martin Pieuchot  wrote:
> 
> On 24/06/20(Wed) 17:10, Vitaliy Makkoveev wrote:
>> While `mbuf' enqueued to `pipexinq' or `pipexoutq' it has the reference
>> to corresponding pipex(4) session as `ph_cookie'. `ph_cookie' is
>> accessed by pipexintr() and it's always defferent context from context
>> where we destroy session. `ph_cookie' is protected only while we destroy
>> session by pipex_timer() but this protection is not effective. While we
>> destroy session related to pppx(4) `ph_cookie' is not potected. While we
>> destroy session related to pppac(4) by `PIPEXSMODE' ioctl() or by
>> closing pppac(4) device node `ph_cookie' is also not protected.
>> 
>> I'am going to use reference counters to protect `ph_cookie' but some
>> additional steps required to be done.
> 
> Please no.  Store an ifidx in session instead of a pointer.  Such
> index are guaranteed to be unique and can be used with if_get(9).

This means I should do if_get(9) before each `ifnet’ usage. Also I
should do checks and introduce error path. It's ugly.

Alternatively, I can pass `ifnet->if_index’ instead of `ifnet’ to
sessinon being linked and store obtained reference up to session
destruction. I should add one error path to pipex_link_session().
It’s ugly but a little bit.

> 
> We deliberately kept if_ref() private to keep the code base coherent.

Could you explain please why if_ref() and if_get() are incoherent? 


Re: [PATCH] net: prevent if_clone_destroy from racing with rest of stack

2020-06-25 Thread Vitaliy Makkoveev
On Thu, Jun 25, 2020 at 11:54:48AM +0200, Martin Pieuchot wrote:
> On 23/06/20(Tue) 16:21, Martin Pieuchot wrote:
> > On 23/06/20(Tue) 04:53, Jason A. Donenfeld wrote:
> > > On 6/23/20, Martin Pieuchot  wrote:
> > > > On 23/06/20(Tue) 01:00, Jason A. Donenfeld wrote:
> > > >> You can crash a system by running something like:
> > > >>
> > > >> for i in 1 2 3; do while true; do ifconfig bridge0 create& ifconfig
> > > >> bridge0 destroy& done& done
> 
> As mentioned already the proposed diff doesn't protect creation of cloning
> devices via open(2).  The above test could be replaced by:
> 
> for i in 1 2 3; do while true; \
>   do cat /dev/tun0& ifconfig tun0 destroy& done& done
> 
> The same could be applied to switch(4) or by replacing the cat(1) magic
> with 'ifconfig bridge0 add vether0.
>

I used this [1] diff with a little modification to tun(4). This
modification is required because ifunit() doesn't know about reservaton
used by if_clone_create(). Also I grab a reference on obtained `ifp'.

I run the commands below. System is stable.

 cut begin 1
for i in 1 2 3; do while true; \
do ifconfig tun0 create& cat /dev/tun0& \
ifconfig tun0 destroy& done& done
 cut end 1 

 cut begin 2
ifconfig vether0 create
for i in 1 2 3; do while true; do ifconfig bridge0 create& \
ifconfig bridge0 add vether0& \
ifconfig bridge0 destroy& done& done
 cut end 2 

1. https://marc.info/?l=openbsd-tech=159307900124245=2


Index: sys/net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.610
diff -u -p -r1.610 if.c
--- sys/net/if.c22 Jun 2020 09:45:13 -  1.610
+++ sys/net/if.c25 Jun 2020 11:53:42 -
@@ -1236,6 +1236,18 @@ if_isconnected(const struct ifnet *ifp0,
return (connected);
 }
 
+/* XXX: reserve unit */
+struct if_clone_unit {
+   LIST_ENTRY(if_clone_unit) ifcu_list;
+   struct if_clone *ifcu_ifc;
+   int ifcu_unit;
+};
+
+LIST_HEAD(, if_clone_unit) if_clone_units =
+   LIST_HEAD_INITIALIZER(if_clone_units);
+
+/* XXX: reserve unit */
+
 /*
  * Create a clone network interface.
  */
@@ -1246,6 +1258,8 @@ if_clone_create(const char *name, int rd
struct ifnet *ifp;
int unit, ret;
 
+   struct if_clone_unit *ifcu, *tifcu;
+
ifc = if_clone_lookup(name, );
if (ifc == NULL)
return (EINVAL);
@@ -1253,10 +1267,28 @@ if_clone_create(const char *name, int rd
if (ifunit(name) != NULL)
return (EEXIST);
 
+   /* XXX: reserve unit */
+   ifcu=malloc(sizeof(*ifcu), M_TEMP, M_WAITOK | M_ZERO);
+
+   LIST_FOREACH(tifcu, _clone_units, ifcu_list) {
+   if (tifcu->ifcu_ifc == ifc && tifcu->ifcu_unit == unit) {
+   free(ifcu, M_TEMP, 0);
+   return (EEXIST);
+   }
+   }
+   ifcu->ifcu_ifc = ifc;
+   ifcu->ifcu_unit = unit;
+   LIST_INSERT_HEAD(_clone_units, ifcu, ifcu_list);
+   /* XXX: reserve unit */
+
+
ret = (*ifc->ifc_create)(ifc, unit);
 
-   if (ret != 0 || (ifp = ifunit(name)) == NULL)
+   if (ret != 0 || (ifp = ifunit(name)) == NULL) {
+   LIST_REMOVE(ifcu, ifcu_list);
+   free(ifcu, M_TEMP, 0);
return (ret);
+   }
 
NET_LOCK();
if_addgroup(ifp, ifc->ifc_name);
@@ -1275,15 +1307,18 @@ if_clone_destroy(const char *name)
 {
struct if_clone *ifc;
struct ifnet *ifp;
-   int ret;
+   int unit, ret;
+
+   struct if_clone_unit *ifcu, *tifcu;
 
-   ifc = if_clone_lookup(name, NULL);
+   ifc = if_clone_lookup(name, );
if (ifc == NULL)
return (EINVAL);
 
ifp = ifunit(name);
if (ifp == NULL)
return (ENXIO);
+   ifp->if_dying = 1;
 
if (ifc->ifc_destroy == NULL)
return (EOPNOTSUPP);
@@ -1298,6 +1333,15 @@ if_clone_destroy(const char *name)
NET_UNLOCK();
ret = (*ifc->ifc_destroy)(ifp);
 
+   /* XXX: release unit */
+   LIST_FOREACH_SAFE(ifcu, _clone_units, ifcu_list, tifcu) {
+   if (ifcu->ifcu_ifc == ifc && ifcu->ifcu_unit == unit) {
+   LIST_REMOVE(ifcu, ifcu_list);
+   free(ifcu, M_TEMP, 0);
+   }
+   }
+   /* XXX: release unit */
+
return (ret);
 }
 
@@ -1749,8 +1793,11 @@ ifunit(const char *name)
KERNEL_ASSERT_LOCKED();
 
TAILQ_FOREACH(ifp, , if_list) {
-   if (strcmp(ifp->if_xname, name) == 0)
+   if (strcmp(ifp->if_xname, name) == 0) {
+   if (ifp->if_dying)
+   ifp = NULL;
return (ifp);
+   }
}
return (NULL);
 }
@@ -1958,6 +2005,7 @@ ifioctl(struct socket *so, u_long cmd, c
ifp = ifunit(ifr->ifr_name);
if (ifp == 

if_bce.c fix hang after resume

2020-06-25 Thread Richard Hopkins
I've managed to produce a patch to fix a hang after resume on my machine and
would much appreciate some help with reviewing the changes, as whilst I've
tried to follow the (excellent) documentation/FAQs this is new to me
especially hardware.

Original problem (on at least 6.6 and 6.7 i386) (dmesg report on bugs@)
* apmd && zzz
resume causes a complete hang due to if_bce.  Other devices are not
reinitialized such as wifi, and not even the CAPS lock LED gets toggled and
the machine is unusable.

This patch produced using the github.com mirror fixes the problem for me so
that resume doesn't cause a hang and bce0 then handles traffic like before the
suspend.

Is there anything I can improve?  Is anyone able to test it works on their
machine too?

Thanks

diff --git if_bce.c if_bce.c
index 7228bba39..223801bcd 100644
--- if_bce.c
+++ if_bce.c
@@ -444,6 +444,7 @@ bce_activate(struct device *self, int act)
         break;
     case DVACT_RESUME:
         if (ifp->if_flags & IFF_UP) {
+            bce_reset(sc);
             bce_init(ifp);
             bce_start(ifp);
         }




Re: make btrace interval event to reciprocal of ticks

2020-06-25 Thread Yuichiro NAITO
From: Martin Pieuchot 
Subject: Re: make btrace interval event to reciprocal of ticks
Date: Thu, 25 Jun 2020 11:36:48 +0200

> On 23/06/20(Tue) 12:04, Yuichiro NAITO wrote:
>> Current btrace has `interval:hz:1` probe that makes periodically events.
>> `interval:hz:1` looks to make events once per second (= 1Hz),
>> but current implementation makes once per tick (= 100Hz on amd64).
>> I think the interval should be counted by reciprocal of ticks so that fit to 
>> Hz.
> 
> Indeed the current implementations assumes it's a number of ticks.  How
> does other tracing tool behave?  Is the behavior you suggest similar to
> bpftrace(8) or dtrace(1)?

I don't know about bpftrace(8).
Dtrace has interval timer in Hz as Lauri says.

> Would it be complicated to add support for other units, like 'ms' or
> 'sec'?  In that case 'profile:s:10' would mean every 10sec while
> 'profile:hz:10' would mean every ~6sec, right?

I feel it's ok to just rename 'interval:hz' to 'interval:ticks' with
no implementation change. (but I hope that 100 is allowed.)
We can know the tick interval from 'hz' in `sysctl -n kern.clockrate`.
So it's easy to understand that 'interval:ticks:N' fires on every N ticks.

And tick resolution is not so high.
In my patch, 51 Hz ~ 100 Hz is calculated to 10 ms (=1 tick) interval.
It might confuse some users.

In my usecase, I want 10ms ~ 1sec intervals.
If N is allowed over 99, 'interval:ticks:N' can be useful to me.

-- 
Yuichiro NAITO
  naito.yuich...@gmail.com



Re: [PATCH] net: prevent if_clone_destroy from racing with rest of stack

2020-06-25 Thread Vitaliy Makkoveev
On Tue, Jun 23, 2020 at 01:00:06AM -0600, Jason A. Donenfeld wrote:
> You can crash a system by running something like:
> 
> for i in 1 2 3; do while true; do ifconfig bridge0 create& ifconfig 
> bridge0 destroy& done& done
> 
> This works with every type of interface I've tried. It appears that
> if_clone_destroy and if_clone_create race with other ioctls, which
> causes a variety of different UaFs or just general logic errors.
> 
> One common root cause appears to be that most ifioctl functions use
> ifunit() to find an interface by name, which traverses if_list. Writes
> to if_list are protected by a lock, but reads are apparently
> unprotected. There's also the question of the life time of the object
> returned from ifunit(). Most things that access 's if_list are
> done without locking, and even if those accesses were to be locked, the
> lock would be released before the object is no longer used, causing the
> UaF in that case as well.
> 
> This patch fixes the issue by making if_clone_{create,destroy} exclusive
> with all other ifioctls.
> ---
>  sys/net/if.c | 36 +++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git sys/net/if.c sys/net/if.c
> index fb2f86f4a7c..9eedea83d32 100644
> --- sys/net/if.c
> +++ sys/net/if.c
> @@ -246,6 +246,11 @@ struct task if_input_task_locked = 
> TASK_INITIALIZER(if_netisr, NULL);
>   */
>  struct rwlock netlock = RWLOCK_INITIALIZER("netlock");
>  
> +/*
> + * Protect modifications to objects owned by ifnet's if_list
> + */
> +struct rwlock iflist_obj_lock = RWLOCK_INITIALIZER("iflist_obj_lock");
> +
>  /*
>   * Network interface utility routines.
>   */
> @@ -1905,7 +1910,7 @@ if_setrdomain(struct ifnet *ifp, int rdomain)
>   * Interface ioctls.
>   */
>  int
> -ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
> +ifioctl_unlocked(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
>  {
>   struct ifnet *ifp;
>   struct ifreq *ifr = (struct ifreq *)data;
> @@ -2432,6 +2437,35 @@ ifioctl_get(u_long cmd, caddr_t data)
>   return (error);
>  }
>  
> +int
> +ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
> +{
> + int ret;
> +
> + switch (cmd) {
> + case SIOCIFCREATE:
> + case SIOCIFDESTROY:
> + rw_enter_write(_obj_lock);
> + break;
> + default:
> + rw_enter_read(_obj_lock);
> + }
> +
> + ret = ifioctl_unlocked(so, cmd, data, p);
> +
> + switch (cmd) {
> + case SIOCIFCREATE:
> + case SIOCIFDESTROY:
> + rw_exit_write(_obj_lock);
> + break;
> + default:
> + rw_exit_read(_obj_lock);
> + }
> +
> + return (ret);
> +}
> +
> +
>  static int
>  if_sffpage_check(const caddr_t data)
>  {
> -- 
> 2.27.0
> 

As mpi@ pointed you can sleep witchin ifioctl(). In most cases you lock
`iflist_obj_lock' for read and you didn't catch deadlock. But you also
can sleep witchin if_clone_create() and if_clone_create() while you lock
`iflist_obj_lock' for write.

As I see the races are:

1. We sleep in if_clone_create() and we don't mark allocated `unit' as
busy. So we can create multiple `ifp's with identical names and break
ifunit().

2. We sleep in if_clone_destroy() and we don't mark `ifp' as dying. So
we can grab this `ifp' by concurent thread.

3. We don't protect `ifp' being used in other cases so we can destroy
it.

Diff below fixes this issues. It's not for commit and I _know_ it's very
ugly. I just want to show the direction to fix this issus as I see it
and it's quickest way.

1. if_clone_create(). We reserve allocated `unit' unit so we can't do
multiple insertion of `ifp' with idential names.

2. if_clone_destroy(). We mark `ifp' as dying. Also we release `unit'
after we do `ifc_destroy' We can't destroy `ifp' more than once.

3. ifunit() will not receive `ifp' marked as dying.

4. We bump `ifp' ref counter in other cases to be sure if_detach() will
wait all concurent threads.

5. I disabled if_deactivate(ifp); in if_bridge. if_detach() will do it
so there is no reason to do it twice. I will do special diff for this.

I have 12:40 now. I run command below  since 10:00 and my system is
still stable.

comments?

 cut begin 
for i in 1 2 3; do while true; do ifconfig bridge0 create& \
ifconfig bridge0 destroy& done& done
 cut end 


Index: sys/net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.610
diff -u -p -r1.610 if.c
--- sys/net/if.c22 Jun 2020 09:45:13 -  1.610
+++ sys/net/if.c25 Jun 2020 09:30:46 -
@@ -1236,6 +1236,18 @@ if_isconnected(const struct ifnet *ifp0,
return (connected);
 }
 
+/* XXX: reserve unit */
+struct if_clone_unit {
+   LIST_ENTRY(if_clone_unit) ifcu_list;
+   struct if_clone *ifcu_ifc;
+   int ifcu_unit;
+};
+
+LIST_HEAD(, if_clone_unit) if_clone_units =
+   

Re: pipex(4): use reference counters for `ifnet'

2020-06-25 Thread Christiano F. Haesbaert
On Thu, 25 Jun 2020 at 14:06, Vitaliy Makkoveev
 wrote:
>
>
>
> > On 25 Jun 2020, at 11:55, Martin Pieuchot  wrote:
> >
> > On 24/06/20(Wed) 17:10, Vitaliy Makkoveev wrote:
> >> While `mbuf' enqueued to `pipexinq' or `pipexoutq' it has the reference
> >> to corresponding pipex(4) session as `ph_cookie'. `ph_cookie' is
> >> accessed by pipexintr() and it's always defferent context from context
> >> where we destroy session. `ph_cookie' is protected only while we destroy
> >> session by pipex_timer() but this protection is not effective. While we
> >> destroy session related to pppx(4) `ph_cookie' is not potected. While we
> >> destroy session related to pppac(4) by `PIPEXSMODE' ioctl() or by
> >> closing pppac(4) device node `ph_cookie' is also not protected.
> >>
> >> I'am going to use reference counters to protect `ph_cookie' but some
> >> additional steps required to be done.
> >
> > Please no.  Store an ifidx in session instead of a pointer.  Such
> > index are guaranteed to be unique and can be used with if_get(9).
>
> This means I should do if_get(9) before each `ifnet’ usage. Also I
> should do checks and introduce error path. It's ugly.
>

My 2 cents,

Not really, this is actually easier to reason with, it produces less
dependencies between data structures and the protocol is kinda clear,
you have to account for the loss of atomicity.
Reference counting is pretty much the worst thing you can do in MP development.
Very often it is actually desirable to pay for the cost of a lookup
(which is not the case here, since it's a silly O(1)) than to pay the
human/debug cost of interlinking data structures.
Also, in an ideal world if_get() would be RCUed (or SMRed) and would
not require bumping a reference count which implies an atomic
operation, it would also remove the need for if_put() since the epoch
is "until we context switch", as the kernel is not preemptive this is
easy to do without introducing new bugs.


> Alternatively, I can pass `ifnet->if_index’ instead of `ifnet’ to
> sessinon being linked and store obtained reference up to session
> destruction. I should add one error path to pipex_link_session().
> It’s ugly but a little bit.
>
> >
> > We deliberately kept if_ref() private to keep the code base coherent.
>
> Could you explain please why if_ref() and if_get() are incoherent?



top: Remove unused handle member

2020-06-25 Thread Klemens Nanni
The number of "remaining" processes in the handle struct is not used at
all, it is is only ever set or decremented.

As far as I can tell from CVS logs, this has been the case since

machine.c revision 1.1
date: 1997/08/14 14:00:22;  author: downsj;  state: Exp;
top 3.4, with a few changes.  Still needs more work.

Removing it also gives room for slightly improving the skip semantics
behind the scroll functionality.

Feedback? OK?


Index: machine.c
===
RCS file: /cvs/src/usr.bin/top/machine.c,v
retrieving revision 1.104
diff -u -p -r1.104 machine.c
--- machine.c   24 Jun 2020 23:56:01 -  1.104
+++ machine.c   25 Jun 2020 11:56:32 -
@@ -64,7 +64,6 @@ static char   **get_proc_args(struct kinfo
 
 struct handle {
struct kinfo_proc **next_proc;  /* points to next valid proc pointer */
-   int remaining;  /* number of pointers remaining */
 };
 
 /* what we consider to be process size: */
@@ -493,7 +492,6 @@ get_process_info(struct system_info *si,
 
/* pass back a handle */
handle.next_proc = pref;
-   handle.remaining = active_procs;
return 
 }
 
@@ -539,11 +537,9 @@ format_comm(struct kinfo_proc *kp)
 }
 
 void
-skip_next_process(struct handle *hndl)
+skip_processes(struct handle *hndl, int n)
 {
-   /* find and remember the next proc structure */
-   hndl->next_proc++;
-   hndl->remaining--;
+   hndl->next_proc += n;
 }
 
 char *
@@ -558,7 +554,6 @@ format_next_process(struct handle *hndl,
 
/* find and remember the next proc structure */
pp = *(hndl->next_proc++);
-   hndl->remaining--;
 
cputime = pp->p_rtime_sec + ((pp->p_rtime_usec + 50) / 100);
 
Index: machine.h
===
RCS file: /cvs/src/usr.bin/top/machine.h,v
retrieving revision 1.28
diff -u -p -r1.28 machine.h
--- machine.h   6 Jan 2020 20:05:10 -   1.28
+++ machine.h   25 Jun 2020 11:56:48 -
@@ -90,7 +90,7 @@ extern void get_system_info(struct s
 extern struct handle
 *get_process_info(struct system_info *, struct process_select *,
 int (*) (const void *, const void *));
-extern void skip_next_process(struct handle *);
+extern void skip_processes(struct handle *, int);
 extern char*format_next_process(struct handle *,
 const char *(*)(uid_t, int), pid_t *);
 extern uid_tproc_owner(pid_t);
Index: top.c
===
RCS file: /cvs/src/usr.bin/top/top.c,v
retrieving revision 1.102
diff -u -p -r1.102 top.c
--- top.c   6 Jan 2020 20:05:10 -   1.102
+++ top.c   25 Jun 2020 11:56:35 -
@@ -569,8 +569,7 @@ restart:
 */
if (skip + active_procs > system_info.p_active)
skip = system_info.p_active - active_procs;
-   for (i = skip; i > 0; i--)
-   skip_next_process(processes);
+   skip_processes(processes, skip);
/* now show the top "n" processes. */
for (i = 0; i < active_procs; i++) {
pid_t pid;



Re: make btrace interval event to reciprocal of ticks

2020-06-25 Thread Lauri Tirkkonen
On Thu, Jun 25 2020 11:36:48 +0200, Martin Pieuchot wrote:
> On 23/06/20(Tue) 12:04, Yuichiro NAITO wrote:
> > Current btrace has `interval:hz:1` probe that makes periodically events.
> > `interval:hz:1` looks to make events once per second (= 1Hz),
> > but current implementation makes once per tick (= 100Hz on amd64).
> > I think the interval should be counted by reciprocal of ticks so that fit 
> > to Hz.
> 
> Indeed the current implementations assumes it's a number of ticks.  How
> does other tracing tool behave?  Is the behavior you suggest similar to
> bpftrace(8) or dtrace(1)?

In DTrace, profile- probes fire at a frequency of  hertz, but
apparently also support suffixes.
http://dtrace.org/guide/chp-profile.html

-- 
Lauri Tirkkonen | lotheac @ IRCnet



Re: [PATCH] net: prevent if_clone_destroy from racing with rest of stack

2020-06-25 Thread Jason A. Donenfeld
On Thu, Jun 25, 2020 at 3:54 AM Vitaliy Makkoveev
 wrote:
> ifp = ifunit(name);
> if (ifp == NULL)
> return (ENXIO);
> +   ifp->if_dying = 1;

Reference counting, plus an explicit tear-down window, and wait
period, like you've proposed sounds like a good idea. You'll probably
want to make if_dying volatile or cast it to that so that the compiler
doesn't reorder it. But I wonder, at this point, why not go full-on
srp/rcu?



Re: [PATCH] net: prevent if_clone_destroy from racing with rest of stack

2020-06-25 Thread Martin Pieuchot
On 23/06/20(Tue) 16:21, Martin Pieuchot wrote:
> On 23/06/20(Tue) 04:53, Jason A. Donenfeld wrote:
> > On 6/23/20, Martin Pieuchot  wrote:
> > > On 23/06/20(Tue) 01:00, Jason A. Donenfeld wrote:
> > >> You can crash a system by running something like:
> > >>
> > >> for i in 1 2 3; do while true; do ifconfig bridge0 create& ifconfig
> > >> bridge0 destroy& done& done

As mentioned already the proposed diff doesn't protect creation of cloning
devices via open(2).  The above test could be replaced by:

for i in 1 2 3; do while true; \
do cat /dev/tun0& ifconfig tun0 destroy& done& done

The same could be applied to switch(4) or by replacing the cat(1) magic
with 'ifconfig bridge0 add vether0.



Re: [PATCH] net: prevent if_clone_destroy from racing with rest of stack

2020-06-25 Thread Jason A. Donenfeld
On Thu, Jun 25, 2020 at 2:49 AM Martin Pieuchot  wrote:
>
> On 24/06/20(Wed) 19:54, Jason A. Donenfeld wrote:
> > On Wed, Jun 24, 2020 at 4:02 AM Martin Pieuchot  wrote:
> > > Yes, that might be a better way.  If I understood your original mail the
> > > issue is related to ifunit(), right?  ifunit() is not used in packet-
> > > processing contexts, that's why we did not protect it by the NET_LOCK().
> > >
> > > I'm not sure if all the ifunit() usages are necessary, many of them are
> > > certainly exposing races with attach/destroy.
> >
> > No, not _just_ ifunit, but ifunit is one of the places where this is
> > hit. But just one.
>
> Which ones then?  I'd be delighted if you could share those facts.

You make it sound as if I'm deliberately concealing it in order to
dangle a tasty orange in front of you, but that's not what I meant.
Just look around in the source code at all of the code that uses an
ifp outside of a reference count or other locking semantics. Grepping
around, for example, I found
ifioctl->ifioctl_get->ifconf->if_list->kaboom.

There's lots of interesting behavior in there, and a pretty good
indication that you really don't want ioctls racing with either clone
or destroy, which is what my patch fixes.



Re: make btrace interval event to reciprocal of ticks

2020-06-25 Thread Martin Pieuchot
On 23/06/20(Tue) 12:04, Yuichiro NAITO wrote:
> Current btrace has `interval:hz:1` probe that makes periodically events.
> `interval:hz:1` looks to make events once per second (= 1Hz),
> but current implementation makes once per tick (= 100Hz on amd64).
> I think the interval should be counted by reciprocal of ticks so that fit to 
> Hz.

Indeed the current implementations assumes it's a number of ticks.  How
does other tracing tool behave?  Is the behavior you suggest similar to
bpftrace(8) or dtrace(1)?

Would it be complicated to add support for other units, like 'ms' or
'sec'?  In that case 'profile:s:10' would mean every 10sec while
'profile:hz:10' would mean every ~6sec, right?

> Following patch changes to calculate 'dp_maxtick' fit to Hz.
> 'dtrq_rate' has number of hz. I think it should be allowed same value of 'hz'
> so that we can use `interval:hz:100` that equals to once per tick on amd64.
> 
> Index: dt_prov_profile.c
> ===
> RCS file: /cvs/src/sys/dev/dt/dt_prov_profile.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 dt_prov_profile.c
> --- dt_prov_profile.c 25 Mar 2020 14:59:23 -  1.2
> +++ dt_prov_profile.c 23 Jun 2020 02:02:27 -
> @@ -75,7 +75,7 @@ dt_prov_profile_alloc(struct dt_probe *d
>   KASSERT(TAILQ_EMPTY(plist));
>   KASSERT(dtp == dtpp_profile || dtp == dtpp_interval);
>  
> - if (dtrq->dtrq_rate <= 0 || dtrq->dtrq_rate >= hz)
> + if (dtrq->dtrq_rate <= 0 || dtrq->dtrq_rate > hz)
>   return EOPNOTSUPP;
>  
>   CPU_INFO_FOREACH(cii, ci) {
> @@ -88,7 +88,7 @@ dt_prov_profile_alloc(struct dt_probe *d
>   return ENOMEM;
>   }
>  
> - dp->dp_maxtick = dtrq->dtrq_rate;
> + dp->dp_maxtick = hz / dtrq->dtrq_rate;
>   dp->dp_cpuid = ci->ci_cpuid;
>  
>   dp->dp_filter = dtrq->dtrq_filter;
> 
> -- 
> Yuichiro NAITO
>   naito.yuich...@gmail.com
> 



Re: pipex(4): use reference counters for `ifnet'

2020-06-25 Thread Martin Pieuchot
On 24/06/20(Wed) 17:10, Vitaliy Makkoveev wrote:
> While `mbuf' enqueued to `pipexinq' or `pipexoutq' it has the reference
> to corresponding pipex(4) session as `ph_cookie'. `ph_cookie' is
> accessed by pipexintr() and it's always defferent context from context
> where we destroy session. `ph_cookie' is protected only while we destroy
> session by pipex_timer() but this protection is not effective. While we
> destroy session related to pppx(4) `ph_cookie' is not potected. While we
> destroy session related to pppac(4) by `PIPEXSMODE' ioctl() or by
> closing pppac(4) device node `ph_cookie' is also not protected.
> 
> I'am going to use reference counters to protect `ph_cookie' but some
> additional steps required to be done.

Please no.  Store an ifidx in session instead of a pointer.  Such
index are guaranteed to be unique and can be used with if_get(9).

We deliberately kept if_ref() private to keep the code base coherent.

> We have `pipex_iface' which holds the reference to external `ifnet'. The
> pipex(4) session also has reference to this `ifnet'. With reference
> counters pipex(4) session can still be in `pipexinq' or `pipexoutq' but
> corresponding `pppx_if' or `pppac_softc' is already destroyed. I want to
> be shure while we do if_detach() no one has reference to this `ifnet'.
> 
> Diff below grabs reference to `ifnet' each time it passed to pipex(4).
> Also while we release this session we release the reference.
> 
> We attach `ifnet' before we have linked sessions and we detach `ifnet'
> after we release all sessions. Now it's safe.
> 
> if_ref() was declared in sys/if.c so I moved if_ref() declaration to
> sys/if.h.
> 
> Index: sys/net/if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.610
> diff -u -p -r1.610 if.c
> --- sys/net/if.c  22 Jun 2020 09:45:13 -  1.610
> +++ sys/net/if.c  24 Jun 2020 13:43:33 -
> @@ -192,7 +192,6 @@ void  if_qstart_compat(struct ifqueue *);
>  
>  void if_ifp_dtor(void *, void *);
>  void if_map_dtor(void *, void *);
> -struct ifnet *if_ref(struct ifnet *);
>  
>  /*
>   * struct if_map
> Index: sys/net/if.h
> ===
> RCS file: /cvs/src/sys/net/if.h,v
> retrieving revision 1.203
> diff -u -p -r1.203 if.h
> --- sys/net/if.h  25 Jul 2019 15:23:39 -  1.203
> +++ sys/net/if.h  24 Jun 2020 13:43:33 -
> @@ -548,6 +548,7 @@ int   if_delgroup(struct ifnet *, const ch
>  void if_group_routechange(struct sockaddr *, struct sockaddr *);
>  struct   ifnet *ifunit(const char *);
>  struct   ifnet *if_get(unsigned int);
> +struct   ifnet *if_ref(struct ifnet *);
>  void if_put(struct ifnet *);
>  void ifnewlladdr(struct ifnet *);
>  void if_congestion(void);
> Index: sys/net/if_pppx.c
> ===
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.90
> diff -u -p -r1.90 if_pppx.c
> --- sys/net/if_pppx.c 24 Jun 2020 08:52:53 -  1.90
> +++ sys/net/if_pppx.c 24 Jun 2020 13:43:33 -
> @@ -714,15 +714,15 @@ pppx_add_session(struct pppx_dev *pxd, s
>   ifp->if_softc = pxi;
>   /* ifp->if_rdomain = req->pr_rdomain; */
>  
> - error = pipex_link_session(session, >pxi_ifcontext);
> - if (error)
> - goto remove;
> -
>   /* XXXSMP breaks atomicity */
>   NET_UNLOCK();
>   if_attach(ifp);
>   NET_LOCK();
>  
> + error = pipex_link_session(session, >pxi_ifcontext);
> + if (error)
> + goto detach;
> +
>   if_addgroup(ifp, "pppx");
>   if_alloc_sadl(ifp);
>  
> @@ -771,7 +771,12 @@ pppx_add_session(struct pppx_dev *pxd, s
>  
>   return (error);
>  
> -remove:
> +detach:
> + /* XXXSMP breaks atomicity */
> + NET_UNLOCK();
> + if_detach(ifp);
> + NET_LOCK();
> +
>   if (RBT_REMOVE(pppx_ifs, _ifs, pxi) == NULL)
>   panic("%s: inconsistent RB tree", __func__);
>   LIST_REMOVE(pxi, pxi_list);
> @@ -860,13 +865,13 @@ pppx_if_destroy(struct pppx_dev *pxd, st
>   CLR(ifp->if_flags, IFF_RUNNING);
>  
>   pipex_unlink_session(session);
> + pipex_rele_session(session);
>  
>   /* XXXSMP breaks atomicity */
>   NET_UNLOCK();
>   if_detach(ifp);
>   NET_LOCK();
>  
> - pipex_rele_session(session);
>   if (RBT_REMOVE(pppx_ifs, _ifs, pxi) == NULL)
>   panic("%s: inconsistent RB tree", __func__);
>   LIST_REMOVE(pxi, pxi_list);
> Index: sys/net/pipex.c
> ===
> RCS file: /cvs/src/sys/net/pipex.c,v
> retrieving revision 1.116
> diff -u -p -r1.116 pipex.c
> --- sys/net/pipex.c   22 Jun 2020 09:38:15 -  1.116
> +++ sys/net/pipex.c   24 Jun 2020 13:43:34 -
> @@ -147,6 +147,7 @@ pipex_iface_init(struct pipex_iface_cont
>  {
>   struct pipex_session *session;
>  
> + if_ref(ifp);
>   

Re: [PATCH] net: prevent if_clone_destroy from racing with rest of stack

2020-06-25 Thread Martin Pieuchot
On 24/06/20(Wed) 19:54, Jason A. Donenfeld wrote:
> On Wed, Jun 24, 2020 at 4:02 AM Martin Pieuchot  wrote:
> > Yes, that might be a better way.  If I understood your original mail the
> > issue is related to ifunit(), right?  ifunit() is not used in packet-
> > processing contexts, that's why we did not protect it by the NET_LOCK().
> >
> > I'm not sure if all the ifunit() usages are necessary, many of them are
> > certainly exposing races with attach/destroy.
> 
> No, not _just_ ifunit, but ifunit is one of the places where this is
> hit. But just one.

Which ones then?  I'd be delighted if you could share those facts.



Re: [PATCH] net: prevent if_clone_destroy from racing with rest of stack

2020-06-25 Thread Vitaliy Makkoveev
> On 23 Jun 2020, at 10:00, Jason A. Donenfeld  wrote:
> 
> You can crash a system by running something like:
> 
>for i in 1 2 3; do while true; do ifconfig bridge0 create& ifconfig 
> bridge0 destroy& done& done
> 
> This works with every type of interface I've tried. It appears that
> if_clone_destroy and if_clone_create race with other ioctls, which
> causes a variety of different UaFs or just general logic errors.
> 
> One common root cause appears to be that most ifioctl functions use
> ifunit() to find an interface by name, which traverses if_list. Writes
> to if_list are protected by a lock, but reads are apparently
> unprotected. There's also the question of the life time of the object
> returned from ifunit(). Most things that access 's if_list are
> done without locking, and even if those accesses were to be locked, the
> lock would be released before the object is no longer used, causing the
> UaF in that case as well.
> 
> This patch fixes the issue by making if_clone_{create,destroy} exclusive
> with all other ifioctls.
> ---
> sys/net/if.c | 36 +++-
> 1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git sys/net/if.c sys/net/if.c
> index fb2f86f4a7c..9eedea83d32 100644
> --- sys/net/if.c
> +++ sys/net/if.c
> @@ -246,6 +246,11 @@ struct task if_input_task_locked = 
> TASK_INITIALIZER(if_netisr, NULL);
>  */
> struct rwlock netlock = RWLOCK_INITIALIZER("netlock");
> 
> +/*
> + * Protect modifications to objects owned by ifnet's if_list
> + */
> +struct rwlock iflist_obj_lock = RWLOCK_INITIALIZER("iflist_obj_lock");
> +
> /*
>  * Network interface utility routines.
>  */
> @@ -1905,7 +1910,7 @@ if_setrdomain(struct ifnet *ifp, int rdomain)
>  * Interface ioctls.
>  */
> int
> -ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
> +ifioctl_unlocked(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
> {
>   struct ifnet *ifp;
>   struct ifreq *ifr = (struct ifreq *)data;
> @@ -2432,6 +2437,35 @@ ifioctl_get(u_long cmd, caddr_t data)
>   return (error);
> }
> 
> +int
> +ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
> +{
> + int ret;
> +
> + switch (cmd) {
> + case SIOCIFCREATE:
> + case SIOCIFDESTROY:
> + rw_enter_write(_obj_lock);
> + break;
> + default:
> + rw_enter_read(_obj_lock);
> + }
> +

You call ifioctl_unlocked() while you holding lock. I guess it’s should be
named ifioctl_locked().

> + ret = ifioctl_unlocked(so, cmd, data, p);
> +
> + switch (cmd) {
> + case SIOCIFCREATE:
> + case SIOCIFDESTROY:
> + rw_exit_write(_obj_lock);
> + break;
> + default:
> + rw_exit_read(_obj_lock);
> + }
> +
> + return (ret);
> +}
> +
> +
> static int
> if_sffpage_check(const caddr_t data)
> {
> -- 
> 2.27.0
>