Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Sebastian Benoit]

Reyk Floeter(r...@openbsd.org) on 2015.10.30 19:25:28 +0100:
> On Fri, Oct 30, 2015 at 06:16:53PM +0100, Sebastian Benoit wrote:
> > 
> > i think it should be documented ;)
> > 
> > otherwise ok
> > 
> 
> Ooops, good point, I missed the manpage.
> 
> It looks about right, but maybe it is better to have it less pf-
> specific (also regarding bluhm's update)?
> 
> Otherwise OK

thanks, just saw your mail.


> 
> Reyk
> 
> > Index: mbuf.9
> > ===
> > RCS file: /cvs/src/share/man/man9/mbuf.9,v
> > retrieving revision 1.91
> > diff -u -p -u -r1.91 mbuf.9
> > --- mbuf.9  8 Oct 2015 14:09:34 -   1.91
> > +++ mbuf.9  30 Oct 2015 17:15:40 -
> > @@ -44,6 +44,8 @@
> >  .Fn MGET "struct mbuf *m" "int how" "int type"
> >  .Ft struct mbuf *
> >  .Fn m_getclr "int how" "int type"
> > +.Ft void
> > +.Fn m_resethdr struct mbuf *
> >  .Ft struct mbuf *
> >  .Fn m_gethdr "int how" "int type"
> >  .Fn MGETHDR "struct mbuf *m" "int how" "int type"
> > @@ -445,6 +447,11 @@ See
> >  .Fn m_get
> >  for a description of
> >  .Fa how .
> > +.It Fn m_resethdr "struct mbuf *"
> > +Deletes all
> > +.Xr pf 4
> > +data and all tags attached to packet
> > +.Fa mbuf .
> >  .It Fn m_gethdr "int how" "int type"
> >  Return a pointer to an mbuf of the type specified after initializing
> >  it to contain a packet header.
> > 
> > 
> > Reyk Floeter(r...@openbsd.org) on 2015.10.30 14:04:52 +0100:
> > > On Fri, Oct 30, 2015 at 01:40:19PM +0100, Alexander Bluhm wrote:
> > > > On Fri, Oct 30, 2015 at 12:56:34PM +0100, Reyk Floeter wrote:
> > > > > --- sys/sys/mbuf.h22 Oct 2015 05:26:06 -  1.198
> > > > > +++ sys/sys/mbuf.h30 Oct 2015 11:30:33 -
> > > > > @@ -410,6 +410,7 @@ structmbuf *m_get(int, int);
> > > > >  struct   mbuf *m_getclr(int, int);
> > > > >  struct   mbuf *m_gethdr(int, int);
> > > > >  struct   mbuf *m_inithdr(struct mbuf *);
> > > > > +void m_resethdr(struct mbuf *);
> > > > >  intm_defrag(struct mbuf *, int);
> > > > 
> > > > The m_resethdr should have the same indent as m_defrag.
> > > > 
> > > 
> > > Really?  The indent of m_defrag() is wrong - many spaces and not like
> > > the other int functions further down - but I didn't dare to touch it.
> > > It sticks in the eye, so I'll fix m_defrag indent as well.
> > > 
> > > > > --- sys/net/if_pair.c 25 Oct 2015 12:59:57 -  1.4
> > > > > +++ sys/net/if_pair.c 30 Oct 2015 11:30:33 -
> > > > > @@ -30,6 +30,11 @@
> > > > >  #include 
> > > > >  #include 
> > > > >  
> > > > > +#include "pf.h"
> > > > > +#if NPF > 0
> > > > > +#include 
> > > > > +#endif
> > > > 
> > > > I think you don't need that anymore.
> > > > 
> > > > > @@ -182,9 +187,13 @@ pairstart(struct ifnet *ifp)
> > > > >  #endif /* NBPFILTER > 0 */
> > > > >  
> > > > >   ifp->if_opackets++;
> > > > > - if (pairedifp != NULL)
> > > > > + if (pairedifp != NULL) {
> > > > > +#if NPF > 0
> > > > > + if (m->m_flags & M_PKTHDR)
> > > > > + m_resethdr(m);
> > > > > +#endif
> > > > 
> > > > Calling m_tag_delete_chain() is not pf specific, so I would do it
> > > > without the #if NPF.
> > > > 
> > > > Otherwise OK bluhm@
> > > > 
> > > > Socket splicing somove() does the same thing.  I will change it to
> > > > use m_resethdr() after that got commited.
> > > 
> > > Cool.
> > > 
> > > I'm going to commit the attached diff for now.
> > > 
> > > Reyk
> > > 
> > > Index: sys/kern/uipc_mbuf.c
> > > ===
> > > RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
> > > retrieving revision 1.208
> > > diff -u -p -u -p -r1.208 uipc_mbuf.c
> > > --- sys/kern/uipc_mbuf.c  22 Oct 2015 05:26:06 -  1.208
> > > +++ sys/kern/uipc_mbuf.c  30 Oct 2015 12:45:50 -
> > > @@ -250,6 +250,18 @@ m_inithdr(struct mbuf *m)
> > >   return (m);
> > >  }
> > >  
> > > +void
> > > +m_resethdr(struct mbuf *m)
> > > +{
> > > + /* like the previous, but keep any associated data and mbufs */
> > > + m->m_flags = M_PKTHDR;
> > > + memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> > > + m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
> > > +
> > > + /* also delete all mbuf tags to reset the state */
> > > + m_tag_delete_chain(m);
> > > +}
> > > +
> > >  struct mbuf *
> > >  m_getclr(int nowait, int type)
> > >  {
> > > Index: sys/sys/mbuf.h
> > > ===
> > > RCS file: /cvs/src/sys/sys/mbuf.h,v
> > > retrieving revision 1.198
> > > diff -u -p -u -p -r1.198 mbuf.h
> > > --- sys/sys/mbuf.h22 Oct 2015 05:26:06 -  1.198
> > > +++ sys/sys/mbuf.h30 Oct 2015 12:45:50 -
> > > @@ -410,7 +410,8 @@ structmbuf *m_get(int, int);
> > >  struct   mbuf *m_getclr(int, int);
> > >  struct   mbuf *m_gethdr(int, int);
> > >  struct   mbuf *m_inithdr(struct mbuf *);
> > > -intm_defrag(struct mbuf *, int);
> > > 

By four CRC32

[Karel Gardas]

Hello,

I'm curious what's need to be done in order to have by-four version of
CRC32 enabled by default let's say at least on amd64? Attached patch
is quite aggressive as I put an option into generic GENERIC, but still
I hope it may be usable as a starting point. Performance of CRC32 went
up from 360 MB/s to 970 MB/s on my e3-1220v3 with this compiled in.
Thanks! Karel

diff --git a/sys/conf/GENERIC b/sys/conf/GENERIC
index 4aefba1..7ab41d1 100644
--- a/sys/conf/GENERIC
+++ b/sys/conf/GENERIC
@@ -43,6 +43,7 @@ optionMSDOSFS # MS-DOS file system
 option FIFO# FIFOs; RECOMMENDED
 option TMPFS   # efficient memory file system
 option FUSE# FUSE
+option BYFOUR  # faster CRC32 implementation

 option SOCKET_SPLICE   # Socket Splicing for TCP and UDP
 option TCP_SACK# Selective Acknowledgements for TCP
diff --git a/sys/lib/libz/Makefile b/sys/lib/libz/Makefile
index 93f04fe..81dc850 100644
--- a/sys/lib/libz/Makefile
+++ b/sys/lib/libz/Makefile
@@ -5,8 +5,6 @@ LIB=z
 NOPIC=
 NOPROFILE=

-# Tweak knobs to generate small libz code
-CPPFLAGS+= -DSLOW -DSMALL -DNOBYFOUR -DNO_GZIP -DDYNAMIC_CRC_TABLE
 CPPFLAGS+= -I. ${ZCPPFLAGS}

 # files to be copied down from libz.
diff --git a/sys/lib/libz/crc32.c b/sys/lib/libz/crc32.c
index eac7d74..67f7775 100644
--- a/sys/lib/libz/crc32.c
+++ b/sys/lib/libz/crc32.c
@@ -29,12 +29,15 @@

 #define local static

-#ifndef _KERNEL
 /* Find a four-byte integer type for crc32_little() and crc32_big(). */
 #ifndef NOBYFOUR
 #  ifdef STDC   /* need ANSI C limits.h to determine sizes */
+#ifndef _KERNEL
 #include 
 #define BYFOUR
+#else
+#include 
+#endif
 #if (UINT_MAX == 0xUL)
typedef unsigned int u4;
 #else
@@ -50,10 +53,12 @@
 #endif
 #  endif /* STDC */
 #endif /* !NOBYFOUR */
-#endif

 /* Definitions for doing the crc four data bytes at a time. */
 #ifdef BYFOUR
+#ifdef _KERNEL
+#include 
+#endif
 #  define REV(w) (((w)>>24)+(((w)>>8)&0xff00)+ \
 (((w)&0xff00)<<8)+(((w)&0xff)<<24))
local unsigned long crc32_little OF((unsigned long,

bridge(4): splassert: bstp_notify_rtage: want 7 have 5

[Reyk Floeter]

Hi,

when testing bridge stp, I got the following kernel messages:

splassert: bstp_notify_rtage: want 7 have 5

Want IPL_NET have IPL_SOFTNET.

I can reproduce it by adding/removing stp ports (stp pair0).

As it seems, bstp_notify_rtage() is either indirectly called from the
bridge ioctl() path, which is protected by splnet(), or from the input
path which should be splsoftnet().  So the assert fails when doing the
ioctl because splnet is higher than splsoftnet.

Should we fix or remove the assert?

OK for fix?

Reyk

Index: sys/net/bridgestp.c
===
RCS file: /cvs/src/sys/net/bridgestp.c,v
retrieving revision 1.60
diff -u -p -u -p -r1.60 bridgestp.c
--- sys/net/bridgestp.c 29 Sep 2015 10:11:40 -  1.60
+++ sys/net/bridgestp.c 30 Oct 2015 10:39:34 -
@@ -1589,7 +1589,7 @@ bstp_notify_rtage(struct bstp_port *bp, 
 {
int age = 0;
 
-   splassert(IPL_NET);
+   splassert(IPL_SOFTNET);
 
switch (bp->bp_protover) {
case BSTP_PROTO_STP:

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Reyk Floeter]

On Fri, Oct 30, 2015 at 11:30:56AM +0100, Alexander Bluhm wrote:
> On Fri, Oct 30, 2015 at 10:43:21AM +0100, Reyk Floeter wrote:
> > Question:
> > > How does pair(4) interact with pf? If a packet crosses a pair
> > > does it create a new state or does pf track the original state?
> > > 
> > 
> > Answer:
> > It does create a new state, you can filter between pair(4) without
> > problems and all features including nat work.
> > But it currently does not clear some of the extended packet settings -
> > like flags, tags, qid etc. - so you can filter on the tag, eg. 
> > 
> > # Assuming pair1 is patched to pair0
> > pass out on pair1 tag FOO
> > pass in on pair0 tagged FOO
> > 
> > The attached diff _removes_ that and resets all pf settings, so the pf
> > rules above will not work anymore.  I think it is better to assume
> > crossing barriers and receiving packets with pair(4) works like a
> > "normal" Ethernet packet.  The following will still work:
> > 
> > # Received packets on pair0 don't carry any existing pf states
> > pass out on pair1
> > pass in on pair0
> > 
> > OK?
> 
> The new semantics is better.
> 
> > +void
> > +pf_pkt_reset(struct mbuf *m)
> > +{
> > +   if (m->m_flags & M_PKTHDR)
> > +   m_tag_delete_chain(m);
> > +
> > +   /* resets state key, pcb reference, qid, tag, and all flags */
> > +   memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> > +}
> 
> You need a packet header mbuf to access m->m_pkthdr.  So either
> assume that M_PKTHDR is set and don't check.  Or put both actions
> into the if block.
> 
> As pf_pkt_addr_changed() accesses the m->m_pkthdr without check, I
> would recomend to remove the "if (m->m_flags & M_PKTHDR)" here,
> too.  You may also put an assert into both functions.
> 
> The default for m->m_pkthdr.pf.prio is IFQ_DEFPRIO, not 0.
> Look at m_inithdr().
> 

Thanks.

Adding asserts in in both functions makes sense, but I'll try it
separately - no caller of pf_pkt_addr_changed() checks for the pkthdr
at the moment and I fear potential fallout.  Maybe it would also make
sense to rename both functions to pf_pkthdr_* to make it clear, but
this can also be done separately.

Here is the updated diff, OK?

Reyk

Index: sys/net/if_pair.c
===
RCS file: /cvs/src/sys/net/if_pair.c,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 if_pair.c
--- sys/net/if_pair.c   25 Oct 2015 12:59:57 -  1.4
+++ sys/net/if_pair.c   30 Oct 2015 10:57:10 -
@@ -30,6 +30,11 @@
 #include 
 #include 
 
+#include "pf.h"
+#if NPF > 0
+#include 
+#endif
+
 #include "bpfilter.h"
 #if NBPFILTER > 0
 #include 
@@ -182,9 +187,13 @@ pairstart(struct ifnet *ifp)
 #endif /* NBPFILTER > 0 */
 
ifp->if_opackets++;
-   if (pairedifp != NULL)
+   if (pairedifp != NULL) {
+#if NPF > 0
+   if (m->m_flags & M_PKTHDR)
+   pf_pkt_reset(m);
+#endif
ml_enqueue(, m);
-   else
+   } else
m_freem(m);
}
 
Index: sys/net/pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.948
diff -u -p -u -p -r1.948 pf.c
--- sys/net/pf.c27 Oct 2015 10:52:17 -  1.948
+++ sys/net/pf.c30 Oct 2015 10:57:11 -
@@ -6701,6 +6701,19 @@ pf_pkt_addr_changed(struct mbuf *m)
m->m_pkthdr.pf.inp = NULL;
 }
 
+/*
+ * should be called when reinjecting the packet with a clean state
+ */
+void
+pf_pkt_reset(struct mbuf *m)
+{
+   m_tag_delete_chain(m);
+
+   /* resets pf state key, pcb, qid, tag, and flags like m_inithdr() */
+   memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
+   m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
+}
+
 #if NPFLOG > 0
 void
 pf_log_matches(struct pf_pdesc *pd, struct pf_rule *rm, struct pf_rule *am,
Index: sys/net/pfvar.h
===
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.421
diff -u -p -u -p -r1.421 pfvar.h
--- sys/net/pfvar.h 13 Oct 2015 19:32:32 -  1.421
+++ sys/net/pfvar.h 30 Oct 2015 10:57:11 -
@@ -1756,6 +1756,7 @@ int   pf_rtlabel_match(struct pf_addr *, s
 intpf_socket_lookup(struct pf_pdesc *);
 struct pf_state_key *pf_alloc_state_key(int);
 void   pf_pkt_addr_changed(struct mbuf *);
+void   pf_pkt_reset(struct mbuf *);
 intpf_state_key_attach(struct pf_state_key *, struct pf_state *, int);
 intpf_translate(struct pf_pdesc *, struct pf_addr *, u_int16_t,
struct pf_addr *, u_int16_t, u_int16_t, int);

Re: pair(4) + bridge(4): use stp to prevent bridge loops

[Sebastian Benoit]

i like this.
ok

Reyk Floeter(r...@openbsd.org) on 2015.10.30 11:34:39 +0100:
> Hi,
> 
> as documented below, pairs in bridges can lead to a loop.
> 
> I looked at "fixing" it but came to the conclusion a) there is no
> satisfying way with mbuf flags/tags to prevent the loop, b) it would
> limit the use cases of pair(4) for network testing in many ways, c)
> the bridge loop causes heavy load but does not lock the system (our
> stack is already preventing this), and d) it can be documented -
> so it is a feature, and not a bug ;)
> 
> Thoughts? OK?
> 
> Reyk
> 
> Index: share/man/man4/pair.4
> ===
> RCS file: /cvs/src/share/man/man4/pair.4,v
> retrieving revision 1.3
> diff -u -p -u -p -r1.3 pair.4
> --- share/man/man4/pair.4 24 Oct 2015 15:46:10 -  1.3
> +++ share/man/man4/pair.4 30 Oct 2015 10:09:34 -
> @@ -47,6 +47,25 @@ Set up a pair of interfaces where each o
>  # ifconfig pair1 patch pair2
>  # route -T 1 exec ping 10.1.1.2
>  .Ed
> +.Pp
> +When adding multiple
> +.Nm
> +to multiple
> +.Xr bridge 4
> +interfaces, it is possible to create a loop;
> +the system load will go up while it is busy sending packets from one
> +bridge to another and back.
> +By design, the driver does not prevent such loops by itself, but it is
> +possible to use the Spanning Tree Protocol (STP) to detect and remove
> +loops in the virtual network topology:
> +.Bd -literal -offset indent
> +# ifconfig pair0 up
> +# ifconfig pair1 rdomain 1 patch pair0 up
> +# ifconfig pair2 up
> +# ifconfig pair3 rdomain 1 patch pair2 up
> +# ifconfig bridge0 add pair0 add pair2 stp pair0 stp pair2 up
> +# ifconfig bridge1 add pair1 add pair3 stp pair1 stp pair3 up
> +.Ed
>  .Sh SEE ALSO
>  .Xr bridge 4 ,
>  .Xr inet 4 ,
> 

-- 

Re: [PATCH] rcs: buf_free/rcsnum_free

[Tobias Stoeckmann]

On Fri, Oct 30, 2015 at 08:52:02AM +0800, Michael W. Bombardieri wrote:
> Sorry. Here is new diff. Hopefully I haven't missed anything else.

You missed OpenCVS, which shares the same code base.

But is OpenCVS worth it anymore?
Even a harsher question: Is it time to tedu it?

Re: [PATCH] rcs: buf_free/rcsnum_free

[Nicholas Marriott]

I think it is never going to rise from the dead.

 Original message 
From: Tobias Stoeckmann  
Date:30/10/2015  10:06  (GMT+00:00) 
To: "Michael W. Bombardieri"  
Cc: Nicholas Marriott ,tech@openbsd.org 
Subject: Re: [PATCH] rcs: buf_free/rcsnum_free 

