Re: nc: missing rpath pledge for -P
On Sat, Jun 10, 2017 at 12:45:01AM +0200, Theo Buehler wrote:
> On Fri, Jun 09, 2017 at 11:59:44PM +0200, Theo Buehler wrote:
> > On Fri, Jun 09, 2017 at 11:55:26PM +0200, Adam Wolk wrote:
> > > On Fri, Jun 09, 2017 at 11:54:03PM +0200, Adam Wolk wrote:
> > > > On Fri, Jun 09, 2017 at 09:28:29PM +, ra...@openmailbox.org wrote:
> > > > > Hello!
> > > > >
> > > > > Here is a patch with a pledge bugfix in netcat and some minor style
> > > > > improvements.
> > > > >
> > > > > An example of how to trigger the bug:
> > > > >
> > > > > $ nc -Ptest -v -c blog.tintagel.pl 443
> > > > > nc: pledge: Operation not permitted
> > > > >
> > > > > credits to
> > > > > * awolk@ for drawing attention to netcat.
> > > > > * Juuso Lapinlampi for suggesting to alphabetically order the
> > > > > #includes.
> > > > > * rajak for pointing out the missing space in the error message.
> > > > > * brynet for pledge style improvements.
> > > > >
> > > > >
> > > >
> > > > OK awolk@ for the updated diff (I'm attaching it inline).
> > >
> > > forgot the diff
> >
> > I'm ok with the diff, although I really wish there was a way to simplify
> > the convoluted mess that is the pledge logic in this program.
> >
> > How many codepaths are there in which the second group of pledge calls
> > actually does anything? Are there any?
> >
>
> The first group of pledges is this:
>
> if (family == AF_UNIX) { // if (usetls) -> bail out on line 393
> if (pledge("stdio rpath wpath cpath tmppath unix", NULL) == -1)
> err(1, "pledge");
> } else if (Fflag) { // if (usetls) -> bail out on line 397
> if (Pflag) {
> if (pledge("stdio inet dns sendfd tty", NULL) == -1)
> err(1, "pledge");
> } else if (pledge("stdio inet dns sendfd", NULL) == -1)
> err(1, "pledge");
> } else if (Pflag) {
> if (pledge("stdio inet dns tty", NULL) == -1)
> err(1, "pledge");
> } else if (usetls) {
> if (pledge("stdio rpath inet dns", NULL) == -1)
> err(1, "pledge");
> } else if (pledge("stdio inet dns", NULL) == -1)
> err(1, "pledge");
>
> So we can only reach the second group of pledges if usetls is set.
> Thus, I think the following diff would be better:
>
OK with me as discussed on IRC.
Re: nc: missing rpath pledge for -P
On Fri, Jun 09, 2017 at 11:59:44PM +0200, Theo Buehler wrote:
> On Fri, Jun 09, 2017 at 11:55:26PM +0200, Adam Wolk wrote:
> > On Fri, Jun 09, 2017 at 11:54:03PM +0200, Adam Wolk wrote:
> > > On Fri, Jun 09, 2017 at 09:28:29PM +, ra...@openmailbox.org wrote:
> > > > Hello!
> > > >
> > > > Here is a patch with a pledge bugfix in netcat and some minor style
> > > > improvements.
> > > >
> > > > An example of how to trigger the bug:
> > > >
> > > > $ nc -Ptest -v -c blog.tintagel.pl 443
> > > > nc: pledge: Operation not permitted
> > > >
> > > > credits to
> > > > * awolk@ for drawing attention to netcat.
> > > > * Juuso Lapinlampi for suggesting to alphabetically order the #includes.
> > > > * rajak for pointing out the missing space in the error message.
> > > > * brynet for pledge style improvements.
> > > >
> > > >
> > >
> > > OK awolk@ for the updated diff (I'm attaching it inline).
> >
> > forgot the diff
>
> I'm ok with the diff, although I really wish there was a way to simplify
> the convoluted mess that is the pledge logic in this program.
>
> How many codepaths are there in which the second group of pledge calls
> actually does anything? Are there any?
>
The first group of pledges is this:
if (family == AF_UNIX) { // if (usetls) -> bail out on line 393
if (pledge("stdio rpath wpath cpath tmppath unix", NULL) == -1)
err(1, "pledge");
} else if (Fflag) { // if (usetls) -> bail out on line 397
if (Pflag) {
if (pledge("stdio inet dns sendfd tty", NULL) == -1)
err(1, "pledge");
} else if (pledge("stdio inet dns sendfd", NULL) == -1)
err(1, "pledge");
} else if (Pflag) {
if (pledge("stdio inet dns tty", NULL) == -1)
err(1, "pledge");
} else if (usetls) {
if (pledge("stdio rpath inet dns", NULL) == -1)
err(1, "pledge");
} else if (pledge("stdio inet dns", NULL) == -1)
err(1, "pledge");
So we can only reach the second group of pledges if usetls is set.
Thus, I think the following diff would be better:
Index: netcat.c
===
RCS file: /var/cvs/src/usr.bin/nc/netcat.c,v
retrieving revision 1.183
diff -u -p -r1.183 netcat.c
--- netcat.c26 May 2017 16:05:35 - 1.183
+++ netcat.c9 Jun 2017 22:36:28 -
@@ -355,6 +355,9 @@ main(int argc, char *argv[])
err(1, "pledge");
} else if (pledge("stdio inet dns sendfd", NULL) == -1)
err(1, "pledge");
+ } else if (Pflag && usetls) {
+ if (pledge("stdio rpath inet dns tty", NULL) == -1)
+ err(1, "pledge");
} else if (Pflag) {
if (pledge("stdio inet dns tty", NULL) == -1)
err(1, "pledge");
@@ -478,12 +481,6 @@ main(int argc, char *argv[])
}
if (usetls) {
- if (Pflag) {
- if (pledge("stdio inet dns tty rpath", NULL) == -1)
- err(1, "pledge");
- } else if (pledge("stdio inet dns rpath", NULL) == -1)
- err(1, "pledge");
-
if (tls_init() == -1)
errx(1, "unable to initialize TLS");
if ((tls_cfg = tls_config_new()) == NULL)
@@ -510,7 +507,7 @@ main(int argc, char *argv[])
if (TLSopt & TLS_NOVERIFY) {
if (tls_expecthash != NULL)
errx(1, "-H and -T noverify may not be used"
- "together");
+ " together");
tls_config_insecure_noverifycert(tls_cfg);
}
if (TLSopt & TLS_MUSTSTAPLE)
Re: nc: missing rpath pledge for -P
On Fri, Jun 09, 2017 at 11:55:26PM +0200, Adam Wolk wrote:
> On Fri, Jun 09, 2017 at 11:54:03PM +0200, Adam Wolk wrote:
> > On Fri, Jun 09, 2017 at 09:28:29PM +, ra...@openmailbox.org wrote:
> > > Hello!
> > >
> > > Here is a patch with a pledge bugfix in netcat and some minor style
> > > improvements.
> > >
> > > An example of how to trigger the bug:
> > >
> > > $ nc -Ptest -v -c blog.tintagel.pl 443
> > > nc: pledge: Operation not permitted
> > >
> > > credits to
> > > * awolk@ for drawing attention to netcat.
> > > * Juuso Lapinlampi for suggesting to alphabetically order the #includes.
> > > * rajak for pointing out the missing space in the error message.
> > > * brynet for pledge style improvements.
> > >
> > >
> >
> > OK awolk@ for the updated diff (I'm attaching it inline).
>
> forgot the diff
I'm ok with the diff, although I really wish there was a way to simplify
the convoluted mess that is the pledge logic in this program.
How many codepaths are there in which the second group of pledge calls
actually does anything? Are there any?
> Index: usr.bin/nc/netcat.c
> ===
> RCS file: /cvs/src/usr.bin/nc/netcat.c,v
> retrieving revision 1.178
> diff -u -p -u -p -r1.178 netcat.c
> --- usr.bin/nc/netcat.c 9 Mar 2017 13:58:00 - 1.178
> +++ usr.bin/nc/netcat.c 9 Jun 2017 21:16:25 -
> @@ -53,8 +53,8 @@
> #include
> #include
> #include
> -#include
> #include
> +#include
> #include "atomicio.h"
>
> #define PORT_MAX 65535
> @@ -340,7 +340,7 @@ main(int argc, char *argv[])
> } else if (pledge("stdio inet dns sendfd", NULL) == -1)
> err(1, "pledge");
> } else if (Pflag) {
> - if (pledge("stdio inet dns tty", NULL) == -1)
> + if (pledge("stdio rpath inet dns tty", NULL) == -1)
> err(1, "pledge");
> } else if (usetls) {
> if (pledge("stdio rpath inet dns", NULL) == -1)
> @@ -461,9 +461,9 @@ main(int argc, char *argv[])
>
> if (usetls) {
> if (Pflag) {
> - if (pledge("stdio inet dns tty rpath", NULL) == -1)
> + if (pledge("stdio rpath inet dns tty", NULL) == -1)
> err(1, "pledge");
> - } else if (pledge("stdio inet dns rpath", NULL) == -1)
> + } else if (pledge("stdio rpath inet dns", NULL) == -1)
> err(1, "pledge");
>
> if (tls_init() == -1)
> @@ -492,7 +492,7 @@ main(int argc, char *argv[])
> if (TLSopt & TLS_NOVERIFY) {
> if (tls_expecthash != NULL)
> errx(1, "-H and -T noverify may not be used"
> - "together");
> + " together");
> tls_config_insecure_noverifycert(tls_cfg);
> }
> if (TLSopt & TLS_MUSTSTAPLE)
Re: nc: missing rpath pledge for -P
On Fri, Jun 09, 2017 at 11:54:03PM +0200, Adam Wolk wrote:
> On Fri, Jun 09, 2017 at 09:28:29PM +, ra...@openmailbox.org wrote:
> > Hello!
> >
> > Here is a patch with a pledge bugfix in netcat and some minor style
> > improvements.
> >
> > An example of how to trigger the bug:
> >
> > $ nc -Ptest -v -c blog.tintagel.pl 443
> > nc: pledge: Operation not permitted
> >
> > credits to
> > * awolk@ for drawing attention to netcat.
> > * Juuso Lapinlampi for suggesting to alphabetically order the #includes.
> > * rajak for pointing out the missing space in the error message.
> > * brynet for pledge style improvements.
> >
> >
>
> OK awolk@ for the updated diff (I'm attaching it inline).
forgot the diff
Index: usr.bin/nc/netcat.c
===
RCS file: /cvs/src/usr.bin/nc/netcat.c,v
retrieving revision 1.178
diff -u -p -u -p -r1.178 netcat.c
--- usr.bin/nc/netcat.c 9 Mar 2017 13:58:00 - 1.178
+++ usr.bin/nc/netcat.c 9 Jun 2017 21:16:25 -
@@ -53,8 +53,8 @@
#include
#include
#include
-#include
#include
+#include
#include "atomicio.h"
#define PORT_MAX 65535
@@ -340,7 +340,7 @@ main(int argc, char *argv[])
} else if (pledge("stdio inet dns sendfd", NULL) == -1)
err(1, "pledge");
} else if (Pflag) {
- if (pledge("stdio inet dns tty", NULL) == -1)
+ if (pledge("stdio rpath inet dns tty", NULL) == -1)
err(1, "pledge");
} else if (usetls) {
if (pledge("stdio rpath inet dns", NULL) == -1)
@@ -461,9 +461,9 @@ main(int argc, char *argv[])
if (usetls) {
if (Pflag) {
- if (pledge("stdio inet dns tty rpath", NULL) == -1)
+ if (pledge("stdio rpath inet dns tty", NULL) == -1)
err(1, "pledge");
- } else if (pledge("stdio inet dns rpath", NULL) == -1)
+ } else if (pledge("stdio rpath inet dns", NULL) == -1)
err(1, "pledge");
if (tls_init() == -1)
@@ -492,7 +492,7 @@ main(int argc, char *argv[])
if (TLSopt & TLS_NOVERIFY) {
if (tls_expecthash != NULL)
errx(1, "-H and -T noverify may not be used"
- "together");
+ " together");
tls_config_insecure_noverifycert(tls_cfg);
}
if (TLSopt & TLS_MUSTSTAPLE)
Re: nc: missing rpath pledge for -P
On Fri, Jun 09, 2017 at 09:28:29PM +, ra...@openmailbox.org wrote: > Hello! > > Here is a patch with a pledge bugfix in netcat and some minor style > improvements. > > An example of how to trigger the bug: > > $ nc -Ptest -v -c blog.tintagel.pl 443 > nc: pledge: Operation not permitted > > credits to > * awolk@ for drawing attention to netcat. > * Juuso Lapinlampi for suggesting to alphabetically order the #includes. > * rajak for pointing out the missing space in the error message. > * brynet for pledge style improvements. > > OK awolk@ for the updated diff (I'm attaching it inline). Would like a second OK on this. Testing results, pre diff: $ nc -H 123 -T noverify -c localhost 22 nc: -H and -T noverify may not be usedtogether $ nc -Ptest -v -c blog.tintagel.pl 443 nc: pledge: Operation not permitted $ Post diff: $ ./nc -H 123 -T noverify -c localhost 22 nc: -H and -T noverify may not be used together $ ./nc -Ptest -v -c blog.tintagel.pl 443 Connection to blog.tintagel.pl 443 port [tcp/https] succeeded! TLS handshake negotiated TLSv1.2/ECDHE-RSA-AES256-GCM-SHA384 with host blog.tintagel.pl Peer name: blog.tintagel.pl Subject: /CN=tintagel.pl Issuer: /C=US/O=Let's Encrypt/CN=Let's Encrypt Authority X3 Valid From: Thu Apr 27 04:01:00 2017 Valid Until: Wed Jul 26 04:01:00 2017 Cert Hash: SHA256:1746b1d2ecdf8ad1fb7e06a6c97154b2c1a87eee65f5654824d0a0dc0af4ba98 OCSP URL: http://ocsp.int-x3.letsencrypt.org/ ^C $ Test on amd64 -current Regards, Adam
Re: nc: missing rpath pledge for -P
sorry! looks like my email client corrupted the patch, here it is hosted on a paste site https://paste.debian.net/970854/
nc: missing rpath pledge for -P
Hello!
Here is a patch with a pledge bugfix in netcat and some minor style
improvements.
An example of how to trigger the bug:
$ nc -Ptest -v -c blog.tintagel.pl 443
nc: pledge: Operation not permitted
credits to
* awolk@ for drawing attention to netcat.
* Juuso Lapinlampi for suggesting to alphabetically order the #includes.
* rajak for pointing out the missing space in the error message.
* brynet for pledge style improvements.
Index: usr.bin/nc/netcat.c
===
RCS file: /cvs/src/usr.bin/nc/netcat.c,v
retrieving revision 1.178
diff -u -p -u -p -r1.178 netcat.c
--- usr.bin/nc/netcat.c 9 Mar 2017 13:58:00 - 1.178
+++ usr.bin/nc/netcat.c 9 Jun 2017 21:16:25 -
@@ -53,8 +53,8 @@
#include
#include
#include
-#include
#include
+#include
#include "atomicio.h"
#define PORT_MAX 65535
@@ -340,7 +340,7 @@ main(int argc, char *argv[])
} else if (pledge("stdio inet dns sendfd", NULL) == -1)
err(1, "pledge");
} else if (Pflag) {
- if (pledge("stdio inet dns tty", NULL) == -1)
+ if (pledge("stdio rpath inet dns tty", NULL) == -1)
err(1, "pledge");
} else if (usetls) {
if (pledge("stdio rpath inet dns", NULL) == -1)
@@ -461,9 +461,9 @@ main(int argc, char *argv[])
if (usetls) {
if (Pflag) {
- if (pledge("stdio inet dns tty rpath", NULL) == -1)
+ if (pledge("stdio rpath inet dns tty", NULL) == -1)
err(1, "pledge");
- } else if (pledge("stdio inet dns rpath", NULL) == -1)
+ } else if (pledge("stdio rpath inet dns", NULL) == -1)
err(1, "pledge");
if (tls_init() == -1)
@@ -492,7 +492,7 @@ main(int argc, char *argv[])
if (TLSopt & TLS_NOVERIFY) {
if (tls_expecthash != NULL)
errx(1, "-H and -T noverify may not be used"
- "together");
+ " together");
tls_config_insecure_noverifycert(tls_cfg);
}
if (TLSopt & TLS_MUSTSTAPLE)
Re: sppp(4)/pppoe(4) dynamic address hack
Stefan Sperling: > inet 0.0.0.0 255.255.255.255 NONE \ > pppoedev em0 authproto pap \ > authname 'testcaller' authkey 'donttell' \ >dynaddr dyndest up > > Tested on my ADSL line and it works as expected. The example in the pppoe(4) man page also includes !/sbin/route add default -ifp pppoe0 0.0.0.1 to specify the default route. What happens to that? -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: brconfig: Unify/fix strtoul(3) handling
No need for temporary variables either, strtonum guarantees through
maxval that its return value won't overflow when casted.
Index: brconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.15
diff -u -p -r1.15 brconfig.c
--- brconfig.c 7 Jun 2017 16:47:29 - 1.15
+++ brconfig.c 9 Jun 2017 17:33:10 -
@@ -220,7 +220,6 @@ bridge_ifsetflag(const char *ifsname, u_
err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname);
req.ifbr_ifsflags |= flag & ~IFBIF_RO_MASK;
-
if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)) < 0)
err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname);
}
@@ -232,12 +231,10 @@ bridge_ifclrflag(const char *ifsname, u_
strlcpy(req.ifbr_name, name, sizeof(req.ifbr_name));
strlcpy(req.ifbr_ifsname, ifsname, sizeof(req.ifbr_ifsname));
-
if (ioctl(s, SIOCBRDGGIFFLGS, (caddr_t)) < 0)
err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname);
req.ifbr_ifsflags &= ~(flag | IFBIF_RO_MASK);
-
if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)) < 0)
err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname);
}
@@ -400,18 +397,13 @@ void
bridge_timeout(const char *arg, int d)
{
struct ifbrparam bp;
- long newtime;
- char *endptr;
+ const char *errstr;
- errno = 0;
- newtime = strtol(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' ||
- (newtime & ~INT_MAX) != 0L ||
- (errno == ERANGE && newtime == LONG_MAX))
- errx(1, "invalid arg for timeout: %s", arg);
+ bp.ifbrp_ctime = strtonum(arg, 0, INT_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for timeout: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
- bp.ifbrp_ctime = newtime;
if (ioctl(s, SIOCBRDGSTO, (caddr_t)) < 0)
err(1, "%s", name);
}
@@ -420,17 +412,13 @@ void
bridge_maxage(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for maxage: %s", arg);
+ bp.ifbrp_maxage = strtonum(arg, 0, UINT8_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for maxage: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
- bp.ifbrp_maxage = v;
if (ioctl(s, SIOCBRDGSMA, (caddr_t)) < 0)
err(1, "%s", name);
}
@@ -439,17 +427,13 @@ void
bridge_priority(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for spanpriority: %s", arg);
+ bp.ifbrp_prio = strtonum(arg, 0, UINT16_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for spanpriority: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
- bp.ifbrp_prio = v;
if (ioctl(s, SIOCBRDGSPRI, (caddr_t)) < 0)
err(1, "%s", name);
}
@@ -478,17 +462,13 @@ void
bridge_fwddelay(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for fwddelay: %s", arg);
+ bp.ifbrp_fwddelay = strtonum(arg, 0, UINT8_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for fwddelay: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
- bp.ifbrp_fwddelay = v;
if (ioctl(s, SIOCBRDGSFD, (caddr_t)) < 0)
err(1, "%s", name);
}
@@ -497,17 +477,13 @@ void
bridge_hellotime(const char *arg, int d)
{
struct ifbrparam bp;
- unsigned long v;
- char *endptr;
+ const char *errstr;
- errno = 0;
- v = strtoul(arg, , 0);
- if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
- (errno == ERANGE && v == ULONG_MAX))
- errx(1, "invalid arg for hellotime: %s", arg);
+ bp.ifbrp_hellotime = strtonum(arg, 0, UINT8_MAX, );
+ if (errstr != NULL)
+ errx(1, "%s arg for hellotime: %s", errstr, arg);
strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
- bp.ifbrp_hellotime = v;
if (ioctl(s, SIOCBRDGSHT, (caddr_t)) < 0)
err(1, "%s", name);
}
@@ -516,17 +492,13 @@ void
bridge_maxaddr(const char *arg, int d)
{
Re: sppp(4)/pppoe(4) dynamic address hack
On Fri, Jun 09, 2017 at 06:33:46PM +0200, Stefan Sperling wrote:
> Currently, sppp(4) interfaces use a destination address of 0.0.0.1 to
> indicate that the destination address should be assigned by the peer:
>
> inet 0.0.0.0 255.255.255.255 NONE \
> pppoedev em0 authproto pap \
> authname 'testcaller' authkey 'donttell' up
> dest 0.0.0.1
>
> As a side effect a route to 0.0.0.1 is added to the routing table.
>
> Apparently, in 5.7 this still used to work with mulitple pppoe interfaces
> which shared a routing table:
> http://marc.info/?l=openbsd-misc=149386503001033=2
>
> At present only one pppoe interface can be used in this way because
> setting a dest address of 0.0.0.1 on additional interfaces now causes
> an error: ifconfig: SIOCAIFADDR: File exists
>
> This diff changes the way dynamic addresses are configured in sppp(4).
> Instead of using magic IP addresses as wildcards, we allow userland to
> control behavior via new ifconfig command flags. 'dynaddr' enables a
> dynamic local address, and 'dyndest' enables a dynamic destination address.
> To disable either of these again, prepend a minus (e.g. -dynaddr).
>
> The above example becomes:
>
> inet 0.0.0.0 255.255.255.255 NONE \
> pppoedev em0 authproto pap \
> authname 'testcaller' authkey 'donttell' \
>dynaddr dyndest up
>
hi.
why do you have to specify 0.0.0.0 *and* dynaddr?
jmc
> Tested on my ADSL line and it works as expected.
>
> I cannot test it on multiple links until next week, but since no route
> for 0.0.0.1 is in the routing table anymore, I expect this to work.
>
> Index: sbin/ifconfig/ifconfig.8
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
> retrieving revision 1.282
> diff -u -p -r1.282 ifconfig.8
> --- sbin/ifconfig/ifconfig.8 12 May 2017 15:11:02 - 1.282
> +++ sbin/ifconfig/ifconfig.8 9 Jun 2017 16:02:23 -
> @@ -1393,6 +1393,8 @@ Clear a previously set service name.
> .Op Cm authkey Ar key
> .Op Cm authname Ar name
> .Op Cm authproto Ar proto
> +.Op Oo Fl Oc Ns Cm dynaddr
> +.Op Oo Fl Oc Ns Cm dyndest
> .Op Oo Fl Oc Ns Cm peerflag Ar flag
> .Op Cm peerkey Ar key
> .Op Cm peername Ar name
> @@ -1419,6 +1421,16 @@ The protocol name can be either
> or
> .Ql none .
> In the latter case, authentication will be turned off.
> +.It Cm dynaddr
> +The local address will be assigned by the peer.
> +.It Cm -dynaddr
> +Disable dynamic assignment of the local address.
> +This is the default.
> +.It Cm dyndest
> +The destination address will be assigned by the peer.
> +.It Cm -dyndest
> +Disable dynamic assignment of the destination address.
> +This is the default.
> .It Cm peerflag Ar flag
> Set a specified PPP flag for the remote authenticator.
> The flag name can be either
> Index: sbin/ifconfig/ifconfig.c
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> retrieving revision 1.342
> diff -u -p -r1.342 ifconfig.c
> --- sbin/ifconfig/ifconfig.c 5 Jun 2017 05:10:23 - 1.342
> +++ sbin/ifconfig/ifconfig.c 9 Jun 2017 15:57:40 -
> @@ -266,6 +266,9 @@ void setseername(const char *, int);
> void setseerkey(const char *, int);
> void setseerflag(const char *, int);
> void unsetseerflag(const char *, int);
> +void sipcpinfo(struct sipcpreq *);
> +void setspppdynaddr(const char *, int);
> +void setspppdyndest(const char *, int);
> void sppp_status(void);
> void sppp_printproto(const char *, struct sauthreq *);
> void setifpriority(const char *, int);
> @@ -446,6 +449,10 @@ const struct cmd {
> { "peerkey",NEXTARG,0, setseerkey },
> { "peerflag", NEXTARG,0, setseerflag },
> { "-peerflag", NEXTARG,0, unsetseerflag },
> + { "dynaddr",1, 0, setspppdynaddr },
> + { "-dynaddr", 0, 0, setspppdynaddr },
> + { "dyndest",1, 0, setspppdyndest },
> + { "-dyndest", 0, 0, setspppdyndest },
> { "nwflag", NEXTARG,0, setifnwflag },
> { "-nwflag",NEXTARG,0, unsetifnwflag },
> { "flowsrc",NEXTARG,0, setpflow_sender },
> @@ -4878,6 +4885,63 @@ unsetseerflag(const char *val, int d
> }
>
> void
> +sipcpinfo(struct sipcpreq *req)
> +{
> + bzero(req, sizeof(*req));
> +
> + ifr.ifr_data = (caddr_t)req;
> + req->cmd = SPPPIOGIPCP;
> + if (ioctl(s, SIOCGSARAMS, ) == -1)
> + err(1, "SIOCGSARAMS(SPPPIOGIPCP)");
> +}
> +
> +void
> +setspppdynaddr(const char *val, int d)
> +{
> + struct sipcpreq scp;
> +
> + sipcpinfo();
> +
> + if (d == 1) {
> + if (scp.flags &
sppp(4)/pppoe(4) dynamic address hack
Currently, sppp(4) interfaces use a destination address of 0.0.0.1 to
indicate that the destination address should be assigned by the peer:
inet 0.0.0.0 255.255.255.255 NONE \
pppoedev em0 authproto pap \
authname 'testcaller' authkey 'donttell' up
dest 0.0.0.1
As a side effect a route to 0.0.0.1 is added to the routing table.
Apparently, in 5.7 this still used to work with mulitple pppoe interfaces
which shared a routing table:
http://marc.info/?l=openbsd-misc=149386503001033=2
At present only one pppoe interface can be used in this way because
setting a dest address of 0.0.0.1 on additional interfaces now causes
an error: ifconfig: SIOCAIFADDR: File exists
This diff changes the way dynamic addresses are configured in sppp(4).
Instead of using magic IP addresses as wildcards, we allow userland to
control behavior via new ifconfig command flags. 'dynaddr' enables a
dynamic local address, and 'dyndest' enables a dynamic destination address.
To disable either of these again, prepend a minus (e.g. -dynaddr).
The above example becomes:
inet 0.0.0.0 255.255.255.255 NONE \
pppoedev em0 authproto pap \
authname 'testcaller' authkey 'donttell' \
dynaddr dyndest up
Tested on my ADSL line and it works as expected.
I cannot test it on multiple links until next week, but since no route
for 0.0.0.1 is in the routing table anymore, I expect this to work.
Index: sbin/ifconfig/ifconfig.8
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
retrieving revision 1.282
diff -u -p -r1.282 ifconfig.8
--- sbin/ifconfig/ifconfig.812 May 2017 15:11:02 - 1.282
+++ sbin/ifconfig/ifconfig.89 Jun 2017 16:02:23 -
@@ -1393,6 +1393,8 @@ Clear a previously set service name.
.Op Cm authkey Ar key
.Op Cm authname Ar name
.Op Cm authproto Ar proto
+.Op Oo Fl Oc Ns Cm dynaddr
+.Op Oo Fl Oc Ns Cm dyndest
.Op Oo Fl Oc Ns Cm peerflag Ar flag
.Op Cm peerkey Ar key
.Op Cm peername Ar name
@@ -1419,6 +1421,16 @@ The protocol name can be either
or
.Ql none .
In the latter case, authentication will be turned off.
+.It Cm dynaddr
+The local address will be assigned by the peer.
+.It Cm -dynaddr
+Disable dynamic assignment of the local address.
+This is the default.
+.It Cm dyndest
+The destination address will be assigned by the peer.
+.It Cm -dyndest
+Disable dynamic assignment of the destination address.
+This is the default.
.It Cm peerflag Ar flag
Set a specified PPP flag for the remote authenticator.
The flag name can be either
Index: sbin/ifconfig/ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.342
diff -u -p -r1.342 ifconfig.c
--- sbin/ifconfig/ifconfig.c5 Jun 2017 05:10:23 - 1.342
+++ sbin/ifconfig/ifconfig.c9 Jun 2017 15:57:40 -
@@ -266,6 +266,9 @@ voidsetseername(const char *, int);
void setseerkey(const char *, int);
void setseerflag(const char *, int);
void unsetseerflag(const char *, int);
+void sipcpinfo(struct sipcpreq *);
+void setspppdynaddr(const char *, int);
+void setspppdyndest(const char *, int);
void sppp_status(void);
void sppp_printproto(const char *, struct sauthreq *);
void setifpriority(const char *, int);
@@ -446,6 +449,10 @@ const struct cmd {
{ "peerkey",NEXTARG,0, setseerkey },
{ "peerflag", NEXTARG,0, setseerflag },
{ "-peerflag", NEXTARG,0, unsetseerflag },
+ { "dynaddr",1, 0, setspppdynaddr },
+ { "-dynaddr", 0, 0, setspppdynaddr },
+ { "dyndest",1, 0, setspppdyndest },
+ { "-dyndest", 0, 0, setspppdyndest },
{ "nwflag", NEXTARG,0, setifnwflag },
{ "-nwflag",NEXTARG,0, unsetifnwflag },
{ "flowsrc",NEXTARG,0, setpflow_sender },
@@ -4878,6 +4885,63 @@ unsetseerflag(const char *val, int d
}
void
+sipcpinfo(struct sipcpreq *req)
+{
+ bzero(req, sizeof(*req));
+
+ ifr.ifr_data = (caddr_t)req;
+ req->cmd = SPPPIOGIPCP;
+ if (ioctl(s, SIOCGSARAMS, ) == -1)
+ err(1, "SIOCGSARAMS(SPPPIOGIPCP)");
+}
+
+void
+setspppdynaddr(const char *val, int d)
+{
+ struct sipcpreq scp;
+
+ sipcpinfo();
+
+ if (d == 1) {
+ if (scp.flags & SIPCP_MYADDR_DYN)
+ return;
+ scp.flags |= SIPCP_MYADDR_DYN;
+ } else {
+ if ((scp.flags & SIPCP_MYADDR_DYN) == 0)
+ return;
+ scp.flags &= ~SIPCP_MYADDR_DYN;
+ }
+
+ scp.cmd =
Re: radix lookup w/o KERNEL_LOCK()
On Fri, Jun 09, 2017 at 04:40:18PM +0200, Martin Pieuchot wrote:
> Now with the correct diff:
OK bluhm@
>
> Index: net/radix.c
> ===
> RCS file: /cvs/src/sys/net/radix.c,v
> retrieving revision 1.56
> diff -u -p -r1.56 radix.c
> --- net/radix.c 24 Jan 2017 10:08:30 - 1.56
> +++ net/radix.c 9 Jun 2017 14:22:16 -
> @@ -48,24 +48,32 @@
>
> #include
>
> -static unsigned int max_keylen;
> -struct radix_node_head *mask_rnhead;
> -static char *addmask_key;
> -static char normal_chars[] = {0, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe,
> -1};
> -static char *rn_zeros, *rn_ones;
> +#define SALEN(sa)(*(u_char *)(sa))
> +
> +/*
> + * Read-only variables, allocated & filled during rn_init().
> + */
> +static char *rn_zeros; /* array of 0s */
> +static char *rn_ones; /* array of 1s */
> +static unsigned int max_keylen;/* size of the above arrays */
> +#define KEYLEN_LIMIT 64 /* maximum allowed keylen */
>
> -struct pool rtmask_pool; /* pool for radix_mask structures */
>
> -#define rn_masktop (mask_rnhead->rnh_treetop)
> +struct radix_node_head *mask_rnhead; /* head of shared mask tree */
> +struct pool rtmask_pool; /* pool for radix_mask structures */
>
> static inline int rn_satisfies_leaf(char *, struct radix_node *, int);
> static inline int rn_lexobetter(void *, void *);
> static inline struct radix_mask *rn_new_radix_mask(struct radix_node *,
> struct radix_mask *);
>
> +int rn_refines(void *, void *);
> +int rn_inithead0(struct radix_node_head *, int);
> +struct radix_node *rn_addmask(void *, int, int);
> struct radix_node *rn_insert(void *, struct radix_node_head *, int *,
> struct radix_node [2]);
> struct radix_node *rn_newpair(void *, int, struct radix_node[2]);
> +void rn_link_dupedkey(struct radix_node *, struct radix_node *, int);
>
> static inline struct radix_node *rn_search(void *, struct radix_node *);
> struct radix_node *rn_search_m(void *, struct radix_node *, void *);
> @@ -205,11 +213,11 @@ rn_satisfies_leaf(char *trial, struct ra
> char *cplim;
> int length;
>
> - length = min(*(u_char *)cp, *(u_char *)cp2);
> + length = min(SALEN(cp), SALEN(cp2));
> if (cp3 == NULL)
> cp3 = rn_ones;
> else
> - length = min(length, *(u_char *)cp3);
> + length = min(length, SALEN(cp3));
> cplim = cp + length;
> cp += skip;
> cp2 += skip;
> @@ -246,9 +254,9 @@ rn_match(void *v_arg, struct radix_node_
>* are probably the most common case...
>*/
> if (t->rn_mask)
> - vlen = *(u_char *)t->rn_mask;
> + vlen = SALEN(t->rn_mask);
> else
> - vlen = *(u_char *)v;
> + vlen = SALEN(v);
> cp = v + off;
> cp2 = t->rn_key + off;
> cplim = v + vlen;
> @@ -361,7 +369,7 @@ rn_insert(void *v_arg, struct radix_node
> caddr_t cp, cp2, cplim;
> int vlen, cmp_res;
>
> - vlen = *(u_char *)v;
> + vlen = SALEN(v);
> cp = v + off;
> cp2 = t->rn_key + off;
> cplim = v + vlen;
> @@ -413,9 +421,9 @@ rn_addmask(void *n_arg, int search, int
> caddr_t cp, cplim;
> int b = 0, mlen, j;
> int maskduplicated, m0, isnormal;
> - static int last_zeroed = 0;
> + char addmask_key[KEYLEN_LIMIT];
>
> - if ((mlen = *(u_char *)netmask) > max_keylen)
> + if ((mlen = SALEN(netmask)) > max_keylen)
> mlen = max_keylen;
> if (skip == 0)
> skip = 1;
> @@ -431,15 +439,11 @@ rn_addmask(void *n_arg, int search, int
> for (cp = addmask_key + mlen; (cp > addmask_key) && cp[-1] == 0;)
> cp--;
> mlen = cp - addmask_key;
> - if (mlen <= skip) {
> - if (m0 >= last_zeroed)
> - last_zeroed = mlen;
> + if (mlen <= skip)
> return (mask_rnhead->rnh_nodes);
> - }
> - if (m0 < last_zeroed)
> - memset(addmask_key + m0, 0, last_zeroed - m0);
> - *addmask_key = last_zeroed = mlen;
> - tm = rn_search(addmask_key, rn_masktop);
> + memset(addmask_key + m0, 0, max_keylen - m0);
> + SALEN(addmask_key) = mlen;
> + tm = rn_search(addmask_key, mask_rnhead->rnh_treetop);
> if (memcmp(addmask_key, tm->rn_key, mlen) != 0)
> tm = NULL;
> if (tm || search)
> @@ -452,7 +456,7 @@ rn_addmask(void *n_arg, int search, int
> memcpy(cp, addmask_key, mlen);
> tm = rn_insert(cp, mask_rnhead, , tm);
> if (maskduplicated) {
> - log(LOG_ERR, "rn_addmask: mask impossibly already in tree\n");
> + log(LOG_ERR, "%s: mask impossibly already in tree\n", __func__);
> free(saved_tm, M_RTABLE, 0);
> return (tm);
> }
> @@ -464,6 +468,9 @@ rn_addmask(void *n_arg, int search, int
> for (cp = netmask +
Re: Towards tcp_input() w/o KERNEL_LOCK()
On Fri, Jun 09, 2017 at 03:54:04PM +0200, Martin Pieuchot wrote: > Updated diff: > > - use a clever trick in sorflush() to make the new assert happy. Perhaps a bit too clever. On my amd64 struct socket has nearly 400 bytes. That may be too much for our kernel stack. > - convert sbappendrecord() and sbappend() for coherency > - fix other nits bluhm@ spotted > - add a missing solock()/sounlock() in filt_sowrite(). Note that >this doesn't make the whole function mp-safe. if it fits on the stack, OK bluhm@
Re: radix lookup w/o KERNEL_LOCK()
On 09/06/17(Fri) 16:23, Martin Pieuchot wrote:
> On 09/06/17(Fri) 15:46, Alexander Bluhm wrote:
> > On Fri, Jun 09, 2017 at 03:11:05PM +0200, Martin Pieuchot wrote:
> > > > The static variable last_zeroed does not look MP safe. If I get
> > > > this code correctly it is only an optimization to avoid multiple
> > > > zeroing in addmask_key. This does not work anyway with addmask_key
> > > > on the stack.
> > >
> > > You're right, I removed the 'static'.
> >
> > As last_zeroed is always 0 now, there is dead code left over.
> > I think this variable should be killed.
>
> I agree.
>
> > > @@ -438,8 +447,8 @@ rn_addmask(void *n_arg, int search, int
> > > }
> > > if (m0 < last_zeroed)
> > > memset(addmask_key + m0, 0, last_zeroed - m0);
> > > - *addmask_key = last_zeroed = mlen;
> > > - tm = rn_search(addmask_key, rn_masktop);
> > > + SALEN(addmask_key) = mlen;
> >
> > This assumes that addmask_key has been zeroed in rn_init().
> > I think now it must be
> > memset(addmask_key + m0, 0, sizeof(addmask_key) - m0);
> > Maybe the sizeof(addmask_key) could be optimized by using mlen
> > or max_keylen instead.
>
> I couldn't convince myself that rn_search() do not check bits
> after 'mlen' so I added the memset() as you suggested.
Now with the correct diff:
Index: net/radix.c
===
RCS file: /cvs/src/sys/net/radix.c,v
retrieving revision 1.56
diff -u -p -r1.56 radix.c
--- net/radix.c 24 Jan 2017 10:08:30 - 1.56
+++ net/radix.c 9 Jun 2017 14:22:16 -
@@ -48,24 +48,32 @@
#include
-static unsigned int max_keylen;
-struct radix_node_head *mask_rnhead;
-static char *addmask_key;
-static char normal_chars[] = {0, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, -1};
-static char *rn_zeros, *rn_ones;
+#define SALEN(sa) (*(u_char *)(sa))
+
+/*
+ * Read-only variables, allocated & filled during rn_init().
+ */
+static char*rn_zeros; /* array of 0s */
+static char*rn_ones; /* array of 1s */
+static unsigned int max_keylen;/* size of the above arrays */
+#define KEYLEN_LIMIT64 /* maximum allowed keylen */
-struct pool rtmask_pool; /* pool for radix_mask structures */
-#define rn_masktop (mask_rnhead->rnh_treetop)
+struct radix_node_head *mask_rnhead; /* head of shared mask tree */
+struct pool rtmask_pool; /* pool for radix_mask structures */
static inline int rn_satisfies_leaf(char *, struct radix_node *, int);
static inline int rn_lexobetter(void *, void *);
static inline struct radix_mask *rn_new_radix_mask(struct radix_node *,
struct radix_mask *);
+int rn_refines(void *, void *);
+int rn_inithead0(struct radix_node_head *, int);
+struct radix_node *rn_addmask(void *, int, int);
struct radix_node *rn_insert(void *, struct radix_node_head *, int *,
struct radix_node [2]);
struct radix_node *rn_newpair(void *, int, struct radix_node[2]);
+void rn_link_dupedkey(struct radix_node *, struct radix_node *, int);
static inline struct radix_node *rn_search(void *, struct radix_node *);
struct radix_node *rn_search_m(void *, struct radix_node *, void *);
@@ -205,11 +213,11 @@ rn_satisfies_leaf(char *trial, struct ra
char *cplim;
int length;
- length = min(*(u_char *)cp, *(u_char *)cp2);
+ length = min(SALEN(cp), SALEN(cp2));
if (cp3 == NULL)
cp3 = rn_ones;
else
- length = min(length, *(u_char *)cp3);
+ length = min(length, SALEN(cp3));
cplim = cp + length;
cp += skip;
cp2 += skip;
@@ -246,9 +254,9 @@ rn_match(void *v_arg, struct radix_node_
* are probably the most common case...
*/
if (t->rn_mask)
- vlen = *(u_char *)t->rn_mask;
+ vlen = SALEN(t->rn_mask);
else
- vlen = *(u_char *)v;
+ vlen = SALEN(v);
cp = v + off;
cp2 = t->rn_key + off;
cplim = v + vlen;
@@ -361,7 +369,7 @@ rn_insert(void *v_arg, struct radix_node
caddr_t cp, cp2, cplim;
int vlen, cmp_res;
- vlen = *(u_char *)v;
+ vlen = SALEN(v);
cp = v + off;
cp2 = t->rn_key + off;
cplim = v + vlen;
@@ -413,9 +421,9 @@ rn_addmask(void *n_arg, int search, int
caddr_t cp, cplim;
int b = 0, mlen, j;
int maskduplicated, m0, isnormal;
- static int last_zeroed = 0;
+ char addmask_key[KEYLEN_LIMIT];
- if ((mlen = *(u_char *)netmask) > max_keylen)
+ if ((mlen = SALEN(netmask)) > max_keylen)
mlen = max_keylen;
if (skip == 0)
skip = 1;
@@ -431,15 +439,11 @@ rn_addmask(void *n_arg, int search, int
for (cp = addmask_key + mlen; (cp > addmask_key) && cp[-1] == 0;)
cp--;
mlen = cp - addmask_key;
- if (mlen <= skip) {
- if (m0 >= last_zeroed)
-
Re: radix lookup w/o KERNEL_LOCK()
On Fri, Jun 09, 2017 at 04:23:13PM +0200, Martin Pieuchot wrote:
> I couldn't convince myself that rn_search() do not check bits
> after 'mlen' so I added the memset() as you suggested.
It looks like you forgot to put the memset() in the diff.
> @@ -432,14 +440,10 @@ rn_addmask(void *n_arg, int search, int
> cp--;
> mlen = cp - addmask_key;
> if (mlen <= skip) {
> - if (m0 >= last_zeroed)
> - last_zeroed = mlen;
> return (mask_rnhead->rnh_nodes);
> }
> - if (m0 < last_zeroed)
> - memset(addmask_key + m0, 0, last_zeroed - m0);
> - *addmask_key = last_zeroed = mlen;
> - tm = rn_search(addmask_key, rn_masktop);
> + SALEN(addmask_key) = mlen;
> + tm = rn_search(addmask_key, mask_rnhead->rnh_treetop);
> if (memcmp(addmask_key, tm->rn_key, mlen) != 0)
> tm = NULL;
> if (tm || search)
With memset() added, OK bluhm@
Re: pfsync and option WITH_PF_LOCK
On Fri, Jun 09, 2017 at 03:48:49PM +0200, Hrvoje Popovski wrote: > On 9.6.2017. 14:55, Alexandr Nedvedicky wrote: > > Hello, > > > > > > On Fri, Jun 09, 2017 at 01:11:01PM +0200, Alexander Bluhm wrote: > >> On Fri, Jun 09, 2017 at 10:55:46AM +0200, Alexandr Nedvedicky wrote: > >>> would it make sense to commit a poor man's solution below, before > >>> pfsync(4) > >>> will get to shape? The idea is to grab PF_LOCK, just before we pass the > >>> data > >>> to PF for further processing. > >> Whatever helps to make progress with pf is fine. We should not > >> delay unlocking pf until someone steps in and refactors pfsync. > >> > >> OK bluhm@ > >> > > I still would like to ask Hrvoje to give it a try first. I believe the fix > > should work, but I could not try it as I don't have working pfsync set up > > available for testing. > > > > thanks and > > regards > > sasha > > > > > Hi all, > > with this diff i don't see any traces as before. > thanks a lot for quick testing. regards sasha
Re: radix lookup w/o KERNEL_LOCK()
On 09/06/17(Fri) 15:46, Alexander Bluhm wrote:
> On Fri, Jun 09, 2017 at 03:11:05PM +0200, Martin Pieuchot wrote:
> > > The static variable last_zeroed does not look MP safe. If I get
> > > this code correctly it is only an optimization to avoid multiple
> > > zeroing in addmask_key. This does not work anyway with addmask_key
> > > on the stack.
> >
> > You're right, I removed the 'static'.
>
> As last_zeroed is always 0 now, there is dead code left over.
> I think this variable should be killed.
I agree.
> > @@ -438,8 +447,8 @@ rn_addmask(void *n_arg, int search, int
> > }
> > if (m0 < last_zeroed)
> > memset(addmask_key + m0, 0, last_zeroed - m0);
> > - *addmask_key = last_zeroed = mlen;
> > - tm = rn_search(addmask_key, rn_masktop);
> > + SALEN(addmask_key) = mlen;
>
> This assumes that addmask_key has been zeroed in rn_init().
> I think now it must be
> memset(addmask_key + m0, 0, sizeof(addmask_key) - m0);
> Maybe the sizeof(addmask_key) could be optimized by using mlen
> or max_keylen instead.
I couldn't convince myself that rn_search() do not check bits
after 'mlen' so I added the memset() as you suggested.
Index: net/radix.c
===
RCS file: /cvs/src/sys/net/radix.c,v
retrieving revision 1.56
diff -u -p -r1.56 radix.c
--- net/radix.c 24 Jan 2017 10:08:30 - 1.56
+++ net/radix.c 9 Jun 2017 14:04:12 -
@@ -48,24 +48,32 @@
#include
-static unsigned int max_keylen;
-struct radix_node_head *mask_rnhead;
-static char *addmask_key;
-static char normal_chars[] = {0, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, -1};
-static char *rn_zeros, *rn_ones;
+#define SALEN(sa) (*(u_char *)(sa))
-struct pool rtmask_pool; /* pool for radix_mask structures */
+/*
+ * Read-only variables, allocated & filled during rn_init().
+ */
+static char*rn_zeros; /* array of 0s */
+static char*rn_ones; /* array of 1s */
+static unsigned int max_keylen;/* size of the above arrays */
+#define KEYLEN_LIMIT64 /* maximum allowed keylen */
-#define rn_masktop (mask_rnhead->rnh_treetop)
+
+struct radix_node_head *mask_rnhead; /* head of shared mask tree */
+struct pool rtmask_pool; /* pool for radix_mask structures */
static inline int rn_satisfies_leaf(char *, struct radix_node *, int);
static inline int rn_lexobetter(void *, void *);
static inline struct radix_mask *rn_new_radix_mask(struct radix_node *,
struct radix_mask *);
+int rn_refines(void *, void *);
+int rn_inithead0(struct radix_node_head *, int);
+struct radix_node *rn_addmask(void *, int, int);
struct radix_node *rn_insert(void *, struct radix_node_head *, int *,
struct radix_node [2]);
struct radix_node *rn_newpair(void *, int, struct radix_node[2]);
+void rn_link_dupedkey(struct radix_node *, struct radix_node *, int);
static inline struct radix_node *rn_search(void *, struct radix_node *);
struct radix_node *rn_search_m(void *, struct radix_node *, void *);
@@ -413,9 +421,9 @@ rn_addmask(void *n_arg, int search, int
caddr_t cp, cplim;
int b = 0, mlen, j;
int maskduplicated, m0, isnormal;
- static int last_zeroed = 0;
+ char addmask_key[KEYLEN_LIMIT];
- if ((mlen = *(u_char *)netmask) > max_keylen)
+ if ((mlen = SALEN(netmask)) > max_keylen)
mlen = max_keylen;
if (skip == 0)
skip = 1;
@@ -432,14 +440,10 @@ rn_addmask(void *n_arg, int search, int
cp--;
mlen = cp - addmask_key;
if (mlen <= skip) {
- if (m0 >= last_zeroed)
- last_zeroed = mlen;
return (mask_rnhead->rnh_nodes);
}
- if (m0 < last_zeroed)
- memset(addmask_key + m0, 0, last_zeroed - m0);
- *addmask_key = last_zeroed = mlen;
- tm = rn_search(addmask_key, rn_masktop);
+ SALEN(addmask_key) = mlen;
+ tm = rn_search(addmask_key, mask_rnhead->rnh_treetop);
if (memcmp(addmask_key, tm->rn_key, mlen) != 0)
tm = NULL;
if (tm || search)
@@ -452,7 +456,7 @@ rn_addmask(void *n_arg, int search, int
memcpy(cp, addmask_key, mlen);
tm = rn_insert(cp, mask_rnhead, , tm);
if (maskduplicated) {
- log(LOG_ERR, "rn_addmask: mask impossibly already in tree\n");
+ log(LOG_ERR, "%s: mask impossibly already in tree\n", __func__);
free(saved_tm, M_RTABLE, 0);
return (tm);
}
@@ -464,6 +468,9 @@ rn_addmask(void *n_arg, int search, int
for (cp = netmask + skip; (cp < cplim) && *(u_char *)cp == 0xff;)
cp++;
if (cp != cplim) {
+ static const char normal_chars[] = {
+ 0, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, -1
+ };
for (j = 0x80; (j & *cp) != 0; j >>= 1)
Re: Towards tcp_input() w/o KERNEL_LOCK()
On 09/06/17(Fri) 00:32, Alexander Bluhm wrote:
> On Tue, Jun 06, 2017 at 05:15:40PM +0200, Martin Pieuchot wrote:
> > TCP/UDP are almost ready to run without KERNEL_LOCK() because accesses
> > to their sockets are serialized via the NET_LOCK(). On the other hand
> > pfkey and routing sockets accesses still rely on the KERNEL_LOCK().
> >
> > Since we're going to work at the socket layer, first to remove the
> > KERNEL_LOCK() from routing/pfkey sockets then to split the NET_LOCK(),
> > we need some tooling to move faster and avoid mistakes.
> >
> > Currently all operations on socket buffers are protected by these
> > locks. I'd like to assert that, at least for all functions used in
> > TCP/UDP layers.
> >
> > The idea is to later change the lock asserted in soassertlocked().
> >
> > Comments, ok?
>
> Good idea, mostly OK.
Updated diff:
- use a clever trick in sorflush() to make the new assert happy.
- convert sbappendrecord() and sbappend() for coherency
- fix other nits bluhm@ spotted
- add a missing solock()/sounlock() in filt_sowrite(). Note that
this doesn't make the whole function mp-safe.
Index: kern/uipc_socket.c
===
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.186
diff -u -p -r1.186 uipc_socket.c
--- kern/uipc_socket.c 31 May 2017 08:55:10 - 1.186
+++ kern/uipc_socket.c 9 Jun 2017 13:48:58 -
@@ -216,7 +216,7 @@ sofree(struct socket *so)
so->so_sp = NULL;
}
#endif /* SOCKET_SPLICE */
- sbrelease(>so_snd);
+ sbrelease(so, >so_snd);
sorflush(so);
pool_put(_pool, so);
}
@@ -440,7 +440,7 @@ restart:
} else if (addr == 0)
snderr(EDESTADDRREQ);
}
- space = sbspace(>so_snd);
+ space = sbspace(so, >so_snd);
if (flags & MSG_OOB)
space += 1024;
if ((atomic && resid > so->so_snd.sb_hiwat) ||
@@ -1041,7 +1041,7 @@ sorflush(struct socket *so)
struct sockbuf *sb = >so_rcv;
struct protosw *pr = so->so_proto;
sa_family_t af = pr->pr_domain->dom_family;
- struct sockbuf asb;
+ struct socket aso;
sb->sb_flags |= SB_NOINTR;
sblock(sb, M_WAITOK,
@@ -1049,16 +1049,16 @@ sorflush(struct socket *so)
: NULL);
socantrcvmore(so);
sbunlock(sb);
- asb = *sb;
+ aso.so_rcv = *sb;
memset(sb, 0, sizeof (*sb));
/* XXX - the memset stomps all over so_rcv */
- if (asb.sb_flags & SB_KNOTE) {
- sb->sb_sel.si_note = asb.sb_sel.si_note;
+ if (aso.so_rcv.sb_flags & SB_KNOTE) {
+ sb->sb_sel.si_note = aso.so_rcv.sb_sel.si_note;
sb->sb_flags = SB_KNOTE;
}
if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose)
- (*pr->pr_domain->dom_dispose)(asb.sb_mb);
- sbrelease();
+ (*pr->pr_domain->dom_dispose)(aso.so_rcv.sb_mb);
+ sbrelease(, _rcv);
}
#ifdef SOCKET_SPLICE
@@ -1270,7 +1270,7 @@ somove(struct socket *so, int wait)
maxreached = 1;
}
}
- space = sbspace(>so_snd);
+ space = sbspace(sosp, >so_snd);
if (so->so_oobmark && so->so_oobmark < len &&
so->so_oobmark < space + 1024)
space += 1024;
@@ -1635,7 +1635,7 @@ sosetopt(struct socket *so, int level, i
goto bad;
}
if (sbcheckreserve(cnt, so->so_snd.sb_wat) ||
- sbreserve(>so_snd, cnt)) {
+ sbreserve(so, >so_snd, cnt)) {
error = ENOBUFS;
goto bad;
}
@@ -1648,7 +1648,7 @@ sosetopt(struct socket *so, int level, i
goto bad;
}
if (sbcheckreserve(cnt, so->so_rcv.sb_wat) ||
- sbreserve(>so_rcv, cnt)) {
+ sbreserve(so, >so_rcv, cnt)) {
error = ENOBUFS;
goto bad;
}
@@ -1990,8 +1990,11 @@ int
filt_sowrite(struct knote *kn, long hint)
{
struct socket *so = kn->kn_fp->f_data;
+ int s;
- kn->kn_data = sbspace(>so_snd);
+ s = solock(so);
+ kn->kn_data = sbspace(so, >so_snd);
+ sounlock(s);
if (so->so_state & SS_CANTSENDMORE) {
kn->kn_flags |= EV_EOF;
kn->kn_fflags = so->so_error;
Index: kern/uipc_usrreq.c
===
RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v
retrieving
Re: pfsync and option WITH_PF_LOCK
On 9.6.2017. 14:55, Alexandr Nedvedicky wrote: > Hello, > > > On Fri, Jun 09, 2017 at 01:11:01PM +0200, Alexander Bluhm wrote: >> On Fri, Jun 09, 2017 at 10:55:46AM +0200, Alexandr Nedvedicky wrote: >>> would it make sense to commit a poor man's solution below, before pfsync(4) >>> will get to shape? The idea is to grab PF_LOCK, just before we pass the data >>> to PF for further processing. >> Whatever helps to make progress with pf is fine. We should not >> delay unlocking pf until someone steps in and refactors pfsync. >> >> OK bluhm@ >> > I still would like to ask Hrvoje to give it a try first. I believe the fix > should work, but I could not try it as I don't have working pfsync set up > available for testing. > > thanks and > regards > sasha > Hi all, with this diff i don't see any traces as before.
Re: radix lookup w/o KERNEL_LOCK()
On Fri, Jun 09, 2017 at 03:11:05PM +0200, Martin Pieuchot wrote: > > The static variable last_zeroed does not look MP safe. If I get > > this code correctly it is only an optimization to avoid multiple > > zeroing in addmask_key. This does not work anyway with addmask_key > > on the stack. > > You're right, I removed the 'static'. As last_zeroed is always 0 now, there is dead code left over. I think this variable should be killed. > @@ -438,8 +447,8 @@ rn_addmask(void *n_arg, int search, int > } > if (m0 < last_zeroed) > memset(addmask_key + m0, 0, last_zeroed - m0); > - *addmask_key = last_zeroed = mlen; > - tm = rn_search(addmask_key, rn_masktop); > + SALEN(addmask_key) = mlen; This assumes that addmask_key has been zeroed in rn_init(). I think now it must be memset(addmask_key + m0, 0, sizeof(addmask_key) - m0); Maybe the sizeof(addmask_key) could be optimized by using mlen or max_keylen instead. bluhm
Re: Route priority support for ospf6d
On Fri, Jun 09, 2017 at 03:28:07PM +0200, Alexander Bluhm wrote:
> On Wed, May 31, 2017 at 02:29:03PM +0200, Florian Riehm wrote:
> > @@ -359,6 +333,7 @@ kr_fib_decouple(void)
> > void
> > kr_dispatch_msg(int fd, short event, void *bula)
> > {
> > + /* XXX this is stupid */
> > dispatch_rtmsg();
> > }
>
> I guess this comment refers to the event_loopexit(NULL) in ospfd code.
> So I would not add it here.
>
This this is from my side, because it is dumb to have a void function that
only does on thing calling another void function.
> > @@ -1377,10 +1376,10 @@ dispatch_rtmsg(void)
> > if (rtm->rtm_flags & (RTF_LLINFO|RTF_BROADCAST))
> > continue;
> >
> > -#ifdef RTF_MPATH
> > if (rtm->rtm_flags & RTF_MPATH)
> > mpath = 1;
> > -#endif
> > + prio = rtm->rtm_priority;
> > +
>
> In ospfd we have at this location
> flags = (prio == RTP_OSPF) ?
> F_OSPFD_INSERTED : F_KERNEL;
> Should we add that here?
Hmmm... this seems to be to detect from where routes are originating.
>
> At the hackathon consensus was to move forward, so get this in.
> OK bluhm@
>
Agreed. OK claudio@
--
:wq Claudio
Re: Route priority support for ospf6d
On Wed, May 31, 2017 at 02:29:03PM +0200, Florian Riehm wrote:
> @@ -359,6 +333,7 @@ kr_fib_decouple(void)
> void
> kr_dispatch_msg(int fd, short event, void *bula)
> {
> + /* XXX this is stupid */
> dispatch_rtmsg();
> }
I guess this comment refers to the event_loopexit(NULL) in ospfd code.
So I would not add it here.
> @@ -1377,10 +1376,10 @@ dispatch_rtmsg(void)
> if (rtm->rtm_flags & (RTF_LLINFO|RTF_BROADCAST))
> continue;
>
> -#ifdef RTF_MPATH
> if (rtm->rtm_flags & RTF_MPATH)
> mpath = 1;
> -#endif
> + prio = rtm->rtm_priority;
> +
In ospfd we have at this location
flags = (prio == RTP_OSPF) ?
F_OSPFD_INSERTED : F_KERNEL;
Should we add that here?
At the hackathon consensus was to move forward, so get this in.
OK bluhm@
Re: rtadvd(8) send unicast advertisements
On Fri, Jun 09, 2017 at 02:48:58PM +0200, Alexander Bluhm wrote: > On Fri, Jun 09, 2017 at 09:16:01AM +, Florian Obser wrote: > > I don't think we need a knob for this, doing it per default if we > > have the source link layer address should be fine. > > > > See RFC 7772 why this is a good thing. > > > > OK? > > I wonder whether the > /* update counter */ > if (rainfo->initcounter < MAX_INITIAL_RTR_ADVERTISEMENTS) > rainfo->initcounter++; > should also be within the if (...sin6_allnodes...) condition. yes it should. missed that one, thanks for spotting, I will commit it with that moved > > anyway, OK bluhm@ > -- I'm not entirely sure you are real.
Re: radix lookup w/o KERNEL_LOCK()
On 08/06/17(Thu) 23:46, Alexander Bluhm wrote:
> On Tue, Jun 06, 2017 at 03:36:12PM +0200, Martin Pieuchot wrote:
> > +#define SALEN(sa) (*(u_char *)sa)
>
> Put () around macro arguments.
> #define SALEN(sa) (*(u_char *)(sa))
Done.
> > -int
> > +static int
> > rn_refines(void *m_arg, void *n_arg)
>
> > -int
> > +static int
> > rn_inithead0(struct radix_node_head *rnh, int offset)
>
> Could you keep these kernel funtions global for debugging?
Sure.
> > static int last_zeroed = 0;
> > + char addmask_key[KEYLEN_LIMIT];
>
> The static variable last_zeroed does not look MP safe. If I get
> this code correctly it is only an optimization to avoid multiple
> zeroing in addmask_key. This does not work anyway with addmask_key
> on the stack.
You're right, I removed the 'static'.
> > *addmask_key = last_zeroed = mlen;
>
> Should this be SALEN(addmask_key)?
Yes and we can drop 'last_zeroed' since it isn't used afterward.
Index: net/radix.c
===
RCS file: /cvs/src/sys/net/radix.c,v
retrieving revision 1.56
diff -u -p -r1.56 radix.c
--- net/radix.c 24 Jan 2017 10:08:30 - 1.56
+++ net/radix.c 9 Jun 2017 13:08:50 -
@@ -48,24 +48,32 @@
#include
-static unsigned int max_keylen;
-struct radix_node_head *mask_rnhead;
-static char *addmask_key;
-static char normal_chars[] = {0, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, -1};
-static char *rn_zeros, *rn_ones;
+#define SALEN(sa) (*(u_char *)(sa))
-struct pool rtmask_pool; /* pool for radix_mask structures */
+/*
+ * Read-only variables, allocated & filled during rn_init().
+ */
+static char*rn_zeros; /* array of 0s */
+static char*rn_ones; /* array of 1s */
+static unsigned int max_keylen;/* size of the above arrays */
+#define KEYLEN_LIMIT64 /* maximum allowed keylen */
-#define rn_masktop (mask_rnhead->rnh_treetop)
+
+struct radix_node_head *mask_rnhead; /* head of shared mask tree */
+struct pool rtmask_pool; /* pool for radix_mask structures */
static inline int rn_satisfies_leaf(char *, struct radix_node *, int);
static inline int rn_lexobetter(void *, void *);
static inline struct radix_mask *rn_new_radix_mask(struct radix_node *,
struct radix_mask *);
+int rn_refines(void *, void *);
+int rn_inithead0(struct radix_node_head *, int);
+struct radix_node *rn_addmask(void *, int, int);
struct radix_node *rn_insert(void *, struct radix_node_head *, int *,
struct radix_node [2]);
struct radix_node *rn_newpair(void *, int, struct radix_node[2]);
+void rn_link_dupedkey(struct radix_node *, struct radix_node *, int);
static inline struct radix_node *rn_search(void *, struct radix_node *);
struct radix_node *rn_search_m(void *, struct radix_node *, void *);
@@ -413,9 +421,10 @@ rn_addmask(void *n_arg, int search, int
caddr_t cp, cplim;
int b = 0, mlen, j;
int maskduplicated, m0, isnormal;
- static int last_zeroed = 0;
+ int last_zeroed = 0;
+ char addmask_key[KEYLEN_LIMIT];
- if ((mlen = *(u_char *)netmask) > max_keylen)
+ if ((mlen = SALEN(netmask)) > max_keylen)
mlen = max_keylen;
if (skip == 0)
skip = 1;
@@ -438,8 +447,8 @@ rn_addmask(void *n_arg, int search, int
}
if (m0 < last_zeroed)
memset(addmask_key + m0, 0, last_zeroed - m0);
- *addmask_key = last_zeroed = mlen;
- tm = rn_search(addmask_key, rn_masktop);
+ SALEN(addmask_key) = mlen;
+ tm = rn_search(addmask_key, mask_rnhead->rnh_treetop);
if (memcmp(addmask_key, tm->rn_key, mlen) != 0)
tm = NULL;
if (tm || search)
@@ -452,7 +461,7 @@ rn_addmask(void *n_arg, int search, int
memcpy(cp, addmask_key, mlen);
tm = rn_insert(cp, mask_rnhead, , tm);
if (maskduplicated) {
- log(LOG_ERR, "rn_addmask: mask impossibly already in tree\n");
+ log(LOG_ERR, "%s: mask impossibly already in tree\n", __func__);
free(saved_tm, M_RTABLE, 0);
return (tm);
}
@@ -464,6 +473,9 @@ rn_addmask(void *n_arg, int search, int
for (cp = netmask + skip; (cp < cplim) && *(u_char *)cp == 0xff;)
cp++;
if (cp != cplim) {
+ static const char normal_chars[] = {
+ 0, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, -1
+ };
for (j = 0x80; (j & *cp) != 0; j >>= 1)
b++;
if (*cp != normal_chars[b] || cp != (cplim - 1))
@@ -488,9 +500,9 @@ rn_lexobetter(void *m_arg, void *n_arg)
* first. The netmasks were normalized before calling this function and
* don't have unneeded trailing zeros.
*/
- if (*mp > *np)
+ if (SALEN(mp) > SALEN(np))
return 1;
- if (*mp < *np)
+ if (SALEN(mp)
Re: pfsync and option WITH_PF_LOCK
Hello, On Fri, Jun 09, 2017 at 01:11:01PM +0200, Alexander Bluhm wrote: > On Fri, Jun 09, 2017 at 10:55:46AM +0200, Alexandr Nedvedicky wrote: > > would it make sense to commit a poor man's solution below, before pfsync(4) > > will get to shape? The idea is to grab PF_LOCK, just before we pass the data > > to PF for further processing. > > Whatever helps to make progress with pf is fine. We should not > delay unlocking pf until someone steps in and refactors pfsync. > > OK bluhm@ > I still would like to ask Hrvoje to give it a try first. I believe the fix should work, but I could not try it as I don't have working pfsync set up available for testing. thanks and regards sasha > > > > regards > > sasha > > > > 8<---8<---8<--8< > > --- src/sys/net/if_pfsync.c Fri Jun 09 09:40:12 2017 +0200 > > > > +++ src/sys/net/if_pfsync.c Fri Jun 09 10:49:44 2017 +0200 > > > > @@ -657,6 +657,7 @@ pfsync_input(struct mbuf **mp, int *offp > > > > struct pfsync_header *ph; > > struct pfsync_subheader subh; > > int offset, noff, len, count, mlen, flags = 0; > > > > + int e; > > > > pfsyncstat_inc(pfsyncs_ipackets); > > > > @@ -733,8 +734,11 @@ pfsync_input(struct mbuf **mp, int *offp > > > > return IPPROTO_DONE; > > } > > > > - if (pfsync_acts[subh.action].in(n->m_data + noff, > > > > - mlen, count, flags) != 0) > > + PF_LOCK(); > > + e = pfsync_acts[subh.action].in(n->m_data + noff, mlen, > > count, > > + flags); > > + PF_UNLOCK(); > > + if (e != 0) > > goto done; > > > > offset += mlen * count; > > 8<---8<---8<--8<
rtadvd(8) send unicast advertisements
I don't think we need a knob for this, doing it per default if we
have the source link layer address should be fine.
See RFC 7772 why this is a good thing.
OK?
diff --git rtadvd.c rtadvd.c
index 1f470a06c31..219b357e50b 100644
--- rtadvd.c
+++ rtadvd.c
@@ -141,7 +141,7 @@ static int prefix_check(struct nd_opt_prefix_info *, struct
rainfo *,
static int nd6_options(struct nd_opt_hdr *, int,
union nd_opts *, u_int32_t);
static void free_ndopts(union nd_opts *);
-static void ra_output(struct rainfo *);
+static void ra_output(struct rainfo *, struct sockaddr_in6 *);
static struct rainfo *if_indextorainfo(int);
static int rdaemon(int);
@@ -293,7 +293,7 @@ die_cb(int sig, short event, void *arg)
}
for (i = 0; i < retrans; i++) {
SLIST_FOREACH(ra, , entry)
- ra_output(ra);
+ ra_output(ra, _allnodes);
sleep(MIN_DELAY_BETWEEN_RAS);
}
exit(0);
@@ -651,11 +651,13 @@ rs_input(int len, struct nd_router_solicit *rs,
ra->rsinput++; /* increment statistics */
- /*
-* Decide whether to send RA according to the rate-limit
-* consideration.
-*/
- {
+ if (ndopts.nd_opts_src_lladdr)
+ ra_output(ra, from);
+ else {
+ /*
+* Decide whether to send RA according to the rate-limit
+* consideration.
+*/
long delay; /* must not be greater than 100 */
struct timeval interval, now, min_delay, tm_tmp, next,
computed;
@@ -1038,6 +1040,8 @@ nd6_options(struct nd_opt_hdr *hdr, int limit,
switch (hdr->nd_opt_type) {
case ND_OPT_SOURCE_LINKADDR:
+ ndopts->nd_opt_array[hdr->nd_opt_type] = hdr;
+ break;
case ND_OPT_TARGET_LINKADDR:
case ND_OPT_REDIRECTED_HEADER:
case ND_OPT_ROUTE_INFO:
@@ -1212,7 +1216,7 @@ if_indextorainfo(int index)
}
static void
-ra_output(struct rainfo *rainfo)
+ra_output(struct rainfo *rainfo, struct sockaddr_in6 *to)
{
struct cmsghdr *cm;
struct in6_pktinfo *pi;
@@ -1225,7 +1229,7 @@ ra_output(struct rainfo *rainfo)
make_packet(rainfo);/* XXX: inefficient */
- sndmhdr.msg_name = _allnodes;
+ sndmhdr.msg_name = to;
sndmhdr.msg_iov[0].iov_base = rainfo->ra_data;
sndmhdr.msg_iov[0].iov_len = rainfo->ra_datalen;
@@ -1263,11 +1267,13 @@ ra_output(struct rainfo *rainfo)
rainfo->initcounter++;
rainfo->raoutput++;
- /* update timestamp */
- gettimeofday(>lastsent, NULL);
+ if (memcmp(to, _allnodes, sizeof(sin6_allnodes)) == 0) {
+ /* update timestamp */
+ gettimeofday(>lastsent, NULL);
- /* reset waiting counter */
- rainfo->waiting = 0;
+ /* reset waiting counter */
+ rainfo->waiting = 0;
+ }
}
/* process RA timer */
@@ -1278,7 +1284,7 @@ timer_cb(int fd, short event, void *data)
log_debug("RA timer on %s is expired", rai->ifname);
- ra_output(rai);
+ ra_output(rai, _allnodes);
ra_timer_update(rai);
evtimer_add(>timer.ev, >timer.tm);
--
I'm not entirely sure you are real.
Re: pfsync and option WITH_PF_LOCK
Hello, would it make sense to commit a poor man's solution below, before pfsync(4) will get to shape? The idea is to grab PF_LOCK, just before we pass the data to PF for further processing. regards sasha 8<---8<---8<--8< --- src/sys/net/if_pfsync.c Fri Jun 09 09:40:12 2017 +0200 +++ src/sys/net/if_pfsync.c Fri Jun 09 10:49:44 2017 +0200 @@ -657,6 +657,7 @@ pfsync_input(struct mbuf **mp, int *offp struct pfsync_header *ph; struct pfsync_subheader subh; int offset, noff, len, count, mlen, flags = 0; + int e; pfsyncstat_inc(pfsyncs_ipackets); @@ -733,8 +734,11 @@ pfsync_input(struct mbuf **mp, int *offp return IPPROTO_DONE; } - if (pfsync_acts[subh.action].in(n->m_data + noff, - mlen, count, flags) != 0) + PF_LOCK(); + e = pfsync_acts[subh.action].in(n->m_data + noff, mlen, count, + flags); + PF_UNLOCK(); + if (e != 0) goto done; offset += mlen * count; 8<---8<---8<--8<
sed(1): missing NUL in pattern space
Hi,
There is an ongoing confusion in sed's source about the concept of EOL:
the code contradicts itself as to whether it should be determined by a
trailing NUL or by looking up the `len' field of the SPACE structure.
At least two commands (`l' and `s') assume that the pattern space is
always NUL-terminated. However, at least two other commands (`c' and
`D') do not comply with that assumption, modifying it by merely updating
its length attribute. As a result, using `l' or `s' after `c' or `D'
produces incorrect output.
For instance, in the following excerpt, `l' should print `bb$':
$ printf 'a\nbb\n' | sed '${l;q;};N;D'
$
bb
For the same underlying reason, it also disregards the deletion of the
pattern space that `c' is supposed to entail:
$ echo abc | sed 'c\
> text
> l'
text
abc$
The substitute command can likewise get confused and add an extra
character after a replacement:
$ printf 'a\nbb\n' | sed '${s/$/x/;q;};N;D'
bbxb
Note how this does not happen when the substitution pattern matches more
than the empty string:
$ printf 'a\nbb\n' | sed '${s/.$//;q;};N;D'
bbx
After an empty match, the character that `s' adds to the pattern space
depends on how memmove(3) modified it beforehand. Here's a very
simplified version of the original sequence of commands from which I
discovered these bugs, where `i' appear instead of `b':
$ printf 'a\nbb\n' | sed '${s/$/x/;q;};N;s/a/ZZi/;D'
bbxi
Similarily, when one believes the pattern space to be empty after a `c'
command, something like `s/.*//' will magically revive one character
from that emptiness:
$ echo abc | sed 'c\
> text
> s/.*//'
text
a
As I came across these in an actual script that I was writing, I was
quite amazed to find out that they went unnoticed for so long, without
anyone ever trying to see what `l' does after `c' or `D'. Indeed, the
`l' command is broken since revision 1.1 of process.c, making that bug
older than I am. It has also been present in the other BSDs, but:
o FreeBSD fixed it by accident in 2004 by adding support for
multibyte encodings;
o NetBSD fixed it by accident in 2014 by importing FreeBSD's sed
(merging their changes afterwards, but that didn't reintroduce
the bug, since they hadn't touched that part of the code).
The more serious bug regarding empty matches in the substitute command
was introduced in OpenBSD in 2011, when schwarze@ decided to rewrite its
main loop, intending to "fix multiple issues regarding the replacement
of zero-length strings", but apparently with the unfortunate side effect
of introducing a new issue regarding the replacement of zero-length
strings. Finally, this commit was adopted by FreeBSD in 2016 when they
synchronised some of their code with OpenBSD's to ease importing other
fixes from it.
Thus, OpenBSD still has the old `l' bug as well as the more recent `s'
one, FreeBSD only has the latter, and since 2014 NetBSD is not longer
affected by any of these, although the fact that `c' and `D' omit to
NUL-terminate the pattern space could be considered a bug in itself,
since at least one piece of code present for 20 years in their tree was
making the contrary assumption. So who knows where else it was made?
Actually, with all due respect, probably not even the original
developers: many of their choices suggest that the end of the pattern
space was not always meant to be encoded as a trailing NUL, but they
nevertheless designed the function `lputs' to only take a simple string
parameter, by which no separate length information could ever be
accessible.
I therefore have the strong feeling that this is not the last bug of
BSD's sed. The remaining ones may be hidden in dark corners, but I do
expect them to show up at some point in the future, considering how
fragile the existing code looks. The overall situation also prevents
any significant improvement to be successfully conducted, as changing
more than two lines will most likely lead to subtle regressions; see the
last few commits at
https://svnweb.freebsd.org/base/head/usr.bin/sed/process.c
and
http://cvsweb.netbsd.org/cgi-bin/cvsweb.cgi/src/usr.bin/sed/process.c
for recent examples of such failed attempts.
Anyway, until I digress even more, here is a patch for that one, forcing
`c' and `D' to put a NUL where `l' and `s' expect one to be. By the
way, whether the fault is on one side or the other is actually unclear.
It does seem redundant to enforce such a convention when one already has
access to the length of the corresponding string. As a matter of fact,
the overwhelming majority of the code does not appear to ever rely on a
NUL being there at all; for instance, `D' and `P' use memchr(3), not
strchr(3), which, besides, allows for the manipulation of NUL bytes
originally present in the input. Hence a more complete patch could
instead try
[PATCH] urndis: change M_WAITOK --> M_WAITOK | M_CANFAIL
Hi,
This diff passes M_CANFAIL to malloc(9) calls which use M_WAITOK but are
tested for failure. OK?
Index: sys/dev/usb/if_urndis.c
===
RCS file: /cvs/src/sys/dev/usb/if_urndis.c,v
retrieving revision 1.65
diff -u -p -u -p -r1.65 if_urndis.c
--- sys/dev/usb/if_urndis.c 8 Apr 2017 02:57:25 - 1.65
+++ sys/dev/usb/if_urndis.c 9 Jun 2017 07:30:32 -
@@ -408,7 +408,7 @@ urndis_ctrl_init(struct urndis_softc *sc
u_int32_trval;
struct rndis_comp_hdr *hdr;
- msg = malloc(sizeof(*msg), M_TEMP, M_WAITOK);
+ msg = malloc(sizeof(*msg), M_TEMP, M_WAITOK | M_CANFAIL);
if (msg == NULL) {
printf("%s: out of memory\n", DEVNAME(sc));
return RNDIS_STATUS_FAILURE;
@@ -454,7 +454,7 @@ urndis_ctrl_halt(struct urndis_softc *sc
struct rndis_halt_req *msg;
u_int32_trval;
- msg = malloc(sizeof(*msg), M_TEMP, M_WAITOK);
+ msg = malloc(sizeof(*msg), M_TEMP, M_WAITOK | M_CANFAIL);
if (msg == NULL) {
printf("%s: out of memory\n", DEVNAME(sc));
return RNDIS_STATUS_FAILURE;
@@ -488,7 +488,7 @@ urndis_ctrl_query(struct urndis_softc *s
u_int32_trval;
struct rndis_comp_hdr *hdr;
- msg = malloc(sizeof(*msg) + qlen, M_TEMP, M_WAITOK);
+ msg = malloc(sizeof(*msg) + qlen, M_TEMP, M_WAITOK | M_CANFAIL);
if (msg == NULL) {
printf("%s: out of memory\n", DEVNAME(sc));
return RNDIS_STATUS_FAILURE;
@@ -541,7 +541,7 @@ urndis_ctrl_set(struct urndis_softc *sc,
u_int32_trval;
struct rndis_comp_hdr *hdr;
- msg = malloc(sizeof(*msg) + len, M_TEMP, M_WAITOK);
+ msg = malloc(sizeof(*msg) + len, M_TEMP, M_WAITOK | M_CANFAIL);
if (msg == NULL) {
printf("%s: out of memory\n", DEVNAME(sc));
return RNDIS_STATUS_FAILURE;
@@ -605,7 +605,7 @@ urndis_ctrl_set_param(struct urndis_soft
else
namelen = 0;
tlen = sizeof(*param) + len + namelen;
- param = malloc(tlen, M_TEMP, M_WAITOK);
+ param = malloc(tlen, M_TEMP, M_WAITOK | M_CANFAIL);
if (param == NULL) {
printf("%s: out of memory\n", DEVNAME(sc));
return RNDIS_STATUS_FAILURE;
@@ -651,7 +651,7 @@ urndis_ctrl_reset(struct urndis_softc *s
u_int32_trval;
struct rndis_comp_hdr *hdr;
- reset = malloc(sizeof(*reset), M_TEMP, M_WAITOK);
+ reset = malloc(sizeof(*reset), M_TEMP, M_WAITOK | M_CANFAIL);
if (reset == NULL) {
printf("%s: out of memory\n", DEVNAME(sc));
return RNDIS_STATUS_FAILURE;
@@ -691,7 +691,7 @@ urndis_ctrl_keepalive(struct urndis_soft
u_int32_trval;
struct rndis_comp_hdr *hdr;
- keep = malloc(sizeof(*keep), M_TEMP, M_WAITOK);
+ keep = malloc(sizeof(*keep), M_TEMP, M_WAITOK | M_CANFAIL);
if (keep == NULL) {
printf("%s: out of memory\n", DEVNAME(sc));
return RNDIS_STATUS_FAILURE;
Re: tcpdump: drop atalk support
On Thu, Jun 08, 2017 at 09:42:44PM +0200, Michal Mazurek wrote: > Let's start by ignoring the existence of AppleTalk in the manpage, > reducing it by 10%. This leaves mention of atalk in the syntax of libpcap. > > A second diff will remove /etc/atalk.names support reducing the amount > of appletalk code significantly. > > Comments? OK? > no objection. aside from pcap, there is mention of atalk in gre(4), which you might want to look at. jmc > Index: usr.sbin/tcpdump/tcpdump.8 > === > RCS file: /cvs/src/usr.sbin/tcpdump/tcpdump.8,v > retrieving revision 1.92 > diff -u -p -r1.92 tcpdump.8 > --- usr.sbin/tcpdump/tcpdump.819 Apr 2017 05:36:13 - 1.92 > +++ usr.sbin/tcpdump/tcpdump.88 Jun 2017 19:36:14 - > @@ -1604,142 +1604,6 @@ requests, and matches them to the replie > .Pq transaction ID . > If a reply does not closely follow the corresponding request, > it might not be parsable. > -.Ss KIP AppleTalk (DDP in UDP) > -AppleTalk DDP packets encapsulated in UDP datagrams > -are de-encapsulated and dumped as DDP packets > -.Pq i.e., all the UDP header information is discarded . > -The file > -.Pa /etc/atalk.names > -is used to translate AppleTalk net and node numbers to names. > -Lines in this file have the form > -.Bl -column "number" "name" -offset indent > -.It Sy "number" Ta Ta Sy "name" > -.It "1.254" Ta Ta "ether" > -.It "16.1" Ta Ta "icsd-net" > -.It "1.254.110" Ta Ta "ace" > -.El > -.Pp > -The first two lines give the names of AppleTalk networks. > -The third line gives the name of a particular host > -(a host is distinguished from a net by the 3rd octet in the number; > -a net number > -.Em must > -have two octets and a host number > -.Em must > -have three octets). > -The number and name should be separated by whitespace (blanks or tabs). > -The > -.Pa /etc/atalk.names > -file may contain blank lines or comment lines > -(lines starting with a > -.Ql # ) . > -.Pp > -AppleTalk addresses are printed in the form > -.Pp > -.D1 Ar net . Ns Ar host . Ns Ar port > -.Pp > -For example: > -.Bd -unfilled -offset indent > -144.1.209.2 > icsd-net.112.220 > -office.2 > icsd-net.112.220 > -jssmag.149.235 > icsd-net.2 > -.Ed > -.Pp > -If > -.Pa /etc/atalk.names > -doesn't exist or doesn't contain an entry for some AppleTalk > -host/net number, addresses are printed in numeric form. > -In the first example, NBP > -.Pq DDP port 2 > -on net 144.1 node 209 > -is sending to whatever is listening on port 220 of net icsd-net node 112. > -The second line is the same except the full name of the source node is known > -.Pq Dq office . > -The third line is a send from port 235 on > -net jssmag node 149 to broadcast on the icsd-net NBP port. > -The broadcast address > -.Pq 255 > -is indicated by a net name with no host number; > -for this reason it is a good idea to keep node names and net names distinct > in > -.Pa /etc/atalk.names . > -.Pp > -NBP > -.Pq name binding protocol > -and ATP > -.Pq AppleTalk transaction protocol > -packets have their contents interpreted. > -Other protocols just dump the protocol name > -.Po > -or number if no name is registered for the protocol > -.Pc > -and packet size. > -.Pp > -NBP packets are formatted like the following examples: > -.Bd -unfilled > -icsd-net.112.220 > jssmag.2: nbp-lkup 190: "=:LaserWriter@*" > -jssmag.209.2 > icsd-net.112.220: nbp-reply 190: "RM1140:LaserWriter@*" 250 > -techpit.2 > icsd-net.112.220: nbp-reply 190: "techpit:LaserWriter@*" 186 > -.Ed > -.Pp > -The first line is a name lookup request for laserwriters sent by > -net icsdi-net host > -112 and broadcast on net jssmag. > -The nbp ID for the lookup is 190. > -The second line shows a reply for this request > -.Pq note that it has the same ID > -from host jssmag.209 saying that it has a laserwriter > -resource named RM1140 registered on port 250. > -The third line is another reply to the same request > -saying host techpit has laserwriter techpit registered on port 186. > -.Pp > -ATP packet formatting is demonstrated by the following example: > -.Bd -unfilled -offset indent > -jssmag.209.165 > helios.132: atp-req 12266<0-7> 0xae030001 > -helios.132 > jssmag.209.165: atp-resp 12266:0 (512) 0xae04 > -helios.132 > jssmag.209.165: atp-resp 12266:1 (512) 0xae04 > -helios.132 > jssmag.209.165: atp-resp 12266:2 (512) 0xae04 > -helios.132 > jssmag.209.165: atp-resp 12266:3 (512) 0xae04 > -helios.132 > jssmag.209.165: atp-resp 12266:4 (512) 0xae04 > -helios.132 > jssmag.209.165: atp-resp 12266:5 (512) 0xae04 > -helios.132 > jssmag.209.165: atp-resp 12266:6 (512) 0xae04 > -helios.132 > jssmag.209.165: atp-resp*12266:7 (512) 0xae04 > -jssmag.209.165 > helios.132: atp-req 12266<3,5> 0xae030001 > -helios.132 > jssmag.209.165: atp-resp 12266:3 (512) 0xae04 > -helios.132 > jssmag.209.165: atp-resp 12266:5 (512) 0xae04 > -jssmag.209.165 > helios.132: atp-rel 12266<0-7> 0xae030001 >
Re: monop(6): correct number of players in the man page
On Thu, Jun 08, 2017 at 11:37:51PM +0200, Frederic Cambus wrote: > Hi tech@, > > The man page says monop monitors a game between 1 to 9 users, but the > program enforces a range from 2 to 9. > > Comments? OK? > ok. jmc > Index: games/monop/monop.6 > === > RCS file: /cvs/src/games/monop/monop.6,v > retrieving revision 1.15 > diff -u -p -r1.15 monop.6 > --- games/monop/monop.6 13 Mar 2015 19:58:40 - 1.15 > +++ games/monop/monop.6 8 Jun 2017 21:15:46 - > @@ -29,7 +29,7 @@ > .\" > .\" @(#)monop.6 6.5 (Berkeley) 3/25/93 > .\" > -.Dd $Mdocdate: March 13 2015 $ > +.Dd $Mdocdate: June 8 2017 $ > .Dt MONOP 6 > .Os > .Sh NAME > @@ -41,7 +41,7 @@ > .Sh DESCRIPTION > .Nm > is reminiscent of the Parker Brother's game Monopoly, and > -monitors a game between 1 to 9 users. > +monitors a game between 2 to 9 users. > It is assumed that the rules of Monopoly are known. > The game follows the standard rules, with the exception that, > if a property goes up for auction and there are only two solvent players, >
