Re: [patch] S_ISDIR() check in yacc TMPDIR

[Theo de Raadt]

> Sending this to tech list as suggested by mmcc@...
> 
> The yacc(1) manual mentions TMPDIR is an extension.
> The following patch re-factors the checks around TMPDIR.
> This adds an explicit check for TMPDIR-is-a-directory.
> With this patch applied, yacc can more reliably determine when it
> should use _PATH_TMP.

I don't see anything which says it should switch to /tmp if
$TMPDIR isn't a directory.

Right now, it nicely fails when mkstemp is called.  All other
TMPDIR-using programs work that way.

So I don't see the point.

[patch] S_ISDIR() check in yacc TMPDIR

[Michael W. Bombardieri]

Hello,

Sending this to tech list as suggested by mmcc@...

The yacc(1) manual mentions TMPDIR is an extension.
The following patch re-factors the checks around TMPDIR.
This adds an explicit check for TMPDIR-is-a-directory.
With this patch applied, yacc can more reliably determine when it
should use _PATH_TMP.

- Michael


Index: main.c
===
RCS file: /cvs/src/usr.bin/yacc/main.c,v
retrieving revision 1.27
diff -u -p -u -r1.27 main.c
--- main.c  10 Oct 2015 14:23:47 -  1.27
+++ main.c  21 Jun 2016 03:47:30 -
@@ -33,6 +33,7 @@
  * SUCH DAMAGE.
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -225,6 +226,18 @@ allocate(size_t n)
return (v);
 }
 
+char *
+tmp_dir(void)
+{
+   struct stat st;
+   char *tmp;
+
+   tmp = getenv("TMPDIR");
+   if (tmp == NULL || *tmp == '\0' || lstat(tmp, ) == -1 || 
!S_ISDIR(st.st_mode))
+   return (_PATH_TMP);
+   return (tmp);
+}
+
 #define TEMPNAME(s, c, d, l)   \
(asprintf(&(s), "%.*s/yacc.%xXX", (int)(l), (d), (c)))
 
@@ -234,9 +247,7 @@ create_file_names(void)
size_t len;
char *tmpdir;
 
-   if ((tmpdir = getenv("TMPDIR")) == NULL || *tmpdir == '\0')
-   tmpdir = _PATH_TMP;
-
+   tmpdir = tmp_dir();
len = strlen(tmpdir);
if (tmpdir[len - 1] == '/')
len--;

Re: sqlite3 update

[Stuart Henderson]

On 2016/06/08 19:01, James Turner wrote:
> I prefer option 2. Switch to the amalgamation with our changes on top.

I've been looking at this. I don't really like any of the options
but this seems the "least worst" one. I'm not terribly happy about
this, but I don't see what else we can really do at present.

Current version of this diff is a horrendous 480K lines / 17MB
uncompressed, so it's at https://junkpile.org/sqlite-3.11.0.diff.gz
rather than in this mail, and I am just starting a ports build
with it.

It was constructed like this:

- Remove the old source files and tools

- Extract verbatim sqlite 3.11.0 from upstream,
https://www.sqlite.org/2016/sqlite-amalgamation-311.zip

- Add the local arc4random patch

- Adjust Makefile (diff for this is below, and also in the above
gzipped diff).

At least the library size has dropped by a decent amount by doing
this, from 3.7M to 3.0M (amd64).

> > > > With this the main file is unfortunately huge but would be easier
> > > > by far to update later. Note that we only have a small change to
> > > > the actual source code (replacing the RNG code) which is easy enough
> > > > to carry across. Most of our changes are to the build infrastructure.

Turns out I forgot about the pthread stubs, without which there
are build failures in mandoc and various ports things unless we link
them with -lpthread. This broke a few things in my first ports test
build, but thanks to guenther's work it should now be valid to pull
this in via an inter-library dependency rather than having to
sprinkle it over mandoc and lots of the ports tree.

Index: Makefile
===
RCS file: /cvs/src/lib/libsqlite3/Makefile,v
retrieving revision 1.15
diff -u -p -r1.15 Makefile
--- Makefile12 Sep 2015 02:08:34 -  1.15
+++ Makefile21 Jun 2016 22:52:47 -
@@ -2,40 +2,10 @@
 
 .include 
 
-.if defined(NOPIC)
-CPPFLAGS +=-DSQLITE_OMIT_LOAD_EXTENSION=1
-.endif
-
-CPPFLAGS +=-I${.OBJDIR} -I${.CURDIR}/tsrc -I${.CURDIR}/src \
-   -I${.CURDIR}/ext/rtree -I${.CURDIR}/ext/fts3
 LIB = sqlite3
 
-.PATH: ${.CURDIR}/tsrc ${.CURDIR}/src ${.CURDIR}/ext/fts3 ${.CURDIR}/ext/rtree
-
-SRCS = alter.c analyze.c attach.c auth.c \
-   backup.c bitvec.c btmutex.c btree.c build.c \
-   callback.c complete.c ctime.c date.c dbstat.c delete.c \
-   expr.c fault.c fkey.c \
-   fts3.c fts3_aux.c fts3_expr.c fts3_hash.c fts3_icu.c fts3_porter.c \
-   fts3_snippet.c fts3_tokenize_vtab.c fts3_tokenizer.c fts3_tokenizer1.c \
-   fts3_unicode.c fts3_unicode2.c fts3_write.c \
-   func.c global.c hash.c \
-   insert.c journal.c legacy.c loadext.c \
-   main.c malloc.c mem1.c \
-   memjournal.c \
-   mutex.c mutex_noop.c mutex_unix.c \
-   notify.c opcodes.c os.c os_unix.c \
-   pager.c parse.c pcache.c pcache1.c pragma.c prepare.c printf.c \
-   random.c resolve.c rowset.c rtree.c select.c status.c \
-   table.c threads.c tokenize.c treeview.c trigger.c \
-   update.c util.c vacuum.c \
-   vdbe.c vdbeapi.c vdbeaux.c vdbeblob.c vdbemem.c vdbesort.c \
-   vdbetrace.c wal.c walker.c where.c wherecode.c whereexpr.c utf.c vtab.c
-
-# so that it works with NO THREADS
-SRCS +=pthread_stub.c
-
-#  mem3.c mem5.c
+SRCS = sqlite3.c
+LDADD =-lpthread
 
 FEATURE_FLAGS =-DSQLITE_ENABLE_COLUMN_METADATA \
-DSQLITE_ENABLE_RTREE \
@@ -62,55 +32,14 @@ CPPFLAGS += $(FEATURE_FLAGS) -DSQLITE_T
 
 FILES = sqlite3.h sqlite3ext.h
 
-opcodes.c: opcodes.h ${.CURDIR}/mkopcodec.awk
-   sort -n -b -k 3 opcodes.h | awk -f ${.CURDIR}/mkopcodec.awk >opcodes.c
-
-
-opcodes.h: parse.h ${.CURDIR}/src/vdbe.c \
-${.CURDIR}/mkopcodeh.awk
-   cat parse.h ${.CURDIR}/src/vdbe.c | \
-   awk -f ${.CURDIR}/mkopcodeh.awk >$@
-
-beforedepend: opcodes.h keywordhash.h
-
-keywordhash.h: mkkeywordhash
-   ${.OBJDIR}/mkkeywordhash >$@
-
-mkkeywordhash: tool/mkkeywordhash.c
-   ${HOSTCC} ${LDSTATIC} -o $@ $< ${LDADD}
-
-lemon: tool/lemon.c
-   ${HOSTCC} ${LDSTATIC} -o $@ $< ${LDADD}
-
-parse.c: parse.y lemon
-   ln -sf ${.CURDIR}/src/parse.y
-   ln -sf ${.CURDIR}/src/lempar.c # XXX tweaked parser
-   ${.OBJDIR}/lemon ${FEATURE_FLAGS} parse.y
-   mv parse.h parse.h.temp
-   awk -f ${.CURDIR}/addopcodes.awk parse.h.temp >parse.h
-   rm parse.h.temp
-
-parse.h: parse.c
-
-tokenize.o tokenize.po tokenize.so: keywordhash.h
-
-CLEANFILES += mkkeywordhash opcodes.c opcodes.h keywordhash.h \
-   parse.* lemon lempar.c
-   
 beforeinstall:
${INSTALL} ${INSTALL_COPY} -o root -g ${SHAREGRP} \
-m ${SHAREMODE} ${.CURDIR}/sqlite3.pc ${DESTDIR}/usr/lib/pkgconfig/
 
 includes:
@for i in ${FILES}; do \
-   cmp -s ${.CURDIR}/src/$$i ${DESTDIR}/usr/include/$$i || \
-   ${INSTALL} ${INSTALL_COPY} -m 444 ${.CURDIR}/src/$$i 
${DESTDIR}/usr/include/$$i; \
+   cmp -s 

Kernel panic pf.c during halting

[Lampshade]

I don't know if this is enough, but I haven't had access to web
using other device when kernel panicked.

sysctl kern.version  
kern.version=OpenBSD 6.0-beta (GENERIC.MP) #2198: Sun Jun 19 11:58:45 MDT 2016
r...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP

It happened after command:
sudo halt -p

What has been written during panic

panic:kernel diagnostic assertion "(sk->inp == NULL) || (sk->inp->inp_pf_sk == 
NULL)"
failed: file "../../../net/pf.c", line 6870

Stopped at Debugger+0x9: leave
TID PID UID
PRFLAGS 0x100010
PFLAGS 0x80
CPU 0
COMMAND pflogd

Debugger() at Debugger+0x9
panic() at panic+0xfe
__assert() at assert+0x25
pf_state_key_unref() at pf_state_key_unref+0xc6
pf_pkt_unlink_state_key() at pf_pkt_unlink_state_key+0x15
m_free() at m_free+0xa0
sbdrop() at sbdrop+0x80
sbflush() at sbflush+0x1f
sbrelease() at sbrelease+0x11
sorflush() at sorflush+0x17c
sofree() at sofree+0xbc
soclose() at soclose+0x92
soo_close() at soo_close+0x1e (or0x1c)
fdrop() at fdrop+0x2c
end trace frame: 0x800032d01da0, count: 0

Re: new feature in pkg_add(1)

[Patrik Lundin]

On Fri, Jun 17, 2016 at 04:02:36PM +0200, Marc Espie wrote:
> I was waiting for snapshots to come up with the new stuff, but it
> looks like amd64 will be a bit late.  Someone is still hiking in
> the mountains...
> 
> 
> A week ago or so, I committed support for some disambiguating
> filter in pkg_add.
> 
> This means that you can now simply install packages for ports which
> have several versions in the tree without having to know the exact
> version.
> 
> For instance,
> 
> pkg_add python%2.7
> will install a python from the lang/python/2.7 branch.
> 

I am the maintainer of the OpenBSD package handling module in ansible
and have for a long time had a request to support more relaxed package
names. Until now the suggested solution has been to use the pkg_add -z
invocation.

My main problem with using pkg_add -z has been that the package names it
allows can not be used with "pkg_info -e" or "pkg_delete".

I notice that pkg_delete does understand "%" which is very nice:
===
# pkg_delete python%2.7
python-2.7.11p1: ok
[...]
===

However, "pkg_info -e" does not understand it:
===
# pkg_info -e python%2.7
Invalid spec: python%2.7
===

I use pkg_info -e to check if a requested package is installed or
not prior to attempting to install/remove it.

The reason for doing this is that it is much faster than just blindly
trying to install a package, and does not hammer mirrors needlessly.

Are there any plans to teach pkg_info -e about "%"? Is it even possible?

The full ansible upstream discussion can be found here if anyone is
interested:
https://github.com/ansible/ansible-modules-extras/issues/97

Anyway, thanks for working on this, and thanks to landry@ for mentioning
this was in the works :).

-- 
Patrik Lundin

Re: pf divert port reuse

[Alexander Bluhm]

On Tue, Jun 21, 2016 at 05:12:39PM +0200, Mike Belopuhov wrote:
> Right, I've found it, but how can you tell that this is a new
> connection if iss changes a lot and you just test if it's greater
> than?  The actual test should be if it's ouside of the window,
> isn't it?

That is very traditional TCP behavior.  The initial sequence number
for outgoing connections does not change a lot, but only increases.

So if the socket is in TIME_WAIT and a new packets arrives that has
a smaller sequence number than the PCB, it belongs to the old
connection and should be dropped.  If the sequence number is higher,
it must be a new connection and the PCB should be reused.  Before
the initial sequence number counter wraps around, the TIME_WAIT
timeout has removed the PCB.

Look into tcp_set_iss_tsm() where we mix randomness with increasing
steps to make this algorithm work.

> > So my diff fixes it because now pf state key attach and tcp reuse
> > are confused in the same way.
> 
> Rather a bit sloppy, but yeah.

Although the TCP reuse algorithm is not responsible for my NAT
problem, I still think just reusing state keys like pf does is
wrong.  It also has to consider the sequence number to drop old SYN
packets that got delayed by the network.

But I will start fixing pf_get_sport() first.

bluhm

Re: pf divert port reuse

[Mike Belopuhov]

On Tue, Jun 21, 2016 at 16:08 +0200, Alexander Bluhm wrote:
> On Tue, Jun 21, 2016 at 02:45:42PM +0200, Mike Belopuhov wrote:
> > You're testing the sequence number
> > of the new state with an existing one which has seen some
> > traffic.. Are you sure this is correct?
> 
> This is exactly what the stack does to distinguish between packets
> that belong to an old connection and between a new one.
>

Right, I've found it, but how can you tell that this is a new
connection if iss changes a lot and you just test if it's greater
than?  The actual test should be if it's ouside of the window,
isn't it?

> > But
> > since they don't belong to the same connection, this is
> > inherently incorrect.
> 
> Here is my logical error.  The receiving TCP stack only thinks it
> is a reuse of an existing connection.  In this case, the sender is
> reponsible to increase the initial sequence number.  But the sender
> has a different port or even a different IP.  It is only NAT reusing
> the port and address which confuses the stack.
>

Yep.

> So my diff fixes it because now pf state key attach and tcp reuse
> are confused in the same way.
>

Rather a bit sloppy, but yeah.

> > Unless I'm wrong, I have to retract my OK and ask you to fix
> > the sport bit instead.
> 
> Yes, fixing it in pf_get_sport() is more correct.  I will try
> to make a diff.
>

Cool.

> It would help a lot debugging such problems, if we would have more
> log messages.  So extend the existing log to the reuse case in
> pf_state_key_attach() ?
> 
> ok?
> 

Sure.