On Fri, Oct 30, 2015 at 08:52:02AM +0800, Michael W. Bombardieri wrote:
> Sorry. Here is new diff. Hopefully I haven't missed anything else.

You missed OpenCVS, which shares the same code base.

But is OpenCVS worth it anymore?
Even a harsher question: Is it time to tedu it?

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Alexander Bluhm]

On Fri, Oct 30, 2015 at 10:43:21AM +0100, Reyk Floeter wrote:
> Question:
> > How does pair(4) interact with pf? If a packet crosses a pair
> > does it create a new state or does pf track the original state?
> > 
> 
> Answer:
> It does create a new state, you can filter between pair(4) without
> problems and all features including nat work.
> But it currently does not clear some of the extended packet settings -
> like flags, tags, qid etc. - so you can filter on the tag, eg. 
> 
> # Assuming pair1 is patched to pair0
> pass out on pair1 tag FOO
> pass in on pair0 tagged FOO
> 
> The attached diff _removes_ that and resets all pf settings, so the pf
> rules above will not work anymore.  I think it is better to assume
> crossing barriers and receiving packets with pair(4) works like a
> "normal" Ethernet packet.  The following will still work:
> 
> # Received packets on pair0 don't carry any existing pf states
> pass out on pair1
> pass in on pair0
> 
> OK?

The new semantics is better.

> +void
> +pf_pkt_reset(struct mbuf *m)
> +{
> + if (m->m_flags & M_PKTHDR)
> + m_tag_delete_chain(m);
> +
> + /* resets state key, pcb reference, qid, tag, and all flags */
> + memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> +}

You need a packet header mbuf to access m->m_pkthdr.  So either
assume that M_PKTHDR is set and don't check.  Or put both actions
into the if block.

As pf_pkt_addr_changed() accesses the m->m_pkthdr without check, I
would recomend to remove the "if (m->m_flags & M_PKTHDR)" here,
too.  You may also put an assert into both functions.

The default for m->m_pkthdr.pf.prio is IFQ_DEFPRIO, not 0.
Look at m_inithdr().

bluhm

Re: pair(4) + bridge(4): use stp to prevent bridge loops

[Reyk Floeter]

On Fri, Oct 30, 2015 at 10:24:15AM +, Stuart Henderson wrote:
> On 2015/10/30 11:34, Reyk Floeter wrote:
> > Hi,
> > 
> > as documented below, pairs in bridges can lead to a loop.
> > 
> > I looked at "fixing" it but came to the conclusion a) there is no
> > satisfying way with mbuf flags/tags to prevent the loop, b) it would
> > limit the use cases of pair(4) for network testing in many ways, c)
> > the bridge loop causes heavy load but does not lock the system (our
> > stack is already preventing this), and d) it can be documented -
> > so it is a feature, and not a bug ;)
> > 
> > Thoughts? OK?
> 
> pair(4) should behave as much like a wire as possible

Indeed,

> so I think this is the correct approach. OK.
> 

Thanks!

> > Reyk
> > 
> > Index: share/man/man4/pair.4
> > ===
> > RCS file: /cvs/src/share/man/man4/pair.4,v
> > retrieving revision 1.3
> > diff -u -p -u -p -r1.3 pair.4
> > --- share/man/man4/pair.4   24 Oct 2015 15:46:10 -  1.3
> > +++ share/man/man4/pair.4   30 Oct 2015 10:09:34 -
> > @@ -47,6 +47,25 @@ Set up a pair of interfaces where each o
> >  # ifconfig pair1 patch pair2
> >  # route -T 1 exec ping 10.1.1.2
> >  .Ed
> > +.Pp
> > +When adding multiple
> > +.Nm
> > +to multiple
> > +.Xr bridge 4
> > +interfaces, it is possible to create a loop;
> > +the system load will go up while it is busy sending packets from one
> > +bridge to another and back.
> > +By design, the driver does not prevent such loops by itself, but it is
> > +possible to use the Spanning Tree Protocol (STP) to detect and remove
> > +loops in the virtual network topology:
> > +.Bd -literal -offset indent
> > +# ifconfig pair0 up
> > +# ifconfig pair1 rdomain 1 patch pair0 up
> > +# ifconfig pair2 up
> > +# ifconfig pair3 rdomain 1 patch pair2 up
> > +# ifconfig bridge0 add pair0 add pair2 stp pair0 stp pair2 up
> > +# ifconfig bridge1 add pair1 add pair3 stp pair1 stp pair3 up
> > +.Ed
> >  .Sh SEE ALSO
> >  .Xr bridge 4 ,
> >  .Xr inet 4 ,
> > 

-- 

pair(4) + pf(4): reset all state on "reinjected" packets

[Reyk Floeter]

Question:
> How does pair(4) interact with pf? If a packet crosses a pair
> does it create a new state or does pf track the original state?
> 

Answer:
It does create a new state, you can filter between pair(4) without
problems and all features including nat work.
But it currently does not clear some of the extended packet settings -
like flags, tags, qid etc. - so you can filter on the tag, eg. 

# Assuming pair1 is patched to pair0
pass out on pair1 tag FOO
pass in on pair0 tagged FOO

The attached diff _removes_ that and resets all pf settings, so the pf
rules above will not work anymore.  I think it is better to assume
crossing barriers and receiving packets with pair(4) works like a
"normal" Ethernet packet.  The following will still work:

# Received packets on pair0 don't carry any existing pf states
pass out on pair1
pass in on pair0

OK?

Reyk

Index: sys/net/if_pair.c
===
RCS file: /cvs/src/sys/net/if_pair.c,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 if_pair.c
--- sys/net/if_pair.c   25 Oct 2015 12:59:57 -  1.4
+++ sys/net/if_pair.c   30 Oct 2015 09:23:44 -
@@ -30,6 +30,11 @@
 #include 
 #include 
 
+#include "pf.h"
+#if NPF > 0
+#include 
+#endif
+
 #include "bpfilter.h"
 #if NBPFILTER > 0
 #include 
@@ -182,9 +187,12 @@ pairstart(struct ifnet *ifp)
 #endif /* NBPFILTER > 0 */
 
ifp->if_opackets++;
-   if (pairedifp != NULL)
+   if (pairedifp != NULL) {
+#if NPF > 0
+   pf_pkt_reset(m);
+#endif
ml_enqueue(, m);
-   else
+   } else
m_freem(m);
}
 
Index: sys/net/pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.948
diff -u -p -u -p -r1.948 pf.c
--- sys/net/pf.c27 Oct 2015 10:52:17 -  1.948
+++ sys/net/pf.c30 Oct 2015 09:23:46 -
@@ -6701,6 +6701,19 @@ pf_pkt_addr_changed(struct mbuf *m)
m->m_pkthdr.pf.inp = NULL;
 }
 
+/*
+ * should be called when reinjecting the packet with a clean state
+ */
+void
+pf_pkt_reset(struct mbuf *m)
+{
+   if (m->m_flags & M_PKTHDR)
+   m_tag_delete_chain(m);
+
+   /* resets state key, pcb reference, qid, tag, and all flags */
+   memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
+}
+
 #if NPFLOG > 0
 void
 pf_log_matches(struct pf_pdesc *pd, struct pf_rule *rm, struct pf_rule *am,
Index: sys/net/pfvar.h
===
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.421
diff -u -p -u -p -r1.421 pfvar.h
--- sys/net/pfvar.h 13 Oct 2015 19:32:32 -  1.421
+++ sys/net/pfvar.h 30 Oct 2015 09:23:46 -
@@ -1756,6 +1756,7 @@ int   pf_rtlabel_match(struct pf_addr *, s
 intpf_socket_lookup(struct pf_pdesc *);
 struct pf_state_key *pf_alloc_state_key(int);
 void   pf_pkt_addr_changed(struct mbuf *);
+void   pf_pkt_reset(struct mbuf *);
 intpf_state_key_attach(struct pf_state_key *, struct pf_state *, int);
 intpf_translate(struct pf_pdesc *, struct pf_addr *, u_int16_t,
struct pf_addr *, u_int16_t, u_int16_t, int);

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Mike Belopuhov]

On Fri, Oct 30, 2015 at 12:56 +0100, Reyk Floeter wrote:
> On Fri, Oct 30, 2015 at 12:29:27PM +0100, Mike Belopuhov wrote:
> > On Fri, Oct 30, 2015 at 12:29 +0100, Mike Belopuhov wrote:
> > > On Fri, Oct 30, 2015 at 12:19 +0100, Reyk Floeter wrote:
> > > > On Fri, Oct 30, 2015 at 11:30:56AM +0100, Alexander Bluhm wrote:
> > > > > On Fri, Oct 30, 2015 at 10:43:21AM +0100, Reyk Floeter wrote:
> > > > > > Question:
> > > > > > > How does pair(4) interact with pf? If a packet crosses a pair
> > > > > > > does it create a new state or does pf track the original state?
> > > > > > > 
> > > > > > 
> > > > > > Answer:
> > > > > > It does create a new state, you can filter between pair(4) without
> > > > > > problems and all features including nat work.
> > > > > > But it currently does not clear some of the extended packet 
> > > > > > settings -
> > > > > > like flags, tags, qid etc. - so you can filter on the tag, eg. 
> > > > > > 
> > > > > > # Assuming pair1 is patched to pair0
> > > > > > pass out on pair1 tag FOO
> > > > > > pass in on pair0 tagged FOO
> > > > > > 
> > > > > > The attached diff _removes_ that and resets all pf settings, so the 
> > > > > > pf
> > > > > > rules above will not work anymore.  I think it is better to assume
> > > > > > crossing barriers and receiving packets with pair(4) works like a
> > > > > > "normal" Ethernet packet.  The following will still work:
> > > > > > 
> > > > > > # Received packets on pair0 don't carry any existing pf states
> > > > > > pass out on pair1
> > > > > > pass in on pair0
> > > > > > 
> > > > > > OK?
> > > > > 
> > > > > The new semantics is better.
> > > > > 
> > > > > > +void
> > > > > > +pf_pkt_reset(struct mbuf *m)
> > > > > > +{
> > > > > > +   if (m->m_flags & M_PKTHDR)
> > > > > > +   m_tag_delete_chain(m);
> > > > > > +
> > > > > > +   /* resets state key, pcb reference, qid, tag, and all flags */
> > > > > > +   memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> > > > > > +}
> > > > > 
> > > > > You need a packet header mbuf to access m->m_pkthdr.  So either
> > > > > assume that M_PKTHDR is set and don't check.  Or put both actions
> > > > > into the if block.
> > > > > 
> > > > > As pf_pkt_addr_changed() accesses the m->m_pkthdr without check, I
> > > > > would recomend to remove the "if (m->m_flags & M_PKTHDR)" here,
> > > > > too.  You may also put an assert into both functions.
> > > > > 
> > > > > The default for m->m_pkthdr.pf.prio is IFQ_DEFPRIO, not 0.
> > > > > Look at m_inithdr().
> > > > > 
> > > > 
> > > > Thanks.
> > > > 
> > > > Adding asserts in in both functions makes sense, but I'll try it
> > > > separately - no caller of pf_pkt_addr_changed() checks for the pkthdr
> > > > at the moment and I fear potential fallout.  Maybe it would also make
> > > > sense to rename both functions to pf_pkthdr_* to make it clear, but
> > > > this can also be done separately.
> > > > 
> > > > Here is the updated diff, OK?
> > > > 
> > > > Reyk
> > > > 
> > > 
> > > As I've told Reyk privately, since this function removes all
> > > mbufs including IPsec ones, etc. it should not be part of pf,
> > > but part of mbuf source code.
> > 
> > mbuf *tags*
> 
> I think pf does handle packet mangling in various ways, which would
> include the intended removal of other tags, but I don't care.
> 
> Here is the updated diff.
> 
> OK?
>

This looks much better.  We could potentially reset some other
fields as well, but this doesn't have to happen right now.

OK mikeb

> Index: sys/kern/uipc_mbuf.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
> retrieving revision 1.208
> diff -u -p -r1.208 uipc_mbuf.c
> --- sys/kern/uipc_mbuf.c  22 Oct 2015 05:26:06 -  1.208
> +++ sys/kern/uipc_mbuf.c  30 Oct 2015 11:30:32 -
> @@ -250,6 +250,18 @@ m_inithdr(struct mbuf *m)
>   return (m);
>  }
>  
> +void
> +m_resethdr(struct mbuf *m)
> +{
> + /* like the previous, but keep any associated data and mbufs */
> + m->m_flags = M_PKTHDR;
> + memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> + m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
> +
> + /* also delete all mbuf tags to reset the state */
> + m_tag_delete_chain(m);
> +}
> +
>  struct mbuf *
>  m_getclr(int nowait, int type)
>  {
> Index: sys/sys/mbuf.h
> ===
> RCS file: /cvs/src/sys/sys/mbuf.h,v
> retrieving revision 1.198
> diff -u -p -r1.198 mbuf.h
> --- sys/sys/mbuf.h22 Oct 2015 05:26:06 -  1.198
> +++ sys/sys/mbuf.h30 Oct 2015 11:30:33 -
> @@ -410,6 +410,7 @@ structmbuf *m_get(int, int);
>  struct   mbuf *m_getclr(int, int);
>  struct   mbuf *m_gethdr(int, int);
>  struct   mbuf *m_inithdr(struct mbuf *);
> +void m_resethdr(struct mbuf *);
>  intm_defrag(struct mbuf *, int);
>  struct   mbuf *m_prepend(struct mbuf *, int, int);
>  struct   mbuf *m_pulldown(struct mbuf *, int, int, int *);
> 

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Reyk Floeter]

On Fri, Oct 30, 2015 at 12:29:27PM +0100, Mike Belopuhov wrote:
> On Fri, Oct 30, 2015 at 12:29 +0100, Mike Belopuhov wrote:
> > On Fri, Oct 30, 2015 at 12:19 +0100, Reyk Floeter wrote:
> > > On Fri, Oct 30, 2015 at 11:30:56AM +0100, Alexander Bluhm wrote:
> > > > On Fri, Oct 30, 2015 at 10:43:21AM +0100, Reyk Floeter wrote:
> > > > > Question:
> > > > > > How does pair(4) interact with pf? If a packet crosses a pair
> > > > > > does it create a new state or does pf track the original state?
> > > > > > 
> > > > > 
> > > > > Answer:
> > > > > It does create a new state, you can filter between pair(4) without
> > > > > problems and all features including nat work.
> > > > > But it currently does not clear some of the extended packet settings -
> > > > > like flags, tags, qid etc. - so you can filter on the tag, eg. 
> > > > > 
> > > > > # Assuming pair1 is patched to pair0
> > > > > pass out on pair1 tag FOO
> > > > > pass in on pair0 tagged FOO
> > > > > 
> > > > > The attached diff _removes_ that and resets all pf settings, so the pf
> > > > > rules above will not work anymore.  I think it is better to assume
> > > > > crossing barriers and receiving packets with pair(4) works like a
> > > > > "normal" Ethernet packet.  The following will still work:
> > > > > 
> > > > > # Received packets on pair0 don't carry any existing pf states
> > > > > pass out on pair1
> > > > > pass in on pair0
> > > > > 
> > > > > OK?
> > > > 
> > > > The new semantics is better.
> > > > 
> > > > > +void
> > > > > +pf_pkt_reset(struct mbuf *m)
> > > > > +{
> > > > > + if (m->m_flags & M_PKTHDR)
> > > > > + m_tag_delete_chain(m);
> > > > > +
> > > > > + /* resets state key, pcb reference, qid, tag, and all flags */
> > > > > + memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> > > > > +}
> > > > 
> > > > You need a packet header mbuf to access m->m_pkthdr.  So either
> > > > assume that M_PKTHDR is set and don't check.  Or put both actions
> > > > into the if block.
> > > > 
> > > > As pf_pkt_addr_changed() accesses the m->m_pkthdr without check, I
> > > > would recomend to remove the "if (m->m_flags & M_PKTHDR)" here,
> > > > too.  You may also put an assert into both functions.
> > > > 
> > > > The default for m->m_pkthdr.pf.prio is IFQ_DEFPRIO, not 0.
> > > > Look at m_inithdr().
> > > > 
> > > 
> > > Thanks.
> > > 
> > > Adding asserts in in both functions makes sense, but I'll try it
> > > separately - no caller of pf_pkt_addr_changed() checks for the pkthdr
> > > at the moment and I fear potential fallout.  Maybe it would also make
> > > sense to rename both functions to pf_pkthdr_* to make it clear, but
> > > this can also be done separately.
> > > 
> > > Here is the updated diff, OK?
> > > 
> > > Reyk
> > > 
> > 
> > As I've told Reyk privately, since this function removes all
> > mbufs including IPsec ones, etc. it should not be part of pf,
> > but part of mbuf source code.
> 
> mbuf *tags*

I think pf does handle packet mangling in various ways, which would
include the intended removal of other tags, but I don't care.

Here is the updated diff.

OK?

Index: sys/kern/uipc_mbuf.c
===
RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
retrieving revision 1.208
diff -u -p -r1.208 uipc_mbuf.c
--- sys/kern/uipc_mbuf.c22 Oct 2015 05:26:06 -  1.208
+++ sys/kern/uipc_mbuf.c30 Oct 2015 11:30:32 -
@@ -250,6 +250,18 @@ m_inithdr(struct mbuf *m)
return (m);
 }
 
