sndioctl.1: group is optional

2020-11-20 Thread Klemens Nanni
Not every control has a group as the manual wording says, i.e.

$ sndioctl
input.level=0.486
input.mute=0
output.level=1.000
output.mute=0
app/aucat0.level=1.000

Feedack? OK?


Index: sndioctl.1
===
RCS file: /cvs/src/usr.bin/sndioctl/sndioctl.1,v
retrieving revision 1.14
diff -u -p -r1.14 sndioctl.1
--- sndioctl.1  27 May 2020 17:34:45 -  1.14
+++ sndioctl.1  20 Nov 2020 16:46:45 -
@@ -72,8 +72,8 @@ The set of available controls depends on
 Commands use the following two formats to display and change
 controls respectively:
 .Pp
-.Dl group/stream[channel].function
-.Dl group/stream[channel].function=value
+.Dl [group/]stream[channel].function
+.Dl [group/]stream[channel].function=value
 .Pp
 On the left-hand side are specified the control group (if any),
 the affected stream name, and the optional channel number.



Re: AUDIORECDEVICE environment variable in sndio lib

2020-11-17 Thread Klemens Nanni
On Tue, Nov 17, 2020 at 06:23:55PM +0100, Peter J. Philipp wrote:
> On Tue, Nov 17, 2020 at 05:09:28PM +, Stuart Henderson wrote:
> > If AUDIORECDEVICE is unset, it would be better to fallback to
> > AUDIODEVICE rather than directly to devany.
> 
> This is a good suggestion!  Thanks!  I have updated the patch.  I also
> appreciate Solene's offer for manpage addition, thanks!
Afaict this will match existing behaviour after your patch with
AUDIORECDEVICE not being set, so that's good.



Re: ldom.conf.5: clarify vcpu strides

2020-11-04 Thread Klemens Nanni
On Wed, Nov 04, 2020 at 10:44:52PM +0100, Mark Kettenis wrote:
> Yeah, that reads better.  On request though.  Can you pick a character
> name from:
> 
> https://www.openbsd.org/lyrics.html#38
> 
> as the name of the domain?  Beluge is the bad guy, so this probably
> should be Marlus.
Sure!

Final version with Marlus, a missing .Pp and another grammar fix.
OK?


Index: ldom.conf.5
===
RCS file: /cvs/src/usr.sbin/ldomctl/ldom.conf.5,v
retrieving revision 1.14
diff -u -p -r1.14 ldom.conf.5
--- ldom.conf.5 14 Sep 2020 19:42:16 -  1.14
+++ ldom.conf.5 4 Nov 2020 22:11:45 -
@@ -38,8 +38,11 @@ If no configuration for the primary doma
 all CPU and memory resources not used by any guest domains.
 .It Ic vcpu Ar number Ns Op : Ns Ar stride
 Declare the number of virtual CPUs assigned to a domain.
-Optionally a stride can be specified to allocate additional virtual CPUs
-but not assign them to a domain.
+Optionally a stride can be specified to allocate
+.Ar stride
+VCPUs at a time but assign only
+.Ar number
+VCPUs to the domain.
 This can be used to distribute virtual CPUs over the available CPU cores.
 .It Ic memory Ar bytes
 Declare the amount of memory assigned to a domain, in bytes.
@@ -117,6 +120,21 @@ domain "salmah" {
 .Pp
 On a machine with 32 cores and 64GB physical memory, this leaves 12 cores and
 58GB memory to the primary domain.
+.Pp
+Use a
+.Ar stride
+step size to distribute VCPUs:
+.Bd -literal -offset indent
+domain "marlus" {
+   vcpu 2:4
+   memory 4G
+   vdisk "/home/marlus/vdisk0"
+}
+.Ed
+.Pp
+On a machine with eight threads per physical core, this allocates two strides
+of four VCPUs each for the guest domain but assigns only two VCPUs to it, i.e.
+makes it occupy an entire physical core while running on two threads only.
 .Sh SEE ALSO
 .Xr eeprom 8 ,
 .Xr ldomctl 8 ,



Re: ldom.conf.5: clarify vcpu strides

2020-11-04 Thread Klemens Nanni
On Wed, Nov 04, 2020 at 09:46:39PM +0100, Mark Kettenis wrote:
> stride is not a factor, so your description makes no sense to me.
ldomctl/config.c uses it as factor:

SIMPLEQ_FOREACH(domain, _list, entry) {
if (strcmp(domain->name, "primary") == 0) {
primary_num_cpus = domain->vcpu;
primary_stride = domain->vcpu_stride;
primary_memory = domain->memory;
}
num_cpus += (domain->vcpu * domain->vcpu_stride);
memory += domain->memory;
}

> a stride of 4 means we allocate VCPUs 4-at-a-time but only assign 1 of
> those to the domain.  It is a step size.
Let's reword, is that any better?


Index: ldom.conf.5
===
RCS file: /cvs/src/usr.sbin/ldomctl/ldom.conf.5,v
retrieving revision 1.14
diff -u -p -r1.14 ldom.conf.5
--- ldom.conf.5 14 Sep 2020 19:42:16 -  1.14
+++ ldom.conf.5 4 Nov 2020 21:37:20 -
@@ -38,8 +38,11 @@ If no configuration for the primary doma
 all CPU and memory resources not used by any guest domains.
 .It Ic vcpu Ar number Ns Op : Ns Ar stride
 Declare the number of virtual CPUs assigned to a domain.
-Optionally a stride can be specified to allocate additional virtual CPUs
-but not assign them to a domain.
+Optionally a stride can be specified to allocate
+.Ar stride
+VCPUs at a time but assign only
+.Ar number
+VCPUs to the domain.
 This can be used to distribute virtual CPUs over the available CPU cores.
 .It Ic memory Ar bytes
 Declare the amount of memory assigned to a domain, in bytes.
@@ -117,6 +120,20 @@ domain "salmah" {
 .Pp
 On a machine with 32 cores and 64GB physical memory, this leaves 12 cores and
 58GB memory to the primary domain.
+.Pp
+Use a
+.Ar stride
+step size to distribute VCPUs:
+.Bd -literal -offset indent
+domain "sun" {
+   vcpu 2:4
+   memory 4G
+   vdisk "/home/sun/vdisk0"
+}
+.Ed
+On a machine with eight threads per physical core, this allocates two strides
+of four VCPUs each for the guest domain but assigns only two VCPUs to it, i.e.
+make it occupy an entire physical core while running on two threads only.
 .Sh SEE ALSO
 .Xr eeprom 8 ,
 .Xr ldomctl 8 ,



Re: [PATCH] tcpdump: Fix missing argument from icmp_print call in print-skip.c

2020-11-04 Thread Klemens Nanni
On Tue, Nov 03, 2020 at 01:15:49PM +0100, Theo Buehler wrote:
> There is quite a bit more that is wrong with print-skip.c than just
> that (try to add it to the Makefile and compile it). It was unhooked
> from the build in 1996.
> 
> Shouldn't it rather be sent to the attic?
OK kn



Re: ldom.conf.5: clarify vcpu strides

2020-11-04 Thread Klemens Nanni
On Mon, Sep 14, 2020 at 07:52:34PM +0200, Klemens Nanni wrote:
> On Wed, Sep 02, 2020 at 04:58:39PM +0200, Stefan Sperling wrote:
> > I would like to suggest an example for the EXAMPLES section which
> > illustrates how a suitable stride factor can be determined (divide the
> > number of desired "unused" cpus by the number of desired "used" cpus):
> We can do with an example, but to me yours does not read obvious enough.
> 
> Also, `vcpu' denotes *virtual* CPUs inside domains, not CPUs on the
> machine, so "CPU" (without "V") reads off in your example and conflicts
> with the otherwise consistent mentions of "virtual CPUs" in this manual.
> 
> Here's my last diff incl. an example which reads a tad clearer to me and
> is placed in the EXAMPLES section instead.
> 
> Feedback? OK?
Ping.  Diff reattached.


Index: ldom.conf.5
===
RCS file: /cvs/src/usr.sbin/ldomctl/ldom.conf.5,v
retrieving revision 1.13
diff -u -p -r1.13 ldom.conf.5
--- ldom.conf.5 21 Feb 2020 19:39:28 -  1.13
+++ ldom.conf.5 14 Sep 2020 17:51:39 -
@@ -38,8 +38,13 @@ If no configuration for the primary doma
 all CPU and memory resources not used by any guest domains.
 .It Ic vcpu Ar number Ns Op : Ns Ar stride
 Declare the number of virtual CPUs assigned to a domain.
-Optionally a stride can be specified to allocate additional virtual CPUs
-but not assign them to a domain.
+Optionally a stride factor can be specified to allocate
+.Ar number
+virtual CPUs
+.Ar stride
+times but not assign more than
+.Ar number
+virtual CPUs to a domain, leaving the rest unassigned.
 This can be used to distribute virtual CPUs over the available CPU cores.
 .It Ic memory Ar bytes
 Declare the amount of memory assigned to a domain, in bytes.
@@ -112,6 +117,20 @@ domain "salmah" {
 .Pp
 On a machine with 32 cores and 64GB physical memory, this leaves 12 cores and
 58GB memory to the primary domain.
+.Pp
+Use
+.Ar stride
+factors to distribute virtual CPUs:
+.Bd -literal -offset indent
+domain "sun" {
+   vcpu 2:4
+   memory 4G
+   vdisk "/home/sun/vdisk0"
+}
+.Ed
+On a machine with eight threads per physical core, this allocates four strides
+of two virtual CPUs to the guest domain but only assigns one stride to it, i.e.
+make it occupy an entire physical core while running on only two threads.
 .Sh SEE ALSO
 .Xr eeprom 8 ,
 .Xr ldomctl 8 ,



Re: unwind(8): query_imsg2str

2020-11-04 Thread Klemens Nanni
On Wed, Nov 04, 2020 at 04:06:13PM +0100, Florian Obser wrote:
> Introduce query_imsg2str() for the printing "qname class type".
OK kn

> @@ -2116,3 +2107,18 @@ resolvers_to_restart(struct uw_conf *oconf, struct 
> uw_conf *nconf)
>   }
>   return restart;
>  }
> +
> +const char*
I'd put a space between "char" and "*".

> +query_imsg2str(struct query_imsg *query_imsg)
> +{



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

2020-11-04 Thread Klemens Nanni
On Tue, Oct 27, 2020 at 06:16:08PM +0100, Jason A. Donenfeld wrote:
> Somebody on IRC mentioned that using ifconfig to set wgallowedips wasn't
> working on macppc. I don't have a macppc to test this on, but it seems
> like the code is assuming that the two values printed out by this test
> program must always be the same:
> 
>   struct s {
>   int i;
>   };
> 
>   struct p {
>   long l;
>   char c;
>   struct s a[];
>   };
> 
>   int main(int argc, char *argv[])
>   {
>   printf("%zu %zu\n", sizeof(struct p), (size_t)&((struct p *)0)->a[0]);
>   return 0;
>   }
> 
> But actually, on my amd64 system, that little test prints out "16 12".
> This patch fixes up ifconfig.c to do the right thing, so that it
> corresponds with how the kernel handles iteration.
> 
> I don't have a macppc in order to test this, but it works on amd64.
Using the wg(4) EXAMPLES section as test, this fixes wg(4) on macppc.
amd64 continues to work for me as well.

FWIW, both platforms produced the same ifconfig.o regardless of using a
void pointer or char pointer cast as guenther suggested.  I don't know
if `char *' might be beneficial on other platform/compiler combinations.

OK?
Otherwise I'll commit this on friday unless I hear objections or further
feedback.

Jason's diff (with `char *' as per guenther) reattached below.


Index: ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.429
diff -u -p -r1.429 ifconfig.c
--- ifconfig.c  7 Oct 2020 14:38:54 -   1.429
+++ ifconfig.c  4 Nov 2020 19:35:20 -
@@ -5696,11 +5696,10 @@ ensurewginterface(void)
err(1, "calloc");
 }
 