> bluhm
> 
> Index: net/pf.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
> retrieving revision 1.977
> diff -u -p -r1.977 pf.c
> --- net/pf.c  15 Jun 2016 11:49:34 -  1.977
> +++ net/pf.c  21 Jun 2016 13:50:01 -
> @@ -669,34 +669,34 @@ pf_state_key_attach(struct pf_state_key 
>si->s->key[PF_SK_STACK]->af &&
>sk->af == si->s->key[PF_SK_STACK]->af &&
>si->s->direction != s->direction))) {
> + int reuse = 0;
> +
>   if (sk->proto == IPPROTO_TCP &&
>   si->s->src.state >= TCPS_FIN_WAIT_2 &&
> - si->s->dst.state >= TCPS_FIN_WAIT_2) {
> + si->s->dst.state >= TCPS_FIN_WAIT_2)
> + reuse = 1;
> + if (pf_status.debug >= LOG_NOTICE) {
> + log(LOG_NOTICE,
> + "pf: %s key attach %s on %s: ",
> + (idx == PF_SK_WIRE) ?
> + "wire" : "stack",
> + reuse ? "reuse" : "failed",
> + s->kif->pfik_name);
> + pf_print_state_parts(s,
> + (idx == PF_SK_WIRE) ?  sk : NULL,
> + (idx == PF_SK_STACK) ?  sk : NULL);
> + addlog(", existing: ");
> + pf_print_state_parts(si->s,
> + (idx == PF_SK_WIRE) ?  sk : NULL,
> + (idx == PF_SK_STACK) ?  sk : NULL);
> + addlog("\n");
> + }
> + if (reuse) {
>   si->s->src.state = si->s->dst.state =
>   TCPS_CLOSED;
>   /* remove late or sks can go away */
>   olds = si->s;
>   } else {
> - if (pf_status.debug >= LOG_NOTICE) {
> - log(LOG_NOTICE,
> - "pf: %s key attach "
> - "failed on %s: ",
> - (idx == PF_SK_WIRE) ?
> - "wire" : "stack",
> - s->kif->pfik_name);
> - pf_print_state_parts(s,
> - (idx == PF_SK_WIRE) ?
> - sk : NULL,
> - (idx == PF_SK_STACK) ?
> - sk : NULL);
> - addlog(", existing: ");
> - pf_print_state_parts(si->s,
> - (idx == PF_SK_WIRE) ?
> -   

Re: Updated man page for release.8

[Bryan Everly]

Attached is my proposed diff for /usr/src/share/man/man8/release.8 
that adds instructions on how to build the install${VERSION}.{fs,iso} 
images.  I have incorporated off-list feedback from Theo Buehler in 
this diff.


Any feedback would be welcome.

Please find attached an updated diff with some additional off-list 
feedback from a second Theo (de Raadt this time).  He asked me to clean 
up the use of the second person language in this man page as well.


Any other feedback?
Index: release.8
===
RCS file: /home/cvs/src/share/man/man8/release.8,v
retrieving revision 1.72
diff -u -p -r1.72 release.8
--- release.8	14 Sep 2015 20:06:59 -	1.72
+++ release.8	21 Jun 2016 14:22:52 -
@@ -34,6 +34,8 @@ Build and install xenocara.
 Make and validate the xenocara release.
 .It
 Make the third party packages.
+.It
+Create boot and installation disk images.
 .El
 .Pp
 The following sections describe each of the required steps in detail.
@@ -89,8 +91,8 @@ This branch
 contains errata, no new features.
 .El
 .Pp
-To update your sources to the versions identified by one of the above
-tags use the commands:
+To update to the versions identified by one of the above tags use 
+the commands:
 .Bd -literal -offset indent
 $ cd /usr/src && cvs up -r TAG -Pd
 $ cd XSRCDIR && cvs up -r TAG -Pd
@@ -99,10 +101,10 @@ $ cd PORTSPATH && cvs up -r TAG -Pd
 .Pp
 Replace
 .Va XSRCDIR
-with the path to your X Window System sources.
+with the path to the desired X Window System sources.
 Replace
 .Va PORTSPATH
-with the path to your ports tree sources, typically
+with the path to the desired ports tree sources, typically
 .Pa /usr/ports .
 The above commands assume an existing source tree.
 .Pp
@@ -118,7 +120,7 @@ See
 .Xr cvs 1
 for more information.
 .Ss 2. Build and install a new kernel
-For safety, you should always build and install a new kernel before
+For safety, always build and install a new kernel before
 building the programs that will use the kernel.
 This ensures that any new system calls, for example, will be present
 when needed.
@@ -126,26 +128,26 @@ To build a kernel the steps are:
 .Pp
 Change the current working directory.
 .Va ${ARCH}
-is the architecture of your machine, e.g.\&
+is the architecture of the machine, e.g.\&
 .Li i386 .
 .Pp
 .Dl $ cd /usr/src/sys/arch/${ARCH}/conf
 .Pp
 Edit the kernel configuration file.
 .Va ${NAME}
-is your kernel configuration file.
-You should
+is the desired kernel configuration file.
+It is
 .Em not
-edit
+recommended to edit
 .Li GENERIC ;
-create your own kernel configuration if you need to make modifications.
-If using
+instaed create a new kernel configuration if modifications are 
+desired.  If using
 .Li GENERIC
-you can skip this step.
-And yes, you may use
+the following step can be skipped.
+And yes,
 .Xr vi 1 ,
 .Xr mg 1 ,
-or any other editor you choose.
+or any other text editor can be used.
 .Pp
 .Dl $ vi ${NAME}
 .Pp
@@ -158,7 +160,7 @@ $ make clean && make
 .Pp
 (In this instance
 .Li "make clean"
-is your friend.)
+is one's friend.)
 .Pp
 Replace the old kernel and reboot.
 The current kernel is copied to
@@ -171,22 +173,22 @@ $ su
 # shutdown -r now
 .Ed
 .Pp
-If the system does not come up you can boot using
+If the system does not come up, it can be booted using
 .Pa /obsd .
 .Ss 3. Build a new system
-Now that you are running your new kernel you can build a new system.
-It's safer (but slower) to remove your object directories and re-create
+Now that the new kernel is running, it is time to build a new system.
+It's safer (but slower) to remove the object directories and re-create
 them before the build.
 The steps are:
 .Pp
-Move all your existing object files out of the way and then remove
+Move all existing object files out of the way and then remove
 them in the background:
 .Bd -literal -offset indent
 $ cd /usr/obj && mkdir -p .old && doas mv * .old && \e
 	doas rm -rf .old &
 .Ed
 .Pp
-Re-build your obj directories:
+Re-build the local obj directories:
 .Pp
 .Dl $ cd /usr/src && make obj
 .Pp
@@ -206,8 +208,8 @@ and
 either by hand or using
 .Xr sysmerge 8 .
 .Pp
-At this point your system is up-to-date and running the code that you
-are going to make into a release.
+At this point the system is up-to-date and running the code that
+will be used to make into a release.
 .Ss 4. Make and validate the system release
 The system release consists of at least one generic kernel,
 some installation media, the release
@@ -253,10 +255,10 @@ exists as an empty directory and
 exists.
 .Va ${RELEASEDIR}
 need not be empty.
-You must be root to create a release:
+It is required to be logged in as root to create a release:
 .Bd -literal -offset indent
 $ su
-# export DESTDIR=your-destdir; export RELEASEDIR=your-releasedir
+# export DESTDIR=the-destdir; export RELEASEDIR=the-releasedir
 # test -d ${DESTDIR} && mv ${DESTDIR} ${DESTDIR}- && \e
 	rm -rf ${DESTDIR}- &
 # mkdir -p ${DESTDIR} 

L2TP: rise download speed with ppp(4) + pipex(4)

[Sergey Ryazanov]

Hello,

I would like to announce that I finally reached 80 mbps of download
via pure L2TP tunnel (without IPsec).

I digged through ppp(4) and pppd(8) code several times and I could not
find any bottlenecks. Looks like we need to change whole design of
ppp(4) to reach any usable download speed.

So I change direction and add pipex(4) support to ppp(4), only in Rx
path for now. And the same machine (Pentium D @3GHz) give me 80 mbps
of download rate instead of 20 mbps when run without pipex.

If somebody wants to reproduce the trick, you need a couple of patches
(one for ppp driver and second for xl2tpd) and utility to configure
pipex session (my version is here
https://github.com/rsa9000/pipexctl). Patches inlined below and
attached to mail due to gmail's rule to mangle whitespaces.

First of all you need to patch and rebuild the kernel. Then you need
to patch xl2tpd package and rebuild it too. Then activate pipex via
sysctl, configure and start xl2tpd as usual. Make sure that packets
are successfully traveling via established link.

Then the most trickly part: you need parse ouput of xl2tpd (to console
or syslog) and fetch Tunnel-Id & Session-Id values of running tunnel.
Then run (consult pipex man page for parameters description):
# pipexctl addsess l2tp 

Command should looks like:
# pipexctl addsess l2tp  AA  BB  A.B.C.D  E.F.G.H 255.255.255.0
I.J.K.L:1701  M.N.O.P:1701  CC  DD

Actually most parameters except your Session-Id could be zero.

My further plans includes following actions:
 - add pipex support to Tx path of ppp(4)
 - backport plugins support from upstream to pppd(8)
 - make a plugin for pppd(8) to facilitate pipex usage (a-la pppol2tp for Linux)
 - add pipex plugin configuration support to xl2tpd (both to our port
and to upstream)

I would like to ask, does someone have any objections against such changes?

I would like to complete this work until next release, but it all
depends on whether I have free time or not.

Kernel patch:
diff -ru sys.orig/net/if_ppp.c sys/net/if_ppp.c
--- sys.orig/net/if_ppp.c 2015-11-20 09:22:09.0 +0300
+++ sys/net/if_ppp.c 2016-06-21 02:41:59.0 +0300
@@ -129,6 +129,10 @@
 #include 
 #include 

+#ifdef PIPEX
+#include 
+#endif
+
 #include "bpfilter.h"

 #ifdef VJC
@@ -199,6 +203,9 @@
 {
  LIST_INIT(_softc_list);
  if_clone_attach(_cloner);
+#ifdef PIPEX
+ pipex_init();
+#endif
 }

 int
@@ -236,6 +243,10 @@
  LIST_INSERT_HEAD(_softc_list, sc, sc_list);
  splx(s);

+#ifdef PIPEX
+ pipex_iface_init(>pipex_iface, >sc_if);
+#endif
+
  return (0);
 }

@@ -248,6 +259,10 @@
  if (sc->sc_devp != NULL)
  return (EBUSY);

+#ifdef PIPEX
+ pipex_iface_fini(>pipex_iface);
+#endif
+
  s = splnet();
  LIST_REMOVE(sc, sc_list);
  splx(s);
@@ -564,6 +579,19 @@
  break;
 #endif

+#ifdef PIPEX
+ case PIPEXSMODE:
+ case PIPEXGMODE:
+ case PIPEXASESSION:
+ case PIPEXDSESSION:
+ case PIPEXCSESSION:
+ case PIPEXGSTAT:
+ s = splnet();
+ error = pipex_ioctl(>pipex_iface, cmd, data);
+ splx(s);
+ return error;
+#endif
+
  default:
  return (-1);
  }
diff -ru sys.orig/net/if_pppvar.h sys/net/if_pppvar.h
--- sys.orig/net/if_pppvar.h 2015-11-06 12:04:36.0 +0300
+++ sys/net/if_pppvar.h 2016-06-21 02:41:59.0 +0300
@@ -140,6 +140,10 @@
  u_char sc_rawin[16]; /* chars as received */
  int sc_rawin_count; /* # in sc_rawin */
  LIST_ENTRY(ppp_softc) sc_list; /* all ppp interfaces */
+
+#ifdef PIPEX
+ struct pipex_iface_context pipex_iface; /* pipex context */
+#endif
 };

 #ifdef _KERNEL
diff -ru sys.orig/net/ppp_tty.c sys/net/ppp_tty.c
--- sys.orig/net/ppp_tty.c 2016-01-25 21:47:00.0 +0300
+++ sys/net/ppp_tty.c 2016-06-21 02:41:59.0 +0300
@@ -122,6 +122,10 @@
 #include 
 #endif

+#ifdef PIPEX
+#include 
+#endif
+
 #include 
 #include 
 #include 

x2tpd patch:
--- network.c.orig
+++ network.c
@@ -25,6 +25,10 @@
 #ifndef LINUX
 # include 
 #endif
+#ifdef OPENBSD
+#include 
+#include 
+#endif
 #include "l2tp.h"
 #include "ipsecmast.h"
 #include "misc.h"/* for IPADDY macro */
@@ -83,6 +87,10 @@

 #endif

+arg=1;
+if (setsockopt(server_socket, IPPROTO_IP, IP_PIPEX, ,
sizeof(arg)) != 0)
+l2tp_log(LOG_CRIT, "setsockopt pipex: %s\n", strerror(errno));
+
 #ifdef USE_KERNEL
 if (gconfig.forceuserspace)
 {

-- 
Sergey
diff -ru sys.orig/net/if_ppp.c sys/net/if_ppp.c
--- sys.orig/net/if_ppp.c	2015-11-20 09:22:09.0 +0300
+++ sys/net/if_ppp.c	2016-06-21 02:41:59.0 +0300
@@ -129,6 +129,10 @@
 #include 
 #include 
 
+#ifdef PIPEX
+#include 
+#endif
+
 #include "bpfilter.h"
 
 #ifdef VJC
@@ -199,6 +203,9 @@
 {
 	LIST_INIT(_softc_list);
 	if_clone_attach(_cloner);
+#ifdef PIPEX
+	pipex_init();
+#endif
 }
 
 int
@@ -236,6 +243,10 @@
 	LIST_INSERT_HEAD(_softc_list, sc, sc_list);
 	splx(s);
 
+#ifdef PIPEX
+	pipex_iface_init(>pipex_iface, >sc_if);
+#endif
+
 	return (0);
 }
 
@@ -248,6 +259,10 @@
 	if (sc->sc_devp != NULL)
 		return (EBUSY);
 
+#ifdef PIPEX
+	pipex_iface_fini(>pipex_iface);
+#endif
+
 	s = splnet();
 	

Re: pf divert port reuse

[Alexander Bluhm]

On Tue, Jun 21, 2016 at 02:45:42PM +0200, Mike Belopuhov wrote:
> You're testing the sequence number
> of the new state with an existing one which has seen some
> traffic.. Are you sure this is correct?

This is exactly what the stack does to distinguish between packets
that belong to an old connection and between a new one.

> But
> since they don't belong to the same connection, this is
> inherently incorrect.

Here is my logical error.  The receiving TCP stack only thinks it
is a reuse of an existing connection.  In this case, the sender is
reponsible to increase the initial sequence number.  But the sender
has a different port or even a different IP.  It is only NAT reusing
the port and address which confuses the stack.

So my diff fixes it because now pf state key attach and tcp reuse
are confused in the same way.

> Unless I'm wrong, I have to retract my OK and ask you to fix
> the sport bit instead.

Yes, fixing it in pf_get_sport() is more correct.  I will try
to make a diff.

It would help a lot debugging such problems, if we would have more
log messages.  So extend the existing log to the reuse case in
pf_state_key_attach() ?

ok?

bluhm

Index: net/pf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.977
diff -u -p -r1.977 pf.c
--- net/pf.c15 Jun 2016 11:49:34 -  1.977
+++ net/pf.c21 Jun 2016 13:50:01 -
@@ -669,34 +669,34 @@ pf_state_key_attach(struct pf_state_key 
 si->s->key[PF_SK_STACK]->af &&
 sk->af == si->s->key[PF_SK_STACK]->af &&
 si->s->direction != s->direction))) {
+   int reuse = 0;
+
if (sk->proto == IPPROTO_TCP &&
si->s->src.state >= TCPS_FIN_WAIT_2 &&
-   si->s->dst.state >= TCPS_FIN_WAIT_2) {
+   si->s->dst.state >= TCPS_FIN_WAIT_2)
+   reuse = 1;
+   if (pf_status.debug >= LOG_NOTICE) {
+   log(LOG_NOTICE,
+   "pf: %s key attach %s on %s: ",
+   (idx == PF_SK_WIRE) ?
+   "wire" : "stack",
+   reuse ? "reuse" : "failed",
+   s->kif->pfik_name);
+   pf_print_state_parts(s,
+   (idx == PF_SK_WIRE) ?  sk : NULL,
+   (idx == PF_SK_STACK) ?  sk : NULL);
+   addlog(", existing: ");
+   pf_print_state_parts(si->s,
+   (idx == PF_SK_WIRE) ?  sk : NULL,
+   (idx == PF_SK_STACK) ?  sk : NULL);
+   addlog("\n");
+   }
+   if (reuse) {
si->s->src.state = si->s->dst.state =
TCPS_CLOSED;
/* remove late or sks can go away */
olds = si->s;
} else {
-   if (pf_status.debug >= LOG_NOTICE) {
-   log(LOG_NOTICE,
-   "pf: %s key attach "
-   "failed on %s: ",
-   (idx == PF_SK_WIRE) ?
-   "wire" : "stack",
-   s->kif->pfik_name);
-   pf_print_state_parts(s,
-   (idx == PF_SK_WIRE) ?
-   sk : NULL,
-   (idx == PF_SK_STACK) ?
-   sk : NULL);
-   addlog(", existing: ");
-   pf_print_state_parts(si->s,
-   (idx == PF_SK_WIRE) ?
-   sk : NULL,
-   (idx == PF_SK_STACK) ?
-   sk : NULL);
-   addlog("\n");
-   }
pool_put(_state_key_pl, sk);
  

Updated man page for release.8

[Bryan Everly]

Attached is my proposed diff for /usr/src/share/man/man8/release.8 that 
adds instructions on how to build the install${VERSION}.{fs,iso} 
images.  I have incorporated off-list feedback from Theo Buehler in this 
diff.


Any feedback would be welcome.

Index: release.8
===
RCS file: /home/cvs/src/share/man/man8/release.8,v
retrieving revision 1.72
diff -u -p -r1.72 release.8
--- release.8	14 Sep 2015 20:06:59 -	1.72
+++ release.8	21 Jun 2016 13:22:40 -
@@ -34,6 +34,8 @@ Build and install xenocara.
 Make and validate the xenocara release.
 .It
 Make the third party packages.
+.It
+Create boot and installation disk images.
 .El
 .Pp
 The following sections describe each of the required steps in detail.
@@ -338,6 +340,29 @@ subsystem of contributed applications is
 for installation, either individually or in bulk.
 This is described in
 .Xr ports 7 .
+.Ss 8. Create boot and installation disk images
+.Pp
+At this point,
+.Va RELEASEDIR
+contains the
+.Ox
+.Sq tarballs
+necessary to install the system by hand or upgrade an existing system.
+.Pp
+The following steps will create the boot and installation images
+install${VERSION}.{fs,iso} suitable for installs without network
+connectivity.  These images contain the
+.Sq tarballs
+and ports built in the previous steps.
+.Bd -literal -offset indent
+# export RELDIR=your-releasedir
+# export RELXDIR=your-xenocara-releasedir
+# cd /usr/src/distrib/${ARCH}/iso && make
+# make install
+# unset RELDIR RELXDIR
+.Ed
+.Pp
+The two installer images are now stored in your release directory.
 .Sh SEE ALSO
 .Xr cvs 1 ,
 .Xr doas 1 ,

Re: pf.conf macro with space

[Stuart Henderson]

On 2016/06/21 08:57, sven falempin wrote:
> A parsing tool is not like hacking into an advanced kernel
> feature with unexpected side effect

That is incorrect.

I'm not implying anything about diffs proposed in this thread, but this
parser *is* sensitive, it's more complex than you think, and incorrect
changes made here certainly could have an unexpected side effect:
people getting access to systems they shouldn't.

Re: pf.conf macro with space

[Stefan Sperling]

On Tue, Jun 21, 2016 at 08:57:44AM -0400, sven falempin wrote:
> I have explain the use of spaced macro,
> a config file that is self explanatory.

I suggest you use underscores instead of spaces in macro names.

Re: pf.conf macro with space

[Mike Belopuhov]

On 21 June 2016 at 14:57, Sebastian Benoit  wrote:
> Henning Brauer(hb-openbsdt...@ml.bsws.de) on 2016.06.21 13:11:16 +0200:
>> * Stefan Sperling  [2016-06-21 11:15]:
>> > Generally, I would appreciate more detailed error messages from the pf.conf
>> > parser. I recall several occasions where pfctl threw "syntax error" and 
>> > more
>> > specific error reporting would have saved me some time with finding the
>> > silly mistake I made. And in this case the ruleset loads successfully even
>> > though, while parsing, we already know it's not going to work as 
>> > intended...
>>
>> true, that's shared by all yacc-style parsers, if the grammar doesn't
>> match you just get syntax error without much of a hint what's wrong.
>
> the quality of the error message depends both on the language (some make
> it harder to error out early) and on the parser.
>
> fail early is better.
>
>> however, the ruleset in this case does NOT load.
>>
>>   $ echo '"a macro with spaces"="foo"\npass from $a\ macro\ 
>> with\ spaces"' | pfctl -nvf -
>> a macro with spaces = "foo"
>> stdin:2: macro 'a' not defined
>> stdin:2: syntax error
>>
>
> sure, thats what the op reported
>
>>
>> > Only as long as it doesn't make the parser code overly complex, of course.
>> > But currently the balance is tilted too much towards terse error messages
>> > for my taste. So I liked benno's first diff.
>>
>> it's just a tiny check indeed, which swings the "cost" (not in
>> financial terms) vs benefit pendulum towards the benefit side, yes.
>
> thx.
>
> here is a diff for all daemons.
>
> ok for this?
>
>

Looks good to me. OK mikeb FWIW

Re: pf.conf macro with space

[Otto Moerbeek]

On Tue, Jun 21, 2016 at 01:11:16PM +0200, Henning Brauer wrote:

> * Stefan Sperling  [2016-06-21 11:15]:
> > Generally, I would appreciate more detailed error messages from the pf.conf
> > parser. I recall several occasions where pfctl threw "syntax error" and more
> > specific error reporting would have saved me some time with finding the
> > silly mistake I made. And in this case the ruleset loads successfully even
> > though, while parsing, we already know it's not going to work as intended...
> 
> true, that's shared by all yacc-style parsers, if the grammar doesn't
> match you just get syntax error without much of a hint what's wrong.

The reason pfctl is not giving much details about the error is also
determined by the way it is written.

Take for example bc(1), also using a yacc parser.
$ bc
1=a
bc: stdin:1: syntax error: = unexpected
a = b c
bc: stdin:2: syntax error: c unexpected
a =&9
bc: stdin:3: illegal character: & unexpected

Which is more helpfull than what pfctl produces in similar cases.

-Otto

> 
> however, the ruleset in this case does NOT load.
> 
>   $ echo '"a macro with spaces"="foo"\npass from $a\ macro\ 
> with\ spaces"' | pfctl -nvf - 
> a macro with spaces = "foo"
> stdin:2: macro 'a' not defined
> stdin:2: syntax error
> 
> > Only as long as it doesn't make the parser code overly complex, of course.
> > But currently the balance is tilted too much towards terse error messages
> > for my taste. So I liked benno's first diff.
> 
> it's just a tiny check indeed, which swings the "cost" (not in
> financial terms) vs benefit pendulum towards the benefit side, yes.
> 
> -- 
> Henning Brauer, h...@bsws.de, henn...@openbsd.org
> BS Web Services GmbH, http://bsws.de, Full-Service ISP
> Secure Hosting, Mail and DNS. Virtual & Dedicated Servers, Root to Fully 
> Managed
> Henning Brauer Consulting, http://henningbrauer.com/