+void
+m_resethdr(struct mbuf *m)
+{
+   /* like the previous, but keep any associated data and mbufs */
+   m->m_flags = M_PKTHDR;
+   memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
+   m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
+
+   /* also delete all mbuf tags to reset the state */
+   m_tag_delete_chain(m);
+}
+
 struct mbuf *
 m_getclr(int nowait, int type)
 {
Index: sys/sys/mbuf.h
===
RCS file: /cvs/src/sys/sys/mbuf.h,v
retrieving revision 1.198
diff -u -p -r1.198 mbuf.h
--- sys/sys/mbuf.h  22 Oct 2015 05:26:06 -  1.198
+++ sys/sys/mbuf.h  30 Oct 2015 11:30:33 -
@@ -410,6 +410,7 @@ struct  mbuf *m_get(int, int);
 struct mbuf *m_getclr(int, int);
 struct mbuf *m_gethdr(int, int);
 struct mbuf *m_inithdr(struct mbuf *);
+void   m_resethdr(struct mbuf *);
 int  m_defrag(struct mbuf *, int);
 struct mbuf *m_prepend(struct mbuf *, int, int);
 struct mbuf *m_pulldown(struct mbuf *, int, int, int *);
Index: sys/net/if_pair.c
===
RCS file: /cvs/src/sys/net/if_pair.c,v
retrieving revision 1.4
diff -u -p -r1.4 if_pair.c
--- sys/net/if_pair.c   25 Oct 2015 12:59:57 -  1.4
+++ sys/net/if_pair.c   30 Oct 2015 11:30:33 -
@@ -30,6 +30,11 @@
 #include 
 #include 
 
+#include "pf.h"
+#if NPF > 0
+#include 
+#endif
+
 #include "bpfilter.h"
 #if NBPFILTER > 0
 #include 
@@ -182,9 

Re: toy zones for openbsd - an undergrad operating systems course assignment

[Karel Gardas]

This is nice! Am I right assuming zone exec is a short-cut for not
need to implement Solaris' zlogin functionality? I'm not sure if I'm
as ordinary global zone user on Solaris able to start process in
another zone where I don't have login credentials. So that may be
difference between your zone and Solaris IIRC. Otherwise your
implementation is simple and elegant. Do you plan to continue on this
with another term students?
Nitpick, btw, I think sys_zone.c and sys/zone.h would be better fit as
other files use mostly singular there, just IMHO!
Anyway, thanks a lot for this code.

On Fri, Oct 30, 2015 at 4:17 AM, David Gwynne  wrote:
> ola,
>
> this semester ive had the fun of setting and marking the assignments
> for the operating systems course they teach at the university of
> queensland (where i work).
>
> the second assignment was creating extremely basic process containers
> using solaris zones as a model. the spec is at
> http://www.uq.id.au/dlg/comp3301/assignment2.pdf.
>
> the diff below is my solution to it.
>
> to be clear, i am not asking for oks on this and definitely do not
> want to see it going into the tree in its current state. it does
> very simplistic and very incomplete process isolation, but does not
> isolate any other aspect of the operating system and therefore
> cannot be considered a useful containment infrastructure.
>
> however, i found it interesting to get my head around this aspect
> of the system, and i figured other people (such as this years
> comp3301 students) would be interested too. i also felt sad i couldnt
> find kritaps mult code anywhere, so i wanted this to be backed up
> by everyone for future posterity.
>
> this should also help explain why ive been slacking these last few
> months.
>
> im happy to explain the implementation if people are interested.
>
> dlg
>
> Index: bin/ps/extern.h
> ===
> RCS file: /cvs/src/bin/ps/extern.h,v
> retrieving revision 1.17
> diff -u -p -r1.17 extern.h
> --- bin/ps/extern.h 29 Jun 2015 15:03:33 -  1.17
> +++ bin/ps/extern.h 30 Oct 2015 02:52:16 -
> @@ -61,6 +61,7 @@ void   nlisterr(struct nlist *);
>  voidp_rssize(const struct kinfo_proc *, VARENT *);
>  voidpagein(const struct kinfo_proc *, VARENT *);
>  voidparsefmt(char *);
> +voidzonefmt(void);
>  voidpcpu(const struct kinfo_proc *, VARENT *);
>  voidpmem(const struct kinfo_proc *, VARENT *);
>  voidpri(const struct kinfo_proc *, VARENT *);
> @@ -83,4 +84,5 @@ void   curwd(const struct kinfo_proc *, V
>  voideuname(const struct kinfo_proc *, VARENT *);
>  voidvsize(const struct kinfo_proc *, VARENT *);
>  voidwchan(const struct kinfo_proc *, VARENT *);
> +voidzvar(const struct kinfo_proc *, VARENT *);
>  __END_DECLS
> Index: bin/ps/keyword.c
> ===
> RCS file: /cvs/src/bin/ps/keyword.c,v
> retrieving revision 1.42
> diff -u -p -r1.42 keyword.c
> --- bin/ps/keyword.c16 Jan 2015 06:39:32 -  1.42
> +++ bin/ps/keyword.c30 Oct 2015 02:52:16 -
> @@ -187,6 +187,7 @@ VAR var[] = {
> {"vsz", "VSZ", NULL, 0, vsize, 5},
> {"wchan", "WCHAN", NULL, LJUST, wchan, KI_WMESGLEN - 1},
> {"xstat", "XSTAT", NULL, 0, pvar, 4, 0, POFF(p_xstat), UINT16, "x"},
> +   {"zone", "ZONE", NULL, 0, zvar, 8, 0, POFF(p_zoneid)},
> {""},
>  };
>
> @@ -243,6 +244,20 @@ parsefmt(char *p)
> }
> if (!vhead)
> errx(1, "no valid keywords");
> +}
> +
> +void
> +zonefmt(void)
> +{
> +   struct varent *vent;
> +
> +   vent = malloc(sizeof(*vent));
> +   if (vent == NULL)
> +   err(1, "zone fmt malloc");
> +
> +   vent->var = findvar("zone");
> +   vent->next = vhead;
> +   vhead = vent;
>  }
>
>  static VAR *
> Index: bin/ps/print.c
> ===
> RCS file: /cvs/src/bin/ps/print.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 print.c
> --- bin/ps/print.c  25 Oct 2015 15:26:53 -  1.64
> +++ bin/ps/print.c  30 Oct 2015 02:52:16 -
> @@ -47,6 +47,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>
> @@ -676,6 +677,25 @@ pvar(const struct kinfo_proc *kp, VARENT
> (void)printf("%*s", v->width, "-");
> else
> printval((char *)kp + v->off, v);
> +}
> +
> +intzone_name(zoneid_t, char *, size_t);
> +
> +void
> +zvar(const struct kinfo_proc *kp, VARENT *ve)
> +{
> +   char zonename[MAXZONENAMELEN];
> +   VAR *v = ve->var;
> +   int width;
> +
> +   if (zone_name(kp->p_zoneid, zonename, sizeof(zonename)) == -1)
> +   err(1, "zone_name");
> +
> +   if (strlen(zonename) > v->width) {
> +   width = v->width - 1;
> +   (void)printf("%*.*s*", width, width, zonename);
> +   } else
> + 

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Mike Belopuhov]

On Fri, Oct 30, 2015 at 12:29 +0100, Mike Belopuhov wrote:
> On Fri, Oct 30, 2015 at 12:19 +0100, Reyk Floeter wrote:
> > On Fri, Oct 30, 2015 at 11:30:56AM +0100, Alexander Bluhm wrote:
> > > On Fri, Oct 30, 2015 at 10:43:21AM +0100, Reyk Floeter wrote:
> > > > Question:
> > > > > How does pair(4) interact with pf? If a packet crosses a pair
> > > > > does it create a new state or does pf track the original state?
> > > > > 
> > > > 
> > > > Answer:
> > > > It does create a new state, you can filter between pair(4) without
> > > > problems and all features including nat work.
> > > > But it currently does not clear some of the extended packet settings -
> > > > like flags, tags, qid etc. - so you can filter on the tag, eg. 
> > > > 
> > > > # Assuming pair1 is patched to pair0
> > > > pass out on pair1 tag FOO
> > > > pass in on pair0 tagged FOO
> > > > 
> > > > The attached diff _removes_ that and resets all pf settings, so the pf
> > > > rules above will not work anymore.  I think it is better to assume
> > > > crossing barriers and receiving packets with pair(4) works like a
> > > > "normal" Ethernet packet.  The following will still work:
> > > > 
> > > > # Received packets on pair0 don't carry any existing pf states
> > > > pass out on pair1
> > > > pass in on pair0
> > > > 
> > > > OK?
> > > 
> > > The new semantics is better.
> > > 
> > > > +void
> > > > +pf_pkt_reset(struct mbuf *m)
> > > > +{
> > > > +   if (m->m_flags & M_PKTHDR)
> > > > +   m_tag_delete_chain(m);
> > > > +
> > > > +   /* resets state key, pcb reference, qid, tag, and all flags */
> > > > +   memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> > > > +}
> > > 
> > > You need a packet header mbuf to access m->m_pkthdr.  So either
> > > assume that M_PKTHDR is set and don't check.  Or put both actions
> > > into the if block.
> > > 
> > > As pf_pkt_addr_changed() accesses the m->m_pkthdr without check, I
> > > would recomend to remove the "if (m->m_flags & M_PKTHDR)" here,
> > > too.  You may also put an assert into both functions.
> > > 
> > > The default for m->m_pkthdr.pf.prio is IFQ_DEFPRIO, not 0.
> > > Look at m_inithdr().
> > > 
> > 
> > Thanks.
> > 
> > Adding asserts in in both functions makes sense, but I'll try it
> > separately - no caller of pf_pkt_addr_changed() checks for the pkthdr
> > at the moment and I fear potential fallout.  Maybe it would also make
> > sense to rename both functions to pf_pkthdr_* to make it clear, but
> > this can also be done separately.
> > 
> > Here is the updated diff, OK?
> > 
> > Reyk
> > 
> 
> As I've told Reyk privately, since this function removes all
> mbufs including IPsec ones, etc. it should not be part of pf,
> but part of mbuf source code.

mbuf *tags*

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Mike Belopuhov]

On Fri, Oct 30, 2015 at 12:19 +0100, Reyk Floeter wrote:
> On Fri, Oct 30, 2015 at 11:30:56AM +0100, Alexander Bluhm wrote:
> > On Fri, Oct 30, 2015 at 10:43:21AM +0100, Reyk Floeter wrote:
> > > Question:
> > > > How does pair(4) interact with pf? If a packet crosses a pair
> > > > does it create a new state or does pf track the original state?
> > > > 
> > > 
> > > Answer:
> > > It does create a new state, you can filter between pair(4) without
> > > problems and all features including nat work.
> > > But it currently does not clear some of the extended packet settings -
> > > like flags, tags, qid etc. - so you can filter on the tag, eg. 
> > > 
> > > # Assuming pair1 is patched to pair0
> > > pass out on pair1 tag FOO
> > > pass in on pair0 tagged FOO
> > > 
> > > The attached diff _removes_ that and resets all pf settings, so the pf
> > > rules above will not work anymore.  I think it is better to assume
> > > crossing barriers and receiving packets with pair(4) works like a
> > > "normal" Ethernet packet.  The following will still work:
> > > 
> > > # Received packets on pair0 don't carry any existing pf states
> > > pass out on pair1
> > > pass in on pair0
> > > 
> > > OK?
> > 
> > The new semantics is better.
> > 
> > > +void
> > > +pf_pkt_reset(struct mbuf *m)
> > > +{
> > > + if (m->m_flags & M_PKTHDR)
> > > + m_tag_delete_chain(m);
> > > +
> > > + /* resets state key, pcb reference, qid, tag, and all flags */
> > > + memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> > > +}
> > 
> > You need a packet header mbuf to access m->m_pkthdr.  So either
> > assume that M_PKTHDR is set and don't check.  Or put both actions
> > into the if block.
> > 
> > As pf_pkt_addr_changed() accesses the m->m_pkthdr without check, I
> > would recomend to remove the "if (m->m_flags & M_PKTHDR)" here,
> > too.  You may also put an assert into both functions.
> > 
> > The default for m->m_pkthdr.pf.prio is IFQ_DEFPRIO, not 0.
> > Look at m_inithdr().
> > 
> 
> Thanks.
> 
> Adding asserts in in both functions makes sense, but I'll try it
> separately - no caller of pf_pkt_addr_changed() checks for the pkthdr
> at the moment and I fear potential fallout.  Maybe it would also make
> sense to rename both functions to pf_pkthdr_* to make it clear, but
> this can also be done separately.
> 
> Here is the updated diff, OK?
> 
> Reyk
> 

As I've told Reyk privately, since this function removes all
mbufs including IPsec ones, etc. it should not be part of pf,
but part of mbuf source code.

pair(4) + bridge(4): use stp to prevent bridge loops

[Reyk Floeter]

Hi,

as documented below, pairs in bridges can lead to a loop.

I looked at "fixing" it but came to the conclusion a) there is no
satisfying way with mbuf flags/tags to prevent the loop, b) it would
limit the use cases of pair(4) for network testing in many ways, c)
the bridge loop causes heavy load but does not lock the system (our
stack is already preventing this), and d) it can be documented -
so it is a feature, and not a bug ;)

Thoughts? OK?

Reyk

Index: share/man/man4/pair.4
===
RCS file: /cvs/src/share/man/man4/pair.4,v
retrieving revision 1.3
diff -u -p -u -p -r1.3 pair.4
--- share/man/man4/pair.4   24 Oct 2015 15:46:10 -  1.3
+++ share/man/man4/pair.4   30 Oct 2015 10:09:34 -
@@ -47,6 +47,25 @@ Set up a pair of interfaces where each o
 # ifconfig pair1 patch pair2
 # route -T 1 exec ping 10.1.1.2
 .Ed
+.Pp
+When adding multiple
+.Nm
+to multiple
+.Xr bridge 4
+interfaces, it is possible to create a loop;
+the system load will go up while it is busy sending packets from one
+bridge to another and back.
+By design, the driver does not prevent such loops by itself, but it is
+possible to use the Spanning Tree Protocol (STP) to detect and remove
+loops in the virtual network topology:
+.Bd -literal -offset indent
+# ifconfig pair0 up
+# ifconfig pair1 rdomain 1 patch pair0 up
+# ifconfig pair2 up
+# ifconfig pair3 rdomain 1 patch pair2 up
+# ifconfig bridge0 add pair0 add pair2 stp pair0 stp pair2 up
+# ifconfig bridge1 add pair1 add pair3 stp pair1 stp pair3 up
+.Ed
 .Sh SEE ALSO
 .Xr bridge 4 ,
 .Xr inet 4 ,

Re: pair(4) + bridge(4): use stp to prevent bridge loops

[Reyk Floeter]

FYI,

it is a good way to test bridge(4) and STP.

# ping 10.1.1.2

works fine in the following setup as rstp blocks bridge1/pair1:

bridge0: flags=41
groups: bridge
priority 32768 hellotime 2 fwddelay 15 maxage 20 holdcnt 6 proto rstp
designated: id fe:e1:ba:d0:9d:91 priority 32768
pair0 flags=bb
port 6 ifpriority 128 ifcost 55 forwarding role designated
pair2 flags=bb
port 9 ifpriority 128 ifcost 55 forwarding role designated
bridge1: flags=41
groups: bridge
priority 32768 hellotime 2 fwddelay 15 maxage 20 holdcnt 6 proto rstp
designated: id fe:e1:ba:d0:9d:91 priority 32768
pair1 flags=ab
port 7 ifpriority 128 ifcost 55 discarding role alternate
pair3 flags=ab
port 10 ifpriority 128 ifcost 55 forwarding role root
pair0: flags=8943 mtu 1500
lladdr fe:e1:ba:d0:9d:91
priority: 0
patch: pair1
groups: pair
media: Ethernet autoselect
status: active
inet 10.1.1.1 netmask 0xff00 broadcast 10.1.1.255
pair1: flags=8943 rdomain 1 mtu 
1500
lladdr fe:e1:ba:d1:93:7b
priority: 0
patch: pair0
groups: pair
media: Ethernet autoselect
status: active
inet 10.1.1.2 netmask 0xff00 broadcast 10.1.1.255
pair2: flags=8943 mtu 1500
lladdr fe:e1:ba:d2:93:8a
priority: 0
patch: pair3
groups: pair
media: Ethernet autoselect
status: active
pair3: flags=8943 rdomain 1 mtu 
1500
lladdr fe:e1:ba:d3:f4:76
priority: 0
patch: pair2
groups: pair
media: Ethernet autoselect
status: active


On Fri, Oct 30, 2015 at 11:34:39AM +0100, Reyk Floeter wrote:
> Hi,
> 
> as documented below, pairs in bridges can lead to a loop.
> 
> I looked at "fixing" it but came to the conclusion a) there is no
> satisfying way with mbuf flags/tags to prevent the loop, b) it would
> limit the use cases of pair(4) for network testing in many ways, c)
> the bridge loop causes heavy load but does not lock the system (our
> stack is already preventing this), and d) it can be documented -
> so it is a feature, and not a bug ;)
> 
> Thoughts? OK?
> 
> Reyk
> 
> Index: share/man/man4/pair.4
> ===
> RCS file: /cvs/src/share/man/man4/pair.4,v
> retrieving revision 1.3
> diff -u -p -u -p -r1.3 pair.4
> --- share/man/man4/pair.4 24 Oct 2015 15:46:10 -  1.3
> +++ share/man/man4/pair.4 30 Oct 2015 10:09:34 -
> @@ -47,6 +47,25 @@ Set up a pair of interfaces where each o
>  # ifconfig pair1 patch pair2
>  # route -T 1 exec ping 10.1.1.2
>  .Ed
> +.Pp
> +When adding multiple
> +.Nm
> +to multiple
> +.Xr bridge 4
> +interfaces, it is possible to create a loop;
> +the system load will go up while it is busy sending packets from one
> +bridge to another and back.
> +By design, the driver does not prevent such loops by itself, but it is
> +possible to use the Spanning Tree Protocol (STP) to detect and remove
> +loops in the virtual network topology:
> +.Bd -literal -offset indent
> +# ifconfig pair0 up
> +# ifconfig pair1 rdomain 1 patch pair0 up
> +# ifconfig pair2 up
> +# ifconfig pair3 rdomain 1 patch pair2 up
> +# ifconfig bridge0 add pair0 add pair2 stp pair0 stp pair2 up
> +# ifconfig bridge1 add pair1 add pair3 stp pair1 stp pair3 up
> +.Ed
>  .Sh SEE ALSO
>  .Xr bridge 4 ,
>  .Xr inet 4 ,
> 

-- 

Re: pair(4) + bridge(4): use stp to prevent bridge loops