-void *
+void
 growwgdata(size_t by)
 {
ptrdiff_t peer_offset, aip_offset;
-   void *ret;
 
if (wg_interface == NULL)
wgdata.wgd_size = sizeof(*wg_interface);
@@ -5721,16 +5720,18 @@ growwgdata(size_t by)
if (wg_aip != NULL)
wg_aip = (void *)wg_interface + aip_offset;
 
-   ret = (void *)wg_interface + wgdata.wgd_size - by;
-   bzero(ret, by);
-
-   return ret;
+   bzero((char *)wg_interface + wgdata.wgd_size - by, by);
 }
 
 void
 setwgpeer(const char *peerkey_b64, int param)
 {
-   wg_peer = growwgdata(sizeof(*wg_peer));
+   growwgdata(sizeof(*wg_peer));
+   if (wg_aip)
+   wg_peer = (struct wg_peer_io *)wg_aip;
+   else
+   wg_peer = _interface->i_peers[0];
+   wg_aip = _peer->p_aips[0];
wg_peer->p_flags |= WG_PEER_HAS_PUBLIC;
WG_LOAD_KEY(wg_peer->p_public, peerkey_b64, "wgpeer");
wg_interface->i_peers_count++;
@@ -5743,7 +5744,7 @@ setwgpeeraip(const char *aip, int param)
if (wg_peer == NULL)
errx(1, "wgaip: wgpeer not set");
 
-   wg_aip = growwgdata(sizeof(*wg_aip));
+   growwgdata(sizeof(*wg_aip));
 
if ((res = inet_net_pton(AF_INET, aip, _aip->a_ipv4,
sizeof(wg_aip->a_ipv4))) != -1) {
@@ -5759,6 +5760,8 @@ setwgpeeraip(const char *aip, int param)
 
wg_peer->p_flags |= WG_PEER_REPLACE_AIPS;
wg_peer->p_aips_count++;
+
+   wg_aip++;
 }
 
 void



Re: ukbd(4): support apple brightness keys

2020-10-29 Thread Klemens Nanni
On Wed, Oct 28, 2020 at 12:08:25AM +0100, Tobias Heider wrote:
> > What about KS_Cmd_BrightnessUp and KS_Cmd_BrightnessDown?
> 
> Right, here's a new diff using those wskbd commands.
> I couldn't find any standardized UHID key codes for brightness keys
> so I chose 232 and 233 which are currently unused and in the RESERVED range.
> 
> I also included the regenerated ukbdmap.c in the diff below for convenience.
Looks good to me and works in combination with your ofw_machdep diff.

OK kn



Re: macppc: fix initial wsconsctl display.brighness

2020-10-29 Thread Klemens Nanni
On Wed, Oct 28, 2020 at 09:06:39PM +0100, Tobias Heider wrote:
> Hi,
> 
> playing around with the display brightness i found that the
> initial state seems to be broken.
> We initiate the value at MAX_BRIGHTNESS while in reality it is much
> lower than that after boot.
> Increasing the brightness won't work after
> booting because wscons thinks we are at 100%, while decreasing the
> brightness in wsconsctl actually makes the display brighter...
> 
> The diff below fixes the problem by setting a sane default during
> initialization.
Yes, this fixes what I reported after testing (the other diff?).

OK kn



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

2020-10-28 Thread Klemens Nanni
On Tue, Oct 27, 2020 at 06:16:08PM +0100, Jason A. Donenfeld wrote:
> I don't have a macppc in order to test this, but it works on amd64.
Using the EXAMPLES code from wg(4) as is, this makes things work on
macppc.



Re: macppc: fix wsconsctl for non-vgafb

2020-10-27 Thread Klemens Nanni
On Mon, Oct 26, 2020 at 06:20:25PM +0100, Tobias Heider wrote:
> The diff below wires the ofw backlight commands to ws_get_param/ws_set_param
> and makes wsconsctl backlight/brightness work on my machine using radeondrm. 
This commit fixed brightness on my PowerBook5,8 but broke
display.contract in that wsconsctl(8) not fails for this knob:

# wsconsctl display.contrast
wsconsctl: WSDISPLAYIO_GETPARAM: Operation not permitted
display.contrast=0.00%
# wsconsctl display.contrast
wsconsctl: WSDISPLAYIO_GETPARAM: Operation not permitted
display.contrast=4294967293.4294967293%



Re: ukbd(4): support apple brightness keys

2020-10-27 Thread Klemens Nanni
On Tue, Oct 27, 2020 at 02:58:55PM +0100, Tobias Heider wrote:
> I think you're missing the previous diff, see
> https://marc.info/?l=openbsd-tech=160373302219962=2
Right, my bad.  -CURRENT with your diff works as expected in all regards.



Re: ukbd(4): support apple brightness keys

2020-10-27 Thread Klemens Nanni
On Tue, Oct 27, 2020 at 12:16:16AM +0100, Tobias Heider wrote:
> the diff below makes the brightness keys work on apple powerbooks > 5,6
> where the keyboard attaches via ukbd(4).
> I'm wondering if it would be better to go through wskbd as is done with
> audio, but we don't have keycodes allocated for brightness up/down.
Running the latest snapshot on my PowerBook5,8 the keys do not work,
neither does `wsconsctl display.brightness=50' or so.

Testing this before your diff, I noticed that the sensor values change
each time I print them (random stack garbage?), same thing when setting
values, i.e. setting brightness to 50 as above shows something like this

display.brightness -> 0.23948792%

Applying your diff changes nothing for me.



Re: push NET_LOCK() down in pf_ioctl.c

2020-10-20 Thread Klemens Nanni
On Tue, Oct 20, 2020 at 01:49:22AM +0200, Alexandr Nedvedicky wrote:
> Currently we need to keep pf_rm_rule() under both locks. The function
> might be calling pf_tag_unref(), pf_dynaddr_remove()... which alter lists,
> which are currently supposed to be protected by PF_LOCK()/NET_LOCK().
Yup.

> updated diff is below. pf_rm_rule() is being called with both locks held.
OK kn



Re: push NET_LOCK() down in pf_ioctl.c

2020-10-17 Thread Klemens Nanni
On Fri, Oct 16, 2020 at 11:37:22AM +0200, Alexandr Nedvedicky wrote:
> I've just found a forgotten diff in my tree. The diff pushes the NET_LCOK()
> further down in PF driver ioctl() path.  The idea is to avoid sleeping while
> holding a NET_LOCK().  this typically may happen when we need to allocate
> memory. The diff is the first step as it takes care of easy/straightforward
> cases of such allocations. The allocations, which still may happen under
> the NET_LOCK() require more work in areas:
> PF tables,
> packet queues,
> transactions,
> 
> the change is fairly large, but mostly mechanical.
Relocating malloc(9) and pool(9) seems good but other pf_*() calls are
now locked inconsistently after your diff.

Given that only reason about "allocations" this is either an oversight
and should be fixed or you need to provide more explanation.

See inline for the spots I'm talking about.

> diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
> index ef7d995e5a7..bac644fa6d1 100644
> --- a/sys/net/pf_ioctl.c
> +++ b/sys/net/pf_ioctl.c
> @@ -1006,10 +1006,10 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int 
> flags, struct proc *p)
>   return (EACCES);
>   }
>  
> - NET_LOCK();
>   switch (cmd) {
>  
>   case DIOCSTART:
> + NET_LOCK();
>   PF_LOCK();
>   if (pf_status.running)
>   error = EEXIST;
> @@ -1025,9 +1025,11 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int 
> flags, struct proc *p)
>   DPFPRINTF(LOG_NOTICE, "pf: started");
>   }
>   PF_UNLOCK();
> + NET_UNLOCK();
>   break;
>  
>   case DIOCSTOP:
> + NET_LOCK();
>   PF_LOCK();
>   if (!pf_status.running)
>   error = ENOENT;
> @@ -1038,6 +1040,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, 
> struct proc *p)
>   DPFPRINTF(LOG_NOTICE, "pf: stopped");
>   }
>   PF_UNLOCK();
> + NET_UNLOCK();
>   break;
>  
>   case DIOCGETQUEUES: {
> @@ -1045,6 +1048,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, 
> struct proc *p)
>   struct pf_queuespec *qs;
>   u_int32_tnr = 0;
>  
> + NET_LOCK();
>   PF_LOCK();
>   pq->ticket = pf_main_ruleset.rules.active.ticket;
>  
> @@ -1056,6 +1060,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, 
> struct proc *p)
>   }
>   pq->nr = nr;
>   PF_UNLOCK();
> + NET_UNLOCK();
>   break;
>   }
>  
> @@ -1064,10 +1069,12 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int 
> flags, struct proc *p)
>   struct pf_queuespec *qs;
>   u_int32_tnr = 0;
>  
> + NET_LOCK();
>   PF_LOCK();
>   if (pq->ticket != pf_main_ruleset.rules.active.ticket) {
>   error = EBUSY;
>   PF_UNLOCK();
> + NET_UNLOCK();
>   break;
>   }
>  
> @@ -1078,10 +1085,12 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int 
> flags, struct proc *p)
>   if (qs == NULL) {
>   error = EBUSY;
>   PF_UNLOCK();
> + NET_UNLOCK();
>   break;
>   }
>   memcpy(>queue, qs, sizeof(pq->queue));
>   PF_UNLOCK();
> + NET_UNLOCK();
>   break;
>   }
>  
> @@ -1091,10 +1100,12 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int 
> flags, struct proc *p)
>   u_int32_tnr;
>   int  nbytes;
>  
> + NET_LOCK();
>   PF_LOCK();
>   if (pq->ticket != pf_main_ruleset.rules.active.ticket) {
>   error = EBUSY;
>   PF_UNLOCK();
> + NET_UNLOCK();
>   break;
>   }
>   nbytes = pq->nbytes;
> @@ -1107,6 +1118,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, 
> struct proc *p)
>   if (qs == NULL) {
>   error = EBUSY;
>   PF_UNLOCK();
> + NET_UNLOCK();
>   break;
>   }
>   memcpy(>queue, qs, sizeof(pq->queue));
> @@ -1121,6 +1133,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, 
> struct proc *p)
>   if (error == 0)
>   pq->nbytes = nbytes;
>   PF_UNLOCK();
> + NET_UNLOCK();
>   break;
>   }
>  
> @@ -1128,38 +1141,44 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int 
> flags, struct proc *p)
>   struct pfioc_queue  *q = (struct pfioc_queue *)addr;
>   struct 

ssh: zap unused family parameter from ssh_connect_direct()

2020-10-11 Thread Klemens Nanni
CVS log shows that the following commit removed usage of it:

sshconnect.c
revision 1.241
date: 2013/10/16 02:31:46;  author: djm;  state: Exp;  lines: +29 -45;
Implement client-side hostname canonicalisation to allow an explicit
search path of domain suffixes to use to convert unqualified host names
to fully-qualified ones for host key matching.
[...]

So it is unused ever since in the only call chain:
ssh(1) main() -> ssh_connect() -> ssh_connect_direct().

I came here after reading the code when ssh(1)'s `-4' would not effect
jump hosts, i.e. `-J' or `ProxyJump'... only to find out later that I
didn't read the manual properly in the first place:

-J destination
[...]
Note that configuration directives supplied on the command-line
generally apply to the destination host and not any specified
jump hosts.  Use ~/.ssh/config to specify configuration for jump
hosts.

Compiles and works fine as before.
Feedback? Objections? OK?


Index: ssh.c
===
RCS file: /cvs/src/usr.bin/ssh/ssh.c,v
retrieving revision 1.537
diff -u -p -r1.537 ssh.c
--- ssh.c   3 Oct 2020 09:22:26 -   1.537
+++ ssh.c   10 Oct 2020 00:35:49 -
@@ -1521,7 +1521,7 @@ main(int ac, char **av)
 
/* Open a connection to the remote host. */
if (ssh_connect(ssh, host, host_arg, addrs, , options.port,
-   options.address_family, options.connection_attempts,
+   options.connection_attempts,
_ms, options.tcp_keep_alive) != 0)
exit(255);
 
Index: sshconnect.c
===
RCS file: /cvs/src/usr.bin/ssh/sshconnect.c,v
retrieving revision 1.339
diff -u -p -r1.339 sshconnect.c
--- sshconnect.c7 Oct 2020 02:26:28 -   1.339
+++ sshconnect.c10 Oct 2020 00:35:47 -
@@ -420,8 +420,8 @@ fail:
  */
 static int
 ssh_connect_direct(struct ssh *ssh, const char *host, struct addrinfo *aitop,
-struct sockaddr_storage *hostaddr, u_short port, int family,
-int connection_attempts, int *timeout_ms, int want_keepalive)
+struct sockaddr_storage *hostaddr, u_short port, int connection_attempts,
+int *timeout_ms, int want_keepalive)
 {
int on = 1, saved_timeout_ms = *timeout_ms;
int oerrno, sock = -1, attempt;
@@ -511,13 +511,13 @@ ssh_connect_direct(struct ssh *ssh, cons
 int
 ssh_connect(struct ssh *ssh, const char *host, const char *host_arg,
 struct addrinfo *addrs, struct sockaddr_storage *hostaddr, u_short port,
-int family, int connection_attempts, int *timeout_ms, int want_keepalive)
+int connection_attempts, int *timeout_ms, int want_keepalive)
 {
int in, out;
 
if (options.proxy_command == NULL) {
return ssh_connect_direct(ssh, host, addrs, hostaddr, port,
-   family, connection_attempts, timeout_ms, want_keepalive);
+   connection_attempts, timeout_ms, want_keepalive);
} else if (strcmp(options.proxy_command, "-") == 0) {
if ((in = dup(STDIN_FILENO)) == -1 ||
(out = dup(STDOUT_FILENO)) == -1) {
Index: sshconnect.h
===
RCS file: /cvs/src/usr.bin/ssh/sshconnect.h,v
retrieving revision 1.42
diff -u -p -r1.42 sshconnect.h
--- sshconnect.h7 Oct 2020 02:22:23 -   1.42
+++ sshconnect.h10 Oct 2020 00:36:25 -
@@ -35,7 +35,7 @@ struct ssh;
 
 int ssh_connect(struct ssh *, const char *, const char *,
struct addrinfo *, struct sockaddr_storage *, u_short,
-   int, int, int *, int);
+   int, int *, int);
 voidssh_kill_proxy_command(void);
 
 voidssh_login(struct ssh *, Sensitive *, const char *,



Re: doas: improve error message

2020-10-08 Thread Klemens Nanni
On Thu, Oct 08, 2020 at 04:23:53PM -0700, Jordan Geoghegan wrote:
> This improved error message would have been useful a few months ago where I
> had a number of end-users of one of my scripts get confused due to the
> cryptic error messages spit out by doas.
The diff does not change behaviour or output for end-users on the
command line;  instead it changes syslog messages which by default are
only readable by root.

As admin going through logs of multiple hosts (in a centralised place)
the proposed change clarifies that whatever was TRIED to be executed
never actually DID execute.



doas: improve error message

2020-10-08 Thread Klemens Nanni
In case `cmd' and `args' in doas.conf(5) do not match, the generated
log message is unclear and might be read as if the command executed but
failed, i.e. returned non-zero:

# cat /etc/doas.conf
permit nopass kn cmd echo args foo
$ doas echo foo
foo
$ doas echo bar
doas: Operation not permitted

The corresponding syslog(3) messages from /var/log/secure:

Oct  9 01:05:14 eru doas: kn ran command echo foo as root from /home/kn
Oct  9 01:05:20 eru doas: failed command for kn: echo bar

The following reads unambiguous and better matches the EPERM wording:

Oct  9 01:05:20 eru doas: command not permitted for kn: echo bar


Feedback? OK?


Index: doas.c
===
RCS file: /cvs/src/usr.bin/doas/doas.c,v
retrieving revision 1.82
diff -u -p -r1.82 doas.c
--- doas.c  18 Oct 2019 17:15:45 -  1.82
+++ doas.c  8 Oct 2020 22:59:45 -
@@ -396,7 +396,7 @@ main(int argc, char **argv)
if (!permit(uid, groups, ngroups, , target, cmd,
(const char **)argv + 1)) {
syslog(LOG_AUTHPRIV | LOG_NOTICE,
-   "failed command for %s: %s", mypw->pw_name, cmdline);
+   "command not permitted for %s: %s", mypw->pw_name, cmdline);
errc(1, EPERM, NULL);
}
 



Re: setitimer(2): ITIMER_REAL: simplify realitexpire()

2020-10-07 Thread Klemens Nanni
On Wed, Oct 07, 2020 at 03:34:36PM -0500, Scott Cheloha wrote:
> kn@ wanted to clean it up a while ago but I wan't far enough along
> with hi-res timeouts to change the code yet.
That was not me.



Re: ls: match historic behavior listing empty directories

2020-10-07 Thread Klemens Nanni
On Sat, Oct 03, 2020 at 09:21:21AM -0600, Todd C. Miller wrote:
> This is adapted from FreeBSD revs 130236 and 130237 which have the
> following log message:
> 
> If we are asked to print the total number of blocks, do so even if we
> have no entries to print (either due to an empty directory or an
> error).  This makes the -l and -s options more consistent, like
> Solaris and (Debian) Linux.  To make this happen, tweak two
> optimizations on the second call to display():
> 
> - Don't skip display() altogether, even if list == NULL.
> - Don't skip the call to the printfn in display() if we
>   need to print the total.
> 
> I've verified that both historic AT ls and GNU ls behave this
> way.
OK kn



Re: Make df output more human friendly in daily(8)

2020-10-02 Thread Klemens Nanni
On Fri, Oct 02, 2020 at 03:41:31PM -0400, Daniel Jakots wrote:
> On Fri, 2 Oct 2020 21:04:20 +0200, Ingo Schwarze 
> wrote:
> 
> > I certainly like this, and it works for me.
> > 
> > But i think a change like this would need more than one OK,
> > and you should wait some days such that developers can raise
> > objections.
> 
> Yes, of course.
> 
> > Just in case you get sufficient OKs and there are no serious
> > objections, see below for two suggested tweaks.
> 
> Thanks for the help!
That is OK with me, but you need to remove all mentions:

$ man -k any=VERBOSESTATUS
afterboot(8) - things to check after the first complete boot
daily, monthly, weekly(8) - periodic system maintenance



Re: diff: pfctl: error message for nonexisting rtable

2020-09-30 Thread Klemens Nanni
On Sun, Sep 20, 2020 at 07:29:38PM +0200, Klemens Nanni wrote:
> Rebased diff after yasouka's pfctl commit;  it still takes care of
> rdomains only, but I'd appreciate folks using `on rdomain' in their
> pf.conf test this.  If this works out I'd like to put it in shortly
> after release and work on rtables next.
Working for me so far, anyone else playing around with it?

I also checked CVS log and the existing rtable_l2() check goes back to
claudio's introduction of `on rdomain N' in pf, i.e. it's been there
from the start as a harmless but needless check and has not been added
after the fact to fix anything.

The time is now, so here's an updated diff after sashan pointed out how
I erroneously checked the rtable ID instead of the rdomain ID in the
ioctl (copy/pasta mistake).

Feedback? OK?


Index: sys/net/pf_ioctl.c
===
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.356
diff -u -p -r1.356 pf_ioctl.c
--- sys/net/pf_ioctl.c  24 Aug 2020 15:41:15 -  1.356
+++ sys/net/pf_ioctl.c  30 Sep 2020 20:48:34 -
@@ -2820,10 +2820,8 @@ pf_rule_copyin(struct pf_rule *from, str
if (to->rtableid >= 0 && !rtable_exists(to->rtableid))
return (EBUSY);
to->onrdomain = from->onrdomain;
-   if (to->onrdomain >= 0 && !rtable_exists(to->onrdomain))
-   return (EBUSY);
-   if (to->onrdomain >= 0) /* make sure it is a real rdomain */
-   to->onrdomain = rtable_l2(to->onrdomain);
+   if (to->onrdomain < 0 || to->onrdomain > RT_TABLEID_MAX)
+   return (EINVAL);
 
for (i = 0; i < PFTM_MAX; i++)
to->timeout[i] = from->timeout[i];
Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.703
diff -u -p -r1.703 parse.y
--- sbin/pfctl/parse.y  17 Sep 2020 14:26:59 -  1.703
+++ sbin/pfctl/parse.y  20 Sep 2020 17:28:10 -
@@ -1216,7 +1216,7 @@ antispoof_opt : LABEL label   {
if ($2 < 0 || $2 > RT_TABLEID_MAX) {
yyerror("invalid rtable id");
YYERROR;
-   } else if (lookup_rtable($2) < 1) {
+   } else if (!lookup_rtable($2)) {
yyerror("rtable %lld does not exist", $2);
YYERROR;
}
@@ -2003,7 +2003,7 @@ filter_opt: USER uids {
if ($2 < 0 || $2 > RT_TABLEID_MAX) {
yyerror("invalid rtable id");
YYERROR;
-   } else if (lookup_rtable($2) < 1) {
+   } else if (!lookup_rtable($2)) {
yyerror("rtable %lld does not exist", $2);
YYERROR;
}
@@ -2481,8 +2481,6 @@ if_item   : STRING{
| RDOMAIN NUMBER{
if ($2 < 0 || $2 > RT_TABLEID_MAX)
yyerror("rdomain %lld outside range", $2);
-   else if (lookup_rtable($2) != 2)
-   yyerror("rdomain %lld does not exist", $2);
 
$$ = calloc(1, sizeof(struct node_if));
if ($$ == NULL)
@@ -5899,10 +5897,6 @@ lookup_rtable(u_int rtableid)
return 0;
}
err(1, "%s", __func__);
-   }
-   if (info.rti_domainid == rtableid) {
-   found[rtableid] = 2;
-   return 2;
}
found[rtableid] = 1;
return 1;



fvwm: remove osrelease from system.fvwmrc

2020-09-29 Thread Klemens Nanni
This is the version string that shows up as part of "OpenBSD 6.8" in the
bottom right window in a default installation.

But as this version stems from the build machine's kernel with which
fvwm was build, it can differ from the version you're running on your
machine which is what you'd expect as user.

I do not use fvwm but this is irritating enough that I'd like to remove
the version number completely;  users can still set the entire string
in their ~/.fvwmrc or so.

The diff could be better but not without changing more of the build
system, hence keep the `all' target and continue using cat(1) and
redirection instead of using cp(1) and or building fvwm in a smarter way
in the first place.

Feedback? Objections? OK?


Index: app/fvwm//sample.fvwmrc/Makefile
===
RCS file: /cvs/xenocara/app/fvwm/sample.fvwmrc/Makefile,v
retrieving revision 1.7
diff -u -p -r1.7 Makefile
--- app/fvwm//sample.fvwmrc/Makefile6 Jan 2008 18:13:58 -   1.7
+++ app/fvwm//sample.fvwmrc/Makefile29 Sep 2020 18:23:16 -
@@ -2,14 +2,12 @@
 
 .include "../Makefile.inc"
 
-OSRELEASE=`/sbin/sysctl -n kern.osrelease 2>&1`
 RM?=rm
 
 depend:
 
 all:
-   @sed -e "s,__osrelease__,${OSRELEASE}," \
-   ${.CURDIR}/system.fvwmrc > .fvwmrc
+   @cat ${.CURDIR}/system.fvwmrc > .fvwmrc
 
 install:
${INSTALL_DATA} .fvwmrc \
Index: app/fvwm//sample.fvwmrc/system.fvwmrc
===
RCS file: /cvs/xenocara/app/fvwm/sample.fvwmrc/system.fvwmrc,v
retrieving revision 1.8
diff -u -p -r1.8 system.fvwmrc
--- app/fvwm//sample.fvwmrc/system.fvwmrc   10 Sep 2012 13:52:04 -  
1.8
+++ app/fvwm//sample.fvwmrc/system.fvwmrc   29 Sep 2020 18:21:42 -
@@ -352,7 +352,7 @@ Key F8  A   M   CirculateDown
 *FvwmPagerFore white
 *FvwmPagerHilight #2d2d2d
 *FvwmPagerGeometry 80x60-1-1
-*FvwmPagerLabel 0 "OpenBSD __osrelease__"
+*FvwmPagerLabel 0 "OpenBSD"
 *FvwmPagerLabel 1 Maker
 *FvwmPagerLabel 2 Mail
 *FvwmPagerLabel 3 Matlab



rdomain.4: on removing rtables

2020-09-22 Thread Klemens Nanni
We have never been able to remove an rtable;  until claudio moved them
explicitly with rtable_l2set() in  if_loop.c:loop_clone_destroy(), i.e.

revision 1.90
date: 2020/01/08 09:09:10;  author: claudio;  state: Exp;  lines: +6 -2;
In loop_clone_destroy() reset the rdomain with rtable_l2set() after
the if_detach() call. In if_detach() various route messages are 
generated
and during that time the rtable_l2() mapping needs to stay.
OK kn@

it would still exist but not be assigned to any valid rdomain. Back then this
could be obvserved with `route -T1 ...' still "working" after having
destroyed lo1.

Reverting claudio's commit on -CURRENT, that is with `netstat -R' now
available, confirms this:

# sysctl kern.version
kern.version=OpenBSD 6.8-beta (GENERIC) #0: Tue Sep 22 21:24:48 CEST 
2020
kn@eru:/sys/arch/amd64/compile/GENERIC
# ifconfig lo1 rdomain 1
# netstat -R
Rdomain 0
  Interfaces: lo0 vio0
  Routing table: 0

Rdomain 1
  Interface: lo1
  Routing table: 1

# ifconfig lo1 destroy
# netstat -R
Rdomain 0
  Interfaces: lo0 vio0
  Routing table: 0

# route -T1 show
Routing tables
# echo $?
0


This is not documented anywhere and I'd certainly not expect it after
reading rtable(4).  The manual says we can delete rdomains and is quiet
about deleting rtables, which can imply that rtables cannot be deleted
but might also imply that rtables are deleted automatically when
rdomains are deleted.

Either way, explicit is better here, I think.

Feedback? OK?


Index: rdomain.4
===
RCS file: /cvs/src/share/man/man4/rdomain.4,v
retrieving revision 1.14
diff -u -p -r1.14 rdomain.4
--- rdomain.4   30 Jul 2020 21:44:34 -  1.14
+++ rdomain.4   22 Sep 2020 19:58:57 -
@@ -146,3 +146,5 @@ and IPv6 support first appeared in
 .Sh CAVEATS
 No tool is available to assign more than one rtable to an rdomain
 other than to the default one (0).
+An rtable cannot be deleted.
+Deleting an rdomain will move its rtable into the default rdomain.



rdomain.4: add netstat -R example

2020-09-22 Thread Klemens Nanni
It's handy and otherwise easily missed when reading up on routing
domains and tables;  wording taken from netstat(1) as is.

Not listing pgrep(1)'s `-T' because examples don't have to be exhaustive
and ps(1) is already demonstrated;  same for top(1) users which more
likely come across its `t' and `T' in the help page anyway (I guess).

Feedback? OK?


Index: rdomain.4
===
RCS file: /cvs/src/share/man/man4/rdomain.4,v
retrieving revision 1.14
diff -u -p -r1.14 rdomain.4
--- rdomain.4   30 Jul 2020 21:44:34 -  1.14
+++ rdomain.4   22 Sep 2020 18:51:29 -
@@ -98,6 +98,10 @@ Put em0 and lo4 in rdomain 4:
 # ifconfig em0 192.0.2.100/24
 .Ed
 .Pp
+List all rdomains with associated interfaces and routing tables:
+.Pp
+.Dl # netstat -R
+.Pp
 Set a default route and localhost reject route within rtable 4:
 .Bd -literal -offset indent
 # route -T4 -qn add -net 127 127.0.0.1 -reject
@@ -129,6 +133,7 @@ Delete rdomain 4 again:
 # ifconfig lo4 destroy
 .Ed
 .Sh SEE ALSO
+.Xr netstat 1 ,
 .Xr ps 1 ,
 .Xr lo 4 ,
 .Xr route 4 ,



Re: fix eeprom(8) on macppc

2020-09-20 Thread Klemens Nanni
On Sun, Sep 20, 2020 at 06:01:08PM +, Miod Vallat wrote:
> I had noticed for years that eeprom(8) always reported failure when
> attempting to change OpenFirmware environment variables on macppc.
> 
> Upon further examination, it doesn't - the variables get changed, but
> error is reported. This is caused by a difference in implementation
> behaviour between Apple's OpenFirmware and Sun's.
> 
> Suggested diff below; clue from NetBSD.
I looked for openprom code there but only found it for SPARC, can you
point me to where you got the "clue" from exactly?

> Before:
> # eeprom boot-command
> boot-command=mac-boot
> # eeprom boot-command=boot
> eeprom: invalid keyword: boot-command
> # eeprom boot-command
> boot-command=boot   <-- yet the value has been changed
> 
> After:
> # eeprom boot-command
> boot-command=mac-boot
> # eeprom boot-command=boot
> # eeprom boot-command
> boot-command=boot
Confirmed on my PowerBook G4.

OK kn



Re: diff: pfctl: error message for nonexisting rtable

2020-09-20 Thread Klemens Nanni
On Tue, Sep 15, 2020 at 02:31:24AM +0200, Klemens Nanni wrote:
> On Tue, Sep 15, 2020 at 12:30:35AM +0200, Klemens Nanni wrote:
> > Actually, that should just work regardless of whether the rounting
> > domain exists at ruleset creation time;  just like it is the case with
> > interface names/groups which may come and go at runtime without
> > requiring changes to the ruleset.
> > 
> > Rules on nonexistent interfaces won't match, routing domains (and
> > ultimately routing tables) should behave the same, I think.
> > 
> > Here's a diff that does this for routing domains allowing me to always
> > use `on rdomain 5' - I've tested it with a few examplatory rulesets and
> > behaviour is as expected.
> > 
> > It will need more eye balling and I am not pushing such changes before
> > release, but if that is a general direction we agree, your proposed
> > `rtable' fix could move along and become just as flexible instead.
> More on this:
> 
>   # ifconfig lo1 rdomain 1
>   # echo pass on rdomain 1 | pfctl -f-
>   # ifconfig lo1 destroy
>   # pfctl -sr 
>  
>   pass on rdomain 1 all flags S/SA
> 
> The ruleset stays valid and continues to work as soon as routing domain
> `1' reappears, there is no reason to require existence of it at ruleset
> creation;  this is safe because routing domains are just normative
> numbers, there's no further state when it comes to filtering - either
> the id on the packet matches the number in the ruleset or it doesn't.
> 
> Routing tables however are more involved as they can be used to *alter*
> a packet's flow in pf.conf(5), so requiring them to be present at
> ruleset creation makes sense to guarantee that pf will only ever change
> routing table ids to valid ones.
> 
> Routing domains can be deleted, but that doesn't invalidate rules like
> `on rdomain 1', which simply won't match when the given id does not
> exist.
> 
> Routing tables however cannot be deleted, they get moved to the default
> routing domain whenever their corresponding routing domain disappears;
> this is in line with only ever loading valid routing table ids into pf.
> 
> So unless I missed something, that ruleset creation (`pfctl -f ...')
> is the only occasion pf actually needs to validate routing table ids:
> they are guaranteed to always exist from then on.
> 
> Given this, my diff looks fine as is and should not change `rtable'
> behaviour - YASUOKA's diff is also fine as is and actually implements
> the validity check I just mentioned, obsoleting my initial feedback.

Rebased diff after yasouka's pfctl commit;  it still takes care of
rdomains only, but I'd appreciate folks using `on rdomain' in their
pf.conf test this.  If this works out I'd like to put it in shortly
after release and work on rtables next.


Index: sys/net/pf_ioctl.c
===
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.356
diff -u -p -r1.356 pf_ioctl.c
--- sys/net/pf_ioctl.c  24 Aug 2020 15:41:15 -  1.356
+++ sys/net/pf_ioctl.c  14 Sep 2020 22:27:55 -
@@ -2820,10 +2820,8 @@ pf_rule_copyin(struct pf_rule *from, str
if (to->rtableid >= 0 && !rtable_exists(to->rtableid))
return (EBUSY);
to->onrdomain = from->onrdomain;
-   if (to->onrdomain >= 0 && !rtable_exists(to->onrdomain))
-   return (EBUSY);
-   if (to->onrdomain >= 0) /* make sure it is a real rdomain */
-   to->onrdomain = rtable_l2(to->onrdomain);
+   if (to->rtableid < 0 || to->rtableid > RT_TABLEID_MAX)
+   return (EINVAL);
 
for (i = 0; i < PFTM_MAX; i++)
to->timeout[i] = from->timeout[i];
Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.703
diff -u -p -r1.703 parse.y
--- sbin/pfctl/parse.y  17 Sep 2020 14:26:59 -  1.703
+++ sbin/pfctl/parse.y  20 Sep 2020 17:28:10 -
@@ -1216,7 +1216,7 @@ antispoof_opt : LABEL label   {
if ($2 < 0 || $2 > RT_TABLEID_MAX) {
yyerror("invalid rtable id");
YYERROR;
-   } else if (lookup_rtable($2) < 1) {
+   } else if (!lookup_rtable($2)) {
yyerror("rtable %lld does not exist", $2);
YYERROR;
}
@@ -2003,7 +2003,7 @@ filter_opt: USER uids {
if ($2 < 0 || $2 > RT_TABLEID_MAX) {

Re: ksh "clear-screen" for vi mode

2020-09-20 Thread Klemens Nanni
On Sun, Sep 20, 2020 at 06:14:22AM -0600, Todd C. Miller wrote:
> On Sun, 20 Sep 2020 05:39:02 +0200, Theo Buehler wrote:
> 
> > This works and appears to match bash's behavior in that it only works
> > in normal mode. I would slightly prefer to also add the command to the
> > nonstandard vi commands in the switch around line 650 to have it
> > available from insert mode as well. This would match zsh's behavior.
> 
> Sure, I was comparing it to bash so didn't support insert mode.
> I agree that it's more useful to have it available there too.
Works as expected, I cannot spot any regression either.

OK kn, but the could need further tweaks.

> Index: bin/ksh/ksh.1
> ===
> RCS file: /cvs/src/bin/ksh/ksh.1,v
> retrieving revision 1.209
> diff -u -p -u -r1.209 ksh.1
> --- bin/ksh/ksh.1 7 Jul 2020 10:33:58 -   1.209
> +++ bin/ksh/ksh.1 20 Sep 2020 12:12:01 -
> @@ -5053,6 +5053,12 @@ Erases previous character.
>  .It ^J | ^M
>  End of line.
>  The current line is read, parsed, and executed by the shell.
> +.It ^L
> +Clear screen.
> +The screen is cleared if the
> +.Ev TERM
> +parameter is set and the terminal supports clearing the screen.
> +The prompt string and the current line are redrawn.
This duplicates the `clear-screen' documentation in the
"Emacs editing mode" paragraph, perhaps just refer to that?

Otherwise it seems harder than necessary to grep for the same
functionality in two places.

Also, at the end of "Vi editing mode" there's another mention of `^L'
which needs updating now.

>  .It ^V
>  Literal next.
>  The next character typed is not treated specially (can be used



Re: ksh "clear-screen" for vi mode

2020-09-20 Thread Klemens Nanni
On Sun, Sep 20, 2020 at 05:39:02AM +0200, Theo Buehler wrote:
> On Sat, Sep 19, 2020 at 03:50:52PM -0600, Todd C. Miller wrote:
> > The vi and emacs edit code are completely separate.  Try the following
> > diff.  I had to rename a few things to avoid clashing with ncurses.h.
> 
> This works and appears to match bash's behavior in that it only works
> in normal mode. I would slightly prefer to also add the command to the
> nonstandard vi commands in the switch around line 650 to have it
> available from insert mode as well. This would match zsh's behavior.
Exactly.

> Either way, ok tb
OK kn for this diff just works and everything else is extra.



Re: diff: pfctl: error message for nonexisting rtable

2020-09-16 Thread Klemens Nanni
On Wed, Sep 16, 2020 at 07:49:19PM +0900, YASUOKA Masahiko wrote:
> New diff is using -1 for ENOENT.
> 
> Also domainid == 0 is a valid domain id, but previous diff cannot make
> a cache of it since 0 is the default value.  So new diff is doing
> 
> - static u_int found[RT_TABLEID_MAX+1];
> + static struct {
> + int  found;
> + int  domainid;
> + }rtables[RT_TABLEID_MAX+1];
> 
> to distinguish the default 0 and domainid 0.
This looks more complicated than it needs to be, but I also don't want
to bikeshed it;  given that the parser is happy with this and we plan to
remove this code alltogether anyway in the next release cycle:  OK kn.

Alternatively, here's a much simpler diff resembling what I had in mind.
Feel free to commit this instead (with my OK), give me an OK for it or
go ahead with yours.

It uses the same function and reflects the fact that every rdomain is a
rtable but not every rtable is also a rdomain (your choice of `domainid'
seems inconsistent with that).

Index: parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.701
diff -u -p -r1.701 parse.y
--- parse.y 28 Jan 2020 15:40:35 -  1.701
+++ parse.y 16 Sep 2020 13:58:23 -
@@ -392,7 +392,7 @@ int  invalid_redirect(struct node_host *
 u_int16_t parseicmpspec(char *, sa_family_t);
 int kw_casecmp(const void *, const void *);
 int map_tos(char *string, int *);
-int rdomain_exists(u_int);
+int lookup_rtable(u_int);
 int filteropts_to_rule(struct pf_rule *, struct filter_opts *);
 
 TAILQ_HEAD(loadanchorshead, loadanchors)
@@ -1216,6 +1216,9 @@ antispoof_opt : LABEL label   {
if ($2 < 0 || $2 > RT_TABLEID_MAX) {
yyerror("invalid rtable id");
YYERROR;
+   } else if (lookup_rtable($2) >= 1) {
+   yyerror("rtable %lld does not exist", $2);
+   YYERROR;
}
antispoof_opts.rtableid = $2;
}
@@ -2000,6 +2003,9 @@ filter_opt: USER uids {
if ($2 < 0 || $2 > RT_TABLEID_MAX) {
yyerror("invalid rtable id");
YYERROR;
+   } else if (lookup_rtable($2) >= 1) {
+   yyerror("rtable %lld does not exist", $2);
+   YYERROR;
}
filter_opts.rtableid = $2;
}
@@ -2475,7 +2481,7 @@ if_item   : STRING{
| RDOMAIN NUMBER{
if ($2 < 0 || $2 > RT_TABLEID_MAX)
yyerror("rdomain %lld outside range", $2);
-   else if (rdomain_exists($2) != 1)
+   else if (lookup_rtable($2) != 2)
yyerror("rdomain %lld does not exist", $2);
 
$$ = calloc(1, sizeof(struct node_if));
@@ -5868,37 +5874,38 @@ map_tos(char *s, int *val)
 }
 
 int
-rdomain_exists(u_int rdomain)
+lookup_rtable(u_int rtableid)
 {
size_t   len;
struct rt_tableinfo  info;
int  mib[6];
static u_int found[RT_TABLEID_MAX+1];
 
-   if (found[rdomain] == 1)
-   return 1;
+   if (found[rtableid])
+   return found[rtableid];
 
mib[0] = CTL_NET;
mib[1] = PF_ROUTE;
mib[2] = 0;
mib[3] = 0;
mib[4] = NET_RT_TABLE;
-   mib[5] = rdomain;
+   mib[5] = rtableid;
 
len = sizeof(info);
if (sysctl(mib, 6, , , NULL, 0) == -1) {
if (errno == ENOENT) {
/* table nonexistent */
+   found[rtableid] = 0;
return 0;
}
err(1, "%s", __func__);
}
-   if (info.rti_domainid == rdomain) {
-   found[rdomain] = 1;
-   return 1;
+   if (info.rti_domainid == rtableid) {
+   found[rtableid] = 2;
+   return 2;
}
-   /* rdomain is a table, but not an rdomain */
-   return 0;
+   found[rtableid] = 1;
+   return 1;
 }
 
 int



Re: diff: pfctl: error message for nonexisting rtable

2020-09-16 Thread Klemens Nanni
On Wed, Sep 16, 2020 at 06:22:00PM +0900, YASUOKA Masahiko wrote:
> Let me continue this separetely.
Yes, let's get your diff in for release and then work out the other
approach.

> Make pfctl check if the rtable really exists when parsing the config.
The diff is a bit hard to read (nothing you can do), but after applying
the code reads good in principal.

> Index: sbin/pfctl/parse.y
> ===
> RCS file: /cvs/src/sbin/pfctl/parse.y,v
> retrieving revision 1.701
> diff -u -p -r1.701 parse.y
> --- sbin/pfctl/parse.y28 Jan 2020 15:40:35 -  1.701
> +++ sbin/pfctl/parse.y16 Sep 2020 09:11:21 -
> @@ -392,7 +392,9 @@ intinvalid_redirect(struct node_host *
>  u_int16_t parseicmpspec(char *, sa_family_t);
>  int   kw_casecmp(const void *, const void *);
>  int   map_tos(char *string, int *);
> +int   get_domainid(u_int);
>  int   rdomain_exists(u_int);
> +int   rtable_exists(u_int);
>  int   filteropts_to_rule(struct pf_rule *, struct filter_opts *);
>  
>  TAILQ_HEAD(loadanchorshead, loadanchors)
> @@ -1217,6 +1219,10 @@ antispoof_opt  : LABEL label   {
>   yyerror("invalid rtable id");
>   YYERROR;
>   }
> + else if (rtable_exists($2) != 1) {
Using the function verb would reads a bit clearer/more intuitive, i.e.

else if (!rtable_exists($2)) {

> + yyerror("rtable %lld does not exist", $2);
> + YYERROR;
> + }
>   antispoof_opts.rtableid = $2;
>   }
>   ;
> @@ -2001,6 +2007,10 @@ filter_opt : USER uids {
>   yyerror("invalid rtable id");
>   YYERROR;
>   }
> + else if (rtable_exists($2) != 1) {
Same here.

> + yyerror("rtable %lld does not exist", $2);
> + YYERROR;
> + }
>   filter_opts.rtableid = $2;
>   }
>   | DIVERTTO STRING PORT portplain {
> @@ -5868,15 +5878,15 @@ map_tos(char *s, int *val)
>  }
>  
>  int
> -rdomain_exists(u_int rdomain)
> +get_domainid(u_int rdomain)
>  {
>   size_t   len;
>   struct rt_tableinfo  info;
>   int  mib[6];
> - static u_int found[RT_TABLEID_MAX+1];
> + static u_int domainid[RT_TABLEID_MAX+1];
>  
> - if (found[rdomain] == 1)
> - return 1;
> + if (domainid[rdomain] != 0)
> + return domainid[rdomain];
>  
>   mib[0] = CTL_NET;
>   mib[1] = PF_ROUTE;
> @@ -5887,17 +5897,37 @@ rdomain_exists(u_int rdomain)
>  
>   len = sizeof(info);
>   if (sysctl(mib, 6, , , NULL, 0) == -1) {
> - if (errno == ENOENT) {
> + if (errno == ENOENT)
>   /* table nonexistent */
> - return 0;
> - }
> - err(1, "%s", __func__);
> - }
> - if (info.rti_domainid == rdomain) {
> - found[rdomain] = 1;
> + domainid[rdomain] = RT_TABLEID_MAX;
This does not look correct, RT_TABLEID_MAX (255) is the biggest *valid*
id, so you cannot use it to denote a nonexistent routing table.

$ doas ifconfig lo255 rdomain 255
$ netstat -R | grep 255
Rdomain 255
  Interface: lo255
  Routing table: 255

Perhaps use `static int domainid[RT_TABLEID_MAX+1]' and `-1' to reflect
ENOENT?

> + else
> + err(1, "%s", __func__);
> + } else
> + domainid[rdomain] = info.rti_domainid;
> +
> + return domainid[rdomain];
> +}
> +
> +int
> +rdomain_exists(u_int rdomain)
> +{
> + int domainid;
> +
> + domainid = get_domainid(rdomain);
> + if (domainid == rdomain)
>   return 1;
> - }
>   /* rdomain is a table, but not an rdomain */
> + return 0;
> +}
> +
> +int
> +rtable_exists(u_int rtable)
> +{
> + int domainid;
> +
> + domainid = get_domainid(rtable);
> + if (domainid < RT_TABLEID_MAX)
As per above, RT_TABLEID_MAX is valid.

> + return 1;
>   return 0;
>  }



Re: diff: pfctl: error message for nonexisting rtable

2020-09-15 Thread Klemens Nanni
On Tue, Sep 15, 2020 at 12:42:27PM +0900, YASUOKA Masahiko wrote:
> It's not clear for me why non-existing rdomain is accepted but
> non-existing rtable is rejected.  I suppose we can make pf(4) can
> handle a packet for the non-existing routing table as if the routing
> table is empty.
Probably possible, but not without further tests or even changes to pf;
I did not want to imply that dynamic `rtable' in pf.conf cannot be done.



Re: trunk: keep interface up on port removal

2020-09-15 Thread Klemens Nanni
On Mon, Sep 14, 2020 at 10:57:16AM +0200, Klemens Nanni wrote:
> I tested removing a single port from trunk and observed that both
> interfaces do end up with the same MAC address, but this happens without
> my diff already - I still don't see any behaviour after my diff wrt. MAC
> addresses or anything else but the UP flag on port interfaces.
I did sloppy testing... tl;dr: MACs have been restored properly before
my diff already.

I looked at this yet again because removing a single port and destroying
the trunk use the same code path, hence it didn't make much sense to me
for them to behave differently on second thought.

No matter which port is removed, its MAC is reset to what it was before
becoming a trunk port.

I refrained from inlining supporting shell code/output this time, please
test yourself to confirm.



Re: diff: pfctl: error message for nonexisting rtable

2020-09-14 Thread Klemens Nanni
On Tue, Sep 15, 2020 at 12:30:35AM +0200, Klemens Nanni wrote:
> Actually, that should just work regardless of whether the rounting
> domain exists at ruleset creation time;  just like it is the case with
> interface names/groups which may come and go at runtime without
> requiring changes to the ruleset.
> 
> Rules on nonexistent interfaces won't match, routing domains (and
> ultimately routing tables) should behave the same, I think.
> 
> Here's a diff that does this for routing domains allowing me to always
> use `on rdomain 5' - I've tested it with a few examplatory rulesets and
> behaviour is as expected.
> 
> It will need more eye balling and I am not pushing such changes before
> release, but if that is a general direction we agree, your proposed
> `rtable' fix could move along and become just as flexible instead.
More on this:

# ifconfig lo1 rdomain 1
# echo pass on rdomain 1 | pfctl -f-
# ifconfig lo1 destroy
# pfctl -sr 
 
pass on rdomain 1 all flags S/SA

The ruleset stays valid and continues to work as soon as routing domain
`1' reappears, there is no reason to require existence of it at ruleset
creation;  this is safe because routing domains are just normative
numbers, there's no further state when it comes to filtering - either
the id on the packet matches the number in the ruleset or it doesn't.

Routing tables however are more involved as they can be used to *alter*
a packet's flow in pf.conf(5), so requiring them to be present at
ruleset creation makes sense to guarantee that pf will only ever change
routing table ids to valid ones.

Routing domains can be deleted, but that doesn't invalidate rules like
`on rdomain 1', which simply won't match when the given id does not
exist.

Routing tables however cannot be deleted, they get moved to the default
routing domain whenever their corresponding routing domain disappears;
this is in line with only ever loading valid routing table ids into pf.

So unless I missed something, that ruleset creation (`pfctl -f ...')
is the only occasion pf actually needs to validate routing table ids:
they are guaranteed to always exist from then on.

Given this, my diff looks fine as is and should not change `rtable'
behaviour - YASUOKA's diff is also fine as is and actually implements
the validity check I just mentioned, obsoleting my initial feedback.



Re: diff: pfctl: error message for nonexisting rtable

2020-09-14 Thread Klemens Nanni
On Mon, Sep 14, 2020 at 02:09:27PM +0900, YASUOKA Masahiko wrote:
> When pf rule with a "on rdomain n" with nonexisting rdomain n causes
> 
>   /etc/pf.conf:XXX: rdomain n does not exist
Actually, that should just work regardless of whether the rounting
domain exists at ruleset creation time;  just like it is the case with
interface names/groups which may come and go at runtime without
requiring changes to the ruleset.

Rules on nonexistent interfaces won't match, routing domains (and
ultimately routing tables) should behave the same, I think.

Here's a diff that does this for routing domains allowing me to always
use `on rdomain 5' - I've tested it with a few examplatory rulesets and
behaviour is as expected.

It will need more eye balling and I am not pushing such changes before
release, but if that is a general direction we agree, your proposed
`rtable' fix could move along and become just as flexible instead.

Discussed with claudio at k2k20.


Index: /sys/net/pf_ioctl.c
===
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.356
diff -u -p -r1.356 pf_ioctl.c
--- /sys/net/pf_ioctl.c 24 Aug 2020 15:41:15 -  1.356
+++ /sys/net/pf_ioctl.c 14 Sep 2020 22:27:55 -
@@ -2820,10 +2820,8 @@ pf_rule_copyin(struct pf_rule *from, str
if (to->rtableid >= 0 && !rtable_exists(to->rtableid))
return (EBUSY);
to->onrdomain = from->onrdomain;
-   if (to->onrdomain >= 0 && !rtable_exists(to->onrdomain))
-   return (EBUSY);
-   if (to->onrdomain >= 0) /* make sure it is a real rdomain */
-   to->onrdomain = rtable_l2(to->onrdomain);
+   if (to->rtableid < 0 || to->rtableid > RT_TABLEID_MAX)
+   return (EINVAL);
 
for (i = 0; i < PFTM_MAX; i++)
to->timeout[i] = from->timeout[i];
Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.701
diff -u -p -r1.701 parse.y
--- sbin/pfctl/parse.y  28 Jan 2020 15:40:35 -  1.701
+++ sbin/pfctl/parse.y  14 Sep 2020 21:52:54 -
@@ -392,7 +392,6 @@ int  invalid_redirect(struct node_host *
 u_int16_t parseicmpspec(char *, sa_family_t);
 int kw_casecmp(const void *, const void *);
 int map_tos(char *string, int *);
-int rdomain_exists(u_int);
 int filteropts_to_rule(struct pf_rule *, struct filter_opts *);
 
 TAILQ_HEAD(loadanchorshead, loadanchors)
@@ -2475,8 +2474,6 @@ if_item   : STRING{
| RDOMAIN NUMBER{
if ($2 < 0 || $2 > RT_TABLEID_MAX)
yyerror("rdomain %lld outside range", $2);
-   else if (rdomain_exists($2) != 1)
-   yyerror("rdomain %lld does not exist", $2);
 
$$ = calloc(1, sizeof(struct node_if));
if ($$ == NULL)
@@ -5865,40 +5862,6 @@ map_tos(char *s, int *val)
return (1);
}
return (0);
-}
-
-int
-rdomain_exists(u_int rdomain)
-{
-   size_t   len;
-   struct rt_tableinfo  info;
-   int  mib[6];
-   static u_int found[RT_TABLEID_MAX+1];
-
-   if (found[rdomain] == 1)
-   return 1;
-
-   mib[0] = CTL_NET;
-   mib[1] = PF_ROUTE;
-   mib[2] = 0;
-   mib[3] = 0;
-   mib[4] = NET_RT_TABLE;
-   mib[5] = rdomain;
-
-   len = sizeof(info);
-   if (sysctl(mib, 6, , , NULL, 0) == -1) {
-   if (errno == ENOENT) {
-   /* table nonexistent */
-   return 0;
-   }
-   err(1, "%s", __func__);
-   }
-   if (info.rti_domainid == rdomain) {
-   found[rdomain] = 1;
-   return 1;
-   }
-   /* rdomain is a table, but not an rdomain */
-   return 0;
 }
 
 int



Re: ldom.conf.5: clarify vcpu strides

2020-09-14 Thread Klemens Nanni
On Wed, Sep 02, 2020 at 04:58:39PM +0200, Stefan Sperling wrote:
> I would like to suggest an example for the EXAMPLES section which
> illustrates how a suitable stride factor can be determined (divide the
> number of desired "unused" cpus by the number of desired "used" cpus):
We can do with an example, but to me yours does not read obvious enough.

Also, `vcpu' denotes *virtual* CPUs inside domains, not CPUs on the
machine, so "CPU" (without "V") reads off in your example and conflicts
with the otherwise consistent mentions of "virtual CPUs" in this manual.

Here's my last diff incl. an example which reads a tad clearer to me and
is placed in the EXAMPLES section instead.

Feedback? OK?


Index: ldom.conf.5
===
RCS file: /cvs/src/usr.sbin/ldomctl/ldom.conf.5,v
retrieving revision 1.13
diff -u -p -r1.13 ldom.conf.5
--- ldom.conf.5 21 Feb 2020 19:39:28 -  1.13
+++ ldom.conf.5 14 Sep 2020 17:51:39 -
@@ -38,8 +38,13 @@ If no configuration for the primary doma
 all CPU and memory resources not used by any guest domains.
 .It Ic vcpu Ar number Ns Op : Ns Ar stride
 Declare the number of virtual CPUs assigned to a domain.
-Optionally a stride can be specified to allocate additional virtual CPUs
-but not assign them to a domain.
+Optionally a stride factor can be specified to allocate
+.Ar number
+virtual CPUs
+.Ar stride
+times but not assign more than
+.Ar number
+virtual CPUs to a domain, leaving the rest unassigned.
 This can be used to distribute virtual CPUs over the available CPU cores.
 .It Ic memory Ar bytes
 Declare the amount of memory assigned to a domain, in bytes.
@@ -112,6 +117,20 @@ domain "salmah" {
 .Pp
 On a machine with 32 cores and 64GB physical memory, this leaves 12 cores and
 58GB memory to the primary domain.
+.Pp
+Use
+.Ar stride
+factors to distribute virtual CPUs:
+.Bd -literal -offset indent
+domain "sun" {
+   vcpu 2:4
+   memory 4G
+   vdisk "/home/sun/vdisk0"
+}
+.Ed
+On a machine with eight threads per physical core, this allocates four strides
+of two virtual CPUs to the guest domain but only assigns one stride to it, i.e.
+make it occupy an entire physical core while running on only two threads.
 .Sh SEE ALSO
 .Xr eeprom 8 ,
 .Xr ldomctl 8 ,



pppoe: little cleanup

2020-09-14 Thread Klemens Nanni
I'm going through the pppoeintr() code path wrt. KERNEL_LOCK(), first
step is discovery packet handling.

Reading the code makes me want to clean/simplify it a bit by zapping
needless variable assignments (dead store because next usage is another
assign) and merging initializations into declerations.

I'm running with this (and other diffs) just fine.

Feedback? OK?


Index: if_pppoe.c
===
RCS file: /cvs/src/sys/net/if_pppoe.c,v
retrieving revision 1.73
diff -u -p -r1.73 if_pppoe.c
--- if_pppoe.c  13 Sep 2020 11:00:40 -  1.73
+++ if_pppoe.c  13 Sep 2020 15:45:00 -
@@ -359,26 +359,22 @@ pppoeintr(void)
 /* Analyze and handle a single received packet while not in session state. */
 static void pppoe_dispatch_disc_pkt(struct mbuf *m, int off)
 {
-   struct pppoe_softc *sc;
+   struct pppoe_softc *sc = NULL;
struct pppoehdr *ph;
struct pppoetag *pt;
struct mbuf *n;
struct ether_header *eh;
-   const char *err_msg, *devname;
-   size_t ac_cookie_len;
-   size_t relay_sid_len;
-   int noff, err, errortag;
-   u_int16_t *max_payload;
+   const char *err_msg = NULL, *devname = "pppoe";
+   size_t ac_cookie_len = 0;
+   size_t relay_sid_len = 0;
+   int noff, err, errortag = 0;
+   u_int16_t *max_payload = NULL;
u_int16_t tag, len;
u_int16_t session, plen;
-   u_int8_t *ac_cookie;
-   u_int8_t *relay_sid;
+   u_int8_t *ac_cookie = NULL;
+   u_int8_t *relay_sid = 0;
u_int8_t code;
 
-   err_msg = NULL;
-   devname = "pppoe";
-   errortag = 0;
-
if (m->m_len < sizeof(*eh)) {
m = m_pullup(m, sizeof(*eh));
if (m == NULL)
@@ -387,13 +383,6 @@ static void pppoe_dispatch_disc_pkt(stru
eh = mtod(m, struct ether_header *);
off += sizeof(*eh);
 
-   ac_cookie = NULL;
-   ac_cookie_len = 0;
-   relay_sid = NULL;
-   relay_sid_len = 0;
-   max_payload = NULL;
-
-   session = 0;
if (m->m_pkthdr.len - off <= PPPOE_HEADERLEN) {
printf("pppoe: packet too short: %d\n", m->m_pkthdr.len);
goto done;
@@ -425,9 +414,6 @@ static void pppoe_dispatch_disc_pkt(stru
/* ignore trailing garbage */
m_adj(m, off + plen - m->m_pkthdr.len);
 
-   tag = 0;
-   len = 0;
-   sc = NULL;
while (off + sizeof(*pt) <= m->m_pkthdr.len) {
n = m_pulldown(m, off, sizeof(*pt), );
if (n == NULL) {



Re: diff: pfctl: error message for nonexisting rtable

2020-09-14 Thread Klemens Nanni
On Mon, Sep 14, 2020 at 02:09:27PM +0900, YASUOKA Masahiko wrote:
> Make pfctl check if the rtable really exists when parsing the config.
I concur, but you can do this with less (duplicated) code.

Instead of copying rdomain_exists() into rtable_exists() with the
`rti_domainid' check omitted, tweak (and rename) rdomain_exists() into
returning the information whether the given ID is just an rtable.

rdomain_exists() merges the "invalid id" and "id is an rtable but not
an rdmomain" cases - make those separate return codes, check/adjust
existing callers and use it for your new checks.



Re: trunk: keep interface up on port removal

2020-09-14 Thread Klemens Nanni
On Sun, Sep 13, 2020 at 06:44:13PM +0100, Stuart Henderson wrote:
> I can't test at the moment, but the other case is removing a port from
> the trunk without destroying the trunk interface itself. That's almost
> certainly what I was testing at the time.
Right, that's different from destroying the trunk.

> The other thing to be aware of is that you may then end up with two
> separate interfaces with the same MAC. This may cause some problems for
> incoming traffic now we don't use weak host model on non-router systems.
> Not sure there is much we can do about that though.
I tested removing a single port from trunk and observed that both
interfaces do end up with the same MAC address, but this happens without
my diff already - I still don't see any behaviour after my diff wrt. MAC
addresses or anything else but the UP flag on port interfaces.



pppoe: move softc list out of NET_LOCK() into new pppoe lock

2020-09-13 Thread Klemens Nanni
This is my first try trading global locks for interface specific ones.

pppoe(4) keeps a list of all its interfaces which is then obviously
traversed during create and destroy.

Currently, the net lock is grabbed for this, but there seems to be no
justification other than reusing^Wabusing an existing lock.

I run this diff with WITNESS and kern.witness=2 on my edgerouter 4
providing my home uplink via pppoe0:  the kernel runs stable, there's
not witness log showing up and creating and destroying hundreds of
additional pppoe(4) devices works without disruption.

Is this the right direction?

Index: if_pppoe.c
===
RCS file: /cvs/src/sys/net/if_pppoe.c,v
retrieving revision 1.73
diff -u -p -r1.73 if_pppoe.c
--- if_pppoe.c  13 Sep 2020 11:00:40 -  1.73
+++ if_pppoe.c  13 Sep 2020 11:31:12 -
@@ -114,15 +114,18 @@ struct pppoetag {
 #definePPPOE_DISC_MAXPADI  4   /* retry PADI four times 
(quickly) */
 #definePPPOE_DISC_MAXPADR  2   /* retry PADR twice */
 
+struct rwlock pppoe_lock = RWLOCK_INITIALIZER("pppoe");
+
 /*
  * Locks used to protect struct members and global data
  *   I   immutable after creation
  *   N   net lock
+ *   p   pppoe lock
  */
 
 struct pppoe_softc {
struct sppp sc_sppp;/* contains a struct ifnet as first 
element */
-   LIST_ENTRY(pppoe_softc) sc_list;/* [N] */
+   LIST_ENTRY(pppoe_softc) sc_list;/* [p] */
unsigned int sc_eth_ifidx;  /* [N] */
 
int sc_state;   /* [N] discovery phase or session 
connected */
@@ -233,7 +236,7 @@ pppoe_clone_create(struct if_clone *ifc,
bpfattach(>sc_sppp.pp_if.if_bpf, >sc_sppp.pp_if, DLT_PPP_ETHER, 
0);
 #endif
 
-   NET_LOCK();
+   rw_enter_write(_lock);
 retry:
unique = arc4random();
LIST_FOREACH(tmpsc, _softc_list, sc_list)
@@ -241,7 +244,7 @@ retry:
goto retry;
sc->sc_unique = unique;
LIST_INSERT_HEAD(_softc_list, sc, sc_list);
-   NET_UNLOCK();
+   rw_exit_write(_lock);
 
return (0);
 }
@@ -252,9 +255,9 @@ pppoe_clone_destroy(struct ifnet *ifp)
 {
struct pppoe_softc *sc = ifp->if_softc;
 
-   NET_LOCK();
+   rw_enter_write(_lock);
LIST_REMOVE(sc, sc_list);
-   NET_UNLOCK();
+   rw_exit_write(_lock);
 
timeout_del(>sc_timeout);
 
@@ -460,8 +463,10 @@ static void pppoe_dispatch_disc_pkt(stru
err_msg = "TAG HUNIQUE ERROR";
break;
}
+   rw_enter_read(_lock);
sc = pppoe_find_softc_by_hunique(mtod(n, caddr_t) + 
noff,
len, m->m_pkthdr.ph_ifidx);
+   rw_exit_read(_lock);
if (sc != NULL)
devname = sc->sc_sppp.pp_if.if_xname;
break;
@@ -668,8 +673,12 @@ pppoe_data_input(struct mbuf *m)
 #ifdef PPPOE_TERM_UNKNOWN_SESSIONS
u_int8_t shost[ETHER_ADDR_LEN];
 #endif
-   if (LIST_EMPTY(_softc_list))
+   rw_enter_read(_lock);
+   if (LIST_EMPTY(_softc_list)) {
+   rw_exit_read(_lock);
goto drop;
+   }
+   rw_exit_read(_lock);
 
KASSERT(m->m_flags & M_PKTHDR);
 
@@ -699,7 +708,9 @@ pppoe_data_input(struct mbuf *m)
goto drop;
 
session = ntohs(ph->session);
+   rw_enter_read(_lock);
sc = pppoe_find_softc_by_session(session, m->m_pkthdr.ph_ifidx);
+   rw_exit_read(_lock);
if (sc == NULL) {
 #ifdef PPPOE_TERM_UNKNOWN_SESSIONS
printf("pppoe (data): input for unknown session 0x%x, sending 
PADT\n",



Re: trunk: keep interface up on port removal

2020-09-13 Thread Klemens Nanni
On Sun, Sep 13, 2020 at 01:23:59PM +0200, Klemens Nanni wrote:
> On Sun, Sep 13, 2020 at 11:31:12AM +0100, Stuart Henderson wrote:
> > On 2020/09/13 11:12, Stuart Henderson wrote:
> > > This has been tried before, I forget what but there were problems
> > 
> > from chat logs when I tried this before:
> > 
> > 14:52 < sthen> if i kill the if_down, no crash, but the mac address doesn't 
> > get updated so i end up with the same one on em0, em1, trunk0
> Thanks for mentioning it, I'll look into whether I can fix this or we
> have revert my commit to avoid breakage around MAC addresses.
> 
> I'd expect such information to be present in CVS log or code (comments).
I checked the code path for changing MACs in trunk(4) at port removal:

trunk_port_destroy(), which used to pull the port interface down, calls
trunk_port_lladdr() which calls if.c:ifnewlladdr() which looks like

void
ifnewlladdr(struct ifnet *ifp)
{
#ifdef INET6
struct ifaddr *ifa;
#endif
struct ifreq ifrq;
short up;
int s;

s = splnet();
up = ifp->if_flags & IFF_UP;

if (up) {
/* go down for a moment... */
ifp->if_flags &= ~IFF_UP;
ifrq.ifr_flags = ifp->if_flags;
(*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t));
}

ifp->if_flags |= IFF_UP;
ifrq.ifr_flags = ifp->if_flags;
(*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t));

#ifdef INET6
...
#endif
if (!up) {
/* go back down */
ifp->if_flags &= ~IFF_UP;
ifrq.ifr_flags = ifp->if_flags;
(*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t));
}
splx(s);
}

So there's a dance around UP interfaces already;  CVS log dates this
code back to 2010 when deraadt rearanged code into ifnewlladdr(), the
previous if.c revision also head this dance around UP.

The if_down() line I removed from trunk(4) dates back to if_trunk.c r1.1
from 2005.


Here's some practical tests further indicating that my diff does not
break anything and whatever sthen encountered at whatever time in the
past is no longer an issue:

With my commit in -CURRENT, the MAC address *is* being restored on my
physical interfaces:

$ { ifconfig em0 ; ifconfig athn0 ; } | grep -e flags= -e lladdr
em0: flags=8843 mtu 1500
lladdr 3c:97:0e:6e:e9:1b
athn0: flags=8843 mtu 1500
lladdr 04:f0:21:30:37:de

$ doas ifconfig trunk0 trunkport em0 trunkport athn0

$ { ifconfig em0 ; ifconfig athn0 ; } | grep -e flags= -e lladdr
em0: 
flags=8b43 mtu 1500
lladdr 3c:97:0e:6e:e9:1b
athn0: flags=8943 mtu 
1500
lladdr 3c:97:0e:6e:e9:1b

$ doas ifconfig trunk0 destroy

$ { ifconfig em0 ; ifconfig athn0 ; } | grep -e flags= -e lladdr
em0: flags=8843 mtu 1500
lladdr 3c:97:0e:6e:e9:1b
athn0: flags=8843 mtu 1500
lladdr 04:f0:21:30:37:de

Observe how UP is set on the physical interfaces, i.e. my diff is in.
MAC addresses of physical interfaces are properly restored.

They are also restored when I create the same trunk0 interface but
generate a random MAC for it as well, i.e. overwriting MACs for all
ports while in the trunk:


$ { ifconfig em0 ; ifconfig athn0 ; } | grep -e flags= -e lladdr
em0: flags=8843 mtu 1500
lladdr 3c:97:0e:6e:e9:1b
athn0: flags=8843 mtu 1500
lladdr 04:f0:21:30:37:de

$ doas ifconfig trunk0 trunkport em0 trunkport athn0 lladdr random

$ { ifconfig em0 ; ifconfig athn0 ; } | grep -e flags= -e lladdr
em0: 
flags=8b43 mtu 1500
lladdr 88:c2:6a:b2:21:41
athn0: flags=8943 mtu 
1500
lladdr 88:c2:6a:b2:21:41

$ doas ifconfig trunk0 destroy

$ { ifconfig em0 ; ifconfig athn0 ; } | grep -e flags= -e lladdr
em0: flags=8843 mtu 1500
lladdr 3c:97:0e:6e:e9:1b
athn0: flags=8843 mtu 1500
lladdr 04:f0:21:30:37:de


I also tested this with vether(4) ports under trunk just to check if
there's anything different for pseudo interfaces, but they behave the
same as em(4) and athn(4) for me regardless of `lladdr random' on trunk.

Am I missing anything?



Re: trunk: keep interface up on port removal

2020-09-13 Thread Klemens Nanni
On Sun, Sep 13, 2020 at 11:31:12AM +0100, Stuart Henderson wrote:
> On 2020/09/13 11:12, Stuart Henderson wrote:
> > This has been tried before, I forget what but there were problems
> 
> from chat logs when I tried this before:
> 
> 14:52 < sthen> if i kill the if_down, no crash, but the mac address doesn't 
> get updated so i end up with the same one on em0, em1, trunk0
Thanks for mentioning it, I'll look into whether I can fix this or we
have revert my commit to avoid breakage around MAC addresses.

I'd expect such information to be present in CVS log or code (comments).



Re: pppoe: start documenting locks

2020-09-13 Thread Klemens Nanni
On Sun, Sep 13, 2020 at 12:23:50PM +0200, Martin Pieuchot wrote:
> Without doing another audit but with the fact that pseudo-device are
> generally run by a thread holding the NET_LOCK() I'd assume it's ok.
Thanks, I'll put it in as its an improvement and comment only (safe);
rest can happen in-tree.

> First we should remove the KRENEL_LOCK() from around pppoeintr().  The
> NET_LOCK() is not an issue right now.
There seem to be some low hanging fruits in pppoe(4) I started with and
am running with already, but I agree that KERNEL_LOCK() is our high
priority problem.

> > Index: if_pppoe.c
> > ===
> > RCS file: /cvs/src/sys/net/if_pppoe.c,v
> > retrieving revision 1.70
> > diff -u -p -r1.70 if_pppoe.c
> > --- if_pppoe.c  28 Jul 2020 09:52:32 -  1.70
> > +++ if_pppoe.c  20 Aug 2020 15:27:09 -
> > @@ -114,27 +115,34 @@ struct pppoetag {
> >  #definePPPOE_DISC_MAXPADI  4   /* retry PADI four times 
> > (quickly) */
> >  #definePPPOE_DISC_MAXPADR  2   /* retry PADR twice */
> >  
> > +/*
> > + * Locks used to protect struct members and global data
> > + *   I   immutable after creation
> > + *   K   kernel lock
> 
> I wouldn't bother repeating 'I' and 'K' if they are not used in the
> description below.
`I' is used and `K' completes the list, but omitting unused locks also
indicates which ones are (not) used up front which is nice.

I'll remove `K', I guess.



pppoe: start documenting locks

2020-09-13 Thread Klemens Nanni


Here's a start at struct pppoe_softc;  for every member I went through
code paths looking for *_LOCK() or *_ASSERT_LOCKED().

Pretty much all members are under the net lock, some are proctected by
both net and kernel lock, e.g. the start routine is called with
KERNEL_LOCK().

I did not go through the sppp struct members yet.

Does this look correct?

>From here on, I think we can start and already pull out a few of those
members and put them under a new pppoe(4) specific lock.


Index: if_pppoe.c
===
RCS file: /cvs/src/sys/net/if_pppoe.c,v
retrieving revision 1.70
diff -u -p -r1.70 if_pppoe.c
--- if_pppoe.c  28 Jul 2020 09:52:32 -  1.70
+++ if_pppoe.c  20 Aug 2020 15:27:09 -
@@ -114,27 +115,34 @@ struct pppoetag {
 #definePPPOE_DISC_MAXPADI  4   /* retry PADI four times 
(quickly) */
 #definePPPOE_DISC_MAXPADR  2   /* retry PADR twice */
 
+/*
+ * Locks used to protect struct members and global data
+ *   I   immutable after creation
+ *   K   kernel lock
+ *   N   net lock
+ */
+
 struct pppoe_softc {
struct sppp sc_sppp;/* contains a struct ifnet as first 
element */
-   LIST_ENTRY(pppoe_softc) sc_list;
-   unsigned int sc_eth_ifidx;
+   LIST_ENTRY(pppoe_softc) sc_list;/* [N] */
+   unsigned int sc_eth_ifidx;  /* [N] */
 
-   int sc_state;   /* discovery phase or session connected 
*/
-   struct ether_addr sc_dest;  /* hardware address of concentrator */
-   u_int16_t sc_session;   /* PPPoE session id */
-
-   char *sc_service_name;  /* if != NULL: requested name of 
service */
-   char *sc_concentrator_name; /* if != NULL: requested concentrator 
id */
-   u_int8_t *sc_ac_cookie; /* content of AC cookie we must echo 
back */
-   size_t sc_ac_cookie_len;/* length of cookie data */
-   u_int8_t *sc_relay_sid; /* content of relay SID we must echo 
back */
-   size_t sc_relay_sid_len;/* length of relay SID data */
-   u_int32_t sc_unique;/* our unique id */
-   struct timeout sc_timeout;  /* timeout while not in session state */
-   int sc_padi_retried;/* number of PADI retries already done 
*/
-   int sc_padr_retried;/* number of PADR retries already done 
*/
+   int sc_state;   /* [N] discovery phase or session 
connected */
+   struct ether_addr sc_dest;  /* [N] hardware address of concentrator 
*/
+   u_int16_t sc_session;   /* [N] PPPoE session id */
+
+   char *sc_service_name;  /* [N] if != NULL: requested name of 
service */
+   char *sc_concentrator_name; /* [N] if != NULL: requested 
concentrator id */
+   u_int8_t *sc_ac_cookie; /* [N] content of AC cookie we must 
echo back */
+   size_t sc_ac_cookie_len;/* [N] length of cookie data */
+   u_int8_t *sc_relay_sid; /* [N] content of relay SID we must 
echo back */
+   size_t sc_relay_sid_len;/* [N] length of relay SID data */
+   u_int32_t sc_unique;/* [I] our unique id */
+   struct timeout sc_timeout;  /* [N] timeout while not in session 
state */
+   int sc_padi_retried;/* [N] number of PADI retries already 
done */
+   int sc_padr_retried;/* [N] number of PADR retries already 
done */
 
-   struct timeval sc_session_time; /* time the session was established */
+   struct timeval sc_session_time; /* [N] time the session was established 
*/
 };
 
 /* incoming traffic will be queued here */



trunk: keep interface up on port removal

2020-09-12 Thread Klemens Nanni
Unconfiguring a member interface from trunk(4) or simply destroying the
trunk pulls the member down for no reason, both comment and code are
there since import, but I see no justification for doing so.

aggr(4) does not pull its member down upon removal either.

I came across this after

$ doas ifconfig trunk0 destroy
$ doas sh /etc/netstart trunk0

yielded no network and I had to manually pull up members.

Feedback? OK?


Index: if_trunk.c
===
RCS file: /cvs/src/sys/net/if_trunk.c,v
retrieving revision 1.149
diff -u -p -r1.149 if_trunk.c
--- if_trunk.c  28 Jul 2020 09:52:32 -  1.149
+++ if_trunk.c  12 Sep 2020 15:41:14 -
@@ -423,10 +423,6 @@ trunk_port_destroy(struct trunk_port *tp
/* Remove multicast addresses from this port */
trunk_ether_cmdmulti(tp, SIOCDELMULTI);
 
-   /* Port has to be down */
-   if (ifp->if_flags & IFF_UP)
-   if_down(ifp);
-
ifpromisc(ifp, 0);
 
if (tr->tr_port_destroy != NULL)



Re: unwind(8): forget learned forwarders when interface disappears

2020-09-12 Thread Klemens Nanni
On Sat, Sep 12, 2020 at 05:11:00PM +0200, Klemens Nanni wrote:
> Bit hard to read, what about aligning like this?
> 
> + if ((rdns_proposal->src == 0 ||
> +  rdns_proposal->src == tmp->src) &&
> + (rdns_proposal->if_index == 0 ||
> + rdns_proposal->if_index == tmp->if_index))
With a space here   ^



Re: unwind(8): forget learned forwarders when interface disappears

2020-09-12 Thread Klemens Nanni
On Sat, Sep 12, 2020 at 04:36:28PM +0200, Florian Obser wrote:
> ... say if you pull a usb stick.
...or if you play with^W^Wrecreate your trunk0 { em0 athn0 } uplink
without checking unwind.

Without this diff, unwind cannot recover;  with it, stuff just works for
me across destroy/create.

OK kn

> diff --git resolver.c resolver.c
> index 874ad5e76b3..8cf72db7250 100644
> --- resolver.c
> +++ resolver.c
> @@ -1952,10 +1952,13 @@ replace_autoconf_forwarders(struct imsg_rdns_proposal 
> *rdns_proposal)
>   }
>  
>   TAILQ_FOREACH(tmp, _forwarder_list, entry) {
> - /* if_index of zero signals to clear all proposals */
> - if (rdns_proposal->src == tmp->src &&
> - (rdns_proposal->if_index == 0 || rdns_proposal->if_index ==
> - tmp->if_index))
> + /*
> +  * if_index of zero signals to clear all proposals
> +  * src of zero signals interface gone
> +  */
> + if ((rdns_proposal->src == 0 || rdns_proposal->src ==
> + tmp->src) && (rdns_proposal->if_index == 0 ||
> + rdns_proposal->if_index == tmp->if_index))
Bit hard to read, what about aligning like this?

+   if ((rdns_proposal->src == 0 ||
+rdns_proposal->src == tmp->src) &&
+   (rdns_proposal->if_index == 0 ||
+   rdns_proposal->if_index == tmp->if_index))

>   continue;
>   if ((uw_forwarder = calloc(1, sizeof(struct uw_forwarder))) ==
>   NULL)



sppp: add free() sizes

2020-09-12 Thread Klemens Nanni
These are the last free(buf, 0) occurences in if_pppoe.c and
if_spppsubr.c changing to non-zero sizes.

I've been running with this the last week without any issues.

Feedback? OK?


Index: if_spppsubr.c
===
RCS file: /cvs/src/sys/net/if_spppsubr.c,v
retrieving revision 1.186
diff -u -p -r1.186 if_spppsubr.c
--- if_spppsubr.c   22 Aug 2020 16:12:12 -  1.186
+++ if_spppsubr.c   3 Sep 2020 21:43:54 -
@@ -750,13 +750,15 @@ sppp_detach(struct ifnet *ifp)
 
/* release authentication data */
if (sp->myauth.name != NULL)
-   free(sp->myauth.name, M_DEVBUF, 0);
+   free(sp->myauth.name, M_DEVBUF, strlen(sp->myauth.name) + 1);
if (sp->myauth.secret != NULL)
-   free(sp->myauth.secret, M_DEVBUF, 0);
+   free(sp->myauth.secret, M_DEVBUF,
+   strlen(sp->myauth.secret) + 1);
if (sp->hisauth.name != NULL)
-   free(sp->hisauth.name, M_DEVBUF, 0);
+   free(sp->hisauth.name, M_DEVBUF, strlen(sp->hisauth.name) + 1);
if (sp->hisauth.secret != NULL)
-   free(sp->hisauth.secret, M_DEVBUF, 0);
+   free(sp->hisauth.secret, M_DEVBUF,
+   strlen(sp->hisauth.secret) + 1);
 }
 
 /*
@@ -4579,9 +4587,11 @@ sppp_set_params(struct sppp *sp, struct 
if (spa->proto == 0) {
/* resetting auth */
if (auth->name != NULL)
-   free(auth->name, M_DEVBUF, 0);
+   free(auth->name, M_DEVBUF,
+   strlen(auth->name) + 1);
if (auth->secret != NULL)
-   free(auth->secret, M_DEVBUF, 0);
+   free(auth->secret, M_DEVBUF,
+   strlen(auth->secret) + 1);
bzero(auth, sizeof *auth);
explicit_bzero(sp->chap_challenge, sizeof 
sp->chap_challenge);
} else {
@@ -4594,7 +4604,8 @@ sppp_set_params(struct sppp *sp, struct 
p = malloc(len, M_DEVBUF, M_WAITOK);
strlcpy(p, spa->name, len);
if (auth->name != NULL)
-   free(auth->name, M_DEVBUF, 0);
+   free(auth->name, M_DEVBUF,
+   strlen(auth->name) + 1);
auth->name = p;
 
if (spa->secret[0] != '\0') {
@@ -4603,7 +4614,8 @@ sppp_set_params(struct sppp *sp, struct 
p = malloc(len, M_DEVBUF, M_WAITOK);
strlcpy(p, spa->secret, len);
if (auth->secret != NULL)
-   free(auth->secret, M_DEVBUF, 0);
+   free(auth->secret, M_DEVBUF,
+   strlen(auth->secret) + 1);
auth->secret = p;
} else if (!auth->secret) {
p = malloc(1, M_DEVBUF, M_WAITOK);



Re: systat: pf: merge NOTES column into NAME

2020-09-08 Thread Klemens Nanni
On Sat, Aug 29, 2020 at 05:13:00PM +0200, Klemens Nanni wrote:
> NOTES stays unused unless pf.conf(5) contains `set loginterface ...' in
> which case it merely amends what can otherwise be part of the NAME
> column.
> 
> Merge the constant NOTES values for conditional counters into their NAME
> values to make the `pf' view look a little nicer and less empty by
> default;  this also saves screen estate for possible future changes,
> e.g. we could increase column widths.
> 
> Here's the difference on my workstation:
> 
> $ systat -b pf
> 
>0 users Load 0.44 0.42 0.44 (1-21 of 56)eru 
> 16:59:13
> TYPE NAME   VALUE   RATE NOTES
>   pf Status   Enabled
>   pf Since  475:09:16
>   pf Debugerr
>   pf Hostid0xc4490f26
> 
>   trunk0 Bytes In  9309436078IPv4
>   trunk0 Bytes In   82089344KIPv6
>   trunk0 Bytes Out  816675408IPv4
>   trunk0 Bytes Out 5264003084IPv6
>   trunk0 Packets In   8394222IPv4, Passed
>   trunk0 Packets In  58044731IPv6, Passed
>   trunk0 Packets In 16593IPv4, Blocked
>   trunk0 Packets In 32233IPv6, Blocked
>   trunk0 Packets Out  6001617IPv4, Passed
>   trunk0 Packets Out 34333653IPv6, Passed
>   trunk0 Packets Out 3625IPv4, Blocked
>   trunk0 Packets Out 6277IPv6, Blocked
> 
>state Count172
>state searches   191574598 112.00
>state inserts   474584   0.28
> 
> $ ./obj/systat -b pf
> 
>0 users Load 0.17 0.31 0.37 (1-21 of 56)eru 
> 17:09:08
> TYPE NAME  VALUE   RATE
>   pf Status  Enabled
>   pf Since 475:19:11
>   pf Debug   err
>   pf Hostid   0xc4490f26
> 
>   trunk0 Bytes In IPv49311776112
>   trunk0 Bytes In IPv6 82089531K
>   trunk0 Bytes Out IPv4817474165
>   trunk0 Bytes Out IPv6   5264003084
>   trunk0 Packets In Passed IPv4  8398989
>   trunk0 Packets In Passed IPv6 58045439
>   trunk0 Packets In Blocked IPv4   16908
>   trunk0 Packets In Blocked IPv6   32680
>   trunk0 Packets Out Passed IPv4 6005423
>   trunk0 Packets Out Passed IPv634333653
>   trunk0 Packets Out Blocked IPv4   3625
>   trunk0 Packets Out Blocked IPv6   6277
> 
>state Count   105
>state searches  191584952 111.96
>state inserts  475839   0.28
> 
> 
> Feedback? OK?
Ping.


Index: pf.c
===
RCS file: /cvs/src/usr.bin/systat/pf.c,v
retrieving revision 1.12
diff -u -p -r1.12 pf.c
--- pf.c15 May 2020 00:56:03 -  1.12
+++ pf.c29 Aug 2020 15:08:54 -
@@ -54,18 +54,16 @@ field_def fields_pf[] = {
{"NAME", 12, 24, 1, FLD_ALIGN_LEFT, -1, 0, 0, 0},
{"VALUE", 8, 10, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
{"RATE", 8, 10, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 60},
-   {"NOTES", 10, 20, 1, FLD_ALIGN_LEFT, -1, 0, 0, 60},
 };
 
 #define FLD_PF_TYPEFIELD_ADDR(fields_pf,0)
 #define FLD_PF_NAMEFIELD_ADDR(fields_pf,1)
 #define FLD_PF_VALUE   FIELD_ADDR(fields_pf,2)
 #define FLD_PF_RATEFIELD_ADDR(fields_pf,3)
-#define FLD_PF_DESCFIELD_ADDR(fields_pf,4)
 
 /* Define views */
 field_def *view_pf_0[] = {
-   FLD_PF_TYPE, FLD_PF_NAME, FLD_PF_VALUE, FLD_PF_RATE, FLD_PF_DESC, NULL
+   FLD_PF_TYPE, FLD_PF_NAME, FLD_PF_VALUE, FLD_PF_RATE, NULL
 };
 
 
@@ -187,19 +185,6 @@ print_fld_double(field_def *fld, double 
return; \
} while (0)
 
-#define ADD_LINE_VD(t, n, v, d) \
-   do {\
-   if (cur >= dispstart && cur < end) {\
-   print_fld_str(FLD_PF_TYPE, (t));\
-   print_fld_str(FLD_PF_NAME, (n));\
-   print_fld_size(FL

Re: undwind(8): request for data

2020-09-03 Thread Klemens Nanni
On Thu, Sep 03, 2020 at 06:13:41PM +0200, Florian Obser wrote:
> Then reload the ruleset and restart unwind:
> 
> # pfctl -f /etc/pf.conf
> # rcctl restart unwind
> 
> You can now get stats on how often your machine talks to the root name 
> servers:
> 
> # pfctl -s label
> rootdns_tcp 2730 0 0 0 0 0 0 0
> rootdns_udp 266 2 187 1 56 1 131 0
It may be worth noting here that reloading the ruleset also resets label
counters, so if you change your pf.conf while running florian's config,
make sure to dump counters beforehand.

> The columns are: label, evaluations, packets total, bytes total,
> packets in, bytes in, packets out, bytes out, state creations
> 
> Please report the stats after a day of normal use, thanks.



Re: wg: count peers per interface not globally

2020-09-02 Thread Klemens Nanni
On Wed, Sep 02, 2020 at 04:12:33PM +1000, Matt Dunwoodie wrote:
> The patch isn't fully correct. When you remove a peer, sc_peer_num
> will decrement, and if you add a new peer afterwards you will have
> duplicate IDs. This is likely to create further headaches. A dedicated
> ID counter in wg_softc should probably be used.
Yes, it's just a small improvement and removing individual peers brings
counting out of sync, although I'd argue its still more useful than the
global counter.

I can give proper peer counting a try soon.



ldom.conf.5: clarify vcpu strides

2020-09-02 Thread Klemens Nanni
They way strides work is everything but intuitive and the manual doesn't
really help;  I've had multiple hackers/users ask me how to use them.

`vcpu 8' assigns eight virtual CPUs to a domain.

`vcpu 8:2' allocates eight VCPUs two times but assigns eight VCPUs
only once, leaving the other eight allocated (read: unusable) but not
assigned to any domain.

`vcpu 8:3' would allocate 24 VCPUs and assign eight to a domain.

This multiplicative property is not obvious from the manual; the way I
read the current wording is `vcpu 8:2' allocating ten VCPUs and assign
eight, i.e. the stride being an additive count.

stsp brought this up and we came up with the following diff.
Feedback? OK?


Index: ldom.conf.5
===
RCS file: /cvs/src/usr.sbin/ldomctl/ldom.conf.5,v
retrieving revision 1.13
diff -u -p -r1.13 ldom.conf.5
--- ldom.conf.5 21 Feb 2020 19:39:28 -  1.13
+++ ldom.conf.5 2 Sep 2020 14:26:58 -
@@ -38,8 +38,13 @@ If no configuration for the primary doma
 all CPU and memory resources not used by any guest domains.
 .It Ic vcpu Ar number Ns Op : Ns Ar stride
 Declare the number of virtual CPUs assigned to a domain.
-Optionally a stride can be specified to allocate additional virtual CPUs
-but not assign them to a domain.
+Optionally a stride factor can be specified to allocate
+.Ar number
+virtual CPUs
+.Ar stride
+times but not assign more than
+.Ar number
+virtual CPUs to a domain, leaving the rest unassigned.
 This can be used to distribute virtual CPUs over the available CPU cores.
 .It Ic memory Ar bytes
 Declare the amount of memory assigned to a domain, in bytes.



wg: count peers per interface not globally

2020-09-01 Thread Klemens Nanni
The driver increases a static peer counter across all wg interfaces when
creating peers, the peer number is only used in debug output, though.

Output from console around recreating an interface  (2 and 4 are the same):

wg1: Receiving handshake response from peer 2
wg1: Receiving keepalive packet from peer 2
wg1: Sending keepalive packet to peer 2
# ifconfig wg0 destroy
wg1: Peer 1 destroyed
wg1: Peer 2 destroyed
wg1: Destroyed interface
# sh /etc/netstart wg0
wg1: Sending handshake initiation to peer 4
wg1: Receiving handshake response from peer 4
wg1: Receiving keepalive packet from peer 4
wg1: Sending keepalive packet to peer 4

I'm looking into an issue caused by having multiple `wgpeer's defined
on a single wg(4) interface and debug log is helpful, but having to
count along is not helpful, so I'd like to replace the global counter
with an interface specific one that's already in place.

There's another completely unused global counter which I remove as well
in the diff below.

Recreating as above now shows this:

# ifconfig wg1 destroy
wg1: Peer 0 destroyed
wg1: Peer 1 destroyed
wg1: Destroyed interface
# sh /etc/netstart wg0
wg1: Peer 0 created
wg1: Sending handshake initiation to peer 0
wg1: Receiving handshake response from peer 0
wg1: Peer 1 created
wg1: Receiving keepalive packet from peer 0

Feedback? OK?


Index: if_wg.c
===
RCS file: /cvs/src/sys/net/if_wg.c,v
retrieving revision 1.13
diff -u -p -r1.13 if_wg.c
--- if_wg.c 27 Aug 2020 21:27:17 -  1.13
+++ if_wg.c 1 Sep 2020 18:35:41 -
@@ -370,8 +370,6 @@ int wg_clone_create(struct if_clone *, i
 intwg_clone_destroy(struct ifnet *);
 void   wgattach(int);
 
-uint64_t   peer_counter = 0;
-uint64_t   keypair_counter = 0;
 struct poolwg_aip_pool;
 struct poolwg_peer_pool;
 struct poolwg_ratelimit_pool;
@@ -398,7 +396,6 @@ wg_peer_create(struct wg_softc *sc, uint
if ((peer = pool_get(_peer_pool, PR_NOWAIT)) == NULL)
return NULL;
 
-   peer->p_id = peer_counter++;
peer->p_sc = sc;
 
noise_remote_init(>p_remote, public, >sc_local);
@@ -442,7 +439,7 @@ wg_peer_create(struct wg_softc *sc, uint
rw_enter_write(>sc_peer_lock);
LIST_INSERT_HEAD(>sc_peer[idx], peer, p_pubkey_entry);
TAILQ_INSERT_TAIL(>sc_peer_seq, peer, p_seq_entry);
-   sc->sc_peer_num++;
+   peer->p_id = sc->sc_peer_num++;
rw_exit_write(>sc_peer_lock);
 
DPRINTF(sc, "Peer %llu created\n", peer->p_id);



systat: pf: merge NOTES column into NAME

2020-08-29 Thread Klemens Nanni
NOTES stays unused unless pf.conf(5) contains `set loginterface ...' in
which case it merely amends what can otherwise be part of the NAME
column.

Merge the constant NOTES values for conditional counters into their NAME
values to make the `pf' view look a little nicer and less empty by
default;  this also saves screen estate for possible future changes,
e.g. we could increase column widths.

Here's the difference on my workstation:

$ systat -b pf

   0 users Load 0.44 0.42 0.44 (1-21 of 56)eru 16:59:13
TYPE NAME   VALUE   RATE NOTES
  pf Status   Enabled
  pf Since  475:09:16
  pf Debugerr
  pf Hostid0xc4490f26

  trunk0 Bytes In  9309436078IPv4
  trunk0 Bytes In   82089344KIPv6
  trunk0 Bytes Out  816675408IPv4
  trunk0 Bytes Out 5264003084IPv6
  trunk0 Packets In   8394222IPv4, Passed
  trunk0 Packets In  58044731IPv6, Passed
  trunk0 Packets In 16593IPv4, Blocked
  trunk0 Packets In 32233IPv6, Blocked
  trunk0 Packets Out  6001617IPv4, Passed
  trunk0 Packets Out 34333653IPv6, Passed
  trunk0 Packets Out 3625IPv4, Blocked
  trunk0 Packets Out 6277IPv6, Blocked

   state Count172
   state searches   191574598 112.00
   state inserts   474584   0.28

$ ./obj/systat -b pf

   0 users Load 0.17 0.31 0.37 (1-21 of 56)eru 17:09:08
TYPE NAME  VALUE   RATE
  pf Status  Enabled
  pf Since 475:19:11
  pf Debug   err
  pf Hostid   0xc4490f26

  trunk0 Bytes In IPv49311776112
  trunk0 Bytes In IPv6 82089531K
  trunk0 Bytes Out IPv4817474165
  trunk0 Bytes Out IPv6   5264003084
  trunk0 Packets In Passed IPv4  8398989
  trunk0 Packets In Passed IPv6 58045439
  trunk0 Packets In Blocked IPv4   16908
  trunk0 Packets In Blocked IPv6   32680
  trunk0 Packets Out Passed IPv4 6005423
  trunk0 Packets Out Passed IPv634333653
  trunk0 Packets Out Blocked IPv4   3625
  trunk0 Packets Out Blocked IPv6   6277

   state Count   105
   state searches  191584952 111.96
   state inserts  475839   0.28


Feedback? OK?


Index: pf.c
===
RCS file: /cvs/src/usr.bin/systat/pf.c,v
retrieving revision 1.12
diff -u -p -r1.12 pf.c
--- pf.c15 May 2020 00:56:03 -  1.12
+++ pf.c29 Aug 2020 15:08:54 -
@@ -54,18 +54,16 @@ field_def fields_pf[] = {
{"NAME", 12, 24, 1, FLD_ALIGN_LEFT, -1, 0, 0, 0},
{"VALUE", 8, 10, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 0},
{"RATE", 8, 10, 1, FLD_ALIGN_RIGHT, -1, 0, 0, 60},
-   {"NOTES", 10, 20, 1, FLD_ALIGN_LEFT, -1, 0, 0, 60},
 };
 
 #define FLD_PF_TYPEFIELD_ADDR(fields_pf,0)
 #define FLD_PF_NAMEFIELD_ADDR(fields_pf,1)
 #define FLD_PF_VALUE   FIELD_ADDR(fields_pf,2)
 #define FLD_PF_RATEFIELD_ADDR(fields_pf,3)
-#define FLD_PF_DESCFIELD_ADDR(fields_pf,4)
 
 /* Define views */
 field_def *view_pf_0[] = {
-   FLD_PF_TYPE, FLD_PF_NAME, FLD_PF_VALUE, FLD_PF_RATE, FLD_PF_DESC, NULL
+   FLD_PF_TYPE, FLD_PF_NAME, FLD_PF_VALUE, FLD_PF_RATE, NULL
 };
 
 
@@ -187,19 +185,6 @@ print_fld_double(field_def *fld, double 
return; \
} while (0)
 
-#define ADD_LINE_VD(t, n, v, d) \
-   do {\
-   if (cur >= dispstart && cur < end) {\
-   print_fld_str(FLD_PF_TYPE, (t));\
-   print_fld_str(FLD_PF_NAME, (n));\
-   print_fld_size(FLD_PF_VALUE, (v));  \
-   print_fld_str(FLD_PF_DESC, (d));\
-   end_line(); \
-   }   \
-   if (++cur >= end)   \
-   return; \
-   } while (0)
-
 #define ADD_LINE_VR(t, n, v, r) \
do {  

Re: wg(4) if_rtrequest

2020-08-26 Thread Klemens Nanni
On Wed, Aug 26, 2020 at 12:20:27PM +1000, Matt Dunwoodie wrote:
> I doing some IPv6 setup, I came across an issue with wg(4) and ndp. The
> local route is created with RTF_LLINFO, which ndp attempts to print. As
> wg is a layer3 tunnel it won't have any link-local information.
> 
> This patch just sets if_rtrequest to p2p_rtrequest. Even if wg is
> technically point-to-multipoint, p2p_rtrequest has the correct
> semantics. ok?
OK kn



Re: top: toggle routing tables

2020-08-25 Thread Klemens Nanni
On Mon, Aug 24, 2020 at 12:52:46AM +0200, Klemens Nanni wrote:
> Add `t' to swap the WAIT column with RTABLE (and vice versa);  WAIT
> is wide enough to fit RTABLE, somewhat adds additional value to STATE
> and seems therefore most appropiate to hide in favour of RTABLE.
> 
> Internally, I renamed the existing CMD_rtable command to filter routing
> tables into CMD_rtableid in order to use CMD_rtable for showing them as
> that seems in line with how CMD_threads is named to show threads, etc.
> 
> format_header() semantics are slightly reworked/improved now that there
> are two changing fields;  instead of conditionally changing, it now
> always updates it accordingly - i think that makes it clearer overall.
> 
> format_next_process() now uses strlcpy() instead of snprintf() for plain
> strings as I had to touch those lines anyway.
> 
> Filtering rtables with `T' does not toggle the column, just like
> filtering users with `u' does not toggle between user and thread id.
> 
> Feedback? OK?
New diff after feedback from jmc and a little cleanup I just committed
to avoid churn here.

Index: display.c
===
RCS file: /cvs/src/usr.bin/top/display.c,v
retrieving revision 1.64
diff -u -p -r1.64 display.c
--- display.c   23 Aug 2020 21:11:55 -  1.64
+++ display.c   25 Aug 2020 07:33:14 -
@@ -826,6 +826,7 @@ show_help(void)
"s time   - change delay between displays to `time' seconds\n"
"T [-]rtable  - show processes associated with routing table 
`rtable'\n"
"   (T+ shows all, T -rtable hides rtable)\n"
+   "t- toggle the display of routing tables\n"
"u [-]user- show processes for `user' (u+ shows all, u -user 
hides user)\n"
"\n");
 
Index: machine.c
===
RCS file: /cvs/src/usr.bin/top/machine.c,v
retrieving revision 1.109
diff -u -p -r1.109 machine.c
--- machine.c   25 Aug 2020 07:27:34 -  1.109
+++ machine.c   25 Aug 2020 07:33:14 -
@@ -75,8 +75,9 @@ struct handle {
 static char header[] =
"  PID XPRI NICE  SIZE   RES STATE WAIT  TIMECPU 
COMMAND";
 
-/* 0123456   -- field to fill in starts at header+6 */
+/* offsets in the header line to start alternative columns */
 #define UNAME_START 6
+#define RTABLE_START 46
 
 #define Proc_format \
"%5d %-8.8s %3d %4d %5s %5s %-9s %-7.7s %6s %5.2f%% %s"
@@ -226,16 +227,16 @@ machine_init(struct statics *statics)
 }
 
 char *
-format_header(char *second_field)
+format_header(char *second_field, char *eighth_field)
 {
-   char *field_name, *thread_field = " TID";
-   char *ptr;
-
-   field_name = second_field ? second_field : thread_field;
+   char *second_fieldp = second_field, *eighth_fieldp = eighth_field, *ptr;
 
ptr = header + UNAME_START;
-   while (*field_name != '\0')
-   *ptr++ = *field_name++;
+   while (*second_fieldp != '\0')
+   *ptr++ = *second_fieldp++;
+   ptr = header + RTABLE_START;
+   while (*eighth_fieldp != '\0')
+   *ptr++ = *eighth_fieldp++;
return (header);
 }
 
@@ -544,13 +545,12 @@ skip_processes(struct handle *hndl, int 
 
 char *
 format_next_process(struct handle *hndl, const char *(*get_userid)(uid_t, int),
-pid_t *pid)
+int rtable, pid_t *pid)
 {
-   char *p_wait;
struct kinfo_proc *pp;
int cputime;
double pct;
-   char second_buf[16];
+   char second_buf[16], eighth_buf[8];
 
/* find and remember the next proc structure */
pp = *(hndl->next_proc++);
@@ -566,7 +566,11 @@ format_next_process(struct handle *hndl,
strlcpy(second_buf, (*get_userid)(pp->p_ruid, 0),
sizeof(second_buf));
 
-   p_wait = pp->p_wmesg[0] ? pp->p_wmesg : "-";
+   if (rtable)
+   snprintf(eighth_buf, sizeof(eighth_buf), "%7d", pp->p_rtableid);
+   else
+   strlcpy(eighth_buf, pp->p_wmesg[0] ? pp->p_wmesg : "-",
+   sizeof(eighth_buf));
 
/* format this entry */
snprintf(fmt, sizeof(fmt), Proc_format, pp->p_pid, second_buf,
@@ -575,7 +579,7 @@ format_next_process(struct handle *hndl,
format_k(pagetok(pp->p_vm_rssize)),
(pp->p_stat == SSLEEP && pp->p_slptime > maxslp) ?
"idle" : state_abbr(pp),
-   p_wait, format_time(cputime), 100.0 * pct,
+   eighth_buf, format_time(cputime), 100.0 * pct,
printable(format_comm(pp)));
 
*pid = pp->p_pid;
Index: machine.h
===
RCS file: /cvs/src/usr.bin/

aggr.4 and trunk.4: omit common ifconfig options

2020-08-23 Thread Klemens Nanni
ifconfig(8)'s TRUNK (LINK AGGREGATION) nicely combines the two drivers
and I'd like to further omit common stuff from the drive specific
manuals.

This aids in the overall design of having options documented in
ifconfig(8) alone unless they're inherently driver specific, e.g.
`trunkproto' which stays in trunk(4).

sys/net/if_trunk.c and sys/net/trunklacp.h confirm that trunk(4) has
indeed the same defaults as aggr(4) when it comes to LACP mode and
timeout:

#define>LACP_DEFAULT_MODE>  >   1 /* Active Mode */
#define>LACP_DEFAULT_TIMEOUT>   >   0 /* Slow Timeout */

Feedback? OK?


Index: share/man/man4/aggr.4
===
RCS file: /cvs/src/share/man/man4/aggr.4,v
retrieving revision 1.2
diff -u -p -r1.2 aggr.4
--- share/man/man4/aggr.4   5 Jul 2019 05:22:57 -   1.2
+++ share/man/man4/aggr.4   23 Aug 2020 23:10:46 -
@@ -63,30 +63,11 @@ and
 .Xr netstart 8
 using the following options:
 .Bl -tag -width Ds
-.It Cm lacpmode Cm active Ns | Ns Cm passive
-Set the LACP mode to either
-.Cm active
-or
-.Cm passive .
-The default is active mode.
-.It Cm lacptimeout Cm fast Ns | Ns Cm slow
-Set the LACP timeout speed to either
-.Cm fast
-or
-.Cm slow .
-The default is slow timeouts.
 .It Cm lladdr Ar etheraddr Ns | Ns Cm random
 Change the link layer address (MAC address) of the interface.
 This should be specified as six colon-separated hex values, or can
 be chosen randomly.
 By default a random MAC address is generated when an interface is created.
-.It Cm trunkport Ar child-iface
-Add
-.Ar child-iface
-as a port.
-.It Cm -trunkport Ar child-iface
-Remove the port
-.Ar child-iface .
 .El
 .\" document the ioctls?
 .Pp
Index: share/man/man4/trunk.4
===
RCS file: /cvs/src/share/man/man4/trunk.4,v
retrieving revision 1.30
diff -u -p -r1.30 trunk.4
--- share/man/man4/trunk.4  12 Aug 2018 23:50:31 -  1.30
+++ share/man/man4/trunk.4  23 Aug 2020 23:12:53 -
@@ -34,15 +34,6 @@ A
 interface can be created using the
 .Ic ifconfig trunk Ns Ar N Ic create
 command.
-It can use different link aggregation protocols specified
-using the
-.Ic trunkproto Ar proto
-option.
-Child interfaces can be added using the
-.Ic trunkport Ar child-iface
-option and removed using the
-.Ic -trunkport Ar child-iface
-option.
 .Pp
 The driver currently supports the trunk protocols
 .Ic broadcast ,
Index: sbin/ifconfig/ifconfig.8
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
retrieving revision 1.356
diff -u -p -r1.356 ifconfig.8
--- sbin/ifconfig/ifconfig.88 Aug 2020 06:06:24 -   1.356
+++ sbin/ifconfig/ifconfig.823 Aug 2020 23:21:28 -
@@ -1824,13 +1824,14 @@ interfaces:
 .It Cm lacpmode Cm active Ns | Ns Cm passive
 Set the LACP trunk mode to either
 .Cm active
-or
+(default) or
 .Cm passive .
 .It Cm lacptimeout Cm fast Ns | Ns Cm slow
 Set the LACP timeout speed to either
 .Cm fast
 or
-.Cm slow .
+.Cm slow
+(default).
 .It Cm trunkport Ar child-iface
 Add
 .Ar child-iface



top: toggle routing tables

2020-08-23 Thread Klemens Nanni
Add `t' to swap the WAIT column with RTABLE (and vice versa);  WAIT
is wide enough to fit RTABLE, somewhat adds additional value to STATE
and seems therefore most appropiate to hide in favour of RTABLE.

Internally, I renamed the existing CMD_rtable command to filter routing
tables into CMD_rtableid in order to use CMD_rtable for showing them as
that seems in line with how CMD_threads is named to show threads, etc.

format_header() semantics are slightly reworked/improved now that there
are two changing fields;  instead of conditionally changing, it now
always updates it accordingly - i think that makes it clearer overall.

format_next_process() now uses strlcpy() instead of snprintf() for plain
strings as I had to touch those lines anyway.

Filtering rtables with `T' does not toggle the column, just like
filtering users with `u' does not toggle between user and thread id.

Feedback? OK?

Index: display.c
===
RCS file: /cvs/src/usr.bin/top/display.c,v
retrieving revision 1.64
diff -u -p -r1.64 display.c
--- display.c   23 Aug 2020 21:11:55 -  1.64
+++ display.c   23 Aug 2020 22:39:47 -
@@ -824,6 +824,7 @@ show_help(void)
"r count pid  - renice process `pid' to nice value `count'\n"
"S- toggle the display of system processes\n"
"s time   - change delay between displays to `time' seconds\n"
+   "t- toggle the display of routing tables\n"
"T [-]rtable  - show processes associated with routing table 
`rtable'\n"
"   (T+ shows all, T -rtable hides rtable)\n"
"u [-]user- show processes for `user' (u+ shows all, u -user 
hides user)\n"
Index: machine.c
===
RCS file: /cvs/src/usr.bin/top/machine.c,v
retrieving revision 1.108
diff -u -p -r1.108 machine.c
--- machine.c   23 Aug 2020 21:11:55 -  1.108
+++ machine.c   23 Aug 2020 22:38:15 -
@@ -75,8 +75,9 @@ struct handle {
 static char header[] =
"  PID XPRI NICE  SIZE   RES STATE WAIT  TIMECPU 
COMMAND";
 
-/* 0123456   -- field to fill in starts at header+6 */
+/* offsets in the header line to start alternative columns */
 #define UNAME_START 6
+#define RTABLE_START 46
 
 #define Proc_format \
"%5d %-8.8s %3d %4d %5s %5s %-9s %-7.7s %6s %5.2f%% %s"
@@ -226,16 +227,20 @@ machine_init(struct statics *statics)
 }
 
 char *
-format_header(char *second_field)
+format_header(char *second_field, char *eighth_field)
 {
-   char *field_name, *thread_field = " TID";
-   char *ptr;
+   char *second_fieldp = second_field, *eighth_fieldp = eighth_field, *ptr;
 
-   field_name = second_field ? second_field : thread_field;
-
-   ptr = header + UNAME_START;
-   while (*field_name != '\0')
-   *ptr++ = *field_name++;
+   if (second_field != NULL) {
+   ptr = header + UNAME_START;
+   while (*second_fieldp != '\0')
+   *ptr++ = *second_fieldp++;
+   }
+   if (eighth_field != NULL) {
+   ptr = header + RTABLE_START;
+   while (*eighth_fieldp != '\0')
+   *ptr++ = *eighth_fieldp++;
+   }
return (header);
 }
 
@@ -414,7 +419,7 @@ get_process_info(struct system_info *si,
 int (*compare) (const void *, const void *))
 {
int show_idle, show_system, show_threads, show_uid, show_pid, show_cmd;
-   int show_rtable, hide_rtable, hide_uid;
+   int show_rtableid, hide_rtableid, hide_uid;
int total_procs, active_procs;
struct kinfo_proc **prefp, *pp;
int what = KERN_PROC_ALL;
@@ -446,8 +451,8 @@ get_process_info(struct system_info *si,
show_uid = sel->uid != (uid_t)-1;
hide_uid = sel->huid != (uid_t)-1;
show_pid = sel->pid != (pid_t)-1;
-   show_rtable = sel->rtableid != -1;
-   hide_rtable = sel->hrtableid != -1;
+   show_rtableid = sel->rtableid != -1;
+   hide_rtableid = sel->hrtableid != -1;
show_cmd = sel->command != NULL;
 
/* count up process states and get pointers to interesting procs */
@@ -476,8 +481,8 @@ get_process_info(struct system_info *si,
(!hide_uid || pp->p_ruid != sel->huid) &&
(!show_uid || pp->p_ruid == sel->uid) &&
(!show_pid || pp->p_pid == sel->pid) &&
-   (!hide_rtable || pp->p_rtableid != sel->hrtableid) 
&&
-   (!show_rtable || pp->p_rtableid == sel->rtableid) &&
+   (!hide_rtableid || pp->p_rtableid != 
sel->hrtableid) &&
+   (!show_rtableid || pp->p_rtableid == sel->rtableid) 
&&
(!show_cmd || cmd_matches(pp, sel->command))) {
*prefp++ = pp;

Re: top: filter by routing table

2020-08-23 Thread Klemens Nanni
On Sun, Aug 23, 2020 at 10:39:21PM +0200, Remi Locherer wrote:
> I like the feature and it works as advertised.
> 
> It would be nice to have a column that displays the rtable id of
> each process when T is used. When I type "T-0" I see a list of procs
> not in rtable 0.  But I still do not know in which one they are.
That's certainly possible, but we need to pick a column which is not
only suitable to omit but also wide enough to fit "RTABLE" as
description, I'd say.

Are you OK with the diff as is?  We can take care of the rest as a
separate diff.



Re: pf: remove ptr_array from struct pf_ruleset

2020-08-23 Thread Klemens Nanni
On Mon, Jul 20, 2020 at 05:07:03PM +0200, Klemens Nanni wrote:
> On Mon, Jul 20, 2020 at 01:14:00PM +0200, Alexandr Nedvedicky wrote:
> > I took a closer look at your change and related area.  Below is an alternate
> > way to fix the bug you've found.
> Thanks for bringing it up again, I forgot to reply earlier.
> 
> > there are few details worth to note:
> > 
> > ptr_array matters to main ruleset only, because it is the only 
> > ruleset covered by MD5 sum for pfsync.
> Correct, as is directly seen by the only caller pf_commit_rules():
> 
> 819 /* Calculate checksum for the main ruleset */
> 820 if (rs == _main_ruleset) {
> 821 error = pf_setup_pfsync_matching(rs);
> 822 if (error != 0)
> 823 return (error);
> 824 }
> 
> > it looks like we should also recompute the MD5sum if we remove the rule
> > from the main ruleset 
> Yes, but that is a separate bug, regardless of how we handle checksum
> calculation and rule lookups.
> 
> My plan was to tackle this next, but reusing existing code instead:
> 
> After my initial diff, pf_setup_pfsync_matching() would merely update
> the checksum, so we could rename the function to something more
> appropiate and call it from pf_purge_rule() instead of duplicating it
> there.
Here is a new diff that amends the previous with the following:

- rename pf_setup_pfsync_matching() to pf_calc_chksum()
- make it void and simplify callees
- rehash in pf_purge_rule() to account for removed `once' rules

> > my diff introduces a new member of pr_ruleset structure, which keeps the
> > array size so we can pass it to free(9f) later.
> I persued this first (as done in other free() size diffs) but eventually
> opted for the removal of ptr_array for the sake of simplicity.
> 
> > I have no pfsync deployment readily available for testing. Will be glad
> > if you can give my diff a try.
> Works as expected in both regards: pfsync picks the right rule and the
> checksum updates after rules expired.
I have tested this with my usual setup where states keep syncing and
`once' rules disappear from the main ruleset on the machine I hit,
compared to -CURRENT however watching `pfctl -vsi | grep Check' no shows
the checksum updating accordingly.

Admins using `once' rules are hopefully aware of this caveat already,
but now the checksum actually indicates out-of-sync rulesets and does
no longer present the same checksum for different rulesets.

Feedback? OK?


Index: if_pfsync.c
===
RCS file: /cvs/src/sys/net/if_pfsync.c,v
retrieving revision 1.277
diff -u -p -r1.277 if_pfsync.c
--- if_pfsync.c 21 Aug 2020 22:59:27 -  1.277
+++ if_pfsync.c 23 Aug 2020 12:09:39 -
@@ -500,6 +500,7 @@ pfsync_state_import(struct pfsync_state 
struct pfi_kif  *kif;
int pool_flags;
int error = ENOMEM;
+   int n = 0;
 
if (sp->creatorid == 0) {
DPFPRINTF(LOG_NOTICE, "pfsync_state_import: "
@@ -524,9 +525,11 @@ pfsync_state_import(struct pfsync_state 
 */
if (sp->rule != htonl(-1) && sp->anchor == htonl(-1) &&
(flags & (PFSYNC_SI_IOCTL | PFSYNC_SI_CKSUM)) && ntohl(sp->rule) <
-   pf_main_ruleset.rules.active.rcount)
-   r = pf_main_ruleset.rules.active.ptr_array[ntohl(sp->rule)];
-   else
+   pf_main_ruleset.rules.active.rcount) {
+   TAILQ_FOREACH(r, pf_main_ruleset.rules.active.ptr, entries)
+   if (ntohl(sp->rule) == n++)
+   break;
+   } else
r = _default_rule;
 
if ((r->max_states && r->states_cur >= r->max_states))
Index: pfvar.h
===
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.495
diff -u -p -r1.495 pfvar.h
--- pfvar.h 28 Jul 2020 16:47:42 -  1.495
+++ pfvar.h 23 Aug 2020 12:09:39 -
@@ -924,7 +924,6 @@ struct pf_ruleset {
struct pf_rulequeue  queues[2];
struct {
struct pf_rulequeue *ptr;
-   struct pf_rule  **ptr_array;
u_int32_trcount;
u_int32_tticket;
int  open;
Index: pf_ioctl.c
===
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.354
diff -u -p -r1.354 pf_ioctl.c
--- pf_ioctl.c  21 Jul 2020 14:13:17 -  1.354
+++ pf_ioctl.c  23 Aug 2020 13:50:53 -
@@ -97,7 +97

Re: top: filter by routing table

2020-08-22 Thread Klemens Nanni
On Sun, Aug 09, 2020 at 09:02:14PM +0200, Klemens Nanni wrote:
> Sometimes I want to see processes outside the default routing table with
> `-T -0', sometimes those in in a specific one with `-T 3' (for testing).
> 
> Since others have poked around with routing tables and/or domains as of
> late, perhaps this deemed useful enough?
> 
> Semantically, filtering is identical to that of users: pick or hide one;
> this makes the code pretty much copy/paste within top(1), manual wording
> and command line flag is taken from pgrep(1).
> 
> pgrep is currently the only way to identify processes by routing table.
> After netstat(1)'s relatively recent addition of `-R', filtering in top
> makes for a quite handy set of tooling around rtable(4).
> 
> Feedback? OK?
Anyone?

One thing is that routing tables are now the only values you can filter
for which are never visible in top - I thought about adding another
RTABLE column, replacing one as `H' does with USERNAME -> TID or so,
but did not deem it necessary/worthwile in the end.

What do you think?

Same diff below.


Index: display.c
===
RCS file: /cvs/src/usr.bin/top/display.c,v
retrieving revision 1.63
diff -u -p -r1.63 display.c
--- display.c   26 Jul 2020 21:59:16 -  1.63
+++ display.c   9 Aug 2020 17:55:56 -
@@ -824,6 +824,8 @@ show_help(void)
"r count pid  - renice process `pid' to nice value `count'\n"
"S- toggle the display of system processes\n"
"s time   - change delay between displays to `time' seconds\n"
+   "T [-]rtable  - show processes associated with routing table 
`rtable'\n"
+   "   (T+ shows all, T -rtable hides rtable)\n"
"u [-]user- show processes for `user' (u+ shows all, u -user 
hides user)\n"
"\n");
 
Index: machine.c
===
RCS file: /cvs/src/usr.bin/top/machine.c,v
retrieving revision 1.107
diff -u -p -r1.107 machine.c
--- machine.c   6 Jul 2020 16:27:59 -   1.107
+++ machine.c   9 Aug 2020 18:06:36 -
@@ -414,7 +414,7 @@ get_process_info(struct system_info *si,
 int (*compare) (const void *, const void *))
 {
int show_idle, show_system, show_threads, show_uid, show_pid, show_cmd;
-   int hide_uid;
+   int show_rtable, hide_rtable, hide_uid;
int total_procs, active_procs;
struct kinfo_proc **prefp, *pp;
int what = KERN_PROC_ALL;
@@ -446,6 +446,8 @@ get_process_info(struct system_info *si,
show_uid = sel->uid != (uid_t)-1;
hide_uid = sel->huid != (uid_t)-1;
show_pid = sel->pid != (pid_t)-1;
+   show_rtable = sel->rtableid != -1;
+   hide_rtable = sel->hrtableid != -1;
show_cmd = sel->command != NULL;
 
/* count up process states and get pointers to interesting procs */
@@ -474,6 +476,8 @@ get_process_info(struct system_info *si,
(!hide_uid || pp->p_ruid != sel->huid) &&
(!show_uid || pp->p_ruid == sel->uid) &&
(!show_pid || pp->p_pid == sel->pid) &&
+   (!hide_rtable || pp->p_rtableid != sel->hrtableid) 
&&
+   (!show_rtable || pp->p_rtableid == sel->rtableid) &&
(!show_cmd || cmd_matches(pp, sel->command))) {
*prefp++ = pp;
active_procs++;
Index: machine.h
===
RCS file: /cvs/src/usr.bin/top/machine.h,v
retrieving revision 1.29
diff -u -p -r1.29 machine.h
--- machine.h   25 Jun 2020 20:38:41 -  1.29
+++ machine.h   9 Aug 2020 17:50:14 -
@@ -77,6 +77,8 @@ struct process_select {
uid_t   uid;/* only this uid (unless uid == -1) */
uid_t   huid;   /* hide this uid (unless huid == -1) */
pid_t   pid;/* only this pid (unless pid == -1) */
+   int rtableid;   /* only this rtable (unless rtableid == 
-1) */
+   int hrtableid;  /* hide this rtable (unless hrtableid 
== -1) */
char   *command;/* only this command (unless == NULL) */
 };
 
Index: top.1
===
RCS file: /cvs/src/usr.bin/top/top.1,v
retrieving revision 1.75
diff -u -p -r1.75 top.1
--- top.1   26 Jul 2020 21:59:16 -  1.75
+++ top.1   9 Aug 2020 18:16:12 -
@@ -38,6 +38,7 @@
 .Op Fl o Oo - Oc Ns Ar field
 .Op Fl p Ar pid
 .Op Fl s Ar time
+.Op Fl T Oo - Oc Ns Ar rtable
 .Op Fl U Oo - Oc Ns Ar user
 .Op Ar number
 .Ek
@@ -179,6 +180,14 @@ Set the delay 

Re: sppp: add size to free() calls

2020-08-22 Thread Klemens Nanni
On Sat, Aug 22, 2020 at 02:32:17PM +0200, Klemens Nanni wrote:
> Another round, this time obvious sizes which are in immediate scope of
> the free() call, e.g. right below the malloc() call.
> 
> This leaves only a few selected free() calls with size zero in
> if_spppsubr.c due to the fact that there is currently no variable to
> keep track of username and password string lengths.
> 
> Feedback? OK?
Sorry, here's the correct version of the diff omitting sizes for the
very string buffers mentioned above.


Index: if_spppsubr.c
===
RCS file: /cvs/src/sys/net/if_spppsubr.c,v
retrieving revision 1.185
diff -u -p -r1.185 if_spppsubr.c
--- if_spppsubr.c   14 Aug 2020 12:17:34 -  1.185
+++ if_spppsubr.c   22 Aug 2020 14:55:49 -
@@ -1737,7 +1737,7 @@ sppp_lcp_RCR(struct sppp *sp, struct lcp
 
len -= 4;
origlen = len;
-   buf = r = malloc (len, M_TEMP, M_NOWAIT);
+   buf = r = malloc (origlen, M_TEMP, M_NOWAIT);
if (! buf)
return (0);
 
@@ -1749,7 +1749,7 @@ sppp_lcp_RCR(struct sppp *sp, struct lcp
p = (void*) (h+1);
for (rlen = 0; len > 1; len -= p[1], p += p[1]) {
if (p[1] < 2 || p[1] > len) {
-   free(buf, M_TEMP, 0);
+   free(buf, M_TEMP, origlen);
return (-1);
}
if (debug)
@@ -1926,7 +1926,7 @@ sppp_lcp_RCR(struct sppp *sp, struct lcp
}
 
  end:
-   free(buf, M_TEMP, 0);
+   free(buf, M_TEMP, origlen);
return (rlen == 0);
 }
 
@@ -2312,7 +2312,7 @@ sppp_ipcp_RCR(struct sppp *sp, struct lc
 {
u_char *buf, *r, *p;
struct ifnet *ifp = >pp_if;
-   int rlen, origlen, debug = ifp->if_flags & IFF_DEBUG;
+   int rlen, origlen, buflen, debug = ifp->if_flags & IFF_DEBUG;
u_int32_t hisaddr, desiredaddr;
 
len -= 4;
@@ -2321,7 +2321,8 @@ sppp_ipcp_RCR(struct sppp *sp, struct lc
 * Make sure to allocate a buf that can at least hold a
 * conf-nak with an `address' option.  We might need it below.
 */
-   buf = r = malloc ((len < 6? 6: len), M_TEMP, M_NOWAIT);
+   buflen = len < 6? 6: len;
+   buf = r = malloc (buflen, M_TEMP, M_NOWAIT);
if (! buf)
return (0);
 
@@ -2332,7 +2333,7 @@ sppp_ipcp_RCR(struct sppp *sp, struct lc
p = (void*) (h+1);
for (rlen = 0; len > 1; len -= p[1], p += p[1]) {
if (p[1] < 2 || p[1] > len) {
-   free(buf, M_TEMP, 0);
+   free(buf, M_TEMP, buflen);
return (-1);
}
if (debug)
@@ -2476,7 +2477,7 @@ sppp_ipcp_RCR(struct sppp *sp, struct lc
}
 
  end:
-   free(buf, M_TEMP, 0);
+   free(buf, M_TEMP, buflen);
return (rlen == 0);
 }
 
@@ -2773,7 +2774,7 @@ sppp_ipv6cp_RCR(struct sppp *sp, struct 
 {
u_char *buf, *r, *p;
struct ifnet *ifp = >pp_if;
-   int rlen, origlen, debug = ifp->if_flags & IFF_DEBUG;
+   int rlen, origlen, buflen, debug = ifp->if_flags & IFF_DEBUG;
struct in6_addr myaddr, desiredaddr, suggestaddr;
int ifidcount;
int type;
@@ -2786,7 +2787,8 @@ sppp_ipv6cp_RCR(struct sppp *sp, struct 
 * Make sure to allocate a buf that can at least hold a
 * conf-nak with an `address' option.  We might need it below.
 */
-   buf = r = malloc ((len < 6? 6: len), M_TEMP, M_NOWAIT);
+   buflen = len < 6? 6: len;
+   buf = r = malloc (buflen, M_TEMP, M_NOWAIT);
if (! buf)
return (0);
 
@@ -2799,7 +2801,7 @@ sppp_ipv6cp_RCR(struct sppp *sp, struct 
for (rlen=0; len>1 && p[1]; len-=p[1], p+=p[1]) {
/* Sanity check option length */
if (p[1] < 2 || p[1] > len) {
-   free(buf, M_TEMP, 0);
+   free(buf, M_TEMP, buflen);
return (-1);
}
if (debug)
@@ -2933,7 +2935,7 @@ sppp_ipv6cp_RCR(struct sppp *sp, struct 
}
 
 end:
-   free(buf, M_TEMP, 0);
+   free(buf, M_TEMP, buflen);
return (rlen == 0);
 }
 
@@ -4475,10 +4477,10 @@ sppp_get_params(struct sppp *sp, struct 
spr->phase = sp->pp_phase;
 
if (copyout(spr, (caddr_t)ifr->ifr_data, sizeof(*spr)) != 0) {
-   free(spr, M_DEVBUF, 0);
+   free(spr, M_DEVBUF, sizeof(*spr));
return EFAULT;
}
-   free(spr, M_DEVBUF, 0);
+   free(spr, M_DEVBUF, sizeof(*spr));
break;
}
case SPPPIOGMAUTH:
@@ -4498,10 +4500,10 @@ sppp_get_params(struct sppp *sp, struct 
strlcpy(spa-&g

sppp: add size to free() calls

2020-08-22 Thread Klemens Nanni
Another round, this time obvious sizes which are in immediate scope of
the free() call, e.g. right below the malloc() call.

This leaves only a few selected free() calls with size zero in
if_spppsubr.c due to the fact that there is currently no variable to
keep track of username and password string lengths.

Feedback? OK?


Index: if_spppsubr.c
===
RCS file: /cvs/src/sys/net/if_spppsubr.c,v
retrieving revision 1.185
diff -u -p -r1.185 if_spppsubr.c
--- if_spppsubr.c   14 Aug 2020 12:17:34 -  1.185
+++ if_spppsubr.c   22 Aug 2020 12:25:37 -
@@ -1737,7 +1737,7 @@ sppp_lcp_RCR(struct sppp *sp, struct lcp
 
len -= 4;
origlen = len;
-   buf = r = malloc (len, M_TEMP, M_NOWAIT);
+   buf = r = malloc (origlen, M_TEMP, M_NOWAIT);
if (! buf)
return (0);
 
@@ -1749,7 +1749,7 @@ sppp_lcp_RCR(struct sppp *sp, struct lcp
p = (void*) (h+1);
for (rlen = 0; len > 1; len -= p[1], p += p[1]) {
if (p[1] < 2 || p[1] > len) {
-   free(buf, M_TEMP, 0);
+   free(buf, M_TEMP, origlen);
return (-1);
}
if (debug)
@@ -1926,7 +1926,7 @@ sppp_lcp_RCR(struct sppp *sp, struct lcp
}
 
  end:
-   free(buf, M_TEMP, 0);
+   free(buf, M_TEMP, origlen);
return (rlen == 0);
 }
 
@@ -2312,7 +2312,7 @@ sppp_ipcp_RCR(struct sppp *sp, struct lc
 {
u_char *buf, *r, *p;
struct ifnet *ifp = >pp_if;
-   int rlen, origlen, debug = ifp->if_flags & IFF_DEBUG;
+   int rlen, origlen, buflen, debug = ifp->if_flags & IFF_DEBUG;
u_int32_t hisaddr, desiredaddr;
 
len -= 4;
@@ -2321,7 +2321,8 @@ sppp_ipcp_RCR(struct sppp *sp, struct lc
 * Make sure to allocate a buf that can at least hold a
 * conf-nak with an `address' option.  We might need it below.
 */
-   buf = r = malloc ((len < 6? 6: len), M_TEMP, M_NOWAIT);
+   buflen = len < 6? 6: len;
+   buf = r = malloc (buflen, M_TEMP, M_NOWAIT);
if (! buf)
return (0);
 
@@ -2332,7 +2333,7 @@ sppp_ipcp_RCR(struct sppp *sp, struct lc
p = (void*) (h+1);
for (rlen = 0; len > 1; len -= p[1], p += p[1]) {
if (p[1] < 2 || p[1] > len) {
-   free(buf, M_TEMP, 0);
+   free(buf, M_TEMP, buflen);
return (-1);
}
if (debug)
@@ -2476,7 +2477,7 @@ sppp_ipcp_RCR(struct sppp *sp, struct lc
}
 
  end:
-   free(buf, M_TEMP, 0);
+   free(buf, M_TEMP, buflen);
return (rlen == 0);
 }
 
@@ -2773,7 +2774,7 @@ sppp_ipv6cp_RCR(struct sppp *sp, struct 
 {
u_char *buf, *r, *p;
struct ifnet *ifp = >pp_if;
-   int rlen, origlen, debug = ifp->if_flags & IFF_DEBUG;
+   int rlen, origlen, buflen, debug = ifp->if_flags & IFF_DEBUG;
struct in6_addr myaddr, desiredaddr, suggestaddr;
int ifidcount;
int type;
@@ -2786,7 +2787,8 @@ sppp_ipv6cp_RCR(struct sppp *sp, struct 
 * Make sure to allocate a buf that can at least hold a
 * conf-nak with an `address' option.  We might need it below.
 */
-   buf = r = malloc ((len < 6? 6: len), M_TEMP, M_NOWAIT);
+   buflen = len < 6? 6: len;
+   buf = r = malloc (buflen, M_TEMP, M_NOWAIT);
if (! buf)
return (0);
 
@@ -2799,7 +2801,7 @@ sppp_ipv6cp_RCR(struct sppp *sp, struct 
for (rlen=0; len>1 && p[1]; len-=p[1], p+=p[1]) {
/* Sanity check option length */
if (p[1] < 2 || p[1] > len) {
-   free(buf, M_TEMP, 0);
+   free(buf, M_TEMP, buflen);
return (-1);
}
if (debug)
@@ -2933,7 +2935,7 @@ sppp_ipv6cp_RCR(struct sppp *sp, struct 
}
 
 end:
-   free(buf, M_TEMP, 0);
+   free(buf, M_TEMP, buflen);
return (rlen == 0);
 }
 
@@ -4475,10 +4477,10 @@ sppp_get_params(struct sppp *sp, struct 
spr->phase = sp->pp_phase;
 
if (copyout(spr, (caddr_t)ifr->ifr_data, sizeof(*spr)) != 0) {
-   free(spr, M_DEVBUF, 0);
+   free(spr, M_DEVBUF, sizeof(*spr));
return EFAULT;
}
-   free(spr, M_DEVBUF, 0);
+   free(spr, M_DEVBUF, sizeof(*spr));
break;
}
case SPPPIOGMAUTH:
@@ -4498,10 +4500,10 @@ sppp_get_params(struct sppp *sp, struct 
strlcpy(spa->name, auth->name, sizeof(spa->name));
 
if (copyout(spa, (caddr_t)ifr->ifr_data, sizeof(*spa)) != 0) {
-   free(spa, M_DEVBUF, 0);
+   free(spa, M_DEVBUF, sizeof(*spa));
return EFAULT;
}
-   free(spa, 

*_clone_create: leave default ifq_maxlen handling to ifq_init()

2020-08-21 Thread Klemens Nanni
Creating a cloned interface requires attaching it in the end, that's how
it works.

All clonable interfaces start with a fresh softc structure that all
zeros after allocation due to malloc(9)'s M_ZERO flag.

After driver dependent setup, all drivers call if_attach() to present
the new interface to the stack.

if_attach() starts with calling if_attach_common() which starts with
preparing the interface queues and therefore calling ifq_init() on the
send queue.

ifq_init() eventually checks the queue's maximum length and defaults to
IFQ_MAXLEN if it is zero, which it always is during this create/attach
path:

if (ifq->ifq_maxlen == 0)
ifq_set_maxlen(ifq, IFQ_MAXLEN);

Now, most clonable interface drivers (except bridge, enc, loop, pppx,
switch, trunk and vlan) initialise the send queue's length to IFQ_MAXLEN
the same way, which seems entirely redundant to me.

The queue API does this in a central place already and it bothered me
why not all drivers did the same in this regard, until I concluded this.

Is my analysis correct?
If so, I'd like to remove the redundant init code and unify drivers a
tiny bit.

Feedback? Objections? OK?


Index: if_aggr.c
===
RCS file: /cvs/src/sys/net/if_aggr.c,v
retrieving revision 1.33
diff -u -p -r1.33 if_aggr.c
--- if_aggr.c   22 Jul 2020 02:16:01 -  1.33
+++ if_aggr.c   21 Aug 2020 20:33:36 -
@@ -561,7 +561,6 @@ aggr_clone_create(struct if_clone *ifc, 
ifp->if_flags = IFF_BROADCAST | IFF_MULTICAST | IFF_SIMPLEX;
ifp->if_xflags = IFXF_CLONED | IFXF_MPSAFE;
ifp->if_link_state = LINK_STATE_DOWN;
-   ifq_set_maxlen(>if_snd, IFQ_MAXLEN);
ether_fakeaddr(ifp);
 
if_counters_alloc(ifp);
Index: if_bpe.c
===
RCS file: /cvs/src/sys/net/if_bpe.c,v
retrieving revision 1.13
diff -u -p -r1.13 if_bpe.c
--- if_bpe.c22 Jul 2020 08:38:51 -  1.13
+++ if_bpe.c21 Aug 2020 20:33:36 -
@@ -189,7 +189,6 @@ bpe_clone_create(struct if_clone *ifc, i
ifp->if_start = bpe_start;
ifp->if_flags = IFF_BROADCAST | IFF_MULTICAST;
ifp->if_xflags = IFXF_CLONED;
-   ifq_set_maxlen(>if_snd, IFQ_MAXLEN);
ether_fakeaddr(ifp);
 
if_counters_alloc(ifp);
Index: if_etherip.c
===
RCS file: /cvs/src/sys/net/if_etherip.c,v
retrieving revision 1.46
diff -u -p -r1.46 if_etherip.c
--- if_etherip.c10 Jul 2020 13:26:41 -  1.46
+++ if_etherip.c21 Aug 2020 20:33:36 -
@@ -150,7 +150,6 @@ etherip_clone_create(struct if_clone *if
ifp->if_start = etherip_start;
ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
ifp->if_xflags = IFXF_CLONED;
-   ifq_set_maxlen(>if_snd, IFQ_MAXLEN);
ifp->if_capabilities = IFCAP_VLAN_MTU;
ether_fakeaddr(ifp);
 
Index: if_gif.c
===
RCS file: /cvs/src/sys/net/if_gif.c,v
retrieving revision 1.130
diff -u -p -r1.130 if_gif.c
--- if_gif.c10 Jul 2020 13:26:41 -  1.130
+++ if_gif.c21 Aug 2020 20:33:36 -
@@ -170,7 +170,6 @@ gif_clone_create(struct if_clone *ifc, i
ifp->if_output = gif_output;
ifp->if_rtrequest = p2p_rtrequest;
ifp->if_type   = IFT_GIF;
-   ifq_set_maxlen(>if_snd, IFQ_MAXLEN);
ifp->if_softc = sc;
 
if_attach(ifp);
Index: if_gre.c
===
RCS file: /cvs/src/sys/net/if_gre.c,v
retrieving revision 1.158
diff -u -p -r1.158 if_gre.c
--- if_gre.c10 Jul 2020 13:26:41 -  1.158
+++ if_gre.c21 Aug 2020 20:33:36 -
@@ -715,7 +715,6 @@ egre_clone_create(struct if_clone *ifc, 
ifp->if_ioctl = egre_ioctl;
ifp->if_start = egre_start;
ifp->if_xflags = IFXF_CLONED;
-   ifq_set_maxlen(>if_snd, IFQ_MAXLEN);
ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
ether_fakeaddr(ifp);
 
@@ -777,7 +776,6 @@ nvgre_clone_create(struct if_clone *ifc,
ifp->if_ioctl = nvgre_ioctl;
ifp->if_start = nvgre_start;
ifp->if_xflags = IFXF_CLONED;
-   ifq_set_maxlen(>if_snd, IFQ_MAXLEN);
ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
ether_fakeaddr(ifp);
 
@@ -849,7 +847,6 @@ eoip_clone_create(struct if_clone *ifc, 
ifp->if_ioctl = eoip_ioctl;
ifp->if_start = eoip_start;
ifp->if_xflags = IFXF_CLONED;
-   ifq_set_maxlen(>if_snd, IFQ_MAXLEN);
ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
ether_fakeaddr(ifp);
 
Index: if_mpe.c
===
RCS file: /cvs/src/sys/net/if_mpe.c,v
retrieving revision 1.96
diff -u -p -r1.96 if_mpe.c
--- if_mpe.c10 Jul 2020 13:26:41 -  1.96
+++ if_mpe.c21 Aug 

Re: pppoe: add sizes to free() calls

2020-08-20 Thread Klemens Nanni
On Thu, Aug 20, 2020 at 03:33:17PM +0200, Klemens Nanni wrote:
> These are straight forward as we either maintain a size variable all the
> way or can reuse strlen() for free() just like it's done during malloc().
> 
> One exception is freeing the softc structure, which is fixed in size;
> `ifconfig pppoe1 create; ifconfig pppoe1 destroy' exercises this code
> path and does not blow up - as expected.
> 
> Running fine on a octeon vdsl2 router.
> 
> Feedback? OK?
Sorry for the noise, Peter J. Philipp pointed out how I missed the +1
byte to account for the NUL character which strlen(9) does not count.
All corresponding malloc calls do strlen() + 1, so without it there's an
off-by-one in the size.

In my setup I do not exercise the service and concentrator name paths,
but playing with `ifconfig pppoe0 [[-]pppoeac foo] [[-]pppoesvc bar]'
doesn't blow up on any of those diff versions, either.

Fixed diff.

Index: if_pppoe.c
===
RCS file: /cvs/src/sys/net/if_pppoe.c,v
retrieving revision 1.70
diff -u -p -r1.70 if_pppoe.c
--- if_pppoe.c  28 Jul 2020 09:52:32 -  1.70
+++ if_pppoe.c  20 Aug 2020 13:50:07 -
@@ -257,15 +267,17 @@ pppoe_clone_destroy(struct ifnet *ifp)
if_detach(ifp);
 
if (sc->sc_concentrator_name)
-   free(sc->sc_concentrator_name, M_DEVBUF, 0);
+   free(sc->sc_concentrator_name, M_DEVBUF,
+   strlen(sc->sc_concentrator_name) + 1);
if (sc->sc_service_name)
-   free(sc->sc_service_name, M_DEVBUF, 0);
+   free(sc->sc_service_name, M_DEVBUF,
+   strlen(sc->sc_service_name) + 1);
if (sc->sc_ac_cookie)
-   free(sc->sc_ac_cookie, M_DEVBUF, 0);
+   free(sc->sc_ac_cookie, M_DEVBUF, sc->sc_ac_cookie_len);
if (sc->sc_relay_sid)
-   free(sc->sc_relay_sid, M_DEVBUF, 0);
+   free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len);
 
-   free(sc, M_DEVBUF, 0);
+   free(sc, M_DEVBUF, sizeof(*sc));
 
return (0);
 }
@@ -547,7 +559,8 @@ breakbreak:
}
if (ac_cookie) {
if (sc->sc_ac_cookie)
-   free(sc->sc_ac_cookie, M_DEVBUF, 0);
+   free(sc->sc_ac_cookie, M_DEVBUF,
+   sc->sc_ac_cookie_len);
sc->sc_ac_cookie = malloc(ac_cookie_len, M_DEVBUF,
M_DONTWAIT);
if (sc->sc_ac_cookie == NULL)
@@ -557,7 +570,8 @@ breakbreak:
}
if (relay_sid) {
if (sc->sc_relay_sid)
-   free(sc->sc_relay_sid, M_DEVBUF, 0);
+   free(sc->sc_relay_sid, M_DEVBUF,
+   sc->sc_relay_sid_len);
sc->sc_relay_sid = malloc(relay_sid_len, M_DEVBUF,
M_DONTWAIT);
if (sc->sc_relay_sid == NULL)
@@ -610,11 +624,12 @@ breakbreak:
sc->sc_state = PPPOE_STATE_INITIAL;
memcpy(>sc_dest, etherbroadcastaddr, sizeof(sc->sc_dest));
if (sc->sc_ac_cookie) {
-   free(sc->sc_ac_cookie, M_DEVBUF, 0);
+   free(sc->sc_ac_cookie, M_DEVBUF,
+   sc->sc_ac_cookie_len);
sc->sc_ac_cookie = NULL;
}
if (sc->sc_relay_sid) {
-   free(sc->sc_relay_sid, M_DEVBUF, 0);
+   free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len);
sc->sc_relay_sid = NULL;
}
sc->sc_ac_cookie_len = 0;
@@ -817,7 +847,8 @@ pppoe_ioctl(struct ifnet *ifp, unsigned 
}
 
if (sc->sc_concentrator_name)
-   free(sc->sc_concentrator_name, M_DEVBUF, 0);
+   free(sc->sc_concentrator_name, M_DEVBUF,
+   strlen(sc->sc_concentrator_name) + 1);
sc->sc_concentrator_name = NULL;
 
len = strlen(parms->ac_name);
@@ -830,7 +861,8 @@ pppoe_ioctl(struct ifnet *ifp, unsigned 
}
 
if (sc->sc_service_name)
-   free(sc->sc_service_name, M_DEVBUF, 0);
+   free(sc->sc_service_name, M_DEVBUF,
+   strlen(sc->sc_service_name) + 1);
sc->sc_service_name = NULL;
 
len = strlen(parms->service_name);
@@ -1175,12 +1207,12 @@ pppoe_disconnect(struct pppoe_softc *sc)
sc->sc_state = PPPOE_STATE_INITIAL;
memcpy(>sc_dest, etherbroadcastaddr,

pppoe: add sizes to free() calls

2020-08-20 Thread Klemens Nanni
These are straight forward as we either maintain a size variable all the
way or can reuse strlen() for free() just like it's done during malloc().

One exception is freeing the softc structure, which is fixed in size;
`ifconfig pppoe1 create; ifconfig pppoe1 destroy' exercises this code
path and does not blow up - as expected.

Running fine on a octeon vdsl2 router.

Feedback? OK?


Index: if_pppoe.c
===
RCS file: /cvs/src/sys/net/if_pppoe.c,v
retrieving revision 1.70
diff -u -p -r1.70 if_pppoe.c
--- if_pppoe.c  28 Jul 2020 09:52:32 -  1.70
+++ if_pppoe.c  19 Aug 2020 23:33:23 -
@@ -257,15 +264,17 @@ pppoe_clone_destroy(struct ifnet *ifp)
if_detach(ifp);
 
if (sc->sc_concentrator_name)
-   free(sc->sc_concentrator_name, M_DEVBUF, 0);
+   free(sc->sc_concentrator_name, M_DEVBUF,
+   strlen(sc->sc_concentrator_name));
if (sc->sc_service_name)
-   free(sc->sc_service_name, M_DEVBUF, 0);
+   free(sc->sc_service_name, M_DEVBUF,
+   strlen(sc->sc_service_name));
if (sc->sc_ac_cookie)
-   free(sc->sc_ac_cookie, M_DEVBUF, 0);
+   free(sc->sc_ac_cookie, M_DEVBUF, sc->sc_ac_cookie_len);
if (sc->sc_relay_sid)
-   free(sc->sc_relay_sid, M_DEVBUF, 0);
+   free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len);
 
-   free(sc, M_DEVBUF, 0);
+   free(sc, M_DEVBUF, sizeof(*sc));
 
return (0);
 }
@@ -547,7 +556,8 @@ breakbreak:
}
if (ac_cookie) {
if (sc->sc_ac_cookie)
-   free(sc->sc_ac_cookie, M_DEVBUF, 0);
+   free(sc->sc_ac_cookie, M_DEVBUF,
+   sc->sc_ac_cookie_len);
sc->sc_ac_cookie = malloc(ac_cookie_len, M_DEVBUF,
M_DONTWAIT);
if (sc->sc_ac_cookie == NULL)
@@ -557,7 +567,8 @@ breakbreak:
}
if (relay_sid) {
if (sc->sc_relay_sid)
-   free(sc->sc_relay_sid, M_DEVBUF, 0);
+   free(sc->sc_relay_sid, M_DEVBUF,
+   sc->sc_relay_sid_len);
sc->sc_relay_sid = malloc(relay_sid_len, M_DEVBUF,
M_DONTWAIT);
if (sc->sc_relay_sid == NULL)
@@ -610,11 +621,12 @@ breakbreak:
sc->sc_state = PPPOE_STATE_INITIAL;
memcpy(>sc_dest, etherbroadcastaddr, sizeof(sc->sc_dest));
if (sc->sc_ac_cookie) {
-   free(sc->sc_ac_cookie, M_DEVBUF, 0);
+   free(sc->sc_ac_cookie, M_DEVBUF,
+   sc->sc_ac_cookie_len);
sc->sc_ac_cookie = NULL;
}
if (sc->sc_relay_sid) {
-   free(sc->sc_relay_sid, M_DEVBUF, 0);
+   free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len);
sc->sc_relay_sid = NULL;
}
sc->sc_ac_cookie_len = 0;
@@ -817,7 +835,8 @@ pppoe_ioctl(struct ifnet *ifp, unsigned 
}
 
if (sc->sc_concentrator_name)
-   free(sc->sc_concentrator_name, M_DEVBUF, 0);
+   free(sc->sc_concentrator_name, M_DEVBUF,
+   strlen(sc->sc_concentrator_name));
sc->sc_concentrator_name = NULL;
 
len = strlen(parms->ac_name);
@@ -830,7 +849,8 @@ pppoe_ioctl(struct ifnet *ifp, unsigned 
}
 
if (sc->sc_service_name)
-   free(sc->sc_service_name, M_DEVBUF, 0);
+   free(sc->sc_service_name, M_DEVBUF,
+   strlen(sc->sc_service_name));
sc->sc_service_name = NULL;
 
len = strlen(parms->service_name);
@@ -1175,12 +1195,12 @@ pppoe_disconnect(struct pppoe_softc *sc)
sc->sc_state = PPPOE_STATE_INITIAL;
memcpy(>sc_dest, etherbroadcastaddr, sizeof(sc->sc_dest));
if (sc->sc_ac_cookie) {
-   free(sc->sc_ac_cookie, M_DEVBUF, 0);
+   free(sc->sc_ac_cookie, M_DEVBUF, sc->sc_ac_cookie_len);
sc->sc_ac_cookie = NULL;
}
sc->sc_ac_cookie_len = 0;
if (sc->sc_relay_sid) {
-   free(sc->sc_relay_sid, M_DEVBUF, 0);
+   free(sc->sc_relay_sid, M_DEVBUF, sc->sc_relay_sid_len);
sc->sc_relay_sid = NULL;
}
sc->sc_relay_sid_len = 0;



Re: openrsync(1): add support for IPv6-only hosts

2020-08-18 Thread Klemens Nanni
On Tue, Aug 18, 2020 at 09:58:56AM +0200, Sasha Romijn wrote:
> The current openrsync client is not able to connect to dual-stack remote 
> hosts, when the local host does not have any IPv4 connectivity. This is 
> because connect() fails with EADDRNOTAVAIL when trying to connect to the 
> remote IPv4 address on an IPv6-only local host - and IPv4 seems to be 
> attempted first. The current client does not try other remote host addresses 
> after EADDRNOTAVAIL, and therefore never tries the remote IPv6 address.
For openrsync(1) this won't happen with `family inet6 inet4' in
resolv.conf(5).

> The included patch allows the client to continue after EADDRNOTAVAIL, making 
> it to try other addresses, until it reaches a working IPv6 address. If the 
> local host is IPv6-only and the remote host IPv4-only, the connection still 
> fails with an error produced from rsync_connect().
Your diff reads correct to me, thanks; regular rsync(1) connects fine
regardless of `family'.

> Perhaps a warning should be issued for the EADDRNOTAVAIL case, like it does 
> for EHOSTUNREACH - but I thought it would be a bit much, as an IPv6-only host 
> would then emit warnings on pretty much every single use of the openrsync 
> client.
Yes, I don't think a warning is warrented here - afterall it can connect
without problems.

OK to commit anyone?



Re: slaacd(8): use correct source link-layer address

2020-08-18 Thread Klemens Nanni
On Tue, Aug 18, 2020 at 06:14:30PM +0200, Florian Obser wrote:
> When sending a router solicitation use the link-layer (mac) address of
> the outgoing interface in the source link-layer address ICMPv6 option
> instead of the address of the last configured autoconf interface.
> 
> It is not the most efficient way to first transform an if_index into
> and interface name and then iterate over all addresses but this is
> also not in the hot path. Under normal operations slaacd will send
> one solicitation when an interface is set to autoconf and then
> never again because it will see unsolicitated router advertisements
> before addresses expire.
Still sensible and works, OK kn.



Re: pppoe: start without kernel lock

2020-08-16 Thread Klemens Nanni
On Sun, Aug 16, 2020 at 07:04:46PM +0200, Klemens Nanni wrote:
> Make sppp(4)/pppoe(4) use the ifq API to send packets outside the big
> lock.
> 
> As far as I understand, pppoe_output() does not require NET_LOCK() since
> if_get(9)/if_put(9) guarantee the validity of the interface pointer and
> no `struct ifnet' member is written to;  similar to how vlan(4) does it.
> 
> This is running on an EdgeRouter 4 behind a VDSL2 modem, i.e. pppoe0
> over vlan7 over cnmac0.
> 
> Do I miss something?
It seems I did:  Even though pppoe doesn't write the ifnet, locks must
be grabbed for each member that is read from.

`if_flags' for example is documented as protected by the NET_LOCK();
pppoe_output() checks if the interface is up and running, yet only some
but not all call paths to pppoe_output() grab the NET_LOCK().

So it seems more work is needed here, I see if I can identify all code
paths wrt. their locking situation.



pppoe: start without kernel lock

2020-08-16 Thread Klemens Nanni
Make sppp(4)/pppoe(4) use the ifq API to send packets outside the big
lock.

As far as I understand, pppoe_output() does not require NET_LOCK() since
if_get(9)/if_put(9) guarantee the validity of the interface pointer and
no `struct ifnet' member is written to;  similar to how vlan(4) does it.

This is running on an EdgeRouter 4 behind a VDSL2 modem, i.e. pppoe0
over vlan7 over cnmac0.

Do I miss something?
Feedback? OK?

Index: if_pppoe.c
===
RCS file: /cvs/src/sys/net/if_pppoe.c,v
retrieving revision 1.70
diff -u -p -r1.70 if_pppoe.c
--- if_pppoe.c  28 Jul 2020 09:52:32 -  1.70
+++ if_pppoe.c  16 Aug 2020 16:18:19 -
@@ -154,7 +154,7 @@ static void pppoe_abort_connect(struct p
 static int  pppoe_ioctl(struct ifnet *, unsigned long, caddr_t);
 static void pppoe_tls(struct sppp *);
 static void pppoe_tlf(struct sppp *);
-static void pppoe_start(struct ifnet *);
+static void pppoe_start(struct ifqueue *);
 
 /* internal timeout handling */
 static void pppoe_timeout(void *);
@@ -208,9 +208,9 @@ pppoe_clone_create(struct if_clone *ifc,
sc->sc_sppp.pp_flags |= PP_KEEPALIVE;   /* use LCP keepalive */
sc->sc_sppp.pp_framebytes = PPPOE_HEADERLEN;/* framing added to ppp 
packets */
sc->sc_sppp.pp_if.if_ioctl = pppoe_ioctl;
-   sc->sc_sppp.pp_if.if_start = pppoe_start;
+   sc->sc_sppp.pp_if.if_qstart = pppoe_start;
sc->sc_sppp.pp_if.if_rtrequest = p2p_rtrequest;
-   sc->sc_sppp.pp_if.if_xflags = IFXF_CLONED;
+   sc->sc_sppp.pp_if.if_xflags = IFXF_CLONED | IFXF_MPSAFE;
sc->sc_sppp.pp_tls = pppoe_tls;
sc->sc_sppp.pp_tlf = pppoe_tlf;
ifq_set_maxlen(>sc_sppp.pp_if.if_snd, IFQ_MAXLEN);
@@ -1351,9 +1351,10 @@ pppoe_tlf(struct sppp *sp)
 }
 
 static void
-pppoe_start(struct ifnet *ifp)
+pppoe_start(struct ifqueue *ifq)
 {
-   struct pppoe_softc *sc = (void *)ifp;
+   struct ifnet *ifp = ifq->ifq_if;
+   struct pppoe_softc *sc = ifp->if_softc;
struct mbuf *m;
size_t len;
u_int8_t *p;
Index: if_spppsubr.c
===
RCS file: /cvs/src/sys/net/if_spppsubr.c,v
retrieving revision 1.185
diff -u -p -r1.185 if_spppsubr.c
--- if_spppsubr.c   14 Aug 2020 12:17:34 -  1.185
+++ if_spppsubr.c   16 Aug 2020 16:12:25 -
@@ -936,7 +936,7 @@ sppp_cp_send(struct sppp *sp, u_short pr
 
ifp->if_obytes += len;
s = splnet();
-   if_start(ifp);
+   ifq_start(>if_snd);
splx(s);
 }
 
@@ -4031,7 +4031,7 @@ sppp_auth_send(const struct cp *cp, stru
 
ifp->if_obytes += len;
s = splnet();
-   if_start(ifp);
+   ifq_start(>if_snd);
splx(s);
 }
 



Re: switch: allow datapath_id and maxflow ioctls for non-root

2020-08-14 Thread Klemens Nanni
On Fri, Jul 31, 2020 at 06:28:32AM +0200, Klemens Nanni wrote:
> ifconfig(8) detects switch(4) through its unique SIOCSWSDPID ioctl and
> further does another switch specific ioctl for the default output
> regardless of configuration and/or members:
> 
>   SIOCSWSDPID struct ifbrparam
>   Set the datapath_id in the OpenFlow protocol of the switch named
>   in ifbrp_name to the value in the ifbrpu_datapath field.
>   
>   SIOCSWGMAXFLOW struct ifbrparam
>   Retrieve the maximum number of flows in the OpenFlow protocol of
>   the switch named in ifbrp_name into the ifbrp_maxflow field.
> 
> This is how it should look like:
> 
>   # ifconfig switch0 create
>   # ifconfig switch0
>   switch0: flags=0<>
>   index 29 llprio 3
>   groups: switch
>   datapath 0x5bea2b5b8e2456cf maxflow 1 maxgroup 1000
> 
> But using ifconfig as unprivileged user makes it fail switch(4)
> interfaces as such and thus interprets them as bridge(4) instead:
> 
>   $ ifconfig switch0
>   switch0: flags=0<>
>   index 29 llprio 3
>   groups: switch
>   priority 32768 hellotime 2 fwddelay 15 maxage 20 holdcnt 6 
> proto rstp
>   designated: id 00:00:00:00:00:00 priority 0
> 
> This is because the above mentioned ioctls are listed together with all
> other bridge and switch related ioctls that set or write things.
> Getting datapath_id and maxflow values however is read-only and crucial
> for ifconfig as demonstrated above, so I'd like to move them out of the
> root check to fix ifconfig.
> 
> Feedback? OK?
Ping.

Last diff after dlg's feedback reattached.


Index: if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.618
diff -u -p -r1.618 if.c
--- if.c5 Aug 2020 11:07:34 -   1.618
+++ if.c5 Aug 2020 22:31:44 -
@@ -2160,9 +2160,7 @@ ifioctl(struct socket *so, u_long cmd, c
case SIOCBRDGSIFCOST:
case SIOCBRDGSTXHC:
case SIOCBRDGSPROTO:
-   case SIOCSWGDPID:
case SIOCSWSPORTNO:
-   case SIOCSWGMAXFLOW:
 #endif
if ((error = suser(p)) != 0)
break;



if_spppsubr.c: zap LOOPALIVECNT

2020-08-13 Thread Klemens Nanni
Unused since

revision 1.138
date: 2015/09/30 09:45:20;  author: sthen;  state: Exp;  lines: +50 
-279;  commitid: 0pACTtU
Sw4WmBBBr;
remove cisco hdlc code from sppp(4), it's no longer used - pppoe(4) 
only uses
ppp framing, and the drivers for sync serial cards have been removed so 
the
sppp code is now only used to support pppoe(4).  ok mpi@, kill it chris@

OK?


Index: if_spppsubr.c
===
RCS file: /cvs/src/sys/net/if_spppsubr.c,v
retrieving revision 1.184
diff -u -p -r1.184 if_spppsubr.c
--- if_spppsubr.c   10 Jul 2020 13:26:42 -  1.184
+++ if_spppsubr.c   13 Aug 2020 21:46:37 -
@@ -68,7 +68,6 @@
 # define UNTIMEOUT(fun, arg, handle)   \
timeout_del(&(handle))
 
-#define LOOPALIVECNT   3   /* loopback detection tries */
 #define MAXALIVECNT3   /* max. missed alive packets */
 #defineNORECV_TIME 15  /* before we get 
worried */
 



Re: PATCH: iostat spacing

2020-08-09 Thread Klemens Nanni
On Sat, Aug 08, 2020 at 04:12:31AM +0200, Klemens Nanni wrote:
> This is OK with me as it fixes the default view, but I think other views
> need fixing as well, e.g.
> 
>   $ iostat -I
>   ttysd0 sd1
> cpu
>  tin tout  KB/t   xfr MB   KB/t   xfr MB  us ni sy sp in 
> id
>   178524 22139320 26.99 10698431 281987.61  11.78 7582117 87218.82   7  0 
>  3  1  0 88
That was a bad example since `-I' shows the running total, hence values
that only ever increase and most likely change in magnitude.

> I'm testing with `dd if=/dev/rsd0c of=/dev/null bs=1m' on a X230 with
> sd0 at scsibus1 targ 0 lun 0:  
> naa.5002538d410a7bce

> I don't have that fast disks at hand, but expanding it by another column
> makes sense to me and could further simplify some of the code.
Here's a diff that expands `-d' (done by default) showing MB values by
two columns and `-D' showing KB values by one column;  `-I' is not
changed for above mentioned reasons.

This seems consistent to me on all disk related views and leaves room
for faster disks, should they ever read/write faster than 1000 MB/s.

Running with above `dd' to get reads, here's the visible change of this
diff:

$ iostat -c2
  tty  sd0   sd1cpu
 tin tout  KB/t  t/s  MB/s   KB/t  t/s  MB/s  us ni sy sp in id
   2  394 60.83  285 16.90  18.43   20  0.36   4  0  3  1  0 91
   1  195 63.97 3954 247.00   2.002  0.00   0  0 11  0  0 89
$ ./obj/iostat -c2
  ttysd0 sd1cpu
 tin tout  KB/t  t/sMB/s   KB/t  t/sMB/s  us ni sy sp in id
   2  393 60.94  294   17.51  18.43   200.36   4  0  3  1  0 91
   1  205 64.00 3560  222.52   0.0000.00   0  0 14  1  2 83

$ iostat -d -c2  
  sd0   sd1 
  KB/t  t/s  MB/s   KB/t  t/s  MB/s 
 61.02  301 17.96  18.43   20  0.35 
 64.00 3613 225.81   0.000  0.00 
$ ./obj/iostat -d -c2
sd0 sd1 
  KB/t  t/sMB/s   KB/t  t/sMB/s 
 61.06  305   18.18  18.43   200.35 
 64.00 3602  225.12   0.0000.00 

$ iostat -D -c2  
  sd0   sd1 
 KB  xfr time  KB  xfr time 
  19260  315 0.04 362   20 0.00 
 230590 3603 0.48   00 0.00 
$ ./obj/iostat -D -c2
   sd0sd1 
  KB  xfr time   KB  xfr time 
   19708  322 0.04  361   20 0.00 
  24 3750 0.5000 0.00

Feedback? OK?


Index: iostat.c
===
RCS file: /cvs/src/usr.sbin/iostat/iostat.c,v
retrieving revision 1.42
diff -u -p -r1.42 iostat.c
--- iostat.c14 Oct 2019 19:22:17 -  1.42
+++ iostat.c9 Aug 2020 22:04:21 -
@@ -224,15 +224,12 @@ header(void)
if (ISSET(todo, SHOW_STATS_1))
for (i = 0; i < dk_ndrive; i++)
if (cur.dk_select[i]) {
-   if (ISSET(todo, SHOW_TOTALS))
-   printf(" %18.18s ", cur.dk_name[i]);
-   else
-   printf(" %16.16s ", cur.dk_name[i]);
+   printf(" %18.18s ", cur.dk_name[i]);
}
if (ISSET(todo, SHOW_STATS_2))
for (i = 0; i < dk_ndrive; i++)
if (cur.dk_select[i])
-   printf(" %16.16s ", cur.dk_name[i]);
+   printf(" %17.17s ", cur.dk_name[i]);
 
if (ISSET(todo, SHOW_CPU))
printf("   cpu");
@@ -252,12 +249,12 @@ header(void)
if (ISSET(todo, SHOW_TOTALS))
printf("  KB/t   xfr MB ");
else
-   printf("  KB/t  t/s  MB/s ");
+   printf("  KB/t  t/sMB/s ");
}
if (ISSET(todo, SHOW_STATS_2))
for (i = 0; i < dk_ndrive; i++)
if (cur.dk_select[i])
-   printf(" KB  xfr time ");
+   printf("  KB  xfr time ");
 
if (ISSET(todo, SHOW_CPU))
printf(" us ni sy sp in id");
@@ -302,7 +299,7 @@ disk_stats(double etime)
if (ISSET(todo, SHOW_TOTALS))
printf(" %6.2f ", mbps / etime);
  

top: filter by routing table

2020-08-09 Thread Klemens Nanni
Sometimes I want to see processes outside the default routing table with
`-T -0', sometimes those in in a specific one with `-T 3' (for testing).

Since others have poked around with routing tables and/or domains as of
late, perhaps this deemed useful enough?

Semantically, filtering is identical to that of users: pick or hide one;
this makes the code pretty much copy/paste within top(1), manual wording
and command line flag is taken from pgrep(1).

pgrep is currently the only way to identify processes by routing table.
After netstat(1)'s relatively recent addition of `-R', filtering in top
makes for a quite handy set of tooling around rtable(4).

Feedback? OK?


Index: display.c
===
RCS file: /cvs/src/usr.bin/top/display.c,v
retrieving revision 1.63
diff -u -p -r1.63 display.c
--- display.c   26 Jul 2020 21:59:16 -  1.63
+++ display.c   9 Aug 2020 17:55:56 -
@@ -824,6 +824,8 @@ show_help(void)
"r count pid  - renice process `pid' to nice value `count'\n"
"S- toggle the display of system processes\n"
"s time   - change delay between displays to `time' seconds\n"
+   "T [-]rtable  - show processes associated with routing table 
`rtable'\n"
+   "   (T+ shows all, T -rtable hides rtable)\n"
"u [-]user- show processes for `user' (u+ shows all, u -user 
hides user)\n"
"\n");
 
Index: machine.c
===
RCS file: /cvs/src/usr.bin/top/machine.c,v
retrieving revision 1.107
diff -u -p -r1.107 machine.c
--- machine.c   6 Jul 2020 16:27:59 -   1.107
+++ machine.c   9 Aug 2020 18:06:36 -
@@ -414,7 +414,7 @@ get_process_info(struct system_info *si,
 int (*compare) (const void *, const void *))
 {
int show_idle, show_system, show_threads, show_uid, show_pid, show_cmd;
-   int hide_uid;
+   int show_rtable, hide_rtable, hide_uid;
int total_procs, active_procs;
struct kinfo_proc **prefp, *pp;
int what = KERN_PROC_ALL;
@@ -446,6 +446,8 @@ get_process_info(struct system_info *si,
show_uid = sel->uid != (uid_t)-1;
hide_uid = sel->huid != (uid_t)-1;
show_pid = sel->pid != (pid_t)-1;
+   show_rtable = sel->rtableid != -1;
+   hide_rtable = sel->hrtableid != -1;
show_cmd = sel->command != NULL;
 
/* count up process states and get pointers to interesting procs */
@@ -474,6 +476,8 @@ get_process_info(struct system_info *si,
(!hide_uid || pp->p_ruid != sel->huid) &&
(!show_uid || pp->p_ruid == sel->uid) &&
(!show_pid || pp->p_pid == sel->pid) &&
+   (!hide_rtable || pp->p_rtableid != sel->hrtableid) 
&&
+   (!show_rtable || pp->p_rtableid == sel->rtableid) &&
(!show_cmd || cmd_matches(pp, sel->command))) {
*prefp++ = pp;
active_procs++;
Index: machine.h
===
RCS file: /cvs/src/usr.bin/top/machine.h,v
retrieving revision 1.29
diff -u -p -r1.29 machine.h
--- machine.h   25 Jun 2020 20:38:41 -  1.29
+++ machine.h   9 Aug 2020 17:50:14 -
@@ -77,6 +77,8 @@ struct process_select {
uid_t   uid;/* only this uid (unless uid == -1) */
uid_t   huid;   /* hide this uid (unless huid == -1) */
pid_t   pid;/* only this pid (unless pid == -1) */
+   int rtableid;   /* only this rtable (unless rtableid == 
-1) */
+   int hrtableid;  /* hide this rtable (unless hrtableid 
== -1) */
char   *command;/* only this command (unless == NULL) */
 };
 
Index: top.1
===
RCS file: /cvs/src/usr.bin/top/top.1,v
retrieving revision 1.75
diff -u -p -r1.75 top.1
--- top.1   26 Jul 2020 21:59:16 -  1.75
+++ top.1   9 Aug 2020 18:16:12 -
@@ -38,6 +38,7 @@
 .Op Fl o Oo - Oc Ns Ar field
 .Op Fl p Ar pid
 .Op Fl s Ar time
+.Op Fl T Oo - Oc Ns Ar rtable
 .Op Fl U Oo - Oc Ns Ar user
 .Op Ar number
 .Ek
@@ -179,6 +180,14 @@ Set the delay between screen updates to
 seconds.
 The value may be fractional, to permit delays of less than 1 second.
 The default delay between updates is 5 seconds.
+.It Fl T Oo - Oc Ns Ar rtable
+Display only processes associated with the specified routing table
+.Ar rtable .
+.Sq T+
+shows processes associated with all routing tables.
+The
+.Sq -
+prefix hides proccesses associated with a single routing table.
 .It Fl U Oo - Oc Ns Ar user
 Show only those processes owned by username or UID
 .Ar user .
@@ -371,6 +380,14 @@ Toggle the display of system processes.
 Set the delay between screen updates to
 .Ar time
 

Re: pfsync: start without kernel lock

2020-08-09 Thread Klemens Nanni
On Sun, Aug 09, 2020 at 06:42:07PM +0300, Vitaliy Makkoveev wrote:
> Does `IFXF_MPSAFE' bit assume that pfsyncioctl() should not rely to
> kernel lock and pfsync(4) related data structures already have their own
> protection?
I say it does not.

There's PF_LOCK(), but it a) has to be enabled manually and b) is not
specific to pfsync(4) alone.

IFXF_MPSAFE is about the driver's start routing alone, it does not
concern the ioctl(2) path.

Does that answer your questions?



pfsync: start without kernel lock

2020-08-09 Thread Klemens Nanni
mvs's vnet(4) diff reminded me of pfsync(4).

This works on my my pair of amd64 firewalls.

Feedback? OK?


Index: if_pfsync.c
===
RCS file: /cvs/src/sys/net/if_pfsync.c,v
retrieving revision 1.275
diff -u -p -r1.275 if_pfsync.c
--- if_pfsync.c 29 Jul 2020 12:08:15 -  1.275
+++ if_pfsync.c 9 Aug 2020 00:52:41 -
@@ -253,7 +253,7 @@ voidpfsync_update_net_tdb(struct pfsync
 intpfsyncoutput(struct ifnet *, struct mbuf *, struct sockaddr *,
struct rtentry *);
 intpfsyncioctl(struct ifnet *, u_long, caddr_t);
-void   pfsyncstart(struct ifnet *);
+void   pfsyncstart(struct ifqueue *);
 void   pfsync_syncdev_state(void *);
 void   pfsync_ifdetach(void *);
 
@@ -339,12 +339,12 @@ pfsync_clone_create(struct if_clone *ifc
ifp->if_softc = sc;
ifp->if_ioctl = pfsyncioctl;
ifp->if_output = pfsyncoutput;
-   ifp->if_start = pfsyncstart;
+   ifp->if_qstart = pfsyncstart;
ifp->if_type = IFT_PFSYNC;
ifq_set_maxlen(>if_snd, IFQ_MAXLEN);
ifp->if_hdrlen = sizeof(struct pfsync_header);
ifp->if_mtu = ETHERMTU;
-   ifp->if_xflags = IFXF_CLONED;
+   ifp->if_xflags = IFXF_CLONED | IFXF_MPSAFE;
timeout_set_proc(>sc_tmo, pfsync_timeout, NULL);
timeout_set_proc(>sc_bulk_tmo, pfsync_bulk_update, NULL);
timeout_set_proc(>sc_bulkfail_tmo, pfsync_bulk_fail, NULL);
@@ -418,9 +418,9 @@ pfsync_clone_destroy(struct ifnet *ifp)
  * Start output on the pfsync interface.
  */
 void
-pfsyncstart(struct ifnet *ifp)
+pfsyncstart(struct ifqueue *ifq)
 {
-   ifq_purge(>if_snd);
+   ifq_purge(ifq);
 }
 
 void



Re: PATCH: better error return for exFAT filesystem

2020-08-09 Thread Klemens Nanni
On Sun, Aug 09, 2020 at 07:48:21PM +1000, Jonathan Gray wrote:
> Thinking about this some more the problem is really the choice of errno.
> It used to be EINVAL but was changed to EFTYPE in
> 
> 
> revision 1.7
> date: 1997/06/20 14:04:30;  author: kstailey;  state: Exp;  lines: +7 -7;
> Change errno cause by mounting invalid filesystems from EINVAL to EFTYPE.
> 
> 
> mount_msdos(8) knows about EINVAL and will print "not an MSDOS filesystem"
> 
> On reading mount(2) EOPNOTSUPP would be suitable for when the kernel
> did not have msdos support built in not when the blocks found are not
> msdos.
> 
> So either mount_msdos gains a EFTYPE case or we go back to EINVAL.
Reverting EFTYPE makes sense to me and having mount_msdos say
"not an MSDOS filesystem" on exFAT is an improvement.

OK kn



Re: vether(4): move `ifnet' out of KERNEL_LOCK()

2020-08-08 Thread Klemens Nanni
On Sun, Aug 09, 2020 at 03:16:50AM +0300, Vitaliy Makkoveev wrote:
> vether(4) is pretty dummy. Nothing denies it to be `IFXF_MPSAFE'.
OK kn



Re: brconfig: strto*l -> strtonum()

2020-08-07 Thread Klemens Nanni
On Wed, Jul 29, 2020 at 07:39:43PM +0200, Klemens Nanni wrote:
> 
> Poking and testing around in brconfig.c for tpmr(4) stuff, I noticed a
> lot of old code around strto*l(3).
> 
> Many pass unbounded `long' values into the `[u]int32_t' struct members
> without limiting them to at least the type size the value is stored in,
> some report wrong commands in error messages, e.g.
> 
>   # ifconfig switch1 portno vether1 9223372036854775808   # LONG_MAX + 1
>   ifconfig: invalid arg for portidx: 9223372036854775808
> 
> some pass values to the kernel that are above limits documented in
> ifconfig(8), e.g.
> 
>   #  ifconfig bridge0 maxage 50
>   ifconfig: bridge0: Invalid argument
> 
> and others overflow in-kernel due to that (causing persistent damage
> that causes deleting and adding bridge members to fix it).
> 
> I'm aware that moving to strtonum(3) drops the ability to pass numbers
> in bases others than ten, e.g. `ifconfig bridge0 ifcost vether0 0x3'
> mut now be `... 15', but all of the options really want a decimal number
> as far as I can judge after reading ifconfig(8) and testing them.
> 
> The only exception to this is switch(4)'s `datapath' which is printed in
> hexadecimal, so I left it untouched such that it can be copy-pasted and
> set as is.
> 
> Diff below applies converts all places to the very same strtonum() idiom
> with proper range checks based on documented or type limits.
> 
> I've tested each option manually: valid values are still valid and but
> more out of range values are catched now, many of them up front in
> ifconfig rather than the kerne's ioctl() path.
> 
> I've also checked most of the ioctl() paths to see what those really
> check for.  One or two diffs for the kernel should follow soon.
> 
> Feedback? OK?
Ping.

Alternatively, we can avoid duplicating the ioctl specific min/max
values in strtonum(3) calls, just use the struct member type's *_MAX
defines and rely on the kernel for appropiate boundary checks - this is
what the code does now.

Feedback? OK?


Index: brconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.28
diff -u -p -r1.28 brconfig.c
--- brconfig.c  5 Aug 2020 06:22:11 -   1.28
+++ brconfig.c  8 Aug 2020 03:04:30 -
@@ -419,18 +419,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, UINT32_MAX, );
+   if (errstr)
+   err(1, "timeout %s is: %s", arg, errstr);
 
strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
-   bp.ifbrp_ctime = newtime;
if (ioctl(sock, SIOCBRDGSTO, (caddr_t)) == -1)
err(1, "%s", ifname);
 }
@@ -439,17 +434,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)
+   errx(1, "maxage %s is: %s", arg, errstr);
 
strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
-   bp.ifbrp_maxage = v;
if (ioctl(sock, SIOCBRDGSMA, (caddr_t)) == -1)
err(1, "%s", ifname);
 }
@@ -458,17 +449,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)
+   errx(1, "spanpriority %s is: %s", arg, errstr);
 
strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
-   bp.ifbrp_prio = v;
if (ioctl(sock, SIOCBRDGSPRI, (caddr_t)) == -1)
err(1, "%s", ifname);
 }
@@ -479,7 +466,7 @@ bridge_protect(const char *ifsname, cons
struct ifbreq breq;
unsigned long v;
char *optlist, *str;
-   char *endptr;
+   const char *errstr;
 
  

Re: PATCH: better error return for exFAT filesystem

2020-08-07 Thread Klemens Nanni
On Fri, Aug 07, 2020 at 12:59:00PM -0700, jo...@armadilloaerospace.com wrote:
> Perform an explicit check for the unsupported exFAT MSDOS filesystem
> instead of letting it fail mysteriously when it gets cluster sizes
> of 0 from the normal fields.
> 
> This causes mount_msdos to report:
> mount_msdos: /dev/sd1i on /root/mnt: filesystem not supported by kernel
> 
> Instead of the more obscure:
> mount_msdos: /dev/sd1i on /root/mnt: Inapropriate file type or format
Much better.

I think this check for the complete string as per
https://github.com/MicrosoftDocs/win32/blob/docs/desktop-src/FileIO/exfat-specification.md#312-filesystemname-field

The FileSystemName field shall contain the name of the file system on 
the volume.

The valid value for this field is, in ASCII characters, "EXFAT ", which 
includes three trailing white spaces.


/* exFAT specification, 3.1.2 FileSystemName Field */
if (strcmp(bsp->bs33.bsOemName, "EXFAT   ") != 0) {
...
}

> Index: msdosfs_vfsops.c
> ===
> RCS file: /cvs/src/sys/msdosfs/msdosfs_vfsops.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 msdosfs_vfsops.c
> --- msdosfs_vfsops.c  24 Jan 2020 03:49:34 -  1.93
> +++ msdosfs_vfsops.c  7 Aug 2020 19:52:04 -
> @@ -298,6 +298,12 @@ msdosfs_mountfs(struct vnode *devvp, str
>   b50 = (struct byte_bpb50 *)bsp->bs50.bsBPB;
>   b710 = (struct byte_bpb710 *)bsp->bs710.bsBPB;
>  
> + /* No support for exFAT filesystems */
> + if (!strncmp("EXFAT", bsp->bs33.bsOemName, 5)) {
> + error = EOPNOTSUPP;
> + goto error_exit;
> + }
> +
>   pmp = malloc(sizeof *pmp, M_MSDOSFSMNT, M_WAITOK | M_ZERO);
>   pmp->pm_mountp = mp;
>  
> 
> 



Re: PATCH: iostat spacing

2020-08-07 Thread Klemens Nanni
On Fri, Aug 07, 2020 at 12:04:59PM -0700, jo...@armadilloaerospace.com wrote:
> IO rates above 100 MB/s are common with SSD; this patch expands the
> column so it stays neatly printed.
This is OK with me as it fixes the default view, but I think other views
need fixing as well, e.g.

$ iostat -I
ttysd0 sd1
cpu
   tin tout  KB/t   xfr MB   KB/t   xfr MB  us ni sy sp in 
id
178524 22139320 26.99 10698431 281987.61  11.78 7582117 87218.82   7  0 
 3  1  0 88

I'm testing with `dd if=/dev/rsd0c of=/dev/null bs=1m' on a X230 with
sd0 at scsibus1 targ 0 lun 0:  naa.5002538d410a7bce

> An argument can be made for expanding it one more for fast M.2 drives.
I don't have that fast disks at hand, but expanding it by another column
makes sense to me and could further simplify some of the code.



Re: tpmr.4, ifconfig.8: document tpmr ioctls and synopsis

2020-08-05 Thread Klemens Nanni
On Wed, Aug 05, 2020 at 03:24:57PM +0100, Jason McIntyre wrote:
> this is in line with all our other pages, so ok. while you're poking
> around in there, the first example in tpmr.4 EXAMPLES would be a whole
> lot nicer with -indent on the display. care to fix that too?
Sure, I'll commit with `.Bd -literal -offset indent'.



tpmr.4, ifconfig.8: document tpmr ioctls and synopsis

2020-08-05 Thread Klemens Nanni
Add missing TPMR section to ifconfig(8) by moving the commands from
the driver's manual to it (copy/paste) and document the ioctl(2)
interface in tpmr(4).

Feedback? OK?


Index: sbin/ifconfig/ifconfig.8
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
retrieving revision 1.353
diff -u -p -r1.353 ifconfig.8
--- sbin/ifconfig/ifconfig.826 Jul 2020 18:34:10 -  1.353
+++ sbin/ifconfig/ifconfig.85 Aug 2020 06:48:38 -
@@ -204,6 +204,7 @@ At least the following devices can be cr
 .Xr svlan 4 ,
 .Xr switch 4 ,
 .Xr tap 4 ,
+.Xr tpmr 4 ,
 .Xr trunk 4 ,
 .Xr tun 4 ,
 .Xr vether 4 ,
@@ -830,6 +831,51 @@ If
 is set to zero, then entries will not be expired.
 .It Cm up
 Start the bridge forwarding packets.
+.El
+.Sh TPMR
+.nr nS 1
+.Bk -words
+.Nm ifconfig
+.Ar tpmr-interface
+.Op Cm add Ar child-iface
+.Op Cm del Ar child-iface
+.Op Oo Fl Oc Ns Cm link0
+.Op Oo Fl Oc Ns Cm link1
+.Op Oo Fl Oc Ns Cm link2
+.Ek
+The following options are available for a
+.Xr tpmr 4
+interface:
+.Bl -tag -width Ds
+.It Cm add Ar child-iface
+Add
+.Ar child-iface
+as a member.
+.It Cm del Ar child-iface
+Remove the member
+.Ar child-iface .
+.It Cm link0
+Disable the filtering of Ethernet frames destined for the TPMR
+component reserved addresses, as specified by IEEE 802.1Q.
+.It Cm -link0
+Enable the filtering of Ethernet frames destined for the TPMR
+component reserved addresses, as specified by IEEE 802.1Q.
+This is the default.
+.It Cm link1
+Disable the filtering of IPv4 and IPv6 packets with
+.Xr pf 4 .
+.It Cm -link1
+Enable the filtering of IPv4 and IPv6 packets with
+.Xr pf 4 .
+Packets will appear to enter or leave the member port interfaces.
+This is the default.
+.It Cm link2
+Disable the filtering of 802.1Q VLAN and QinQ SVLAN packets.
+.It Cm -link2
+Enable the filtering of 802.1Q VLAN and QinQ SVLAN packets.
+.Xr pf 4 .
+Packets will appear to enter or leave the member port interfaces.
+This is the default.
 .El
 .Sh CARP
 .nr nS 1
Index: share/man/man4/tpmr.4
===
RCS file: /cvs/src/share/man/man4/tpmr.4,v
retrieving revision 1.6
diff -u -p -r1.6 tpmr.4
--- share/man/man4/tpmr.4   22 Jul 2020 04:08:46 -  1.6
+++ share/man/man4/tpmr.4   5 Aug 2020 06:58:58 -
@@ -40,48 +40,6 @@ command or by setting up a
 .Xr hostname.if 5
 configuration file for
 .Xr netstart 8 .
-The interface itself can be configured with
-.Xr ifconfig 8 ;
-see its manual page for more information.
-.Pp
-.Nm
-interfaces may be configured with
-.Xr ifconfig 8
-and
-.Xr netstart 8
-using the following options:
-.Bl -tag -width Ds
-.It Cm add Ar child-iface
-Add
-.Ar child-iface
-as a member.
-.It Cm del Ar child-iface
-Remove the member
-.Ar child-iface .
-.It Cm link0
-Disable the filtering of Ethernet frames destined for the TPMR
-component reserved addresses, as specified by IEEE 802.1Q.
-.It Cm -link0
-Enable the filtering of Ethernet frames destined for the TPMR
-component reserved addresses, as specified by IEEE 802.1Q.
-This is the default.
-.It Cm link1
-Disable the filtering of IPv4 and IPv6 packets with
-.Xr pf 4 .
-.It Cm -link1
-Enable the filtering of IPv4 and IPv6 packets with
-.Xr pf 4 .
-Packets will appear to enter or leave the member port interfaces.
-This is the default.
-.It Cm link2
-Disable the filtering of 802.1Q VLAN and QinQ SVLAN packets.
-.It Cm -link2
-Enable the filtering of 802.1Q VLAN and QinQ SVLAN packets.
-.Xr pf 4 .
-Packets will appear to enter or leave the member port interfaces.
-This is the default.
-.El
-.\" document the ioctls?
 .Pp
 Other forms of Ethernet bridging are available using the
 .Xr bridge 4
@@ -92,6 +50,20 @@ using the
 and
 .Xr trunk 4
 drivers.
+.Sh IOCTLS
+The following
+.Xr ioctl 2
+calls and their structures are commonly use by
+.Nm
+and
+.Xr bridge 4 :
+.Pp
+.Bl -bullet -offset indent -compact
+.It
+.Dv SIOCBRDGADD
+.It
+.Dv SIOCBRDGDEL
+.El
 .Sh EXAMPLES
 .Nm
 can be used to cross-connect Ethernet devices that support different



Re: switch: allow datapath_id and maxflow ioctls for non-root

2020-08-05 Thread Klemens Nanni
On Wed, Aug 05, 2020 at 11:00:00AM +1000, David Gwynne wrote:
> can't they be caught by the default case now?
Obviously...


Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.617
diff -u -p -r1.617 if.c
--- net/if.c4 Aug 2020 09:32:05 -   1.617
+++ net/if.c5 Aug 2020 06:24:33 -
@@ -2163,9 +2163,7 @@ ifioctl(struct socket *so, u_long cmd, c
case SIOCBRDGSIFCOST:
case SIOCBRDGSTXHC:
case SIOCBRDGSPROTO:
-   case SIOCSWGDPID:
case SIOCSWSPORTNO:
-   case SIOCSWGMAXFLOW:
 #endif
if ((error = suser(p)) != 0)
break;



Re: ksh.1: Mention Co-processes in $!

2020-08-01 Thread Klemens Nanni
On Sat, Aug 01, 2020 at 06:06:32PM +0100, Jason McIntyre wrote:
> hmm. so then the current text ("the last background process") already
> covers all these cases. why single out co-processes?
Yes, "background process" technically covers co-processes, but at least
for me "background processes" aka. jobs refer to those spawned with `&'
where co-processes are the definitive term for those spawned with `|&',
hence my brain wants to grep for "co-proc" and see `$!' mentioning it
to confirm that `$!' is indeed not just about jobs.

> for the reader, i think the idea of background is easier to understand
> as an umberella term, rather than asynchronous, even if it's maybe not
> so correct.
Fair enough;  I think the last diff is a slight improvement due to the
above mentioned, but I also don't have a strong opinion about it.

If the impression is that I'm complicating stuff then I'll gladly drop
the diff.



Re: ksh.1: Mention Co-processes in $!

2020-08-01 Thread Klemens Nanni
On Sat, Aug 01, 2020 at 05:40:07PM +0100, Jason McIntyre wrote:
> i'm worried that you're blurring the distinction between asynchronous
> and co-process for the reader. i think that's relevant because, as you
> say, a page like sh(1) does not document co-processes, whereas ksh(1)
> does.
You raise a valid point.

> as i understand it, ksh explains co-processes as a type of asynchronous
> process. and the setting of "!" is applicable to both. is that right?
Correct.  Both background jobs (`&') and co-processes (`|&') are
asynchronous and co-processes are background jobs by definition.

What makes co-processes different is a two-way pipe between them and
the shell, i.e. you can read and write in both directions.

> so could your text be better written as:
> 
>   Process ID of the last background or asynchronous process started.
>   If no processes have been started, the parameter is not set.
> 
> this would catch the setting of "!" for both async and co-process,
> without making the text read like the two are identical.
> 
> if your concern is that you need to be able to /co-process, then i'd
> rewrite the text to not blur the two:
> 
>   Process ID of the last background, co-process, or asynchronous
>   process started.
> 
> but i dislike that, because it means the text suffers because of an
> arbitrary request to make a specific search work.
Indeed, I'd like to have `/co-process' show it.

> i guess we could add a note to the "Some notes concerning co-processes"
> section if it needs more clarity.
I don't think this is needed.

Background jobs as well as co-processes are async and background jobs or
rather `&' is documented as "asynchronous command" in other places of
the manual, so how about this?


Index: ksh.1
===
RCS file: /cvs/src/bin/ksh/ksh.1,v
retrieving revision 1.209
diff -u -p -r1.209 ksh.1
--- ksh.1   7 Jul 2020 10:33:58 -   1.209
+++ ksh.1   1 Aug 2020 16:59:10 -
@@ -1247,8 +1247,8 @@ The following special parameters are imp
 set directly using assignments:
 .Bl -tag -width "1 ... 9"
 .It Ev \&!
-Process ID of the last background process started.
-If no background processes have been started, the parameter is not set.
+Process ID of the last background process or co-process started.
+If no asynchronous processes have been started, the parameter is not set.
 .It Ev 

ksh.1: Mention Co-processes in $!

2020-08-01 Thread Klemens Nanni
Otherwise it is not clear whether $! will be set or not.  This way,
`/Co-proc' brings me to *all* relevant spots in the manual.

Snippet to demonstrate how $! is set for an asynchronous process:

$ ksh -c ': |& echo $!' 
67163

FWIW, sh(1) doesn't document Co-processes (whis is fine/correct) and
bash(1) says this about $!:

!  Expands to the process ID of the job most recently placed into
   the background, whether executed as an asynchronous command or
   using the bg builtin (see JOB CONTROL below).

Feedback? OK?


Index: ksh.1
===
RCS file: /cvs/src/bin/ksh/ksh.1,v
retrieving revision 1.209
diff -u -p -r1.209 ksh.1
--- ksh.1   7 Jul 2020 10:33:58 -   1.209
+++ ksh.1   1 Aug 2020 15:50:04 -
@@ -1247,8 +1247,9 @@ The following special parameters are imp
 set directly using assignments:
 .Bl -tag -width "1 ... 9"
 .It Ev \&!
-Process ID of the last background process started.
-If no background processes have been started, the parameter is not set.
+Process ID of the last background process or asynchronous process (Co-process)
+started.
+If no processes have been started, the parameter is not set.
 .It Ev 

ifconfig: print tpmr(4) members

2020-07-31 Thread Klemens Nanni
This diff is to be applied on top of my other diff on tech@ with subject
"ifconfig: merge switch_status() into bridge_status()".

It hooks completes the output of tpmr intefaces in what I think is the
simplest and least intrusive way.

tpmr is a trivial bridge and has no specific ioctls, so to distinguish
it from the rest we must rely on the interface name;  assuming that it
is tpmr because neither is_bridge() nor is_switch() return success is
not possible due to the way ifconfig is designed: it runs all *_status()
commands for all interface types.

An alternative approach would be to make ifconfig try all the various
bridge related ioctls on all bridge-like interfaces and quiet down all
failures such output stays clean, but I dislike this shotgun approach
and prefer testing for different drivers where possible.

With this last piece in, I could finally document tpmr under ifconfig(8)
(and move on the next drivers in need of love).

Feedback? OK?


--- brconfig.c.orig Fri Jul 31 08:58:03 2020
+++ brconfig.c  Fri Jul 31 09:16:59 2020
@@ -775,15 +775,28 @@
return (1);
 }
 
+/* no tpmr(4) specific ioctls, name is enough if ifconfig.c:printif() passed */
+int
+is_tpmr(void)
+{
+   return (strncmp(ifname, "tpmr", sizeof("tpmr") - 1) == 0);
+}
+
 void
 bridge_status(void)
 {
struct ifbrparam bp1, bp2;
-   int isswitch = is_switch();
+   int isswitch;
 
+   if (is_tpmr()) {
+   bridge_list("\t");
+   return;
+   }
+
if (!is_bridge())
return;
 
+   isswitch = is_switch();
if (isswitch)
switch_cfg("\t");
else



Re: ifconfig: merge switch_status() into bridge_status()

2020-07-31 Thread Klemens Nanni
On Wed, Jul 29, 2020 at 02:21:42PM +0200, Klemens Nanni wrote:
> This is to reduce duplicate code and pave the way for a single
> bridge_status() that covers all bridge like interfaces: bridge(4),
> switch(4) and tpmr(4).
A duplicate bridge_cfg() call snuck in, fixed diff below.

Feedback? OK?

> + if (isswitch)
> + switch_cfg("\t");
> + else
> + bridge_cfg("\t");
> +
>   bridge_cfg("\t");
>  
>   bridge_list("\t");


Index: brconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.26
diff -u -p -r1.26 brconfig.c
--- brconfig.c  29 Jul 2020 12:13:28 -  1.26
+++ brconfig.c  31 Jul 2020 06:32:54 -
@@ -54,6 +54,7 @@ void bridge_ifclrflag(const char *, u_in
 
 void bridge_list(char *);
 void bridge_cfg(const char *);
+void switch_cfg(const char *);
 void bridge_badrule(int, char **, int);
 void bridge_showrule(struct ifbrlreq *);
 int is_switch(void);
@@ -778,17 +779,24 @@ void
 bridge_status(void)
 {
struct ifbrparam bp1, bp2;
+   int isswitch = is_switch();
 
-   if (!is_bridge() || is_switch())
+   if (!is_bridge())
return;
 
-   bridge_cfg("\t");
+   if (isswitch)
+   switch_cfg("\t");
+   else
+   bridge_cfg("\t");
 
bridge_list("\t");
 
if (aflag && !ifaliases)
return;
 
+   if (isswitch)
+   return;
+
strlcpy(bp1.ifbrp_name, ifname, sizeof(bp1.ifbrp_name));
if (ioctl(sock, SIOCBRDGGCACHE, (caddr_t)) == -1)
return;
@@ -1146,8 +1154,8 @@ is_switch()
return (1);
 }
 
-static void
-switch_cfg(char *delim)
+void
+switch_cfg(const char *delim)
 {
struct ifbrparam bp;
 
@@ -1168,20 +1176,6 @@ switch_cfg(char *delim)
err(1, "%s", ifname);
 
printf(" maxgroup %d\n", bp.ifbrp_maxgroup);
-}
-
-void
-switch_status(void)
-{
-   if (!is_switch())
-   return;
-
-   switch_cfg("\t");
-
-   bridge_list("\t");
-
-   if (aflag && !ifaliases)
-   return;
 }
 
 void
Index: ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.424
diff -u -p -r1.424 ifconfig.c
--- ifconfig.c  3 Jul 2020 17:42:50 -   1.424
+++ ifconfig.c  31 Jul 2020 06:24:17 -
@@ -3507,7 +3507,6 @@ status(int link, struct sockaddr_dl *sdl
phys_status(0);
 #ifndef SMALL
bridge_status();
-   switch_status();
 #endif
 }
 
Index: ifconfig.h
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.h,v
retrieving revision 1.2
diff -u -p -r1.2 ifconfig.h
--- ifconfig.h  24 Oct 2019 18:54:10 -  1.2
+++ ifconfig.h  31 Jul 2020 06:24:18 -
@@ -69,7 +69,6 @@ void bridge_flushrule(const char *, int)
 int is_bridge(void);
 void bridge_status(void);
 int bridge_rule(int, char **, int);
-void switch_status(void);
 void switch_datapathid(const char *, int);
 void switch_portno(const char *, const char *);
 



Re: switch: allow datapath_id and maxflow ioctls for non-root

2020-07-30 Thread Klemens Nanni
On Fri, Jul 31, 2020 at 06:28:32AM +0200, Klemens Nanni wrote:
> ifconfig(8) detects switch(4) through its unique SIOCSWSDPID ioctl and
> further does another switch specific ioctl for the default output
> regardless of configuration and/or members:
> 
>   SIOCSWSDPID struct ifbrparam
>   Set the datapath_id in the OpenFlow protocol of the switch named
>   in ifbrp_name to the value in the ifbrpu_datapath field.
Sorry, stupid copy/pasta mistake;  ifconfig uses the read-only "get"
ioctl of course for regular output which my diff addresses:

SIOCSWGDPID
Retrieve the datapath_id in the OpenFlow protocol of the switch
named in ifbrp_name into the ifbrpu_datapath field.



switch: allow datapath_id and maxflow ioctls for non-root

2020-07-30 Thread Klemens Nanni
ifconfig(8) detects switch(4) through its unique SIOCSWSDPID ioctl and
further does another switch specific ioctl for the default output
regardless of configuration and/or members:

SIOCSWSDPID struct ifbrparam
Set the datapath_id in the OpenFlow protocol of the switch named
in ifbrp_name to the value in the ifbrpu_datapath field.

SIOCSWGMAXFLOW struct ifbrparam
Retrieve the maximum number of flows in the OpenFlow protocol of
the switch named in ifbrp_name into the ifbrp_maxflow field.

This is how it should look like:

# ifconfig switch0 create
# ifconfig switch0
switch0: flags=0<>
index 29 llprio 3
groups: switch
datapath 0x5bea2b5b8e2456cf maxflow 1 maxgroup 1000

But using ifconfig as unprivileged user makes it fail switch(4)
interfaces as such and thus interprets them as bridge(4) instead:

$ ifconfig switch0
switch0: flags=0<>
index 29 llprio 3
groups: switch
priority 32768 hellotime 2 fwddelay 15 maxage 20 holdcnt 6 
proto rstp
designated: id 00:00:00:00:00:00 priority 0

This is because the above mentioned ioctls are listed together with all
other bridge and switch related ioctls that set or write things.
Getting datapath_id and maxflow values however is read-only and crucial
for ifconfig as demonstrated above, so I'd like to move them out of the
root check to fix ifconfig.

Feedback? OK?


Index: sys/net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.616
diff -u -p -r1.616 if.c
--- sys/net/if.c24 Jul 2020 18:17:14 -  1.616
+++ sys/net/if.c31 Jul 2020 04:13:40 -
@@ -2170,13 +2170,15 @@ ifioctl(struct socket *so, u_long cmd, c
case SIOCBRDGSIFCOST:
case SIOCBRDGSTXHC:
case SIOCBRDGSPROTO:
-   case SIOCSWGDPID:
case SIOCSWSPORTNO:
-   case SIOCSWGMAXFLOW:
 #endif
if ((error = suser(p)) != 0)
break;
/* FALLTHROUGH */
+#if NBRIDGE > 0
+   case SIOCSWGDPID:
+   case SIOCSWGMAXFLOW:
+#endif
default:
error = ((*so->so_proto->pr_usrreq)(so, PRU_CONTROL,
(struct mbuf *) cmd, (struct mbuf *) data,



rdomain.4: route -T takes an rtable, not rdomain

2020-07-29 Thread Klemens Nanni
Multiple rtables may exist in the default rdomain (0), that is their
corresponding rdomains/lo(4) interfaces do not have to exist.

This demonstrates it;  first, nothing but default, so route(8) fails:

# netstat -R
Rdomain 0
  Interfaces: lo0 vio0 enc0
  Routing table: 0

# route -T1 exec id -R
route: routing table 1: No such file or directory

Then create an rdomain and with it an rtable:

# ifconfig lo1 rdomain 1
# netstat -R
Rdomain 0
  Interfaces: lo0 vio0 enc0
  Routing table: 0

Rdomain 1
  Interface: lo1
  Routing table: 1

This makes route(8) work, but it keeps working when we remove the
rdomain again since the rtable persits:

# route -T1 exec id -R
1
# ifconfig lo1 destroy
# netstat -R
Rdomain 0
  Interfaces: lo0 vio0 enc0
  Routing tables: 0 1

# route -T1 exec id -R
1


I'm not sure yet, whether this is intentional or in fact a bug.
Either ways, the manual should be fixed - route(8)'s synopsis says the
same, just like ping(8)'s `-V rtable':

$ man -hs8 route
route [-dnqtv] [-T rtable] command [[modifiers] args]

Feedback? Objections? OK?


Index: share/man/man4/rdomain.4
===
RCS file: /cvs/src/share/man/man4/rdomain.4,v
retrieving revision 1.13
diff -u -p -r1.13 rdomain.4
--- share/man/man4/rdomain.41 Feb 2020 15:00:20 -   1.13
+++ share/man/man4/rdomain.430 Jul 2020 01:56:39 -
@@ -98,7 +98,7 @@ Put em0 and lo4 in rdomain 4:
 # ifconfig em0 192.0.2.100/24
 .Ed
 .Pp
-Set a default route and localhost reject route within rdomain 4:
+Set a default route and localhost reject route within rtable 4:
 .Bd -literal -offset indent
 # route -T4 -qn add -net 127 127.0.0.1 -reject
 # route -T4 -n add default 192.0.2.1
@@ -106,7 +106,7 @@ Set a default route and localhost reject
 .Pp
 Start
 .Xr sshd 8
-in rdomain 4:
+in rtable 4:
 .Pp
 .Dl # route -T4 exec /usr/sbin/sshd
 .Pp



brconfig: strto*l -> strtonum()

2020-07-29 Thread Klemens Nanni


Poking and testing around in brconfig.c for tpmr(4) stuff, I noticed a
lot of old code around strto*l(3).

Many pass unbounded `long' values into the `[u]int32_t' struct members
without limiting them to at least the type size the value is stored in,
some report wrong commands in error messages, e.g.

# ifconfig switch1 portno vether1 9223372036854775808   # LONG_MAX + 1
ifconfig: invalid arg for portidx: 9223372036854775808

some pass values to the kernel that are above limits documented in
ifconfig(8), e.g.

#  ifconfig bridge0 maxage 50
ifconfig: bridge0: Invalid argument

and others overflow in-kernel due to that (causing persistent damage
that causes deleting and adding bridge members to fix it).

I'm aware that moving to strtonum(3) drops the ability to pass numbers
in bases others than ten, e.g. `ifconfig bridge0 ifcost vether0 0x3'
mut now be `... 15', but all of the options really want a decimal number
as far as I can judge after reading ifconfig(8) and testing them.

The only exception to this is switch(4)'s `datapath' which is printed in
hexadecimal, so I left it untouched such that it can be copy-pasted and
set as is.

Diff below applies converts all places to the very same strtonum() idiom
with proper range checks based on documented or type limits.

I've tested each option manually: valid values are still valid and but
more out of range values are catched now, many of them up front in
ifconfig rather than the kerne's ioctl() path.

I've also checked most of the ioctl() paths to see what those really
check for.  One or two diffs for the kernel should follow soon.

Feedback? OK?


Index: brconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.26
diff -u -p -r1.26 brconfig.c
--- brconfig.c  29 Jul 2020 12:13:28 -  1.26
+++ brconfig.c  29 Jul 2020 17:06:09 -
@@ -418,18 +418,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, UINT32_MAX, );
+   if (errstr)
+   err(1, "timeout %s is: %s", arg, errstr);
 
strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
-   bp.ifbrp_ctime = newtime;
if (ioctl(sock, SIOCBRDGSTO, (caddr_t)) == -1)
err(1, "%s", ifname);
 }
@@ -438,17 +433,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, 6, 40, );
+   if (errstr)
+   errx(1, "maxage %s is: %s", arg, errstr);
 
strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
-   bp.ifbrp_maxage = v;
if (ioctl(sock, SIOCBRDGSMA, (caddr_t)) == -1)
err(1, "%s", ifname);
 }
@@ -457,17 +448,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, 61440, );
+   if (errstr)
+   errx(1, "spanpriority %s is: %s", arg, errstr);
 
strlcpy(bp.ifbrp_name, ifname, sizeof(bp.ifbrp_name));
-   bp.ifbrp_prio = v;
if (ioctl(sock, SIOCBRDGSPRI, (caddr_t)) == -1)
err(1, "%s", ifname);
 }
@@ -478,7 +465,7 @@ bridge_protect(const char *ifsname, cons
struct ifbreq breq;
unsigned long v;
char *optlist, *str;
-   char *endptr;
+   const char *errstr;
 
strlcpy(breq.ifbr_name, ifname, sizeof(breq.ifbr_name));
strlcpy(breq.ifbr_ifsname, ifsname, sizeof(breq.ifbr_ifsname));
@@ -491,11 +478,9 @@ bridge_protect(const char *ifsname, cons
 
str = strtok(optlist, ",");
while (str != NULL) {
-   errno = 0;
-   v = strtoul(str, , 0);
-   if (str[0] == '\0' || endptr[0] != '\0' || v == 0 || v > 31 ||
-   (errno == ERANGE && v == ULONG_MAX))
-   err(1, "invalid value for protected domain: %s", str);
+   v = strtonum(str, 1, 31, );
+   if (errstr)
+   err(1, 

Re: hostname.if '!' commands and rdomains

2020-07-29 Thread Klemens Nanni
On Wed, Jul 29, 2020 at 09:05:14AM -0600, Theo de Raadt wrote:
> Claudio Jeker  wrote:
> > But:
> > $ route -T2 exec id -R
> > 2
> > $ route -T2 exec route -T0 exec id -R
> > route: setrtable: Operation not permitted
> > 
> > Only root can change the rdomain if it is currently != 0.
> 
> That worry was stated in my email, but not so accurately, thank you.
> So now you can't make a rdomain-0 !command in the global scope.
Indeed, my example was incomplete, but as netstart(8) runs as root this
is not a problem - unless of course `!' commands do stuff as
unprivileged users in foreign routing domains.

With that in mind, I'm getting more convinced that forcing the routing
domain in hostname.if(5) is not feasible.



Re: hostname.if '!' commands and rdomains

2020-07-29 Thread Klemens Nanni
On Wed, Jul 29, 2020 at 05:33:14PM +0300, Kapetanakis Giannis wrote:
> Wouldn't this break those who already have
> !route -T2 
> 
> in their hostname.if files?
No,

$ route -T1 exec id -R
1
$ route -T0 exec route -T1 exec id -R
1



Re: hostname.if '!' commands and rdomains

2020-07-29 Thread Klemens Nanni
On Wed, Jul 29, 2020 at 11:54:17AM +0200, Matthieu Herrb wrote:
> When I'm configuring an interface with a spécific rdomain, I'd assume
> that '!' commands (especially /sbin/route commands) are executed in
> the rdomain for this interface.
I see where you're coming from, but the diff seems flawed.

I don't have a better approach for this at hand, but as it stands, I'm
happy with having to do `route -T1 exec' manually if need be.

> I know that parsing this file is complex and somehow fragile but still
> I tried to write a patch.
In any way, /usr/src/distrib/miniroot/install.sub:parse_hn_line() will
require the same treatment to keep netstart(8) and installer in sync.

Also, if something like this went in, I'd like to see hostname.if(5)
explicitly document `!' behaviour wrt. to the routing domain.

> --- netstart.orig Wed Jul 29 11:19:53 2020
> +++ netstart  Wed Jul 29 11:52:39 2020
> @@ -67,8 +67,16 @@
>   _cmds[${#_cmds[*]}]="ifconfig $_if ${_c[@]} up;dhclient $_if"
>   V4_DHCPCONF=true
>   ;;
> + rdomain) ((${#_c[*]} == 2)) || return
> + _cmds[${#_cmds[*]}]="ifconfig $_if rdomain ${_c[_name]}"
> + _rdomain=${_c[_name]}
> + ;;
This assumes `rdomain' is set on its own hostname.if(5) line, so config
like "up rdomain 1" won't set `_rdomain' here...

>   '!'*)   _cmd=$(print -- "${_c[@]}" | sed 's/\$if/'$_if'/g')
> - _cmds[${#_cmds[*]}]="${_cmd#!}"
> + if [[ $_rdomain -ne 0 ]]; then
> +_cmds[${#_cmds[*]}]="/sbin/route -T$_rdomain exec 
> ${_cmd#!}"
> + else
> +_cmds[${#_cmds[*]}]="${_cmd#!}"
> + fi
>   ;;
and that causes the interface to be in rdomain 1 while executing
in rdomain 0.



ifconfig: merge switch_status() into bridge_status()

2020-07-29 Thread Klemens Nanni
This is to reduce duplicate code and pave the way for a single
bridge_status() that covers all bridge like interfaces: bridge(4),
switch(4) and tpmr(4).

Feedback? OK?

Index: brconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.26
diff -u -p -r1.26 brconfig.c
--- brconfig.c  29 Jul 2020 12:13:28 -  1.26
+++ brconfig.c  29 Jul 2020 12:17:04 -
@@ -54,6 +54,7 @@ void bridge_ifclrflag(const char *, u_in
 
 void bridge_list(char *);
 void bridge_cfg(const char *);
+void switch_cfg(const char *);
 void bridge_badrule(int, char **, int);
 void bridge_showrule(struct ifbrlreq *);
 int is_switch(void);
@@ -778,10 +779,16 @@ void
 bridge_status(void)
 {
struct ifbrparam bp1, bp2;
+   int isswitch = is_switch();
 
-   if (!is_bridge() || is_switch())
+   if (!is_bridge())
return;
 
+   if (isswitch)
+   switch_cfg("\t");
+   else
+   bridge_cfg("\t");
+
bridge_cfg("\t");
 
bridge_list("\t");
@@ -789,6 +796,9 @@ bridge_status(void)
if (aflag && !ifaliases)
return;
 
+   if (isswitch)
+   return;
+
strlcpy(bp1.ifbrp_name, ifname, sizeof(bp1.ifbrp_name));
if (ioctl(sock, SIOCBRDGGCACHE, (caddr_t)) == -1)
return;
@@ -1146,8 +1156,8 @@ is_switch()
return (1);
 }
 
-static void
-switch_cfg(char *delim)
+void
+switch_cfg(const char *delim)
 {
struct ifbrparam bp;
 
@@ -1168,20 +1178,6 @@ switch_cfg(char *delim)
err(1, "%s", ifname);
 
printf(" maxgroup %d\n", bp.ifbrp_maxgroup);
-}
-
-void
-switch_status(void)
-{
-   if (!is_switch())
-   return;
-
-   switch_cfg("\t");
-
-   bridge_list("\t");
-
-   if (aflag && !ifaliases)
-   return;
 }
 
 void
Index: ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.424
diff -u -p -r1.424 ifconfig.c
--- ifconfig.c  3 Jul 2020 17:42:50 -   1.424
+++ ifconfig.c  29 Jul 2020 12:17:06 -
@@ -3507,7 +3507,6 @@ status(int link, struct sockaddr_dl *sdl
phys_status(0);
 #ifndef SMALL
bridge_status();
-   switch_status();
 #endif
 }
 
Index: ifconfig.h
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.h,v
retrieving revision 1.2
diff -u -p -r1.2 ifconfig.h
--- ifconfig.h  24 Oct 2019 18:54:10 -  1.2
+++ ifconfig.h  29 Jul 2020 12:17:06 -
@@ -69,7 +69,6 @@ void bridge_flushrule(const char *, int)
 int is_bridge(void);
 void bridge_status(void);
 int bridge_rule(int, char **, int);
-void switch_status(void);
 void switch_datapathid(const char *, int);
 void switch_portno(const char *, const char *);
 



Re: ifconfig: remove redundant bridge checks

2020-07-28 Thread Klemens Nanni
On Tue, Jul 28, 2020 at 07:09:17PM +0200, Klemens Nanni wrote:
> bridge_status() and switch_status() do the regular sanity check with
> SIOCGIFFLAGS, but both functions also call is_switch(), bridge_status()
> also calls is_bridge().
> 
> Those is_*() helpers do the same SIOCGIFFLAGS sanity check, making those
> in *_status() entirely redundant, so I'd like to remove them.
Small correction: is_bridge() does SIOCGIFFLAGS, is_switch() does not.

Below is a new diff that removes the SIOCGIFFLAGS check form is_bridge()
as well, leaving the two is_*() helpers to their driver specific ioctls
alone.

> I'm here since the tpmr(4) ioctl interface transition from trunk to
> bridge semantics is now complete, so ifconfig(8) now requires tpmr bits
> to show its members in bridge fashion.
> 
> One way would be duplicate code into is_tpmr() and tpmr_status() which
> I've already done, but another approach is to unify all bridge like
> interfaces under bridge_status().
With this in, merging switch_status() into bridge_status() is a trivial
diff, adding tpmr awareness to the mix would then be another diff after
that.

> Either ways, diff below cleans up and makes for simpler code.
So this effectively just removes SIOCGIFFLAGS in brconfig.c which are of
no use, imho.  ifconfig.c:getinfo() already checks interfaces flags,
even more than once, for all interfaces.

> Feedback? OK?


Index: brconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.25
diff -u -p -r1.25 brconfig.c
--- brconfig.c  22 Jan 2020 06:24:07 -  1.25
+++ brconfig.c  29 Jul 2020 00:58:40 -
@@ -762,14 +762,8 @@ bridge_holdcnt(const char *value, int d)
 int
 is_bridge()
 {
-   struct ifreq ifr;
struct ifbaconf ifbac;
 
-   strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));
-
-   if (ioctl(sock, SIOCGIFFLAGS, (caddr_t)) == -1)
-   return (0);
-
ifbac.ifbac_len = 0;
strlcpy(ifbac.ifbac_name, ifname, sizeof(ifbac.ifbac_name));
if (ioctl(sock, SIOCBRDGRTS, (caddr_t)) == -1) {
@@ -783,16 +777,11 @@ is_bridge()
 void
 bridge_status(void)
 {
-   struct ifreq ifr;
struct ifbrparam bp1, bp2;
 
if (!is_bridge() || is_switch())
return;
 
-   strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));
-   if (ioctl(sock, SIOCGIFFLAGS, (caddr_t)) == -1)
-   return;
-
bridge_cfg("\t");
 
bridge_list("\t");
@@ -1184,13 +1173,7 @@ switch_cfg(char *delim)
 void
 switch_status(void)
 {
-   struct ifreq ifr;
-
if (!is_switch())
-   return;
-
-   strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));
-   if (ioctl(sock, SIOCGIFFLAGS, (caddr_t)) == -1)
return;
 
switch_cfg("\t");



ifconfig: remove redundant bridge checks

2020-07-28 Thread Klemens Nanni
bridge_status() and switch_status() do the regular sanity check with
SIOCGIFFLAGS, but both functions also call is_switch(), bridge_status()
also calls is_bridge().

Those is_*() helpers do the same SIOCGIFFLAGS sanity check, making those
in *_status() entirely redundant, so I'd like to remove them.

I'm here since the tpmr(4) ioctl interface transition from trunk to
bridge semantics is now complete, so ifconfig(8) now requires tpmr bits
to show its members in bridge fashion.

One way would be duplicate code into is_tpmr() and tpmr_status() which
I've already done, but another approach is to unify all bridge like
interfaces under bridge_status().

Either ways, diff below cleans up and makes for simpler code.

Feedback? OK?


Index: brconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.25
diff -u -p -r1.25 brconfig.c
--- brconfig.c  22 Jan 2020 06:24:07 -  1.25
+++ brconfig.c  28 Jul 2020 17:02:05 -
@@ -783,16 +783,11 @@ is_bridge()
 void
 bridge_status(void)
 {
-   struct ifreq ifr;
struct ifbrparam bp1, bp2;
 
if (!is_bridge() || is_switch())
return;
 
-   strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));
-   if (ioctl(sock, SIOCGIFFLAGS, (caddr_t)) == -1)
-   return;
-
bridge_cfg("\t");
 
bridge_list("\t");
@@ -1184,13 +1179,7 @@ switch_cfg(char *delim)
 void
 switch_status(void)
 {
-   struct ifreq ifr;
-
if (!is_switch())
-   return;
-
-   strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));
-   if (ioctl(sock, SIOCGIFFLAGS, (caddr_t)) == -1)
return;
 
switch_cfg("\t");



  1   2   3   4   5   6   7   8   9   10   >