Re: pf.conf macro with space

[sven falempin]

On Tue, Jun 21, 2016 at 8:30 AM, Stefan Sperling  wrote:

> On Tue, Jun 21, 2016 at 01:11:16PM +0200, Henning Brauer wrote:
> > however, the ruleset in this case does NOT load.
> >   $ echo '"a macro with spaces"="foo"\npass from $a\
> macro\ with\ spaces"' | pfctl -nvf -
> > a macro with spaces = "foo"
> > stdin:2: macro 'a' not defined
> > stdin:2: syntax error
>
> Ah, good. I tested with just a macro definition, but not use.
>
>
I have explain the use of spaced macro,
a config file that is self explanatory.

I've been around a while, and i saw a lot of effort
making configuration file clean and meaningful.
A recent example is the doas.conf file.

A parsing tool is not like hacking into an advanced kernel
feature with unexpected side effect, so the feature of fully
handling macro is
 * not an hassle
 * a plus, as you can define element in a more clear way
 * not a risk


BTW,
There s better way to change the parse.y to kill the space,
not using STRING.
It s in the samples of the lexer, for c code.

I hope someone will help the case for spaces in macro.

Cheers.

-- 
-
() ascii ribbon campaign - against html e-mail
/\

Re: pf.conf macro with space

[Sebastian Benoit]