[Stuart Henderson]

On 2015/10/30 11:34, Reyk Floeter wrote:
> Hi,
> 
> as documented below, pairs in bridges can lead to a loop.
> 
> I looked at "fixing" it but came to the conclusion a) there is no
> satisfying way with mbuf flags/tags to prevent the loop, b) it would
> limit the use cases of pair(4) for network testing in many ways, c)
> the bridge loop causes heavy load but does not lock the system (our
> stack is already preventing this), and d) it can be documented -
> so it is a feature, and not a bug ;)
> 
> Thoughts? OK?

pair(4) should behave as much like a wire as possible, so I think this
is the correct approach. OK.

> Reyk
> 
> Index: share/man/man4/pair.4
> ===
> RCS file: /cvs/src/share/man/man4/pair.4,v
> retrieving revision 1.3
> diff -u -p -u -p -r1.3 pair.4
> --- share/man/man4/pair.4 24 Oct 2015 15:46:10 -  1.3
> +++ share/man/man4/pair.4 30 Oct 2015 10:09:34 -
> @@ -47,6 +47,25 @@ Set up a pair of interfaces where each o
>  # ifconfig pair1 patch pair2
>  # route -T 1 exec ping 10.1.1.2
>  .Ed
> +.Pp
> +When adding multiple
> +.Nm
> +to multiple
> +.Xr bridge 4
> +interfaces, it is possible to create a loop;
> +the system load will go up while it is busy sending packets from one
> +bridge to another and back.
> +By design, the driver does not prevent such loops by itself, but it is
> +possible to use the Spanning Tree Protocol (STP) to detect and remove
> +loops in the virtual network topology:
> +.Bd -literal -offset indent
> +# ifconfig pair0 up
> +# ifconfig pair1 rdomain 1 patch pair0 up
> +# ifconfig pair2 up
> +# ifconfig pair3 rdomain 1 patch pair2 up
> +# ifconfig bridge0 add pair0 add pair2 stp pair0 stp pair2 up
> +# ifconfig bridge1 add pair1 add pair3 stp pair1 stp pair3 up
> +.Ed
>  .Sh SEE ALSO
>  .Xr bridge 4 ,
>  .Xr inet 4 ,
> 

Re: enhanced use-after-free detection for malloc v2

[Daniel Micay]

On 26/10/15 04:19 PM, Daniel Micay wrote:
> This is an improved revision of my earlier patch.
> 
> It now validates the junk data in the delayed_chunks array in an atexit 
> handler
> too, rather than just when allocations are swapped out.
> 
> It will now catch this simple UAF 100% of the time:
> 
> #include 
> #include 
> 
> int main(void) {
>   size_t i;
>   char *p;
>   for (i = 0; i < 32; i++) {
> p = malloc(16);
> if (!p) return 1;
>   }
> 
>   p = malloc(16);
>   if (!p) return 1;
>   free(p);
>   *p = 5;
> 
>   for (i = 0; i < 4; i++) {
> p = malloc(16);
> if (!p) return 1;
> free(p);
>   }
>   return 0;
> }
> 
> In general, it depends on the allocation still being in the delayed chunks
> array when the use-after-free happens. This means a larger delayed chunks
> array would improve the detection rate.

That revision was missing a NULL check as it got lost when refactoring
and the test cases weren't short enough to trigger it. Properly working
implementation:

diff --git a/stdlib/malloc.c b/stdlib/malloc.c
index 424dd77..c408594 100644
--- a/stdlib/malloc.c
+++ b/stdlib/malloc.c
@@ -182,6 +182,7 @@ struct malloc_readonly {
int malloc_freeunmap;   /* mprotect free pages PROT_NONE? */
int malloc_hint;/* call madvice on free pages?  */
int malloc_junk;/* junk fill? */
+   int malloc_validate;/* validate junk */
int malloc_move;/* move allocations to end of page? */
int malloc_realloc; /* always realloc? */
int malloc_xmalloc; /* xmalloc behaviour? */
@@ -218,6 +219,8 @@ static void malloc_exit(void);
 #define CALLER NULL
 #endif

+static void validate_delayed_chunks(void);
+
 /* low bits of r->p determine size: 0 means >= page size and p->size
holding
  *  real size, otherwise r->size is a shift count, or 1 for malloc(0)
  */
@@ -560,6 +563,12 @@ omalloc_init(struct dir_info **dp)
case 'J':
mopts.malloc_junk = 2;
break;
+   case 'v':
+   mopts.malloc_validate = 0;
+   break;
+   case 'V':
+   mopts.malloc_validate = 1;
+   break;
case 'n':
case 'N':
break;
@@ -608,6 +617,9 @@ omalloc_init(struct dir_info **dp)
}
}

+   if (!mopts.malloc_junk)
+   mopts.malloc_validate = 0;
+
 #ifdef MALLOC_STATS
if (mopts.malloc_stats && (atexit(malloc_exit) == -1)) {
static const char q[] = "malloc() warning: atexit(2) failed."
@@ -616,6 +628,12 @@ omalloc_init(struct dir_info **dp)
}
 #endif /* MALLOC_STATS */

+   if (mopts.malloc_validate && (atexit(validate_delayed_chunks) == -1)) {
+   static const char q[] = "malloc() warning: atexit(2) failed."
+   " Will not be able to check for use after free\n";
+   write(STDERR_FILENO, q, sizeof(q) - 1);
+   }
+
while ((mopts.malloc_canary = arc4random()) == 0)
;

@@ -1190,6 +1208,35 @@ malloc(size_t size)
 /*DEF_STRONG(malloc);*/

 static void
+validate_junk(void *p) {
+   struct region_info *r;
+   struct dir_info *pool = getpool();
+   size_t byte, sz;
+   if (p == NULL)
+   return;
+   r = find(pool, p);
+   if (r == NULL)
+   wrterror("bogus pointer in validate_junk", p);
+   REALSIZE(sz, r);
+   for (byte = 0; byte < sz; byte++) {
+   if (((char *)p)[byte] != SOME_FREEJUNK) {
+   wrterror("use after free", p);
+   return;
+   }
+   }
+}
+
+static void
+validate_delayed_chunks(void) {
+   struct dir_info *pool = getpool();
+   int i;
+   if (pool == NULL)
+   return;
+   for (i = 0; i < MALLOC_DELAYED_CHUNK_MASK + 1; i++)
+   validate_junk(pool->delayed_chunks[i]);
+}
+
+static void
 ofree(void *p)
 {
struct dir_info *pool = getpool();
@@ -1253,6 +1300,8 @@ ofree(void *p)
wrterror("double free", p);
return;
}
+   if (mopts.malloc_validate)
+   validate_junk(p);
pool->delayed_chunks[i] = tmp;
}
if (p != NULL) {

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Mike Belopuhov]

On Fri, Oct 30, 2015 at 13:25 +0100, Reyk Floeter wrote:
> On Fri, Oct 30, 2015 at 12:45:31PM +0100, Mike Belopuhov wrote:
> > On Fri, Oct 30, 2015 at 12:56 +0100, Reyk Floeter wrote:
> > > On Fri, Oct 30, 2015 at 12:29:27PM +0100, Mike Belopuhov wrote:
> > > > On Fri, Oct 30, 2015 at 12:29 +0100, Mike Belopuhov wrote:
> > > > > On Fri, Oct 30, 2015 at 12:19 +0100, Reyk Floeter wrote:
> > > > > > On Fri, Oct 30, 2015 at 11:30:56AM +0100, Alexander Bluhm wrote:
> > > > > > > On Fri, Oct 30, 2015 at 10:43:21AM +0100, Reyk Floeter wrote:
> > > > > > > > Question:
> > > > > > > > > How does pair(4) interact with pf? If a packet crosses a pair
> > > > > > > > > does it create a new state or does pf track the original 
> > > > > > > > > state?
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > Answer:
> > > > > > > > It does create a new state, you can filter between pair(4) 
> > > > > > > > without
> > > > > > > > problems and all features including nat work.
> > > > > > > > But it currently does not clear some of the extended packet 
> > > > > > > > settings -
> > > > > > > > like flags, tags, qid etc. - so you can filter on the tag, eg. 
> > > > > > > > 
> > > > > > > > # Assuming pair1 is patched to pair0
> > > > > > > > pass out on pair1 tag FOO
> > > > > > > > pass in on pair0 tagged FOO
> > > > > > > > 
> > > > > > > > The attached diff _removes_ that and resets all pf settings, so 
> > > > > > > > the pf
> > > > > > > > rules above will not work anymore.  I think it is better to 
> > > > > > > > assume
> > > > > > > > crossing barriers and receiving packets with pair(4) works like 
> > > > > > > > a
> > > > > > > > "normal" Ethernet packet.  The following will still work:
> > > > > > > > 
> > > > > > > > # Received packets on pair0 don't carry any existing pf states
> > > > > > > > pass out on pair1
> > > > > > > > pass in on pair0
> > > > > > > > 
> > > > > > > > OK?
> > > > > > > 
> > > > > > > The new semantics is better.
> > > > > > > 
> > > > > > > > +void
> > > > > > > > +pf_pkt_reset(struct mbuf *m)
> > > > > > > > +{
> > > > > > > > +   if (m->m_flags & M_PKTHDR)
> > > > > > > > +   m_tag_delete_chain(m);
> > > > > > > > +
> > > > > > > > +   /* resets state key, pcb reference, qid, tag, and all 
> > > > > > > > flags */
> > > > > > > > +   memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> > > > > > > > +}
> > > > > > > 
> > > > > > > You need a packet header mbuf to access m->m_pkthdr.  So either
> > > > > > > assume that M_PKTHDR is set and don't check.  Or put both actions
> > > > > > > into the if block.
> > > > > > > 
> > > > > > > As pf_pkt_addr_changed() accesses the m->m_pkthdr without check, I
> > > > > > > would recomend to remove the "if (m->m_flags & M_PKTHDR)" here,
> > > > > > > too.  You may also put an assert into both functions.
> > > > > > > 
> > > > > > > The default for m->m_pkthdr.pf.prio is IFQ_DEFPRIO, not 0.
> > > > > > > Look at m_inithdr().
> > > > > > > 
> > > > > > 
> > > > > > Thanks.
> > > > > > 
> > > > > > Adding asserts in in both functions makes sense, but I'll try it
> > > > > > separately - no caller of pf_pkt_addr_changed() checks for the 
> > > > > > pkthdr
> > > > > > at the moment and I fear potential fallout.  Maybe it would also 
> > > > > > make
> > > > > > sense to rename both functions to pf_pkthdr_* to make it clear, but
> > > > > > this can also be done separately.
> > > > > > 
> > > > > > Here is the updated diff, OK?
> > > > > > 
> > > > > > Reyk
> > > > > > 
> > > > > 
> > > > > As I've told Reyk privately, since this function removes all
> > > > > mbufs including IPsec ones, etc. it should not be part of pf,
> > > > > but part of mbuf source code.
> > > > 
> > > > mbuf *tags*
> > > 
> > > I think pf does handle packet mangling in various ways, which would
> > > include the intended removal of other tags, but I don't care.
> > > 
> > > Here is the updated diff.
> > > 
> > > OK?
> > >
> > 
> > This looks much better.  We could potentially reset some other
> > fields as well, but this doesn't have to happen right now.
> > 
> 
> I agree, I left them out for now as they need more thoughts and don't
> do much harm. The candidates are:
> 
>   - ph_cookie (seems to be used by pipex)
>   - flowid(only for a hack in trunk?!)
>   - ether_vtag
> 
> Everything else is either set by m_resethdr or by if_input().
>

Right.

> btw., it looks somewhat ugly that some members in the pkthdr are
> prefixed with ph_ while others are not.
> 
> Reyk
> 

ph_ is a new development.  flowid should have used it.

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Alexander Bluhm]

On Fri, Oct 30, 2015 at 12:56:34PM +0100, Reyk Floeter wrote:
> --- sys/sys/mbuf.h22 Oct 2015 05:26:06 -  1.198
> +++ sys/sys/mbuf.h30 Oct 2015 11:30:33 -
> @@ -410,6 +410,7 @@ structmbuf *m_get(int, int);
>  struct   mbuf *m_getclr(int, int);
>  struct   mbuf *m_gethdr(int, int);
>  struct   mbuf *m_inithdr(struct mbuf *);
> +void m_resethdr(struct mbuf *);
>  intm_defrag(struct mbuf *, int);

The m_resethdr should have the same indent as m_defrag.

> --- sys/net/if_pair.c 25 Oct 2015 12:59:57 -  1.4
> +++ sys/net/if_pair.c 30 Oct 2015 11:30:33 -
> @@ -30,6 +30,11 @@
>  #include 
>  #include 
>  
> +#include "pf.h"
> +#if NPF > 0
> +#include 
> +#endif

I think you don't need that anymore.

> @@ -182,9 +187,13 @@ pairstart(struct ifnet *ifp)
>  #endif /* NBPFILTER > 0 */
>  
>   ifp->if_opackets++;
> - if (pairedifp != NULL)
> + if (pairedifp != NULL) {
> +#if NPF > 0
> + if (m->m_flags & M_PKTHDR)
> + m_resethdr(m);
> +#endif

Calling m_tag_delete_chain() is not pf specific, so I would do it
without the #if NPF.

Otherwise OK bluhm@

Socket splicing somove() does the same thing.  I will change it to
use m_resethdr() after that got commited.

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Reyk Floeter]

On Fri, Oct 30, 2015 at 01:40:19PM +0100, Alexander Bluhm wrote:
> On Fri, Oct 30, 2015 at 12:56:34PM +0100, Reyk Floeter wrote:
> > --- sys/sys/mbuf.h  22 Oct 2015 05:26:06 -  1.198
> > +++ sys/sys/mbuf.h  30 Oct 2015 11:30:33 -
> > @@ -410,6 +410,7 @@ struct  mbuf *m_get(int, int);
> >  struct mbuf *m_getclr(int, int);
> >  struct mbuf *m_gethdr(int, int);
> >  struct mbuf *m_inithdr(struct mbuf *);
> > +void   m_resethdr(struct mbuf *);
> >  int  m_defrag(struct mbuf *, int);
> 
> The m_resethdr should have the same indent as m_defrag.
> 

Really?  The indent of m_defrag() is wrong - many spaces and not like
the other int functions further down - but I didn't dare to touch it.
It sticks in the eye, so I'll fix m_defrag indent as well.

> > --- sys/net/if_pair.c   25 Oct 2015 12:59:57 -  1.4
> > +++ sys/net/if_pair.c   30 Oct 2015 11:30:33 -
> > @@ -30,6 +30,11 @@
> >  #include 
> >  #include 
> >  
> > +#include "pf.h"
> > +#if NPF > 0
> > +#include 
> > +#endif
> 
> I think you don't need that anymore.
> 
> > @@ -182,9 +187,13 @@ pairstart(struct ifnet *ifp)
> >  #endif /* NBPFILTER > 0 */
> >  
> > ifp->if_opackets++;
> > -   if (pairedifp != NULL)
> > +   if (pairedifp != NULL) {
> > +#if NPF > 0
> > +   if (m->m_flags & M_PKTHDR)
> > +   m_resethdr(m);
> > +#endif
> 
> Calling m_tag_delete_chain() is not pf specific, so I would do it
> without the #if NPF.
> 
> Otherwise OK bluhm@
> 
> Socket splicing somove() does the same thing.  I will change it to
> use m_resethdr() after that got commited.

Cool.

I'm going to commit the attached diff for now.

Reyk

Index: sys/kern/uipc_mbuf.c
===
RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
retrieving revision 1.208
diff -u -p -u -p -r1.208 uipc_mbuf.c
--- sys/kern/uipc_mbuf.c22 Oct 2015 05:26:06 -  1.208
+++ sys/kern/uipc_mbuf.c30 Oct 2015 12:45:50 -
@@ -250,6 +250,18 @@ m_inithdr(struct mbuf *m)
return (m);
 }
 
+void
+m_resethdr(struct mbuf *m)
+{
+   /* like the previous, but keep any associated data and mbufs */
+   m->m_flags = M_PKTHDR;
+   memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
+   m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
+
+   /* also delete all mbuf tags to reset the state */
+   m_tag_delete_chain(m);
+}
+
 struct mbuf *
 m_getclr(int nowait, int type)
 {
Index: sys/sys/mbuf.h
===
RCS file: /cvs/src/sys/sys/mbuf.h,v
retrieving revision 1.198
diff -u -p -u -p -r1.198 mbuf.h
--- sys/sys/mbuf.h  22 Oct 2015 05:26:06 -  1.198
+++ sys/sys/mbuf.h  30 Oct 2015 12:45:50 -
@@ -410,7 +410,8 @@ struct  mbuf *m_get(int, int);
 struct mbuf *m_getclr(int, int);
 struct mbuf *m_gethdr(int, int);
 struct mbuf *m_inithdr(struct mbuf *);
-int  m_defrag(struct mbuf *, int);
+void   m_resethdr(struct mbuf *);
+intm_defrag(struct mbuf *, int);
 struct mbuf *m_prepend(struct mbuf *, int, int);
 struct mbuf *m_pulldown(struct mbuf *, int, int, int *);
 struct mbuf *m_pullup(struct mbuf *, int);
Index: sys/net/if_pair.c
===
RCS file: /cvs/src/sys/net/if_pair.c,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 if_pair.c
--- sys/net/if_pair.c   25 Oct 2015 12:59:57 -  1.4
+++ sys/net/if_pair.c   30 Oct 2015 12:45:50 -
@@ -182,9 +182,11 @@ pairstart(struct ifnet *ifp)
 #endif /* NBPFILTER > 0 */
 
ifp->if_opackets++;
-   if (pairedifp != NULL)
+   if (pairedifp != NULL) {
+   if (m->m_flags & M_PKTHDR)
+   m_resethdr(m);
ml_enqueue(, m);
-   else
+   } else
m_freem(m);
}
 

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Reyk Floeter]