Henning Brauer(hb-openbsdt...@ml.bsws.de) on 2016.06.21 13:11:16 +0200:
> * Stefan Sperling  [2016-06-21 11:15]:
> > Generally, I would appreciate more detailed error messages from the pf.conf
> > parser. I recall several occasions where pfctl threw "syntax error" and more
> > specific error reporting would have saved me some time with finding the
> > silly mistake I made. And in this case the ruleset loads successfully even
> > though, while parsing, we already know it's not going to work as intended...
> 
> true, that's shared by all yacc-style parsers, if the grammar doesn't
> match you just get syntax error without much of a hint what's wrong.

the quality of the error message depends both on the language (some make
it harder to error out early) and on the parser.

fail early is better.

> however, the ruleset in this case does NOT load.
> 
>   $ echo '"a macro with spaces"="foo"\npass from $a\ macro\ 
> with\ spaces"' | pfctl -nvf - 
> a macro with spaces = "foo"
> stdin:2: macro 'a' not defined
> stdin:2: syntax error
>

sure, thats what the op reported

>
> > Only as long as it doesn't make the parser code overly complex, of course.
> > But currently the balance is tilted too much towards terse error messages
> > for my taste. So I liked benno's first diff.
> 
> it's just a tiny check indeed, which swings the "cost" (not in
> financial terms) vs benefit pendulum towards the benefit side, yes.