On Fri, Oct 30, 2015 at 12:45:31PM +0100, Mike Belopuhov wrote:
> On Fri, Oct 30, 2015 at 12:56 +0100, Reyk Floeter wrote:
> > On Fri, Oct 30, 2015 at 12:29:27PM +0100, Mike Belopuhov wrote:
> > > On Fri, Oct 30, 2015 at 12:29 +0100, Mike Belopuhov wrote:
> > > > On Fri, Oct 30, 2015 at 12:19 +0100, Reyk Floeter wrote:
> > > > > On Fri, Oct 30, 2015 at 11:30:56AM +0100, Alexander Bluhm wrote:
> > > > > > On Fri, Oct 30, 2015 at 10:43:21AM +0100, Reyk Floeter wrote:
> > > > > > > Question:
> > > > > > > > How does pair(4) interact with pf? If a packet crosses a pair
> > > > > > > > does it create a new state or does pf track the original state?
> > > > > > > > 
> > > > > > > 
> > > > > > > Answer:
> > > > > > > It does create a new state, you can filter between pair(4) without
> > > > > > > problems and all features including nat work.
> > > > > > > But it currently does not clear some of the extended packet 
> > > > > > > settings -
> > > > > > > like flags, tags, qid etc. - so you can filter on the tag, eg. 
> > > > > > > 
> > > > > > > # Assuming pair1 is patched to pair0
> > > > > > > pass out on pair1 tag FOO
> > > > > > > pass in on pair0 tagged FOO
> > > > > > > 
> > > > > > > The attached diff _removes_ that and resets all pf settings, so 
> > > > > > > the pf
> > > > > > > rules above will not work anymore.  I think it is better to assume
> > > > > > > crossing barriers and receiving packets with pair(4) works like a
> > > > > > > "normal" Ethernet packet.  The following will still work:
> > > > > > > 
> > > > > > > # Received packets on pair0 don't carry any existing pf states
> > > > > > > pass out on pair1
> > > > > > > pass in on pair0
> > > > > > > 
> > > > > > > OK?
> > > > > > 
> > > > > > The new semantics is better.
> > > > > > 
> > > > > > > +void
> > > > > > > +pf_pkt_reset(struct mbuf *m)
> > > > > > > +{
> > > > > > > + if (m->m_flags & M_PKTHDR)
> > > > > > > + m_tag_delete_chain(m);
> > > > > > > +
> > > > > > > + /* resets state key, pcb reference, qid, tag, and all flags */
> > > > > > > + memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> > > > > > > +}
> > > > > > 
> > > > > > You need a packet header mbuf to access m->m_pkthdr.  So either
> > > > > > assume that M_PKTHDR is set and don't check.  Or put both actions
> > > > > > into the if block.
> > > > > > 
> > > > > > As pf_pkt_addr_changed() accesses the m->m_pkthdr without check, I
> > > > > > would recomend to remove the "if (m->m_flags & M_PKTHDR)" here,
> > > > > > too.  You may also put an assert into both functions.
> > > > > > 
> > > > > > The default for m->m_pkthdr.pf.prio is IFQ_DEFPRIO, not 0.
> > > > > > Look at m_inithdr().
> > > > > > 
> > > > > 
> > > > > Thanks.
> > > > > 
> > > > > Adding asserts in in both functions makes sense, but I'll try it
> > > > > separately - no caller of pf_pkt_addr_changed() checks for the pkthdr
> > > > > at the moment and I fear potential fallout.  Maybe it would also make
> > > > > sense to rename both functions to pf_pkthdr_* to make it clear, but
> > > > > this can also be done separately.
> > > > > 
> > > > > Here is the updated diff, OK?
> > > > > 
> > > > > Reyk
> > > > > 
> > > > 
> > > > As I've told Reyk privately, since this function removes all
> > > > mbufs including IPsec ones, etc. it should not be part of pf,
> > > > but part of mbuf source code.
> > > 
> > > mbuf *tags*
> > 
> > I think pf does handle packet mangling in various ways, which would
> > include the intended removal of other tags, but I don't care.
> > 
> > Here is the updated diff.
> > 
> > OK?
> >
> 
> This looks much better.  We could potentially reset some other
> fields as well, but this doesn't have to happen right now.
> 

I agree, I left them out for now as they need more thoughts and don't
do much harm. The candidates are:

- ph_cookie (seems to be used by pipex)
- flowid(only for a hack in trunk?!)
- ether_vtag

Everything else is either set by m_resethdr or by if_input().

btw., it looks somewhat ugly that some members in the pkthdr are
prefixed with ph_ while others are not.

Reyk

> OK mikeb
> 
> > Index: sys/kern/uipc_mbuf.c
> > ===
> > RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
> > retrieving revision 1.208
> > diff -u -p -r1.208 uipc_mbuf.c
> > --- sys/kern/uipc_mbuf.c22 Oct 2015 05:26:06 -  1.208
> > +++ sys/kern/uipc_mbuf.c30 Oct 2015 11:30:32 -
> > @@ -250,6 +250,18 @@ m_inithdr(struct mbuf *m)
> > return (m);
> >  }
> >  
> > +void
> > +m_resethdr(struct mbuf *m)
> > +{
> > +   /* like the previous, but keep any associated data and mbufs */
> > +   m->m_flags = M_PKTHDR;
> > +   memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> > +   m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
> > +
> > +   /* also delete all mbuf tags to reset the state */
> > +   m_tag_delete_chain(m);
> > +}
> > +
> >  struct mbuf *
> >  m_getclr(int nowait, int type)
> >  {
> > Index: 

Re: toy zones for openbsd - an undergrad operating systems course assignment

[David Gwynne]

> On 30 Oct 2015, at 9:13 PM, Karel Gardas  wrote:
> 
> This is nice! Am I right assuming zone exec is a short-cut for not
> need to implement Solaris' zlogin functionality? I'm not sure if I'm
> as ordinary global zone user on Solaris able to start process in
> another zone where I don't have login credentials. So that may be
> difference between your zone and Solaris IIRC. Otherwise your
> implementation is simple and elegant. Do you plan to continue on this
> with another term students?

zone exec is a simple abstraction on top of the zone_enter syscall. zlogin on 
solaris is a less simple abstraction on top of that syscall.

an ordinary user in the global zone cannot call zone_enter (and zone exec by 
extension), only root in the gz can. the same is true for zlogin in solaris. 
also note that zlogin on solaris without any arguments doesnt require auth in 
the target zone, it fakes a successful login as root.

as for the future, i dont know if ill be asked to help out with the operating 
systems course again next year, and maybe something more trendy will pop up 
before then that they could implement. outside that, i already have enough 
things im working on.

if this scratches someones itch and they have enough time to tinker with it, 
they're more than welcome to do so.

dlg

> Nitpick, btw, I think sys_zone.c and sys/zone.h would be better fit as
> other files use mostly singular there, just IMHO!
> Anyway, thanks a lot for this code.
> 
> On Fri, Oct 30, 2015 at 4:17 AM, David Gwynne  wrote:
>> ola,
>> 
>> this semester ive had the fun of setting and marking the assignments
>> for the operating systems course they teach at the university of
>> queensland (where i work).
>> 
>> the second assignment was creating extremely basic process containers
>> using solaris zones as a model. the spec is at
>> http://www.uq.id.au/dlg/comp3301/assignment2.pdf.
>> 
>> the diff below is my solution to it.
>> 
>> to be clear, i am not asking for oks on this and definitely do not
>> want to see it going into the tree in its current state. it does
>> very simplistic and very incomplete process isolation, but does not
>> isolate any other aspect of the operating system and therefore
>> cannot be considered a useful containment infrastructure.
>> 
>> however, i found it interesting to get my head around this aspect
>> of the system, and i figured other people (such as this years
>> comp3301 students) would be interested too. i also felt sad i couldnt
>> find kritaps mult code anywhere, so i wanted this to be backed up
>> by everyone for future posterity.
>> 
>> this should also help explain why ive been slacking these last few
>> months.
>> 
>> im happy to explain the implementation if people are interested.
>> 
>> dlg
>> 
>> Index: bin/ps/extern.h
>> ===
>> RCS file: /cvs/src/bin/ps/extern.h,v
>> retrieving revision 1.17
>> diff -u -p -r1.17 extern.h
>> --- bin/ps/extern.h 29 Jun 2015 15:03:33 -  1.17
>> +++ bin/ps/extern.h 30 Oct 2015 02:52:16 -
>> @@ -61,6 +61,7 @@ void   nlisterr(struct nlist *);
>> voidp_rssize(const struct kinfo_proc *, VARENT *);
>> voidpagein(const struct kinfo_proc *, VARENT *);
>> voidparsefmt(char *);
>> +voidzonefmt(void);
>> voidpcpu(const struct kinfo_proc *, VARENT *);
>> voidpmem(const struct kinfo_proc *, VARENT *);
>> voidpri(const struct kinfo_proc *, VARENT *);
>> @@ -83,4 +84,5 @@ void   curwd(const struct kinfo_proc *, V
>> voideuname(const struct kinfo_proc *, VARENT *);
>> voidvsize(const struct kinfo_proc *, VARENT *);
>> voidwchan(const struct kinfo_proc *, VARENT *);
>> +voidzvar(const struct kinfo_proc *, VARENT *);
>> __END_DECLS
>> Index: bin/ps/keyword.c
>> ===
>> RCS file: /cvs/src/bin/ps/keyword.c,v
>> retrieving revision 1.42
>> diff -u -p -r1.42 keyword.c
>> --- bin/ps/keyword.c16 Jan 2015 06:39:32 -  1.42
>> +++ bin/ps/keyword.c30 Oct 2015 02:52:16 -
>> @@ -187,6 +187,7 @@ VAR var[] = {
>>{"vsz", "VSZ", NULL, 0, vsize, 5},
>>{"wchan", "WCHAN", NULL, LJUST, wchan, KI_WMESGLEN - 1},
>>{"xstat", "XSTAT", NULL, 0, pvar, 4, 0, POFF(p_xstat), UINT16, "x"},
>> +   {"zone", "ZONE", NULL, 0, zvar, 8, 0, POFF(p_zoneid)},
>>{""},
>> };
>> 
>> @@ -243,6 +244,20 @@ parsefmt(char *p)
>>}
>>if (!vhead)
>>errx(1, "no valid keywords");
>> +}
>> +
>> +void
>> +zonefmt(void)
>> +{
>> +   struct varent *vent;
>> +
>> +   vent = malloc(sizeof(*vent));
>> +   if (vent == NULL)
>> +   err(1, "zone fmt malloc");
>> +
>> +   vent->var = findvar("zone");
>> +   vent->next = vhead;
>> +   vhead = vent;
>> }
>> 
>> static VAR *
>> Index: bin/ps/print.c
>> ===
>> RCS file: 

Re: toy zones for openbsd - an undergrad operating systems course assignment

[Karel Gardas]

On Fri, Oct 30, 2015 at 1:11 PM, David Gwynne  wrote:
>
>> On 30 Oct 2015, at 9:13 PM, Karel Gardas  wrote:
>>
>> This is nice! Am I right assuming zone exec is a short-cut for not
>> need to implement Solaris' zlogin functionality? I'm not sure if I'm
>> as ordinary global zone user on Solaris able to start process in
>> another zone where I don't have login credentials. So that may be
>> difference between your zone and Solaris IIRC. Otherwise your
>> implementation is simple and elegant. Do you plan to continue on this
>> with another term students?
>
> zone exec is a simple abstraction on top of the zone_enter syscall. zlogin on 
> solaris is a less simple abstraction on top of that syscall.
>
> an ordinary user in the global zone cannot call zone_enter (and zone exec by 
> extension), only root in the gz can. the same is true for zlogin in solaris. 
> also note that zlogin on solaris without any arguments doesnt require auth in 
> the target zone, it fakes a successful login as root.

Indeed, my mistake in reading your patch. Also I always use zlogin -C
on Solaris so I completely overlooked this simplification. Thanks for
clarification.

Re: rt_ifix for ip6_forward()

[Alexander Bluhm]

On Thu, Oct 29, 2015 at 03:51:57PM +0100, Martin Pieuchot wrote:
> Stop using rt_ifp in this function.
> 
> ok?

OK bluhm@

> 
> Index: netinet6/ip6_forward.c
> ===
> RCS file: /cvs/src/sys/netinet6/ip6_forward.c,v
> retrieving revision 1.85
> diff -u -p -r1.85 ip6_forward.c
> --- netinet6/ip6_forward.c28 Oct 2015 12:14:25 -  1.85
> +++ netinet6/ip6_forward.c29 Oct 2015 14:36:35 -
> @@ -89,6 +89,7 @@ ip6_forward(struct mbuf *m, int srcrt)
>   struct ip6_hdr *ip6 = mtod(m, struct ip6_hdr *);
>   struct sockaddr_in6 *dst;
>   struct rtentry *rt;
> + struct ifnet *ifp = NULL;
>   int error = 0, type = 0, code = 0;
>   struct mbuf *mcopy = NULL;
>  #ifdef IPSEC
> @@ -362,11 +363,12 @@ reroute:
>* Also, don't send redirect if forwarding using a route
>* modified by a redirect.
>*/
> - if (rt->rt_ifp->if_index == m->m_pkthdr.ph_ifidx && !srcrt &&
> + ifp = if_get(rt->rt_ifidx);
> + if (rt->rt_ifidx == m->m_pkthdr.ph_ifidx && !srcrt &&
>   ip6_sendredirects &&
>   (rt->rt_flags & (RTF_DYNAMIC|RTF_MODIFIED)) == 0) {
> - if ((rt->rt_ifp->if_flags & IFF_POINTOPOINT) &&
> - nd6_is_addr_neighbor(_forward_rt.ro_dst, rt->rt_ifp)) {
> + if ((ifp->if_flags & IFF_POINTOPOINT) &&
> + nd6_is_addr_neighbor(_forward_rt.ro_dst, ifp)) {
>   /*
>* If the incoming interface is equal to the outgoing
>* one, the link attached to the interface is
> @@ -405,7 +407,7 @@ reroute:
>   ip6->ip6_dst.s6_addr16[1] = 0;
>  
>  #if NPF > 0
> - if (pf_test(AF_INET6, PF_FWD, rt->rt_ifp, ) != PF_PASS) {
> + if (pf_test(AF_INET6, PF_FWD, ifp, ) != PF_PASS) {
>   m_freem(m);
>   goto senderr;
>   }
> @@ -420,21 +422,23 @@ reroute:
>   /* tag as generated to skip over pf_test on rerun */
>   m->m_pkthdr.pf.flags |= PF_TAG_GENERATED;
>   srcrt = 1;
> + if_put(ifp);
> + ifp = NULL;
>   goto reroute;
>   }
>  #endif
> - in6_proto_cksum_out(m, rt->rt_ifp);
> + in6_proto_cksum_out(m, ifp);
>  
>   /* Check the size after pf_test to give pf a chance to refragment. */
> - if (m->m_pkthdr.len > rt->rt_ifp->if_mtu) {
> + if (m->m_pkthdr.len > ifp->if_mtu) {
>   if (mcopy)
>   icmp6_error(mcopy, ICMP6_PACKET_TOO_BIG, 0,
> - rt->rt_ifp->if_mtu);
> + ifp->if_mtu);
>   m_freem(m);
>   goto freert;
>   }
>  
> - error = nd6_output(rt->rt_ifp, m, dst, rt);
> + error = nd6_output(ifp, m, dst, rt);
>   if (error) {
>   ip6stat.ip6s_cantforward++;
>   } else {
> @@ -490,5 +494,5 @@ senderr:
>   ip6_forward_rt.ro_rt = NULL;
>   }
>  #endif
> - return;
> + if_put(ifp);
>  }

ntpd(8): remove SIGINFO reports, we have ntpctl now

[Reyk Floeter]

Hi,

is anybody still using SIGINFO to get reports from ntpd?
It predates ntpctl that is a sufficient replacement.

OK?

Reyk

Index: ntp.c
===
RCS file: /cvs/src/usr.sbin/ntpd/ntp.c,v
retrieving revision 1.138
diff -u -p -u -p -r1.138 ntp.c
--- ntp.c   23 Oct 2015 14:52:20 -  1.138
+++ ntp.c   30 Oct 2015 14:35:45 -
@@ -40,7 +40,6 @@
 #definePFD_MAX 3
 
 volatile sig_atomic_t   ntp_quit = 0;
-volatile sig_atomic_t   ntp_report = 0;
 struct imsgbuf *ibuf_main;
 struct imsgbuf *ibuf_dns;
 struct ntpd_conf   *conf;
@@ -48,14 +47,12 @@ struct ctl_conns ctl_conns;
 u_int   peer_cnt;
 u_int   sensors_cnt;
 extern u_intconstraint_cnt;
-time_t  lastreport;
 
 void   ntp_sighdlr(int);
 intntp_dispatch_imsg(void);
 intntp_dispatch_imsg_dns(void);
 void   peer_add(struct ntp_peer *);
 void   peer_remove(struct ntp_peer *);
-void   report_peers(int);
 
 void
 ntp_sighdlr(int sig)
@@ -65,9 +62,6 @@ ntp_sighdlr(int sig)
case SIGTERM:
ntp_quit = 1;
break;
-   case SIGINFO:
-   ntp_report = 1;
-   break;
}
 }
 
@@ -160,7 +154,6 @@ ntp_main(int pipe_prnt[2], int fd_ctl, s
 
signal(SIGTERM, ntp_sighdlr);
signal(SIGINT, ntp_sighdlr);
-   signal(SIGINFO, ntp_sighdlr);
signal(SIGPIPE, SIG_IGN);
signal(SIGHUP, SIG_IGN);
signal(SIGCHLD, SIG_DFL);
@@ -209,9 +202,6 @@ ntp_main(int pipe_prnt[2], int fd_ctl, s
TAILQ_FOREACH(p, >ntp_peers, entry)
peer_cnt++;
 
-   /* wait 5 min before reporting first status to let things settle down */
-   lastreport = getmonotime() + (5 * 60) - REPORT_INTERVAL;
-
while (ntp_quit == 0) {
if (peer_cnt > idx2peer_elms) {
if ((newp = reallocarray(idx2peer, peer_cnt,
@@ -419,8 +409,6 @@ ntp_main(int pipe_prnt[2], int fd_ctl, s
if (s->next <= getmonotime())
sensor_query(s);
}
-   report_peers(ntp_report);
-   ntp_report = 0;
}
 
msgbuf_write(_main->w);
@@ -787,60 +775,4 @@ error_interval(void)
interval = INTERVAL_QUERY_PATHETIC * QSCALE_OFF_MAX / QSCALE_OFF_MIN;
r = arc4random_uniform(interval / 10);
return (interval + r);
-}
-
-void
-report_peers(int always)
-{
-   time_t now;
-   u_int badpeers = 0;
-   u_int badsensors = 0;
-   struct ntp_peer *p;
-   struct ntp_sensor *s;
-
-   TAILQ_FOREACH(p, >ntp_peers, entry) {
-   if (p->trustlevel < TRUSTLEVEL_BADPEER)
-   badpeers++;
-   }
-   TAILQ_FOREACH(s, >ntp_sensors, entry) {
-   if (!s->update.good)
-   badsensors++;
-   }
-
-   now = getmonotime();
-   if (!always) {
-   if ((peer_cnt == 0 || badpeers == 0 || badpeers < peer_cnt / 2)
-   && (sensors_cnt == 0 || badsensors == 0 ||
-   badsensors < sensors_cnt / 2))
-   return;
-
-   if (lastreport + REPORT_INTERVAL > now)
-   return;
-   }
-   lastreport = now;
-   if (peer_cnt > 0) {
-   log_warnx("%u out of %u peers valid", peer_cnt - badpeers,
-   peer_cnt);
-   TAILQ_FOREACH(p, >ntp_peers, entry) {
-   if (p->trustlevel < TRUSTLEVEL_BADPEER) {
-   const char *a = "not resolved";
-   const char *pool = "";
-   if (p->addr)
-   a = log_sockaddr(
-   (struct sockaddr *)>addr->ss);
-   if (p->addr_head.pool)
-   pool = "from pool ";
-   log_warnx("bad peer %s%s (%s)",
-   pool, p->addr_head.name, a);
-   }
-   }
-   }
-   if (sensors_cnt > 0) {
-   log_warnx("%u out of %u sensors valid",
-   sensors_cnt - badsensors, sensors_cnt);
-   TAILQ_FOREACH(s, >ntp_sensors, entry) {
-   if (!s->update.good)
-   log_warnx("bad sensor %s", s->device);
-   }
-   }
 }
Index: ntpd.8
===
RCS file: /cvs/src/usr.sbin/ntpd/ntpd.8,v
retrieving revision 1.39
diff -u -p -u -p -r1.39 ntpd.8
--- ntpd.8  26 Mar 2015 20:34:54 -  1.39
+++ ntpd.8  30 Oct 2015 14:35:45 -
@@ -119,14 +119,6 @@ typically
 and its initial clock drift from
 .Pa /var/db/ntpd.drift .
 Clock drift is periodically written to the drift file thereafter.
-.Pp
-When

Re: [PATCH] rcs: buf_free/rcsnum_free

[Nicholas Marriott]

Sorry, the one I pointed out in ci.c is wrong:

>   rcs_close(pb.file);
> - if (rev_str != NULL)
> - rcsnum_free(pb.newrev);
> + rcsnum_free(pb.newrev);
>   pb.newrev = NULL;

pb.newrev can be changed by checkin_init or checkin_update, in which
case it shouldn't be freed, so this check needs to stay the same.

The rest is ok nicm



On Fri, Oct 30, 2015 at 08:52:02AM +0800, Michael W. Bombardieri wrote:
> On Thu, Oct 29, 2015 at 10:54:09AM +, Nicholas Marriott wrote:
> > Hi
> > 
> > You missed ci.c:316 and a few in rcs.c and rcsdiff.c
> 
> Sorry. Here is new diff. Hopefully I haven't missed anything else.
> 
> 
> 
> Index: buf.c
> ===
> RCS file: /cvs/src/usr.bin/rcs/buf.c,v
> retrieving revision 1.25
> diff -u -p -u -r1.25 buf.c
> --- buf.c 13 Jun 2015 20:15:21 -  1.25
> +++ buf.c 30 Oct 2015 01:11:28 -
> @@ -138,6 +138,8 @@ out:
>  void
>  buf_free(BUF *b)
>  {
> + if (b == NULL)
> + return;
>   free(b->cb_buf);
>   free(b);
>  }
> Index: ci.c
> ===
> RCS file: /cvs/src/usr.bin/rcs/ci.c,v
> retrieving revision 1.222
> diff -u -p -u -r1.222 ci.c
> --- ci.c  5 Sep 2015 09:38:23 -   1.222
> +++ ci.c  30 Oct 2015 01:11:28 -
> @@ -313,8 +313,7 @@ checkin_main(int argc, char **argv)
>   }
>  
>   rcs_close(pb.file);
> - if (rev_str != NULL)
> - rcsnum_free(pb.newrev);
> + rcsnum_free(pb.newrev);
>   pb.newrev = NULL;
>   }
>  
> @@ -369,12 +368,9 @@ checkin_diff_file(struct checkin_params 
>  
>   return (b3);
>  out:
> - if (b1 != NULL)
> - buf_free(b1);
> - if (b2 != NULL)
> - buf_free(b2);
> - if (b3 != NULL)
> - buf_free(b3);
> + buf_free(b1);
> + buf_free(b2);
> + buf_free(b3);
>   free(path1);
>   free(path2);
>  
> Index: diff3.c
> ===
> RCS file: /cvs/src/usr.bin/rcs/diff3.c,v
> retrieving revision 1.37
> diff -u -p -u -r1.37 diff3.c
> --- diff3.c   5 Sep 2015 09:47:08 -   1.37
> +++ diff3.c   30 Oct 2015 01:11:28 -
> @@ -234,14 +234,10 @@ merge_diff3(char **av, int flags)
>   warnx("warning: overlaps or other problems during merge");
>  
>  out:
> - if (b2 != NULL)
> - buf_free(b2);
> - if (b3 != NULL)
> - buf_free(b3);
> - if (d1 != NULL)
> - buf_free(d1);
> - if (d2 != NULL)
> - buf_free(d2);
> + buf_free(b2);
> + buf_free(b3);
> + buf_free(d1);
> + buf_free(d2);
>  
>   (void)unlink(path1);
>   (void)unlink(path2);
> @@ -354,14 +350,10 @@ rcs_diff3(RCSFILE *rf, char *workfile, R
>   warnx("warning: overlaps or other problems during merge");
>  
>  out:
> - if (b2 != NULL)
> - buf_free(b2);
> - if (b3 != NULL)
> - buf_free(b3);
> - if (d1 != NULL)
> - buf_free(d1);
> - if (d2 != NULL)
> - buf_free(d2);
> + buf_free(b2);
> + buf_free(b3);
> + buf_free(d1);
> + buf_free(d2);
>  
>   (void)unlink(path1);
>   (void)unlink(path2);
> Index: ident.c
> ===
> RCS file: /cvs/src/usr.bin/rcs/ident.c,v
> retrieving revision 1.30
> diff -u -p -u -r1.30 ident.c
> --- ident.c   2 Oct 2014 06:23:15 -   1.30
> +++ ident.c   30 Oct 2015 01:11:28 -
> @@ -156,8 +156,7 @@ ident_line(FILE *fp)
>  
>   found++;
>  out:
> - if (bp != NULL)
> - buf_free(bp);
> + buf_free(bp);
>  }
>  
>  __dead void
> Index: rcs.c
> ===
> RCS file: /cvs/src/usr.bin/rcs/rcs.c,v
> retrieving revision 1.83
> diff -u -p -u -r1.83 rcs.c
> --- rcs.c 13 Jun 2015 20:15:21 -  1.83
> +++ rcs.c 30 Oct 2015 01:11:28 -
> @@ -177,10 +177,8 @@ rcs_close(RCSFILE *rfp)
>   free(rlp);
>   }
>  
> - if (rfp->rf_head != NULL)
> - rcsnum_free(rfp->rf_head);
> - if (rfp->rf_branch != NULL)
> - rcsnum_free(rfp->rf_branch);
> + rcsnum_free(rfp->rf_head);
> + rcsnum_free(rfp->rf_branch);
>  
>   if (rfp->rf_file != NULL)
>   fclose(rfp->rf_file);
> @@ -1406,10 +1404,8 @@ rcs_freedelta(struct rcs_delta *rdp)
>  {
>   struct rcs_branch *rb;
>  
> - if (rdp->rd_num != NULL)
> - rcsnum_free(rdp->rd_num);
> - if (rdp->rd_next != NULL)
> - rcsnum_free(rdp->rd_next);
> + rcsnum_free(rdp->rd_num);
> + rcsnum_free(rdp->rd_next);
>  
>   free(rdp->rd_author);
>   free(rdp->rd_locker);
> Index: rcsclean.c
> ===
> RCS file: 

Re: boot from softraid, backspace in passphrase prompt

[Miod Vallat]

> I want correct typing mistakes when booting from softraid crypto disks.
> Can we handle at least the backspace key, plz^Hease? :)

This calls for a libsa gets() replacement, which will honour bounds.
What about the plumbing diff below, so that softraid-capable bootblocks
can use the new getln() routine instead of rolling their own?

Index: arch/amd64/stand/pxeboot/Makefile
===
RCS file: /OpenBSD/src/sys/arch/amd64/stand/pxeboot/Makefile,v
retrieving revision 1.24
diff -u -p -r1.24 Makefile
--- arch/amd64/stand/pxeboot/Makefile   2 Sep 2015 01:52:26 -   1.24
+++ arch/amd64/stand/pxeboot/Makefile   30 Oct 2015 16:27:31 -
@@ -27,9 +27,10 @@ SRCS+=   softraid.c
 SRCS+= boot.c cmd.c vars.c bootarg.c
 
 .PATH: ${S}/lib/libsa
-SRCS+= alloc.c exit.c getchar.c getfile.c gets.c globals.c putchar.c strcmp.c \
-   strlen.c strncmp.c memcmp.c memcpy.c memset.c printf.c snprintf.c \
-   strerror.c strncpy.c strtol.c strtoll.c ctime.c strlcpy.c strlcat.c
+SRCS+= alloc.c exit.c getchar.c getfile.c getln.c globals.c putchar.c \
+   strcmp.c strlen.c strncmp.c memcmp.c memcpy.c memset.c printf.c \
+   snprintf.c strerror.c strncpy.c strtol.c strtoll.c ctime.c strlcpy.c \
+   strlcat.c
 SRCS+= aes_xts.c explicit_bzero.c hmac_sha1.c pbkdf2.c rijndael.c sha1.c
 
 SRCS+= close.c closeall.c dev.c disklabel.c dkcksum.c fstat.c ioctl.c lseek.c \
Index: arch/aviion/stand/boot/boot.c
===
RCS file: /OpenBSD/src/sys/arch/aviion/stand/boot/boot.c,v
retrieving revision 1.5
diff -u -p -r1.5 boot.c
--- arch/aviion/stand/boot/boot.c   24 Feb 2014 20:15:37 -  1.5
+++ arch/aviion/stand/boot/boot.c   30 Oct 2015 16:27:31 -
@@ -109,7 +109,7 @@ boot(const char *args, uint bootdev, uin
for (;;) {
if (ask != 0) {
printf("boot: ");
-   gets(line);
+   getln(line, sizeof line);
if (line[0] == '\0')
continue;
 
Index: arch/aviion/stand/libsa/Makefile
===
RCS file: /OpenBSD/src/sys/arch/aviion/stand/libsa/Makefile,v
retrieving revision 1.6
diff -u -p -r1.6 Makefile
--- arch/aviion/stand/libsa/Makefile19 Nov 2014 20:01:35 -  1.6
+++ arch/aviion/stand/libsa/Makefile30 Oct 2015 16:27:31 -
@@ -12,7 +12,7 @@ S=${.CURDIR}/../../../..
 SRCS=  clock.c delay.S exec.c fault.c parse_args.c setjmp.S
 
 .PATH: ${S}/lib/libsa
-SRCS+= alloc.c memcpy.c exit.c getfile.c gets.c globals.c loadfile.c \
+SRCS+= alloc.c memcpy.c exit.c getfile.c getln.c globals.c loadfile.c \
printf.c strerror.c memset.c memcmp.c strncpy.c strcmp.c strlen.c \
strlcpy.c strlcat.c snprintf.c strchr.c strtol.c \
close.c closeall.c dev.c dkcksum.c \
Index: arch/hppa/stand/libsa/Makefile
===
RCS file: /OpenBSD/src/sys/arch/hppa/stand/libsa/Makefile,v
retrieving revision 1.19
diff -u -p -r1.19 Makefile
--- arch/hppa/stand/libsa/Makefile  13 Jul 2014 09:26:08 -  1.19
+++ arch/hppa/stand/libsa/Makefile  30 Oct 2015 16:27:31 -
@@ -20,7 +20,7 @@ SRCS= machdep.c pdc.c itecons.c dev_hppa
ct.c dk.c lf.c lif.c cmd_hppa.c loadfile.c elf32.c elf64.c
 
 # stand routines
-SRCS+= alloc.c exit.c getfile.c gets.c getchar.c globals.c \
+SRCS+= alloc.c exit.c getfile.c getln.c getchar.c globals.c \
printf.c putchar.c strerror.c strtol.c strchr.c ctime.c snprintf.c
 
 # io routines
Index: arch/hppa64/stand/libsa/Makefile
===
RCS file: /OpenBSD/src/sys/arch/hppa64/stand/libsa/Makefile,v
retrieving revision 1.5
diff -u -p -r1.5 Makefile
--- arch/hppa64/stand/libsa/Makefile13 Jul 2014 09:26:08 -  1.5
+++ arch/hppa64/stand/libsa/Makefile30 Oct 2015 16:27:31 -
@@ -21,7 +21,7 @@ SRCS= machdep.c pdc.c itecons.c dev_hppa
ct.c dk.c lf.c lif.c cmd_hppa64.c
 
 # stand routines
-SRCS+= alloc.c exit.c getfile.c gets.c getchar.c globals.c \
+SRCS+= alloc.c exit.c getfile.c getln.c getchar.c globals.c \
printf.c putchar.c strerror.c strtol.c strchr.c ctime.c loadfile.c \
snprintf.c
 
Index: arch/loongson/stand/libsa/Makefile
===
RCS file: /OpenBSD/src/sys/arch/loongson/stand/libsa/Makefile,v
retrieving revision 1.6
diff -u -p -r1.6 Makefile
--- arch/loongson/stand/libsa/Makefile  13 Jul 2014 09:26:08 -  1.6
+++ arch/loongson/stand/libsa/Makefile  30 Oct 2015 16:27:32 -
@@ -16,7 +16,7 @@ CFLAGS+= ${CEXTRAFLAGS} ${SAABI} -nostdi
-I${.OBJDIR}
 
 # stand routines
-SRCS=  alloc.c cons.c ctime.c exit.c getchar.c getfile.c gets.c globals.c \
+SRCS=  alloc.c cons.c ctime.c exit.c getchar.c getfile.c getln.c globals.c 

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Sebastian Benoit]

i think it should be documented ;)

otherwise ok

Index: mbuf.9
===
RCS file: /cvs/src/share/man/man9/mbuf.9,v
retrieving revision 1.91
diff -u -p -u -r1.91 mbuf.9
--- mbuf.9  8 Oct 2015 14:09:34 -   1.91
+++ mbuf.9  30 Oct 2015 17:15:40 -
@@ -44,6 +44,8 @@
 .Fn MGET "struct mbuf *m" "int how" "int type"
 .Ft struct mbuf *
 .Fn m_getclr "int how" "int type"
+.Ft void
+.Fn m_resethdr struct mbuf *
 .Ft struct mbuf *
 .Fn m_gethdr "int how" "int type"
 .Fn MGETHDR "struct mbuf *m" "int how" "int type"
@@ -445,6 +447,11 @@ See
 .Fn m_get
 for a description of
 .Fa how .
+.It Fn m_resethdr "struct mbuf *"
+Deletes all
+.Xr pf 4
+data and all tags attached to packet
+.Fa mbuf .
 .It Fn m_gethdr "int how" "int type"
 Return a pointer to an mbuf of the type specified after initializing
 it to contain a packet header.


Reyk Floeter(r...@openbsd.org) on 2015.10.30 14:04:52 +0100:
> On Fri, Oct 30, 2015 at 01:40:19PM +0100, Alexander Bluhm wrote:
> > On Fri, Oct 30, 2015 at 12:56:34PM +0100, Reyk Floeter wrote:
> > > --- sys/sys/mbuf.h22 Oct 2015 05:26:06 -  1.198
> > > +++ sys/sys/mbuf.h30 Oct 2015 11:30:33 -
> > > @@ -410,6 +410,7 @@ structmbuf *m_get(int, int);
> > >  struct   mbuf *m_getclr(int, int);
> > >  struct   mbuf *m_gethdr(int, int);
> > >  struct   mbuf *m_inithdr(struct mbuf *);
> > > +void m_resethdr(struct mbuf *);
> > >  intm_defrag(struct mbuf *, int);
> > 
> > The m_resethdr should have the same indent as m_defrag.
> > 
> 
> Really?  The indent of m_defrag() is wrong - many spaces and not like
> the other int functions further down - but I didn't dare to touch it.
> It sticks in the eye, so I'll fix m_defrag indent as well.
> 
> > > --- sys/net/if_pair.c 25 Oct 2015 12:59:57 -  1.4
> > > +++ sys/net/if_pair.c 30 Oct 2015 11:30:33 -
> > > @@ -30,6 +30,11 @@
> > >  #include 
> > >  #include 
> > >  
> > > +#include "pf.h"
> > > +#if NPF > 0
> > > +#include 
> > > +#endif
> > 
> > I think you don't need that anymore.
> > 
> > > @@ -182,9 +187,13 @@ pairstart(struct ifnet *ifp)
> > >  #endif /* NBPFILTER > 0 */
> > >  
> > >   ifp->if_opackets++;
> > > - if (pairedifp != NULL)
> > > + if (pairedifp != NULL) {
> > > +#if NPF > 0
> > > + if (m->m_flags & M_PKTHDR)
> > > + m_resethdr(m);
> > > +#endif
> > 
> > Calling m_tag_delete_chain() is not pf specific, so I would do it
> > without the #if NPF.
> > 
> > Otherwise OK bluhm@
> > 
> > Socket splicing somove() does the same thing.  I will change it to
> > use m_resethdr() after that got commited.
> 
> Cool.
> 
> I'm going to commit the attached diff for now.
> 
> Reyk
> 
> Index: sys/kern/uipc_mbuf.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
> retrieving revision 1.208
> diff -u -p -u -p -r1.208 uipc_mbuf.c
> --- sys/kern/uipc_mbuf.c  22 Oct 2015 05:26:06 -  1.208
> +++ sys/kern/uipc_mbuf.c  30 Oct 2015 12:45:50 -
> @@ -250,6 +250,18 @@ m_inithdr(struct mbuf *m)
>   return (m);
>  }
>  
> +void
> +m_resethdr(struct mbuf *m)
> +{
> + /* like the previous, but keep any associated data and mbufs */
> + m->m_flags = M_PKTHDR;
> + memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> + m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
> +
> + /* also delete all mbuf tags to reset the state */
> + m_tag_delete_chain(m);
> +}
> +
>  struct mbuf *
>  m_getclr(int nowait, int type)
>  {
> Index: sys/sys/mbuf.h
> ===
> RCS file: /cvs/src/sys/sys/mbuf.h,v
> retrieving revision 1.198
> diff -u -p -u -p -r1.198 mbuf.h
> --- sys/sys/mbuf.h22 Oct 2015 05:26:06 -  1.198
> +++ sys/sys/mbuf.h30 Oct 2015 12:45:50 -
> @@ -410,7 +410,8 @@ structmbuf *m_get(int, int);
>  struct   mbuf *m_getclr(int, int);
>  struct   mbuf *m_gethdr(int, int);
>  struct   mbuf *m_inithdr(struct mbuf *);
> -intm_defrag(struct mbuf *, int);
> +void m_resethdr(struct mbuf *);
> +int  m_defrag(struct mbuf *, int);
>  struct   mbuf *m_prepend(struct mbuf *, int, int);
>  struct   mbuf *m_pulldown(struct mbuf *, int, int, int *);
>  struct   mbuf *m_pullup(struct mbuf *, int);
> Index: sys/net/if_pair.c
> ===
> RCS file: /cvs/src/sys/net/if_pair.c,v
> retrieving revision 1.4
> diff -u -p -u -p -r1.4 if_pair.c
> --- sys/net/if_pair.c 25 Oct 2015 12:59:57 -  1.4
> +++ sys/net/if_pair.c 30 Oct 2015 12:45:50 -
> @@ -182,9 +182,11 @@ pairstart(struct ifnet *ifp)
>  #endif /* NBPFILTER > 0 */
>  
>   ifp->if_opackets++;
> - if (pairedifp != NULL)
> + if (pairedifp != NULL) {
> + if (m->m_flags & M_PKTHDR)
> + 

login(3) routines data integrity patch

[Chris Turner]

Hello -

I was testing some login data collection scripts (on a VM)
and discovered that in certain cases, it was possible for a
login record to not be fully commited to disk prior to
system shutdown, resulting in the last(1) entry for the
login not being visible. (was doing e.g. ssh root@testbox
to generate wtmp login records and then powering off the vm
to see if my code processed unclean shutdown records correctly).

I could see in some scenarios, aside from generating incorrect
data, this incorrect record could be used to facillitate hiding
presence of a successful compromise.

The attached patch calls fsync(2) on related FD's in the login(3)
routines, which corrected the problem on my test machine,
and imho might be a good idea in general.

The patch was generated on 5.8 current, but based on a
rudimentary check of head it looks like it should apply cleanly
and is 3 lines of diff if not :)

It might be useful also to have last(1) warn of truncated/
incomplete records as were the case in my (I think now lost)
corrupt wtmp file.. I did not attempt to implement this.

Please let me know if there are any questions or concerns
and thanks for a great system.

Thanks,

- Chris
Index: lib/libutil/login.c
===
RCS file: /cvs/src/lib/libutil/login.c,v
retrieving revision 1.10
diff -u -p -r1.10 login.c
--- lib/libutil/login.c	2 Aug 2005 21:46:23 -	1.10
+++ lib/libutil/login.c	30 Oct 2015 15:53:04 -
@@ -62,10 +62,12 @@ login(struct utmp *utp)
 			(void)memcpy(utp->ut_host, old_ut.ut_host, UT_HOSTSIZE);
 		(void)lseek(fd, (off_t)(tty * sizeof(struct utmp)), SEEK_SET);
 		(void)write(fd, utp, sizeof(struct utmp));
+		(void)fsync(fd);
 		(void)close(fd);
 	}
 	if ((fd = open(_PATH_WTMP, O_WRONLY|O_APPEND, 0)) >= 0) {
 		(void)write(fd, utp, sizeof(struct utmp));
+		(void)fsync(fd);
 		(void)close(fd);
 	}
 }
Index: lib/libutil/logout.c
===
RCS file: /cvs/src/lib/libutil/logout.c,v
retrieving revision 1.8
diff -u -p -r1.8 logout.c
--- lib/libutil/logout.c	2 Aug 2005 21:46:23 -	1.8
+++ lib/libutil/logout.c	30 Oct 2015 15:53:34 -
@@ -60,6 +60,7 @@ logout(const char *line)
 		(void)write(fd, , sizeof(UTMP));
 		rval = 1;
 	}
+	(void)fsync(fd);
 	(void)close(fd);
 	return(rval);
 }
Index: lib/libutil/logwtmp.c
===
RCS file: /cvs/src/lib/libutil/logwtmp.c,v
retrieving revision 1.9
diff -u -p -r1.9 logwtmp.c
--- lib/libutil/logwtmp.c	2 Aug 2005 21:46:23 -	1.9
+++ lib/libutil/logwtmp.c	30 Oct 2015 15:46:38 -
@@ -57,5 +57,6 @@ logwtmp(const char *line, const char *na
 		sizeof(struct utmp))
 			(void) ftruncate(fd, buf.st_size);
 	}