thx.

here is a diff for all daemons.

ok for this?


diff --git sbin/iked/parse.y sbin/iked/parse.y
index 958e51a..6455a00 100644
--- sbin/iked/parse.y
+++ sbin/iked/parse.y
@@ -73,6 +73,7 @@ intlookup(char *);
 int lgetc(int);
 int lungetc(int);
 int findeol(void);
+int has_whitespace(const char *);
 
 TAILQ_HEAD(symhead, sym)symhead = TAILQ_HEAD_INITIALIZER(symhead);
 struct sym {
@@ -1006,6 +1007,10 @@ string   : string STRING
 varset : STRING '=' string
{
log_debug("%s = \"%s\"\n", $1, $3);
+   if (has_whitespace($1)) {
+   yyerror("macro name cannot contain whitespace");
+   YYERROR;
+   }
if (symset($1, $3, 0) == -1)
err(1, "cannot store variable");
free($1);
@@ -1234,6 +1239,16 @@ findeol(void)
 }
 
 int
+has_whitespace(const char *s) {
+   while (*s != '\0') {
+   if (isspace((unsigned char)*s))
+   return 1;
+   s++;
+   }
+   return 0;
+}
+
+int
 yylex(void)
 {
unsigned charbuf[8096];
diff --git sbin/ipsecctl/parse.y sbin/ipsecctl/parse.y
index fe9cee0..67fbd72 100644
--- sbin/ipsecctl/parse.y
+++ sbin/ipsecctl/parse.y
@@ -71,6 +71,7 @@ intlookup(char *);
 int lgetc(int);
 int lungetc(int);
 int findeol(void);
+int has_whitespace(const char *);
 
 TAILQ_HEAD(symhead, sym)symhead = TAILQ_HEAD_INITIALIZER(symhead);
 struct sym {
@@ -883,6 +884,10 @@ varset : STRING '=' string
{
if (ipsec->opts & IPSECCTL_OPT_VERBOSE)
printf("%s = \"%s\"\n", $1, $3);
+   if (has_whitespace($1)) {
+   yyerror("macro name cannot contain whitespace");
+   YYERROR;
+   }
if (symset($1, $3, 0) == -1)
err(1, "cannot store variable");
free($1);
@@ -1092,6 +1097,16 @@ findeol(void)
 }
 
 int
+has_whitespace(const char *s) {
+   while (*s != '\0') {
+   if (isspace((unsigned char)*s))
+   return 1;
+   s++;
+   }
+   return 0;
+}
+
+int
 yylex(void)
 {
u_char   buf[8096];
diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
index 934438c..9846063 100644
--- sbin/pfctl/parse.y
+++ sbin/pfctl/parse.y
@@ -85,6 +85,7 @@ intlookup(char *);
 int lgetc(int);
 int lungetc(int);
 int findeol(void);
+int has_whitespace(const char *);
 
 TAILQ_HEAD(symhead, sym)symhead = TAILQ_HEAD_INITIALIZER(symhead);
 struct sym {
@@ -714,6 +715,10 @@ numberstring   : NUMBER
{
 varset : STRING '=' varstring  {
if (pf->opts & PF_OPT_VERBOSE)
printf("%s = \"%s\"\n", $1, $3);
+   if (has_whitespace($1)) {
+   yyerror("macro name cannot contain whitespace");
+   YYERROR;
+   }
if (symset($1, $3, 0) == -1)
err(1, "cannot store variable %s", $1);

Re: pf divert port reuse

[Mike Belopuhov]

On 21 June 2016 at 14:00, Alexander Bluhm  wrote:
> On Tue, Jun 21, 2016 at 11:24:14AM +0200, Mike Belopuhov wrote:
>> So pf reused the port while some TCP segments were still in flight?
>
> No.  The old state was in FIN_WAIT_2 and the socket in TIME_WAIT.

Ah indeed, but hold on.  You're testing the sequence number
of the new state with an existing one which has seen some
traffic.. Are you sure this is correct?  The initial TCP
sequence number is modulated so it can be larger or smaller
that the one of the existing state.

In this code we're reusing the existing state key instead of
inserting a new one, so since you don't want to reuse it and
instead prefer to fail, you're limiting the decision based on
the comparison of the new and old TCP sequence numbers.  But
since they don't belong to the same connection, this is
inherently incorrect.

Unless I'm wrong, I have to retract my OK and ask you to fix
the sport bit instead.

> They were idling for 25 seconds.  Then a new state was created and
> Nat pf_get_sport() did choose the port of the old state.  The
> collision was on the PF_SK_STACK side, but pf_find_state_all() is
> called with PF_IN.  So this is not recognized.
>

If what I've said above is correct, perhaps we need a different
lookup function to take stack side keys into account and avoid
doing the search twice.

>> But this is key_attach stage not port allocation...  isn't that too
>> late? When we fail the state key attachment we drop the connection.
>
> I guess that the SYN-Retry fixes it.  But only one of 1000 connections
> fails so it is hard to debug.  My use case is quite strange, I do
> nat-to and divert-to in the same rule.
>
>> I'm ok to add this safeguard, but can't we apply additional logic
>> into the port allocation code to do a better job?
>
> In my case I ended up with inconsistent state and socket as they
> have different reuse policies.  This might be the case in other
> places too.  Fixes in other places might avoid that situation.  But
> here something goes wrong, so I think here something should be
> fixed.
>
>> Does this port get allocated via pf_get_sport or is it a static port
>> assignment that clashes with the port range NAT uses?
>
> It is not a static port, pf_get_sport() chooses it.
>
> Discussion with markus@ concluded that my diff is wrong for sloppy
> states.  There the sequence number is not updated in the old state.
> So here is a new diff that fixes this.
>
> ok?
>
> bluhm
>

Re: pf.conf macro with space

[Stefan Sperling]

On Tue, Jun 21, 2016 at 01:11:16PM +0200, Henning Brauer wrote:
> however, the ruleset in this case does NOT load.
>   $ echo '"a macro with spaces"="foo"\npass from $a\ macro\ 
> with\ spaces"' | pfctl -nvf - 
> a macro with spaces = "foo"
> stdin:2: macro 'a' not defined
> stdin:2: syntax error

Ah, good. I tested with just a macro definition, but not use.

Re: pf divert port reuse

[Alexander Bluhm]

On Tue, Jun 21, 2016 at 11:24:14AM +0200, Mike Belopuhov wrote:
> So pf reused the port while some TCP segments were still in flight?

No.  The old state was in FIN_WAIT_2 and the socket in TIME_WAIT.
They were idling for 25 seconds.  Then a new state was created and
Nat pf_get_sport() did choose the port of the old state.  The
collision was on the PF_SK_STACK side, but pf_find_state_all() is
called with PF_IN.  So this is not recognized.

> But this is key_attach stage not port allocation...  isn't that too
> late? When we fail the state key attachment we drop the connection.

I guess that the SYN-Retry fixes it.  But only one of 1000 connections
fails so it is hard to debug.  My use case is quite strange, I do
nat-to and divert-to in the same rule.

> I'm ok to add this safeguard, but can't we apply additional logic
> into the port allocation code to do a better job?

In my case I ended up with inconsistent state and socket as they
have different reuse policies.  This might be the case in other
places too.  Fixes in other places might avoid that situation.  But
here something goes wrong, so I think here something should be
fixed.

> Does this port get allocated via pf_get_sport or is it a static port
> assignment that clashes with the port range NAT uses?

It is not a static port, pf_get_sport() chooses it.

Discussion with markus@ concluded that my diff is wrong for sloppy
states.  There the sequence number is not updated in the old state.
So here is a new diff that fixes this.

ok?

bluhm

Index: net/pf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.977
diff -u -p -r1.977 pf.c
--- net/pf.c15 Jun 2016 11:49:34 -  1.977
+++ net/pf.c21 Jun 2016 11:16:26 -
@@ -671,7 +671,9 @@ pf_state_key_attach(struct pf_state_key 
 si->s->direction != s->direction))) {
if (sk->proto == IPPROTO_TCP &&
si->s->src.state >= TCPS_FIN_WAIT_2 &&
-   si->s->dst.state >= TCPS_FIN_WAIT_2) {
+   si->s->dst.state >= TCPS_FIN_WAIT_2 &&
+   ((si->s->state_flags & PFSTATE_SLOPPY) ||
+   SEQ_GT(s->src.seqlo, si->s->src.seqlo))) {
si->s->src.state = si->s->dst.state =
TCPS_CLOSED;
/* remove late or sks can go away */

Re: ppp_compressors

[Mike Belopuhov]

On 21 June 2016 at 13:43, Jeremie Courreges-Anglas  wrote:
>
> We don't support modules, so no need to reserve space for additional PPP
> compression methods.
>
> ok?
>

Sure.

ppp_compressors

[Jeremie Courreges-Anglas]

We don't support modules, so no need to reserve space for additional PPP
compression methods.

ok?

Index: if_ppp.c
===
RCS file: /cvs/src/sys/net/if_ppp.c,v
retrieving revision 1.99
diff -u -p -p -u -r1.99 if_ppp.c
--- if_ppp.c23 May 2016 15:22:44 -  1.99
+++ if_ppp.c21 Jun 2016 11:40:00 -
@@ -169,13 +169,12 @@ struct mbuf   *ppp_pkt_mbuf(struct ppp_pkt
 #ifdef PPP_COMPRESS
 /*
  * List of compressors we know about.
- * We leave some space so maybe we can modload compressors.
  */
 
 extern struct compressor ppp_bsd_compress;
 extern struct compressor ppp_deflate, ppp_deflate_draft;
 
-struct compressor *ppp_compressors[8] = {
+struct compressor *ppp_compressors[] = {
 #if DO_BSD_COMPRESS && defined(PPP_BSDCOMP)
_bsd_compress,
 #endif

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: pf divert port reuse

[Mike Belopuhov]

On 21 June 2016 at 00:14, Alexander Bluhm  wrote:
> Hi,
>
> I have seen a problem with pf divert when the dynamic port in a nat
> rule got reused.  The function pf_state_key_attach() reused the
> state as it was in TCPS_FIN_WAIT_2.  The corresponding socket was
> not reused, as the the TCPS_TIME_WAIT case in tcp_input() has
> additional checks for timestamps and sequence numbers.  When I port
> the condition SEQ_GT(th->th_seq, tp->rcv_nxt) from the stack to pf,
> the socket and state are kept in sync.  Then divert works fine.
>
> ok?
>
> bluhm
>

So pf reused the port while some TCP segments were still in flight?
But this is key_attach stage not port allocation...  isn't that too
late? When we fail the state key attachment we drop the connection.
I'm ok to add this safeguard, but can't we apply additional logic
into the port allocation code to do a better job?

Does this port get allocated via pf_get_sport or is it a static port
assignment that clashes with the port range NAT uses?

Re: pf.conf macro with space

[Stefan Sperling]

On Tue, Jun 21, 2016 at 10:37:45AM +0200, Henning Brauer wrote:
> * Sebastian Benoit  [2016-06-21 10:15]:
> > same thing without a stupid helper function, pointed out by henning.
> 
> I'm actually not quite sure we need or want this. From my PoV, making
> the tools too much of a nanny is against unix spirit. Macros with
> spaces don't actually cause harm, they just don't work. Not too
> unexpected apparently given that, afair at least, nobody spoke up on it
> in more than a decade.
> 
> So, do we really want this extra check? I'm unsure.
> If not, short mention in the manpage or just leave things as they are?

Generally, I would appreciate more detailed error messages from the pf.conf
parser. I recall several occasions where pfctl threw "syntax error" and more
specific error reporting would have saved me some time with finding the
silly mistake I made. And in this case the ruleset loads successfully even
though, while parsing, we already know it's not going to work as intended...

Only as long as it doesn't make the parser code overly complex, of course.
But currently the balance is tilted too much towards terse error messages
for my taste. So I liked benno's first diff.

Re: pf.conf macro with space

[Henning Brauer]

* Sebastian Benoit  [2016-06-21 10:15]:
> same thing without a stupid helper function, pointed out by henning.

I'm actually not quite sure we need or want this. From my PoV, making
the tools too much of a nanny is against unix spirit. Macros with
spaces don't actually cause harm, they just don't work. Not too
unexpected apparently given that, afair at least, nobody spoke up on it
in more than a decade.

So, do we really want this extra check? I'm unsure.
If not, short mention in the manpage or just leave things as they are?

-- 
Henning Brauer, h...@bsws.de, henn...@openbsd.org
BS Web Services GmbH, http://bsws.de, Full-Service ISP
Secure Hosting, Mail and DNS. Virtual & Dedicated Servers, Root to Fully Managed
Henning Brauer Consulting, http://henningbrauer.com/

Re: pf.conf macro with space

[Sebastian Benoit]

Stefan Sperling(s...@stsp.name) on 2016.06.21 10:23:13 +0200:
> On Tue, Jun 21, 2016 at 10:14:52AM +0200, Sebastian Benoit wrote:
> > 
> > same thing without a stupid helper function, pointed out by henning.
> > 
> > diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
> > index 934438c..426cd93 100644
> > --- sbin/pfctl/parse.y
> > +++ sbin/pfctl/parse.y
> > @@ -714,6 +714,10 @@ numberstring   : NUMBER
> > {
> >  varset : STRING '=' varstring  {
> > if (pf->opts & PF_OPT_VERBOSE)
> > printf("%s = \"%s\"\n", $1, $3);
> > +   if (strchr($1, ' ') != NULL) {
> 
> The previous version used isspace(3). Now, what about tabs? Do we not care?

*sigh*

i need coffee. two. big.
 
> > +   yyerror("macro name cannot contain whitespace");
> > +   YYERROR;
> > +   }
> > if (symset($1, $3, 0) == -1)
> > err(1, "cannot store variable %s", $1);
> > free($1);
> 

-- 

Re: pf.conf macro with space

[Stefan Sperling]

On Tue, Jun 21, 2016 at 10:14:52AM +0200, Sebastian Benoit wrote:
> 
> same thing without a stupid helper function, pointed out by henning.
> 
> diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
> index 934438c..426cd93 100644
> --- sbin/pfctl/parse.y
> +++ sbin/pfctl/parse.y
> @@ -714,6 +714,10 @@ numberstring : NUMBER
> {
>  varset   : STRING '=' varstring  {
>   if (pf->opts & PF_OPT_VERBOSE)
>   printf("%s = \"%s\"\n", $1, $3);
> + if (strchr($1, ' ') != NULL) {

The previous version used isspace(3). Now, what about tabs? Do we not care?

> + yyerror("macro name cannot contain whitespace");
> + YYERROR;
> + }
>   if (symset($1, $3, 0) == -1)
>   err(1, "cannot store variable %s", $1);
>   free($1);

Re: pf.conf macro with space

[Florian Obser]

On Tue, Jun 21, 2016 at 10:14:52AM +0200, Sebastian Benoit wrote:
> 
> same thing without a stupid helper function, pointed out by henning.

OK florian@ (for all parse.y instances we have, oh and as usual you
forgot cwm in your list :) )

> 
> diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
> index 934438c..426cd93 100644
> --- sbin/pfctl/parse.y
> +++ sbin/pfctl/parse.y
> @@ -714,6 +714,10 @@ numberstring : NUMBER
> {
>  varset   : STRING '=' varstring  {
>   if (pf->opts & PF_OPT_VERBOSE)
>   printf("%s = \"%s\"\n", $1, $3);
> + if (strchr($1, ' ') != NULL) {
> + yyerror("macro name cannot contain whitespace");
> + YYERROR;
> + }
>   if (symset($1, $3, 0) == -1)
>   err(1, "cannot store variable %s", $1);
>   free($1);
> 
> 
> Sebastian Benoit(be...@openbsd.org) on 2016.06.21 01:18:33 +0200:
> > sven falempin(sven.falem...@gmail.com) on 2016.06.20 17:38:40 -0400:
> > > Dear Tech Readers,
> > > 
> > > in a pf.conf file one can do
> > > "silly things" = egress
> > 
> > Thanks for your diff, but
> > 
> > one, i dont think spaces in macros are useful in pf.conf.
> > 
> > second, we want to keep this consistent across all the parse.y in our code,
> > so we have to think how this affects these(*)
> > 
> > Below is a diff that disallows "silly things".
> > 
> > I thinks it's easier to check that spaces in macros can be done without.
> > 
> 

-- 
I'm not entirely sure you are real.

Re: pf.conf macro with space

[Sebastian Benoit]

same thing without a stupid helper function, pointed out by henning.

diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
index 934438c..426cd93 100644
--- sbin/pfctl/parse.y
+++ sbin/pfctl/parse.y
@@ -714,6 +714,10 @@ numberstring   : NUMBER
{
 varset : STRING '=' varstring  {
if (pf->opts & PF_OPT_VERBOSE)
printf("%s = \"%s\"\n", $1, $3);
+   if (strchr($1, ' ') != NULL) {
+   yyerror("macro name cannot contain whitespace");
+   YYERROR;
+   }
if (symset($1, $3, 0) == -1)
err(1, "cannot store variable %s", $1);
free($1);


Sebastian Benoit(be...@openbsd.org) on 2016.06.21 01:18:33 +0200:
> sven falempin(sven.falem...@gmail.com) on 2016.06.20 17:38:40 -0400:
> > Dear Tech Readers,
> > 
> > in a pf.conf file one can do
> > "silly things" = egress
> 
> Thanks for your diff, but
> 
> one, i dont think spaces in macros are useful in pf.conf.
> 
> second, we want to keep this consistent across all the parse.y in our code,
> so we have to think how this affects these(*)
> 
> Below is a diff that disallows "silly things".
> 
> I thinks it's easier to check that spaces in macros can be done without.
> 

check KTRPOINT() before calling ktrpledge()

[Michal Mazurek]

Don't ktrace pledge if it is not enabled.

Index: sys/kern/kern_pledge.c
===
RCS file: /cvs/src/sys/kern/kern_pledge.c,v
retrieving revision 1.170
diff -u -p -r1.170 kern_pledge.c
--- sys/kern/kern_pledge.c  7 Jun 2016 01:31:54 -   1.170
+++ sys/kern/kern_pledge.c  21 Jun 2016 06:32:41 -
@@ -574,7 +574,8 @@ pledge_fail(struct proc *p, int error, u
printf("%s(%d): syscall %d \"%s\"\n", p->p_comm, p->p_pid,
p->p_pledge_syscall, codes);
 #ifdef KTRACE
-   ktrpledge(p, error, code, p->p_pledge_syscall);
+   if (KTRPOINT(p, KTR_PLEDGE))
+   ktrpledge(p, error, code, p->p_pledge_syscall);
 #endif
/* Send uncatchable SIGABRT for coredump */
memset(, 0, sizeof sa);


-- 
Michal Mazurek