+	(void) fsync(fd);
 	(void) close(fd);
 }

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Alexander Bluhm]

On Fri, Oct 30, 2015 at 02:04:52PM +0100, Reyk Floeter wrote:
> > Socket splicing somove() does the same thing.  I will change it to
> > use m_resethdr() after that got commited.

I just compared code in somove() with m_resethdr().  Socket splicing
has to clear the whole packet header, not only the pf part.  I think
this is also useful for pair(4) as it should behave like a cable.

Resetting the other m_flags seems wrong.  I have put the M_PKTHDR
into an assert.  If it is not an packet header, the memset() could
overwrite mbuf data.

ok?

bluhm

Index: kern/uipc_mbuf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_mbuf.c,v
retrieving revision 1.209
diff -u -p -r1.209 uipc_mbuf.c
--- kern/uipc_mbuf.c30 Oct 2015 12:54:36 -  1.209
+++ kern/uipc_mbuf.c30 Oct 2015 17:13:58 -
@@ -253,13 +253,17 @@ m_inithdr(struct mbuf *m)
 void
 m_resethdr(struct mbuf *m)
 {
-   /* like the previous, but keep any associated data and mbufs */
-   m->m_flags = M_PKTHDR;
-   memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
-   m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
+   int len = m->m_pkthdr.len;
+
+   KASSERT(m->m_flags & M_PKTHDR);
 
-   /* also delete all mbuf tags to reset the state */
+   /* delete all mbuf tags to reset the state */
m_tag_delete_chain(m);
+
+   /* like m_inithdr(), but keep any associated data and mbufs */
+   memset(>m_pkthdr, 0, sizeof(m->m_pkthdr));
+   m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
+   m->m_pkthdr.len = len;
 }
 
 struct mbuf *
Index: kern/uipc_socket.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.142
diff -u -p -r1.142 uipc_socket.c
--- kern/uipc_socket.c  24 Aug 2015 14:28:25 -  1.142
+++ kern/uipc_socket.c  30 Oct 2015 17:06:57 -
@@ -1325,10 +1325,8 @@ somove(struct socket *so, int wait)
goto release;
m->m_nextpkt = NULL;
if (m->m_flags & M_PKTHDR) {
-   m_tag_delete_chain(m);
-   memset(>m_pkthdr, 0, sizeof(m->m_pkthdr));
+   m_resethdr(m);
m->m_pkthdr.len = len;
-   m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
}
 
/* Send window update to source peer as receive buffer has changed. */

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Mike Belopuhov]

On Fri, Oct 30, 2015 at 18:16 +0100, Sebastian Benoit wrote:
> 
> i think it should be documented ;)
> 
> otherwise ok
> 
> Index: mbuf.9
> ===
> RCS file: /cvs/src/share/man/man9/mbuf.9,v
> retrieving revision 1.91
> diff -u -p -u -r1.91 mbuf.9
> --- mbuf.98 Oct 2015 14:09:34 -   1.91
> +++ mbuf.930 Oct 2015 17:15:40 -
> @@ -44,6 +44,8 @@
>  .Fn MGET "struct mbuf *m" "int how" "int type"
>  .Ft struct mbuf *
>  .Fn m_getclr "int how" "int type"
> +.Ft void
> +.Fn m_resethdr struct mbuf *
>  .Ft struct mbuf *
>  .Fn m_gethdr "int how" "int type"
>  .Fn MGETHDR "struct mbuf *m" "int how" "int type"
> @@ -445,6 +447,11 @@ See
>  .Fn m_get
>  for a description of
>  .Fa how .
> +.It Fn m_resethdr "struct mbuf *"
> +Deletes all
> +.Xr pf 4
> +data and all tags attached to packet
> +.Fa mbuf .

I'd phrase it "attached to an mbuf", but otherwise OK mikeb

>  .It Fn m_gethdr "int how" "int type"
>  Return a pointer to an mbuf of the type specified after initializing
>  it to contain a packet header.
> 

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Alexander Bluhm]

On Fri, Oct 30, 2015 at 06:48:16PM +0100, Mike Belopuhov wrote:
> On Fri, Oct 30, 2015 at 18:27 +0100, Alexander Bluhm wrote:
> > On Fri, Oct 30, 2015 at 02:04:52PM +0100, Reyk Floeter wrote:
> > > > Socket splicing somove() does the same thing.  I will change it to
> > > > use m_resethdr() after that got commited.
> > 
> > I just compared code in somove() with m_resethdr().  Socket splicing
> > has to clear the whole packet header, not only the pf part.  I think
> > this is also useful for pair(4) as it should behave like a cable.
> > 
> > Resetting the other m_flags seems wrong.  I have put the M_PKTHDR
> > into an assert.  If it is not an packet header, the memset() could
> > overwrite mbuf data.
> > 
> > ok?
> >
> 
> Resetting flags that are set on input (e.g. M_CONF, M_AUTH, etc.) is
> not wrong since input on the other pair has not happened yet and it
> might provide more problems than shortcuts.  I'm not certain which
> flags may still be of use before the other pair figures it on its
> own.  Do you have any examples of such flags?

I think flags that must be preserverd are
- M_ZEROIZE
- M_EXT

In sys/mbuf.h Flags are split into mbuf flags and mbuf pkthdr flags.
As we clear the packet header but keep the data, we should probably
only clear the mbuf pkthdr flags.

Something like this flag combination?

bluhm

Index: kern/uipc_mbuf.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_mbuf.c,v
retrieving revision 1.209
diff -u -p -r1.209 uipc_mbuf.c
--- kern/uipc_mbuf.c30 Oct 2015 12:54:36 -  1.209
+++ kern/uipc_mbuf.c30 Oct 2015 18:00:59 -
@@ -253,13 +253,18 @@ m_inithdr(struct mbuf *m)
 void
 m_resethdr(struct mbuf *m)
 {
-   /* like the previous, but keep any associated data and mbufs */
-   m->m_flags = M_PKTHDR;
-   memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
-   m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
+   int len = m->m_pkthdr.len;
+
+   KASSERT(m->m_flags & M_PKTHDR);
+   m->m_flags &= (M_EXT|M_PKTHDR|M_EOR|M_EXTWR|M_ZEROIZE);
 
-   /* also delete all mbuf tags to reset the state */
+   /* delete all mbuf tags to reset the state */
m_tag_delete_chain(m);
+
+   /* like m_inithdr(), but keep any associated data and mbufs */
+   memset(>m_pkthdr, 0, sizeof(m->m_pkthdr));
+   m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
+   m->m_pkthdr.len = len;
 }
 
 struct mbuf *
Index: kern/uipc_socket.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.142
diff -u -p -r1.142 uipc_socket.c
--- kern/uipc_socket.c  24 Aug 2015 14:28:25 -  1.142
+++ kern/uipc_socket.c  30 Oct 2015 17:06:57 -
@@ -1325,10 +1325,8 @@ somove(struct socket *so, int wait)
goto release;
m->m_nextpkt = NULL;
if (m->m_flags & M_PKTHDR) {
-   m_tag_delete_chain(m);
-   memset(>m_pkthdr, 0, sizeof(m->m_pkthdr));
+   m_resethdr(m);
m->m_pkthdr.len = len;
-   m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
}
 
/* Send window update to source peer as receive buffer has changed. */

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Reyk Floeter]

On Fri, Oct 30, 2015 at 06:16:53PM +0100, Sebastian Benoit wrote:
> 
> i think it should be documented ;)
> 
> otherwise ok
> 

Ooops, good point, I missed the manpage.

It looks about right, but maybe it is better to have it less pf-
specific (also regarding bluhm's update)?

Otherwise OK

Reyk

> Index: mbuf.9
> ===
> RCS file: /cvs/src/share/man/man9/mbuf.9,v
> retrieving revision 1.91
> diff -u -p -u -r1.91 mbuf.9
> --- mbuf.98 Oct 2015 14:09:34 -   1.91
> +++ mbuf.930 Oct 2015 17:15:40 -
> @@ -44,6 +44,8 @@
>  .Fn MGET "struct mbuf *m" "int how" "int type"
>  .Ft struct mbuf *
>  .Fn m_getclr "int how" "int type"
> +.Ft void
> +.Fn m_resethdr struct mbuf *
>  .Ft struct mbuf *
>  .Fn m_gethdr "int how" "int type"
>  .Fn MGETHDR "struct mbuf *m" "int how" "int type"
> @@ -445,6 +447,11 @@ See
>  .Fn m_get
>  for a description of
>  .Fa how .
> +.It Fn m_resethdr "struct mbuf *"
> +Deletes all
> +.Xr pf 4
> +data and all tags attached to packet
> +.Fa mbuf .
>  .It Fn m_gethdr "int how" "int type"
>  Return a pointer to an mbuf of the type specified after initializing
>  it to contain a packet header.
> 
> 
> Reyk Floeter(r...@openbsd.org) on 2015.10.30 14:04:52 +0100:
> > On Fri, Oct 30, 2015 at 01:40:19PM +0100, Alexander Bluhm wrote:
> > > On Fri, Oct 30, 2015 at 12:56:34PM +0100, Reyk Floeter wrote:
> > > > --- sys/sys/mbuf.h  22 Oct 2015 05:26:06 -  1.198
> > > > +++ sys/sys/mbuf.h  30 Oct 2015 11:30:33 -
> > > > @@ -410,6 +410,7 @@ struct  mbuf *m_get(int, int);
> > > >  struct mbuf *m_getclr(int, int);
> > > >  struct mbuf *m_gethdr(int, int);
> > > >  struct mbuf *m_inithdr(struct mbuf *);
> > > > +void   m_resethdr(struct mbuf *);
> > > >  int  m_defrag(struct mbuf *, int);
> > > 
> > > The m_resethdr should have the same indent as m_defrag.
> > > 
> > 
> > Really?  The indent of m_defrag() is wrong - many spaces and not like
> > the other int functions further down - but I didn't dare to touch it.
> > It sticks in the eye, so I'll fix m_defrag indent as well.
> > 
> > > > --- sys/net/if_pair.c   25 Oct 2015 12:59:57 -  1.4
> > > > +++ sys/net/if_pair.c   30 Oct 2015 11:30:33 -
> > > > @@ -30,6 +30,11 @@
> > > >  #include 
> > > >  #include 
> > > >  
> > > > +#include "pf.h"
> > > > +#if NPF > 0
> > > > +#include 
> > > > +#endif
> > > 
> > > I think you don't need that anymore.
> > > 
> > > > @@ -182,9 +187,13 @@ pairstart(struct ifnet *ifp)
> > > >  #endif /* NBPFILTER > 0 */
> > > >  
> > > > ifp->if_opackets++;
> > > > -   if (pairedifp != NULL)
> > > > +   if (pairedifp != NULL) {
> > > > +#if NPF > 0
> > > > +   if (m->m_flags & M_PKTHDR)
> > > > +   m_resethdr(m);
> > > > +#endif
> > > 
> > > Calling m_tag_delete_chain() is not pf specific, so I would do it
> > > without the #if NPF.
> > > 
> > > Otherwise OK bluhm@
> > > 
> > > Socket splicing somove() does the same thing.  I will change it to
> > > use m_resethdr() after that got commited.
> > 
> > Cool.
> > 
> > I'm going to commit the attached diff for now.
> > 
> > Reyk
> > 
> > Index: sys/kern/uipc_mbuf.c
> > ===
> > RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
> > retrieving revision 1.208
> > diff -u -p -u -p -r1.208 uipc_mbuf.c
> > --- sys/kern/uipc_mbuf.c22 Oct 2015 05:26:06 -  1.208
> > +++ sys/kern/uipc_mbuf.c30 Oct 2015 12:45:50 -
> > @@ -250,6 +250,18 @@ m_inithdr(struct mbuf *m)
> > return (m);
> >  }
> >  
> > +void
> > +m_resethdr(struct mbuf *m)
> > +{
> > +   /* like the previous, but keep any associated data and mbufs */
> > +   m->m_flags = M_PKTHDR;
> > +   memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> > +   m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
> > +
> > +   /* also delete all mbuf tags to reset the state */
> > +   m_tag_delete_chain(m);
> > +}
> > +
> >  struct mbuf *
> >  m_getclr(int nowait, int type)
> >  {
> > Index: sys/sys/mbuf.h
> > ===
> > RCS file: /cvs/src/sys/sys/mbuf.h,v
> > retrieving revision 1.198
> > diff -u -p -u -p -r1.198 mbuf.h
> > --- sys/sys/mbuf.h  22 Oct 2015 05:26:06 -  1.198
> > +++ sys/sys/mbuf.h  30 Oct 2015 12:45:50 -
> > @@ -410,7 +410,8 @@ struct  mbuf *m_get(int, int);
> >  struct mbuf *m_getclr(int, int);
> >  struct mbuf *m_gethdr(int, int);
> >  struct mbuf *m_inithdr(struct mbuf *);
> > -int  m_defrag(struct mbuf *, int);
> > +void   m_resethdr(struct mbuf *);
> > +intm_defrag(struct mbuf *, int);
> >  struct mbuf *m_prepend(struct mbuf *, int, int);
> >  struct mbuf *m_pulldown(struct mbuf *, int, int, int *);
> >  struct mbuf *m_pullup(struct mbuf *, int);
> > Index: sys/net/if_pair.c
> > ===

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Mike Belopuhov]

On Fri, Oct 30, 2015 at 19:05 +0100, Alexander Bluhm wrote:
> On Fri, Oct 30, 2015 at 06:48:16PM +0100, Mike Belopuhov wrote:
> > On Fri, Oct 30, 2015 at 18:27 +0100, Alexander Bluhm wrote:
> > > On Fri, Oct 30, 2015 at 02:04:52PM +0100, Reyk Floeter wrote:
> > > > > Socket splicing somove() does the same thing.  I will change it to
> > > > > use m_resethdr() after that got commited.
> > > 
> > > I just compared code in somove() with m_resethdr().  Socket splicing
> > > has to clear the whole packet header, not only the pf part.  I think
> > > this is also useful for pair(4) as it should behave like a cable.
> > > 
> > > Resetting the other m_flags seems wrong.  I have put the M_PKTHDR
> > > into an assert.  If it is not an packet header, the memset() could
> > > overwrite mbuf data.
> > > 
> > > ok?
> > >
> > 
> > Resetting flags that are set on input (e.g. M_CONF, M_AUTH, etc.) is
> > not wrong since input on the other pair has not happened yet and it
> > might provide more problems than shortcuts.  I'm not certain which
> > flags may still be of use before the other pair figures it on its
> > own.  Do you have any examples of such flags?
> 
> I think flags that must be preserverd are
> - M_ZEROIZE
> - M_EXT
> 
> In sys/mbuf.h Flags are split into mbuf flags and mbuf pkthdr flags.
> As we clear the packet header but keep the data, we should probably
> only clear the mbuf pkthdr flags.
> 
> Something like this flag combination?
>

Yes, please. OK mikeb

> bluhm
> 
> Index: kern/uipc_mbuf.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_mbuf.c,v
> retrieving revision 1.209
> diff -u -p -r1.209 uipc_mbuf.c
> --- kern/uipc_mbuf.c  30 Oct 2015 12:54:36 -  1.209
> +++ kern/uipc_mbuf.c  30 Oct 2015 18:00:59 -
> @@ -253,13 +253,18 @@ m_inithdr(struct mbuf *m)
>  void
>  m_resethdr(struct mbuf *m)
>  {
> - /* like the previous, but keep any associated data and mbufs */
> - m->m_flags = M_PKTHDR;
> - memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> - m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
> + int len = m->m_pkthdr.len;
> +
> + KASSERT(m->m_flags & M_PKTHDR);
> + m->m_flags &= (M_EXT|M_PKTHDR|M_EOR|M_EXTWR|M_ZEROIZE);
>  
> - /* also delete all mbuf tags to reset the state */
> + /* delete all mbuf tags to reset the state */
>   m_tag_delete_chain(m);
> +
> + /* like m_inithdr(), but keep any associated data and mbufs */
> + memset(>m_pkthdr, 0, sizeof(m->m_pkthdr));
> + m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
> + m->m_pkthdr.len = len;
>  }
>  
>  struct mbuf *
> Index: kern/uipc_socket.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.142
> diff -u -p -r1.142 uipc_socket.c
> --- kern/uipc_socket.c24 Aug 2015 14:28:25 -  1.142
> +++ kern/uipc_socket.c30 Oct 2015 17:06:57 -
> @@ -1325,10 +1325,8 @@ somove(struct socket *so, int wait)
>   goto release;
>   m->m_nextpkt = NULL;
>   if (m->m_flags & M_PKTHDR) {
> - m_tag_delete_chain(m);
> - memset(>m_pkthdr, 0, sizeof(m->m_pkthdr));
> + m_resethdr(m);
>   m->m_pkthdr.len = len;
> - m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
>   }
>  
>   /* Send window update to source peer as receive buffer has changed. */

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Mike Belopuhov]

On Fri, Oct 30, 2015 at 18:27 +0100, Alexander Bluhm wrote:
> On Fri, Oct 30, 2015 at 02:04:52PM +0100, Reyk Floeter wrote:
> > > Socket splicing somove() does the same thing.  I will change it to
> > > use m_resethdr() after that got commited.
> 
> I just compared code in somove() with m_resethdr().  Socket splicing
> has to clear the whole packet header, not only the pf part.  I think
> this is also useful for pair(4) as it should behave like a cable.
> 
> Resetting the other m_flags seems wrong.  I have put the M_PKTHDR
> into an assert.  If it is not an packet header, the memset() could
> overwrite mbuf data.
> 
> ok?
>

Resetting flags that are set on input (e.g. M_CONF, M_AUTH, etc.) is
not wrong since input on the other pair has not happened yet and it
might provide more problems than shortcuts.  I'm not certain which
flags may still be of use before the other pair figures it on its
own.  Do you have any examples of such flags?

Otherwise I like the diff.

> bluhm
> 
> Index: kern/uipc_mbuf.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_mbuf.c,v
> retrieving revision 1.209
> diff -u -p -r1.209 uipc_mbuf.c
> --- kern/uipc_mbuf.c  30 Oct 2015 12:54:36 -  1.209
> +++ kern/uipc_mbuf.c  30 Oct 2015 17:13:58 -
> @@ -253,13 +253,17 @@ m_inithdr(struct mbuf *m)
>  void
>  m_resethdr(struct mbuf *m)
>  {
> - /* like the previous, but keep any associated data and mbufs */
> - m->m_flags = M_PKTHDR;
> - memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> - m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
> + int len = m->m_pkthdr.len;
> +
> + KASSERT(m->m_flags & M_PKTHDR);
>  
> - /* also delete all mbuf tags to reset the state */
> + /* delete all mbuf tags to reset the state */
>   m_tag_delete_chain(m);
> +
> + /* like m_inithdr(), but keep any associated data and mbufs */
> + memset(>m_pkthdr, 0, sizeof(m->m_pkthdr));
> + m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
> + m->m_pkthdr.len = len;
>  }
>  
>  struct mbuf *
> Index: kern/uipc_socket.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.142
> diff -u -p -r1.142 uipc_socket.c
> --- kern/uipc_socket.c24 Aug 2015 14:28:25 -  1.142
> +++ kern/uipc_socket.c30 Oct 2015 17:06:57 -
> @@ -1325,10 +1325,8 @@ somove(struct socket *so, int wait)
>   goto release;
>   m->m_nextpkt = NULL;
>   if (m->m_flags & M_PKTHDR) {
> - m_tag_delete_chain(m);
> - memset(>m_pkthdr, 0, sizeof(m->m_pkthdr));
> + m_resethdr(m);
>   m->m_pkthdr.len = len;
> - m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
>   }
>  
>   /* Send window update to source peer as receive buffer has changed. */

Re: pair(4) + pf(4): reset all state on "reinjected" packets

[Reyk Floeter]

On Fri, Oct 30, 2015 at 07:05:09PM +0100, Alexander Bluhm wrote:
> On Fri, Oct 30, 2015 at 06:48:16PM +0100, Mike Belopuhov wrote:
> > On Fri, Oct 30, 2015 at 18:27 +0100, Alexander Bluhm wrote:
> > > On Fri, Oct 30, 2015 at 02:04:52PM +0100, Reyk Floeter wrote:
> > > > > Socket splicing somove() does the same thing.  I will change it to
> > > > > use m_resethdr() after that got commited.
> > > 
> > > I just compared code in somove() with m_resethdr().  Socket splicing
> > > has to clear the whole packet header, not only the pf part.  I think
> > > this is also useful for pair(4) as it should behave like a cable.
> > > 
> > > Resetting the other m_flags seems wrong.  I have put the M_PKTHDR
> > > into an assert.  If it is not an packet header, the memset() could
> > > overwrite mbuf data.
> > > 
> > > ok?
> > >
> > 
> > Resetting flags that are set on input (e.g. M_CONF, M_AUTH, etc.) is
> > not wrong since input on the other pair has not happened yet and it
> > might provide more problems than shortcuts.  I'm not certain which
> > flags may still be of use before the other pair figures it on its
> > own.  Do you have any examples of such flags?
> 
> I think flags that must be preserverd are
> - M_ZEROIZE
> - M_EXT
> 
> In sys/mbuf.h Flags are split into mbuf flags and mbuf pkthdr flags.
> As we clear the packet header but keep the data, we should probably
> only clear the mbuf pkthdr flags.
> 
> Something like this flag combination?
> 
> bluhm
> 

Yes, OK reyk@

tested with pair(4) ... ipsec on pair(4) ... routed ipsec on pair(4) ...
(pair0 -> ipsec -> pair1 -> $ext_if) ... bridge/pair stp ...

> Index: kern/uipc_mbuf.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_mbuf.c,v
> retrieving revision 1.209
> diff -u -p -r1.209 uipc_mbuf.c
> --- kern/uipc_mbuf.c  30 Oct 2015 12:54:36 -  1.209
> +++ kern/uipc_mbuf.c  30 Oct 2015 18:00:59 -
> @@ -253,13 +253,18 @@ m_inithdr(struct mbuf *m)
>  void
>  m_resethdr(struct mbuf *m)
>  {
> - /* like the previous, but keep any associated data and mbufs */
> - m->m_flags = M_PKTHDR;
> - memset(>m_pkthdr.pf, 0, sizeof(m->m_pkthdr.pf));
> - m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
> + int len = m->m_pkthdr.len;
> +
> + KASSERT(m->m_flags & M_PKTHDR);
> + m->m_flags &= (M_EXT|M_PKTHDR|M_EOR|M_EXTWR|M_ZEROIZE);
>  
> - /* also delete all mbuf tags to reset the state */
> + /* delete all mbuf tags to reset the state */
>   m_tag_delete_chain(m);
> +
> + /* like m_inithdr(), but keep any associated data and mbufs */
> + memset(>m_pkthdr, 0, sizeof(m->m_pkthdr));
> + m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
> + m->m_pkthdr.len = len;
>  }
>  
>  struct mbuf *
> Index: kern/uipc_socket.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.142
> diff -u -p -r1.142 uipc_socket.c
> --- kern/uipc_socket.c24 Aug 2015 14:28:25 -  1.142
> +++ kern/uipc_socket.c30 Oct 2015 17:06:57 -
> @@ -1325,10 +1325,8 @@ somove(struct socket *so, int wait)
>   goto release;
>   m->m_nextpkt = NULL;
>   if (m->m_flags & M_PKTHDR) {
> - m_tag_delete_chain(m);
> - memset(>m_pkthdr, 0, sizeof(m->m_pkthdr));
> + m_resethdr(m);
>   m->m_pkthdr.len = len;
> - m->m_pkthdr.pf.prio = IFQ_DEFPRIO;
>   }
>  
>   /* Send window update to source peer as receive buffer has changed. */

-- 

Re: Stop using rt_ifp in nd6*

[Alexander Bluhm]

On Thu, Oct 29, 2015 at 03:54:29PM +0100, Martin Pieuchot wrote:
> When we already had a valid ``ifp'' I used it.  Since defrouter_lookup()
> is only doing a comparison, let's use interface indexes.
> 
> ok?

OK bluhm@

> Index: netinet6/nd6.c
> ===
> RCS file: /cvs/src/sys/netinet6/nd6.c,v
> retrieving revision 1.166
> diff -u -p -r1.166 nd6.c
> --- netinet6/nd6.c29 Oct 2015 14:28:34 -  1.166
> +++ netinet6/nd6.c29 Oct 2015 14:49:03 -
> @@ -658,7 +658,7 @@ nd6_lookup(struct in6_addr *addr6, int c
>*/
>   if ((rt->rt_flags & RTF_GATEWAY) || (rt->rt_flags & RTF_LLINFO) == 0 ||
>   rt->rt_gateway->sa_family != AF_LINK || rt->rt_llinfo == NULL ||
> - (ifp != NULL && rt->rt_ifp != ifp)) {
> + (ifp != NULL && rt->rt_ifidx != ifp->if_index)) {
>   if (create) {
>   char addr[INET6_ADDRSTRLEN];
>   nd6log((LOG_DEBUG, "%s: failed to lookup %s (if=%s)\n",
> @@ -751,17 +751,19 @@ nd6_free(struct rtentry *rt, int gc)
>   struct llinfo_nd6 *ln = (struct llinfo_nd6 *)rt->rt_llinfo, *next;
>   struct in6_addr in6 = satosin6(rt_key(rt))->sin6_addr;
>   struct nd_defrouter *dr;
> + struct ifnet *ifp;
>   int s;
>  
>   /*
>* we used to have pfctlinput(PRC_HOSTDEAD) here.
>* even though it is not harmful, it was not really necessary.
>*/
> + ifp = if_get(rt->rt_ifidx);
>  
>   s = splsoftnet();
>   if (!ip6_forwarding) {
>   dr = defrouter_lookup((rt_key(rt))->sin6_addr,
> - rt->rt_ifp);
> + rt->rt_ifidx);
>  
>   if (dr != NULL && dr->expire &&
>   ln->ln_state == ND6_LLINFO_STALE && gc) {
> @@ -783,6 +785,7 @@ nd6_free(struct rtentry *rt, int gc)
>   } else
>   nd6_llinfo_settimer(ln, (long)nd6_gctimer * hz);
>   splx(s);
> + if_put(ifp);
>   return (ln->ln_next);
>   }
>  
> @@ -792,7 +795,7 @@ nd6_free(struct rtentry *rt, int gc)
>* is in the Default Router List.
>* See a corresponding comment in nd6_na_input().
>*/
> - rt6_flush(, rt->rt_ifp);
> + rt6_flush(, ifp);
>   }
>  
>   if (dr) {
> @@ -839,9 +842,11 @@ nd6_free(struct rtentry *rt, int gc)
>* caches, and disable the route entry not to be used in already
>* cached routes.
>*/
> - rtdeletemsg(rt, rt->rt_ifp->if_rdomain);
> + rtdeletemsg(rt, ifp->if_rdomain);
>   splx(s);
>  
> + if_put(ifp);
> +
>   return (next);
>  }
>  
> @@ -899,7 +904,8 @@ nd6_rtrequest(struct ifnet *ifp, int req
>   _any) && rt_mask(rt) && (rt_mask(rt)->sa_len == 0 ||
>   IN6_ARE_ADDR_EQUAL(&(satosin6(rt_mask(rt)))->sin6_addr,
>   _any {
> - dr = defrouter_lookup((gate)->sin6_addr, ifp);
> + dr = defrouter_lookup((gate)->sin6_addr,
> + ifp->if_index);
>   if (dr)
>   dr->installed = 0;
>   }
> Index: netinet6/nd6.h
> ===
> RCS file: /cvs/src/sys/netinet6/nd6.h,v
> retrieving revision 1.52
> diff -u -p -r1.52 nd6.h
> --- netinet6/nd6.h28 Oct 2015 12:14:25 -  1.52
> +++ netinet6/nd6.h29 Oct 2015 14:42:35 -
> @@ -299,7 +299,7 @@ int prelist_update(struct nd_prefix *, s
>  int nd6_prelist_add(struct nd_prefix *, struct nd_defrouter *,
>   struct nd_prefix **);
>  void pfxlist_onlink_check(void);
> -struct nd_defrouter *defrouter_lookup(struct in6_addr *, struct ifnet *);
> +struct nd_defrouter *defrouter_lookup(struct in6_addr *, unsigned int);
>  
>  struct nd_prefix *nd6_prefix_lookup(struct nd_prefix *);
>  int in6_ifdel(struct ifnet *, struct in6_addr *);
> Index: netinet6/nd6_nbr.c
> ===
> RCS file: /cvs/src/sys/netinet6/nd6_nbr.c,v
> retrieving revision 1.97
> diff -u -p -r1.97 nd6_nbr.c
> --- netinet6/nd6_nbr.c22 Oct 2015 15:37:47 -  1.97
> +++ netinet6/nd6_nbr.c29 Oct 2015 14:47:42 -
> @@ -730,7 +730,7 @@ nd6_na_input(struct mbuf *m, int off, in
>   ln->ln_byhint = 0;
>   if (!ND6_LLINFO_PERMANENT(ln)) {
>   nd6_llinfo_settimer(ln,
> - (long)ND_IFINFO(rt->rt_ifp)->reachable * 
> hz);
> + (long)ND_IFINFO(ifp)->reachable * hz);
>   }
>   } else {
>   ln->ln_state = ND6_LLINFO_STALE;
> @@ -851,7 +851,7 @@ nd6_na_input(struct mbuf *m, int off, in
>* context.  However, we keep it just for safety.
>  

Re: toy zones for openbsd - an undergrad operating systems course assignment

[Kristaps Dzonsons]

> however, i found it interesting to get my head around this aspect
> of the system, and i figured other people (such as this years
> comp3301 students) would be interested too. i also felt sad i couldnt
> find kritaps mult code anywhere, so i wanted this to be backed up
> by everyone for future posterity.

Dug up and assembled:

 http://kristaps.bsd.lv/mult

(The NetBSD version is more in-depth, but that'll take longer to dig up.)

Best,

Kristaps