Re: ADMtec aue(4) interface supporting VLAN_MTU ?

[David Gwynne]

> On 23 May 2020, at 7:54 am, Christopher Zimmermann  wrote:
> 
> On Tue, Apr 21, 2020 at 04:12:16PM -0700, Chris Cappuccio wrote:
>> Tom Smyth [tom.sm...@wirelessconnect.eu] wrote:
>>> Hi Chrisz,
>>> 
>>> 4 bytes for the vlan header .. have you tried increasing the parent
>>> intetface mtu by 4bytes
>>> 
>> 
>> IFCAP_VLAN_MTU is a direct bypass for this. "hardmtu" on the parent interface
>> is perhaps more interesting as it will limit everything including these
>> encapsulations
> 
> IFAP_VLAN_MTU will affect how the hardmtu is passed on to vlan child 
> interfaces. The vlan interfaces won't care for the parent's "soft" mtu AFAICS:

That is correct.

> hardmtu = ifp0->if_hardmtu;
> if (!ISSET(ifp0->if_capabilities, IFCAP_VLAN_MTU))
>   hardmtu -= EVL_ENCAPLEN;
> 
> Linux uses a MTU of 1536 for Pegasus chips. We always default to a hardmtu of 
> 1500. So the hardmtu can't be the cause for my interface not managing 
> full-size vlan.

We should set the hardmtu of the Pegasus chips higher then. Does Linux include 
the Ethernet header in that MTU there?

dlg

> 
> Christopher
> 
> 
> -- 
> http://gmerlin.de
> OpenPGP: http://gmerlin.de/christopher.pub
> CB07 DA40 B0B6 571D 35E2  0DEF 87E2 92A7 13E5 DEE1
> 

Re: carp: send only IPv4 carp packets on dual stack interface

[David Gwynne]

> On 23 May 2020, at 8:44 am, Christopher Zimmermann  wrote:
> 
> On Sun, Jan 19, 2020 at 01:32:17PM +, Stuart Henderson wrote:
>> On 2020/01/19 00:11, Sebastian Benoit wrote:
>>> chr...@openbsd.org(chr...@openbsd.org) on 2020.01.18 06:18:21 +0100:
>>> > On Wed, Jan 15, 2020 at 12:47:28PM +0100, Sebastian Benoit wrote:
>>> > >Christopher Zimmermann(chr...@openbsd.org) on 2020.01.15 11:55:43 +0100:
>>> > >>Hi,
>>> > >>
>>> > >>as far as I can see a dual stack carp interface does not care whether it
>>> > >>receives advertisements addressed to IPv4 or IPv6. Any one will do.
>>> > >>So I propose to send IPv6 advertisements only when IPv4 is not possible.
>>> > >>
>>> > >>Why?
>>> > >>
>>> > >>- Noise can be reduced by using unicast advertisements.
>>> > >>  This is only possible for IPv4 by ``ifconfig carppeer``.
>>> > >>  I don't like flooding the whole network with carp advertisements when
>>> > >>  I may also unicast them.
>>> > >
>>> > >Maybe i'm getting confused, but in the problem description you were 
>>> > >talking
>>> > >about v6 vs v4, and here you argue about unicast (vs multicast?) being
>>> > >better. Thats orthogonal, isnt it?
>>> >
>>> > Yes, kind of. The point is we support ``carppeer`` for IPv4, but not for
>>> > IPv6.
>>> >
>>> > >>- breaking IPv6 connectivity (for example by running iked without -6)
>>> > >>  will start a preempt-war, because failing ip6_output will cause the
>>> > >>  demote counter to be increased. That's what hit me.
>>> > >
>>> > >But the whole point of carp is to notice broken connectivity. If you run 
>>> > >v6
>>> > >on an interface, you want to know if its working, no?
>>> >
>>> > I grant you that much. But what kind of failures do you hope to detect
>>> > on the _sending_ carp master, that would not also affect the backup?
>>> 
>>> sure: misconfigured pf. Missing routes. Buggy switch.
>> 
>> misconfigured mac address filter on switch.
> 
> I'm afraid you guys haven't yet got the point I'm trying to make.
> 
> Current behaviour is that in a dual-stack carp setup failover only happens 
> when advertisements on _both_ AFs fail to reach the backup.
> A node in backup state will stay in backup state as long as it receives _any_ 
> advertisements.
> In my mind this is the only sensible way for a backup node to react.
> 
> If a backup node that fails to receive advertisements of only one AF would 
> transition to master it would in most cases start a preempt war. 
> So why do we even send dual-stack advertisements?
> The only effect those dual-stack ipv6 advertisements currently have is that 
> they prevent failover when ipv4 connectivity breaks.
> 
> I would propose to choose one "sentinel" AF (in this case ipv4) and failover 
> whenever advertisements of this AF fail to reach the backup.
> 
> Monitoring multiple AFs is not helpful, because there is no good way in which 
> to react to a failure that affects only one AF.

I don't know if this helps, but at work we use separate carp interfaces for v4 
and v6. It ends up looking a bit like this:

# cat /etc/hostname.vlan871:
parent aggr0 vnetid 871
inet alias 192.0.2.2/24
inet6 alias 2001:db8:871::2/64
up

# cat /etc/hostname.carp40871
carpdev vlan871 vhid 47
-inet6
-group carp
group ipv4g
inet alias 192.0.2.1/24
up

# cat /etc/hostname.carp60871
carpdev vlan871 vhid 61
-group carp
group ipv6g
inet6 alias 2001:db8:871::1/64
up

This let's us run a pair of firewalls one active for v4 and the other for v6. 
We don't do any af-to in PF, so it works pretty well. But yeah, it means v4 and 
v6 fail separately.

> 
>>> > >At the very least, this needs some more thought and testing in all > 
>>> > >>the ways
>>> > >carp can be configured.
>>> >
>>> > Anyway, my main concern indeed is the broadcast noise generated by carp
>>> > and I would be equally happy if we had a ``carppeer6`` option. Would
>>> > that be considered?
>>> 
>>> of course carppeer should work with v6, and as claudio says without an extra
>>> keyword in ifconfig, but thats a trivial detail.
>>> 
>> 
>> Currently carp only handles one address per af, setting carppeer twice
>> changes the current peer address rather than adding another. A trivial
>> implementation that sets the v4 peer address if a v4 address is passed
>> in, and sets the v6 peer address if a v6 address is passed in, that
>> would mean things work differently with
>> 
>> ifconfig carp1 carppeer $foo
>> ifconfig carp1 carppeer $bar
>> 
>> depending on whether foo/bar are v4 or v6. Also removing a configured
>> carppeer address to reset to multicast is just done with -carppeer
>> with no way to indicate the af.
>> 
>> It would work pretty nicely if you could set multiple carppeer addresses
>> (of whatever af) and remove them individually. That's a more complex
>> change (carp would need to keep a list of peers per af rather than a
>> single address) but without something like that they can't really be
>> equals and it feels like shoehorning both afs into the same keyword
>> will just be confusing

Re: ldomctl: Make init-sytem -n check vcpu and memory constraints

[Theo de Raadt]

Klemens Nanni  wrote:

> On Fri, May 22, 2020 at 05:57:37PM -0600, Theo de Raadt wrote:
> > There is an internal VCPU #define, but the keyword is vcpu, and there
> > appears to be nothing coming from the system which is an uppercase VCPU
> > 
> > We don't have pfctl spitting out messages like: invalid RDOMAIN, because
> > rdomain is the keyword, there is no reason to arbitrarily change things
> > from LOWERCASE to UPPERCASE or even CamelCase.
> Yes, although those error messages read fine to me with uppercase "VCPU"
> as well, regardless of whether the keyword is "vcpu" or "cpu", because
> they talk about the resources per se that are managed, not a particular
> line in the configuration file that is ill written.

I think your argument is completely hopeless.

My comment is about UPPERCASE vs lowercase.

Not about whether it is cpu or vcpu.  But about UPPERCASE.

Why not make it VcPu then?

You are trying to defend an arbitrary difference.

Re: ldomctl: Make init-sytem -n check vcpu and memory constraints

[Theo de Raadt]

Klemens Nanni  wrote:

> On Fri, May 22, 2020 at 05:57:37PM -0600, Theo de Raadt wrote:
> > There is an internal VCPU #define, but the keyword is vcpu, and there
> > appears to be nothing coming from the system which is an uppercase VCPU
> > 
> > We don't have pfctl spitting out messages like: invalid RDOMAIN, because
> > rdomain is the keyword, there is no reason to arbitrarily change things
> > from LOWERCASE to UPPERCASE or even CamelCase.
> Yes, although those error messages read fine to me with uppercase "VCPU"
> as well, regardless of whether the keyword is "vcpu" or "cpu", because
> they talk about the resources per se that are managed, not a particular
> line in the configuration file that is ill written.

That makes no sense.

> Fine with me either way.

Really? Then how about this?

Obvious DOMAIN refers to the resources, and so does MEMORY.  Obviously
these have nothing to do with the grammer.  Anyone will be able to
figure it out.  Right?

Index: config.c
===
RCS file: /cvs/src/usr.sbin/ldomctl/config.c,v
retrieving revision 1.38
diff -u -p -u -r1.38 config.c
--- config.c22 May 2020 21:54:20 -  1.38
+++ config.c22 May 2020 23:53:34 -
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/ldomctl/parse.y,v
retrieving revision 1.18
diff -u -p -u -r1.18 parse.y
--- parse.y 21 Feb 2020 19:39:28 -  1.18
+++ parse.y 23 May 2020 00:26:30 -
@@ -123,7 +123,7 @@ domain  : DOMAIN STRING optnl '{' optnl 
struct domain *odomain;
SIMPLEQ_FOREACH(odomain, &conf->domain_list, entry)
if (strcmp(odomain->name, $2) == 0) {
-   yyerror("duplicate domain name: %s", 
$2);
+   yyerror("duplicate DOMAIN name: %s", 
$2);
YYERROR;
}
domain = xzalloc(sizeof(struct domain));
@@ -141,7 +141,7 @@ domain  : DOMAIN STRING optnl '{' optnl 
YYERROR;
}
if ( domain->memory == 0) {
-   yyerror("memory is required: %s",
+   yyerror("MEMORY is required: %s",
domain->name);
YYERROR;
}

Re: ldomctl: Make init-sytem -n check vcpu and memory constraints

[Klemens Nanni]

On Fri, May 22, 2020 at 05:57:37PM -0600, Theo de Raadt wrote:
> There is an internal VCPU #define, but the keyword is vcpu, and there
> appears to be nothing coming from the system which is an uppercase VCPU
> 
> We don't have pfctl spitting out messages like: invalid RDOMAIN, because
> rdomain is the keyword, there is no reason to arbitrarily change things
> from LOWERCASE to UPPERCASE or even CamelCase.
Yes, although those error messages read fine to me with uppercase "VCPU"
as well, regardless of whether the keyword is "vcpu" or "cpu", because
they talk about the resources per se that are managed, not a particular
line in the configuration file that is ill written.

Fine with me either way.

Re: ldomctl: Make init-sytem -n check vcpu and memory constraints

[Klemens Nanni]

On Fri, May 22, 2020 at 05:50:32PM -0600, Theo de Raadt wrote:
> Your diff added a new upper case.
No, it did not;  neither of the two diffs changed any error message,
wording or case.  Both reordered already existing lines only.

Re: ldomctl: Make init-sytem -n check vcpu and memory constraints

[Theo de Raadt]

There is an internal VCPU #define, but the keyword is vcpu, and there
appears to be nothing coming from the system which is an uppercase VCPU

We don't have pfctl spitting out messages like: invalid RDOMAIN, because
rdomain is the keyword, there is no reason to arbitrarily change things
from LOWERCASE to UPPERCASE or even CamelCase.

That's why I asked twice.

Index: config.c
===
RCS file: /cvs/src/usr.sbin/ldomctl/config.c,v
retrieving revision 1.38
diff -u -p -u -r1.38 config.c
--- config.c22 May 2020 21:54:20 -  1.38
+++ config.c22 May 2020 23:53:34 -
@@ -162,7 +162,7 @@ pri_link_core(struct md *md, struct md_n
 
cpu = pri_find_cpu(pid);
if (cpu == NULL)
-   errx(1, "couldn't determine core for VCPU 
%lld\n", pid);
+   errx(1, "couldn't determine core for vcpu 
%lld\n", pid);
cpu->core = core;
}
}
@@ -2819,7 +2819,7 @@ build_config(const char *filename, int n
if (primary_memory == 0 && total_memory > memory)
primary_memory = total_memory - memory;
if (num_cpus > total_cpus || primary_num_cpus == 0)
-   errx(1, "not enough VCPU resources available");
+   errx(1, "not enough vcpu resources available");
if (memory > total_memory || primary_memory == 0)
errx(1, "not enough memory available");
 

Re: ldomctl: Make init-sytem -n check vcpu and memory constraints

[Theo de Raadt]

Klemens Nanni  wrote:

> On Fri, May 22, 2020 at 05:05:14PM -0600, Theo de Raadt wrote:
> > Then why is the grammer keyword lowercase?
> Because it's easier to type and practically all grammar keywords in all
> of base's *.conf files are lowercase.

Your diff added a new upper case.

> Send a diff if you want to change stuff.

The diff you didn't wait for OK's

the diff you commited

The diff you needed to back out.

Want attitude?

Re: ldomctl: Make init-sytem -n check vcpu and memory constraints

[Klemens Nanni]

On Fri, May 22, 2020 at 05:05:14PM -0600, Theo de Raadt wrote:
> Then why is the grammer keyword lowercase?
Because it's easier to type and practically all grammar keywords in all
of base's *.conf files are lowercase.

Send a diff if you want to change stuff.

Re: ldomctl: Make init-sytem -n check vcpu and memory constraints

[Theo de Raadt]

Klemens Nanni  wrote:

> On Fri, May 22, 2020 at 04:39:03PM -0600, Theo de Raadt wrote:
> > Why is VCPU upper case?
> Probably because it is an abbreviation?  Wherever I look, "CPU" is upper
> case.  ldom.conf(5) says "virutal CPUs", which reads as fine as "VCPUs"
> to me - "vcpus" would be off.
> 
> > I don't see any other place it is upper case.
> 
>   $ cvs blame config.c |grep VCPU
>   Annotations for config.c
>   ***
>   1.11 (kettenis 01-Dec-12):  errx(1, 
> "couldn't determine core for VCPU %lld\n", pid);
>   1.2  (kettenis 24-Nov-12):  errx(1, "not enough 
> VCPU resources available");
> 

Then why is the grammer keyword lowercase?

Come on.

Re: ldomctl: Make init-sytem -n check vcpu and memory constraints

[Klemens Nanni]

On Fri, May 22, 2020 at 04:39:03PM -0600, Theo de Raadt wrote:
> Why is VCPU upper case?
Probably because it is an abbreviation?  Wherever I look, "CPU" is upper
case.  ldom.conf(5) says "virutal CPUs", which reads as fine as "VCPUs"
to me - "vcpus" would be off.

> I don't see any other place it is upper case.

$ cvs blame config.c |grep VCPU
Annotations for config.c
***
1.11 (kettenis 01-Dec-12):  errx(1, 
"couldn't determine core for VCPU %lld\n", pid);
1.2  (kettenis 24-Nov-12):  errx(1, "not enough 
VCPU resources available");

Re: ldomctl: Make init-sytem -n check vcpu and memory constraints

[Kurt Mosiejczuk]

On Fri, May 22, 2020 at 04:39:03PM -0600, Theo de Raadt wrote:
> Why is VCPU upper case?

> I don't see any other place it is upper case.

I don't know. It was that way before the change. It's in the file twice
as uppercase, both times when using errx(3). It's not used as lowercase
when using errx. It has been that way for 7+ years. It can easily be
changed though.

--Kurt

Re: carp: send only IPv4 carp packets on dual stack interface

[Christopher Zimmermann]

On Sun, Jan 19, 2020 at 01:32:17PM +, Stuart Henderson wrote:

On 2020/01/19 00:11, Sebastian Benoit wrote:

chr...@openbsd.org(chr...@openbsd.org) on 2020.01.18 06:18:21 +0100:
> On Wed, Jan 15, 2020 at 12:47:28PM +0100, Sebastian Benoit wrote:
> >Christopher Zimmermann(chr...@openbsd.org) on 2020.01.15 11:55:43 +0100:
> >>Hi,
> >>
> >>as far as I can see a dual stack carp interface does not care whether it
> >>receives advertisements addressed to IPv4 or IPv6. Any one will do.
> >>So I propose to send IPv6 advertisements only when IPv4 is not possible.
> >>
> >>Why?
> >>
> >>- Noise can be reduced by using unicast advertisements.
> >>  This is only possible for IPv4 by ``ifconfig carppeer``.
> >>  I don't like flooding the whole network with carp advertisements when
> >>  I may also unicast them.
> >
> >Maybe i'm getting confused, but in the problem description you were talking
> >about v6 vs v4, and here you argue about unicast (vs multicast?) being
> >better. Thats orthogonal, isnt it?
>
> Yes, kind of. The point is we support ``carppeer`` for IPv4, but not for
> IPv6.
>
> >>- breaking IPv6 connectivity (for example by running iked without -6)
> >>  will start a preempt-war, because failing ip6_output will cause the
> >>  demote counter to be increased. That's what hit me.
> >
> >But the whole point of carp is to notice broken connectivity. If you run v6
> >on an interface, you want to know if its working, no?
>
> I grant you that much. But what kind of failures do you hope to detect
> on the _sending_ carp master, that would not also affect the backup?

sure: misconfigured pf. Missing routes. Buggy switch.


misconfigured mac address filter on switch.


I'm afraid you guys haven't yet got the point I'm trying to make.

Current behaviour is that in a dual-stack carp setup failover only 
happens when advertisements on _both_ AFs fail to reach the backup.
A node in backup state will stay in backup state as long as it receives 
_any_ advertisements.

In my mind this is the only sensible way for a backup node to react.

If a backup node that fails to receive advertisements of only one AF 
would transition to master it would in most cases start a preempt war. 


So why do we even send dual-stack advertisements?
The only effect those dual-stack ipv6 advertisements currently have is 
that they prevent failover when ipv4 connectivity breaks.


I would propose to choose one "sentinel" AF (in this case ipv4) and 
failover whenever advertisements of this AF fail to reach the backup.


Monitoring multiple AFs is not helpful, because there is no good way in 
which to react to a failure that affects only one AF.


> >At the very least, this needs some more thought and testing in all 
> >the ways

> >carp can be configured.
>
> Anyway, my main concern indeed is the broadcast noise generated by carp
> and I would be equally happy if we had a ``carppeer6`` option. Would
> that be considered?

of course carppeer should work with v6, and as claudio says without an extra
keyword in ifconfig, but thats a trivial detail.



Currently carp only handles one address per af, setting carppeer twice
changes the current peer address rather than adding another. A trivial
implementation that sets the v4 peer address if a v4 address is passed
in, and sets the v6 peer address if a v6 address is passed in, that
would mean things work differently with

ifconfig carp1 carppeer $foo
ifconfig carp1 carppeer $bar

depending on whether foo/bar are v4 or v6. Also removing a configured
carppeer address to reset to multicast is just done with -carppeer
with no way to indicate the af.

It would work pretty nicely if you could set multiple carppeer addresses
(of whatever af) and remove them individually. That's a more complex
change (carp would need to keep a list of peers per af rather than a
single address) but without something like that they can't really be
equals and it feels like shoehorning both afs into the same keyword
will just be confusing.



--
http://gmerlin.de
OpenPGP: http://gmerlin.de/christopher.pub
CB07 DA40 B0B6 571D 35E2  0DEF 87E2 92A7 13E5 DEE1

Re: ldomctl: Make init-sytem -n check vcpu and memory constraints

[Theo de Raadt]

Why is VCPU upper case?

I don't see any other place it is upper case.


Klemens Nanni  wrote:

> On Fri, May 22, 2020 at 11:35:34PM +0200, Mark Kettenis wrote:
> > That can't be right.  We get the number of available CPUs for example
> > from the PRI so pri_init() has to be called before we check.
> Yup, I fooled myself with poor testing - the commit is already reverted
> with an explanation.  Didn't want to rush anything.
> 
> Here's version two that merely moves the "-n" check after the constraint
> checks;  pri_init() continues to be called before using either of
> `total_cpus' or `total_memory'.
> 
>   $ cat ldom.conf
>   domain guest {
>   vdisk "/dev/null"
>   vcpu 1
>   memory 1G
>   }
>   $ ./obj/ldomctl init-system -n ldom.conf ; echo $?
>   0
>   $ sed -i /memory/s,1,128, ldom.conf 
>   $ ./obj/ldomctl init-system -n ldom.conf
>   ldomctl: not enough memory available
>   $ sed -i /cpu/s,1,128, ldom.conf 
>   $ ./obj/ldomctl init-system -n ldom.conf
>   ldomctl: not enough VCPU resources available
> 
> 
> Feedback? OK?
> 
> 
> Index: config.c
> ===
> RCS file: /cvs/src/usr.sbin/ldomctl/config.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 config.c
> --- config.c  17 Mar 2020 21:24:22 -  1.36
> +++ config.c  22 May 2020 22:11:59 -
> @@ -2792,8 +2798,6 @@ build_config(const char *filename, int n
>   SIMPLEQ_INIT(&conf.domain_list);
>   if (parse_config(filename, &conf) < 0)
>   exit(1);
> - if (noaction)
> - exit(0);
>  
>   pri = md_read("pri");
>   if (pri == NULL)
> @@ -2822,6 +2826,9 @@ build_config(const char *filename, int n
>   errx(1, "not enough VCPU resources available");
>   if (memory > total_memory || primary_memory == 0)
>   errx(1, "not enough memory available");
> +
> + if (noaction)
> + exit(0);
>  
>   hvmd_init(hvmd);
>   primary = primary_init();
> 

Re: ldomctl: Make init-sytem -n check vcpu and memory constraints

[Klemens Nanni]

On Fri, May 22, 2020 at 11:35:34PM +0200, Mark Kettenis wrote:
> That can't be right.  We get the number of available CPUs for example
> from the PRI so pri_init() has to be called before we check.
Yup, I fooled myself with poor testing - the commit is already reverted
with an explanation.  Didn't want to rush anything.

Here's version two that merely moves the "-n" check after the constraint
checks;  pri_init() continues to be called before using either of
`total_cpus' or `total_memory'.

$ cat ldom.conf
domain guest {
vdisk "/dev/null"
vcpu 1
memory 1G
}
$ ./obj/ldomctl init-system -n ldom.conf ; echo $?
0
$ sed -i /memory/s,1,128, ldom.conf 
$ ./obj/ldomctl init-system -n ldom.conf
ldomctl: not enough memory available
$ sed -i /cpu/s,1,128, ldom.conf 
$ ./obj/ldomctl init-system -n ldom.conf
ldomctl: not enough VCPU resources available


Feedback? OK?


Index: config.c
===
RCS file: /cvs/src/usr.sbin/ldomctl/config.c,v
retrieving revision 1.36
diff -u -p -r1.36 config.c
--- config.c17 Mar 2020 21:24:22 -  1.36
+++ config.c22 May 2020 22:11:59 -
@@ -2792,8 +2798,6 @@ build_config(const char *filename, int n
SIMPLEQ_INIT(&conf.domain_list);
if (parse_config(filename, &conf) < 0)
exit(1);
-   if (noaction)
-   exit(0);
 
pri = md_read("pri");
if (pri == NULL)
@@ -2822,6 +2826,9 @@ build_config(const char *filename, int n
errx(1, "not enough VCPU resources available");
if (memory > total_memory || primary_memory == 0)
errx(1, "not enough memory available");
+
+   if (noaction)
+   exit(0);
 
hvmd_init(hvmd);
primary = primary_init();

Re: Remove useless line from daemon class in login.conf

[Theo de Raadt]

Stuart Henderson  wrote:

> On 2020/05/22 17:06, Daniel Jakots wrote:
> > Hi,
> > 
> > We used to have different numbers of blowfish rounds between the
> > default and daemon classes in login.conf. On Jun 26, 2016, tedu
> > committed "upgrade selected login.conf to use auto rounds for bcrypt"
> > for amd64, sparc64, i386, and maccpc [1].
> > 
> > Since the class daemon inherits from the default class, the 
> > :localcipher=blowfish,a:\
> > is a duplicate.
> > 
> > Here's a diff to remove them.
> 
> I'm OK with unifying these settings, but FWIW I never switched to auto
> for these, it doesn't seem all that sensible for somebody with the ability
> to generate enough load on the machine to be able to reduce the strength
> of bcrypt down to the 64 (2^6) rounds minimum.

Yes, that is problematic.

The minimum should be probably be raised, we should consider if auto
should even exist anymore.

Re: Remove useless line from daemon class in login.conf

[Stuart Henderson]

On 2020/05/22 17:06, Daniel Jakots wrote:
> Hi,
> 
> We used to have different numbers of blowfish rounds between the
> default and daemon classes in login.conf. On Jun 26, 2016, tedu
> committed "upgrade selected login.conf to use auto rounds for bcrypt"
> for amd64, sparc64, i386, and maccpc [1].
> 
> Since the class daemon inherits from the default class, the 
> :localcipher=blowfish,a:\
> is a duplicate.
> 
> Here's a diff to remove them.

I'm OK with unifying these settings, but FWIW I never switched to auto
for these, it doesn't seem all that sensible for somebody with the ability
to generate enough load on the machine to be able to reduce the strength
of bcrypt down to the 64 (2^6) rounds minimum.

Re: ADMtec aue(4) interface supporting VLAN_MTU ?

[Christopher Zimmermann]

On Tue, Apr 21, 2020 at 04:12:16PM -0700, Chris Cappuccio wrote:

Tom Smyth [tom.sm...@wirelessconnect.eu] wrote:

Hi Chrisz,

4 bytes for the vlan header .. have you tried increasing the parent
intetface mtu by 4bytes



IFCAP_VLAN_MTU is a direct bypass for this. "hardmtu" on the parent interface
is perhaps more interesting as it will limit everything including these
encapsulations


IFAP_VLAN_MTU will affect how the hardmtu is passed on to vlan child 
interfaces. The vlan interfaces won't care for the parent's "soft" mtu 
AFAICS:


hardmtu = ifp0->if_hardmtu;
if (!ISSET(ifp0->if_capabilities, IFCAP_VLAN_MTU))
hardmtu -= EVL_ENCAPLEN;

Linux uses a MTU of 1536 for Pegasus chips. We always default to a 
hardmtu of 1500. So the hardmtu can't be the cause for my interface not 
managing full-size vlan.


Christopher


--
http://gmerlin.de
OpenPGP: http://gmerlin.de/christopher.pub
CB07 DA40 B0B6 571D 35E2  0DEF 87E2 92A7 13E5 DEE1

Re: ldomctl: Make init-sytem -n check vcpu and memory constraints

[Mark Kettenis]

> Date: Fri, 22 May 2020 23:05:21 +0200
> From: Klemens Nanni 
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> 
> 
> kmos noted that `-n' wouldn't bark at overallocation.
> 
> Hoisting PRI reading and moving the `-n' check after constraint checking
> makes it bail on invalid configs just as expected:
> 
>   $ cat ldom.conf
>   domain guest {
>   vdisk "/dev/null"
>   vcpu 128
>   memory 1G
>   }
>   $ ldomctl init-sytem -n ldom.conf ; echo $?
>   0
>   $ ./obj/ldomctl init-sytem -n ldom.conf
>   ldomctl: not enough VCPU resources available
> 
> 
> OK?

That can't be right.  We get the number of available CPUs for example
from the PRI so pri_init() has to be called before we check.

> Index: config.c
> ===
> RCS file: /cvs/src/usr.sbin/ldomctl/config.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 config.c
> --- config.c  17 Mar 2020 21:24:22 -  1.36
> +++ config.c  22 May 2020 21:01:17 -
> @@ -2792,18 +2798,6 @@ build_config(const char *filename, int n
>   SIMPLEQ_INIT(&conf.domain_list);
>   if (parse_config(filename, &conf) < 0)
>   exit(1);
> - if (noaction)
> - exit(0);
> -
> - pri = md_read("pri");
> - if (pri == NULL)
> - err(1, "unable to get PRI");
> - hvmd = md_read("hv.md");
> - if (hvmd == NULL)
> - err(1, "unable to get Hypervisor MD");
> -
> - pri_init(pri);
> - pri_alloc_memory(hv_membase, hv_memsize);
>  
>   SIMPLEQ_FOREACH(domain, &conf.domain_list, entry) {
>   if (strcmp(domain->name, "primary") == 0) {
> @@ -2822,6 +2816,19 @@ build_config(const char *filename, int n
>   errx(1, "not enough VCPU resources available");
>   if (memory > total_memory || primary_memory == 0)
>   errx(1, "not enough memory available");
> +
> + if (noaction)
> + exit(0);
> +
> + pri = md_read("pri");
> + if (pri == NULL)
> + err(1, "unable to get PRI");
> + hvmd = md_read("hv.md");
> + if (hvmd == NULL)
> + err(1, "unable to get Hypervisor MD");
> +
> + pri_init(pri);
> + pri_alloc_memory(hv_membase, hv_memsize);
>  
>   hvmd_init(hvmd);
>   primary = primary_init();
> 
> 

Re: ldomctl: Make init-sytem -n check vcpu and memory constraints

[Theo de Raadt]

Why upper case?


Klemens Nanni  wrote:

> 
> 
> kmos noted that `-n' wouldn't bark at overallocation.
> 
> Hoisting PRI reading and moving the `-n' check after constraint checking
> makes it bail on invalid configs just as expected:
> 
>   $ cat ldom.conf
>   domain guest {
>   vdisk "/dev/null"
>   vcpu 128
>   memory 1G
>   }
>   $ ldomctl init-sytem -n ldom.conf ; echo $?
>   0
>   $ ./obj/ldomctl init-sytem -n ldom.conf
>   ldomctl: not enough VCPU resources available
> 
> 
> OK?
> 
> 
> Index: config.c
> ===
> RCS file: /cvs/src/usr.sbin/ldomctl/config.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 config.c
> --- config.c  17 Mar 2020 21:24:22 -  1.36
> +++ config.c  22 May 2020 21:01:17 -
> @@ -2792,18 +2798,6 @@ build_config(const char *filename, int n
>   SIMPLEQ_INIT(&conf.domain_list);
>   if (parse_config(filename, &conf) < 0)
>   exit(1);
> - if (noaction)
> - exit(0);
> -
> - pri = md_read("pri");
> - if (pri == NULL)
> - err(1, "unable to get PRI");
> - hvmd = md_read("hv.md");
> - if (hvmd == NULL)
> - err(1, "unable to get Hypervisor MD");
> -
> - pri_init(pri);
> - pri_alloc_memory(hv_membase, hv_memsize);
>  
>   SIMPLEQ_FOREACH(domain, &conf.domain_list, entry) {
>   if (strcmp(domain->name, "primary") == 0) {
> @@ -2822,6 +2816,19 @@ build_config(const char *filename, int n
>   errx(1, "not enough VCPU resources available");
>   if (memory > total_memory || primary_memory == 0)
>   errx(1, "not enough memory available");
> +
> + if (noaction)
> + exit(0);
> +
> + pri = md_read("pri");
> + if (pri == NULL)
> + err(1, "unable to get PRI");
> + hvmd = md_read("hv.md");
> + if (hvmd == NULL)
> + err(1, "unable to get Hypervisor MD");
> +
> + pri_init(pri);
> + pri_alloc_memory(hv_membase, hv_memsize);
>  
>   hvmd_init(hvmd);
>   primary = primary_init();
> 

Re: ldomctl: Make init-sytem -n check vcpu and memory constraints

[Kurt Mosiejczuk]

On Fri, May 22, 2020 at 11:05:21PM +0200, Klemens Nanni wrote:

> kmos noted that `-n' wouldn't bark at overallocation.

> Hoisting PRI reading and moving the `-n' check after constraint checking
> makes it bail on invalid configs just as expected:

>   $ cat ldom.conf
>   domain guest {
>   vdisk "/dev/null"
>   vcpu 128
>   memory 1G
>   }
>   $ ldomctl init-sytem -n ldom.conf ; echo $?
>   0
>   $ ./obj/ldomctl init-sytem -n ldom.conf
>   ldomctl: not enough VCPU resources available

> OK?

Tested and works for me.

ok kmos

--Kurt

> 
> Index: config.c
> ===
> RCS file: /cvs/src/usr.sbin/ldomctl/config.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 config.c
> --- config.c  17 Mar 2020 21:24:22 -  1.36
> +++ config.c  22 May 2020 21:01:17 -
> @@ -2792,18 +2798,6 @@ build_config(const char *filename, int n
>   SIMPLEQ_INIT(&conf.domain_list);
>   if (parse_config(filename, &conf) < 0)
>   exit(1);
> - if (noaction)
> - exit(0);
> -
> - pri = md_read("pri");
> - if (pri == NULL)
> - err(1, "unable to get PRI");
> - hvmd = md_read("hv.md");
> - if (hvmd == NULL)
> - err(1, "unable to get Hypervisor MD");
> -
> - pri_init(pri);
> - pri_alloc_memory(hv_membase, hv_memsize);
>  
>   SIMPLEQ_FOREACH(domain, &conf.domain_list, entry) {
>   if (strcmp(domain->name, "primary") == 0) {
> @@ -2822,6 +2816,19 @@ build_config(const char *filename, int n
>   errx(1, "not enough VCPU resources available");
>   if (memory > total_memory || primary_memory == 0)
>   errx(1, "not enough memory available");
> +
> + if (noaction)
> + exit(0);
> +
> + pri = md_read("pri");
> + if (pri == NULL)
> + err(1, "unable to get PRI");
> + hvmd = md_read("hv.md");
> + if (hvmd == NULL)
> + err(1, "unable to get Hypervisor MD");
> +
> + pri_init(pri);
> + pri_alloc_memory(hv_membase, hv_memsize);
>  
>   hvmd_init(hvmd);
>   primary = primary_init();
> 

Remove useless line from daemon class in login.conf

[Daniel Jakots]

Hi,

We used to have different numbers of blowfish rounds between the
default and daemon classes in login.conf. On Jun 26, 2016, tedu
committed "upgrade selected login.conf to use auto rounds for bcrypt"
for amd64, sparc64, i386, and maccpc [1].

Since the class daemon inherits from the default class, the 
:localcipher=blowfish,a:\
is a duplicate.

Here's a diff to remove them.

Cheers,
Daniel

[1]: 
https://github.com/openbsd/src/commit/69b58a8d03f019fa368cc0ddb22481f4f3f36671

Index: etc.amd64/login.conf
===
RCS file: /cvs/src/etc/etc.amd64/login.conf,v
retrieving revision 1.14
diff -u -p -r1.14 login.conf
--- etc.amd64/login.conf11 Mar 2020 15:41:48 -  1.14
+++ etc.amd64/login.conf22 May 2020 20:57:14 -
@@ -64,7 +64,6 @@ daemon:\
:openfiles-max=1024:\
:openfiles-cur=128:\
:stacksize-cur=8M:\
-   :localcipher=blowfish,a:\
:tc=default:
 
 #
Index: etc.i386/login.conf
===
RCS file: /cvs/src/etc/etc.i386/login.conf,v
retrieving revision 1.9
diff -u -p -r1.9 login.conf
--- etc.i386/login.conf 5 Nov 2019 19:03:46 -   1.9
+++ etc.i386/login.conf 22 May 2020 20:57:14 -
@@ -64,7 +64,6 @@ daemon:\
:openfiles-max=1024:\
:openfiles-cur=128:\
:stacksize-cur=8M:\
-   :localcipher=blowfish,a:\
:tc=default:
 
 #
Index: etc.macppc/login.conf
===
RCS file: /cvs/src/etc/etc.macppc/login.conf,v
retrieving revision 1.12
diff -u -p -r1.12 login.conf
--- etc.macppc/login.conf   12 Mar 2020 15:32:22 -  1.12
+++ etc.macppc/login.conf   22 May 2020 20:57:14 -
@@ -64,7 +64,6 @@ daemon:\
:openfiles-max=1024:\
:openfiles-cur=128:\
:stacksize-cur=8M:\
-   :localcipher=blowfish,a:\
:tc=default:
 
 #
Index: etc.sparc64/login.conf
===
RCS file: /cvs/src/etc/etc.sparc64/login.conf,v
retrieving revision 1.12
diff -u -p -r1.12 login.conf
--- etc.sparc64/login.conf  5 Nov 2019 19:03:47 -   1.12
+++ etc.sparc64/login.conf  22 May 2020 20:57:14 -
@@ -64,7 +64,6 @@ daemon:\
:openfiles-max=1024:\
:openfiles-cur=128:\
:stacksize-cur=8M:\
-   :localcipher=blowfish,a:\
:tc=default:
 
 #

ldomctl: Make init-sytem -n check vcpu and memory constraints

[Klemens Nanni]

kmos noted that `-n' wouldn't bark at overallocation.

Hoisting PRI reading and moving the `-n' check after constraint checking
makes it bail on invalid configs just as expected:

$ cat ldom.conf
domain guest {
vdisk "/dev/null"
vcpu 128
memory 1G
}
$ ldomctl init-sytem -n ldom.conf ; echo $?
0
$ ./obj/ldomctl init-sytem -n ldom.conf
ldomctl: not enough VCPU resources available


OK?


Index: config.c
===
RCS file: /cvs/src/usr.sbin/ldomctl/config.c,v
retrieving revision 1.36
diff -u -p -r1.36 config.c
--- config.c17 Mar 2020 21:24:22 -  1.36
+++ config.c22 May 2020 21:01:17 -
@@ -2792,18 +2798,6 @@ build_config(const char *filename, int n
SIMPLEQ_INIT(&conf.domain_list);
if (parse_config(filename, &conf) < 0)
exit(1);
-   if (noaction)
-   exit(0);
-
-   pri = md_read("pri");
-   if (pri == NULL)
-   err(1, "unable to get PRI");
-   hvmd = md_read("hv.md");
-   if (hvmd == NULL)
-   err(1, "unable to get Hypervisor MD");
-
-   pri_init(pri);
-   pri_alloc_memory(hv_membase, hv_memsize);
 
SIMPLEQ_FOREACH(domain, &conf.domain_list, entry) {
if (strcmp(domain->name, "primary") == 0) {
@@ -2822,6 +2816,19 @@ build_config(const char *filename, int n
errx(1, "not enough VCPU resources available");
if (memory > total_memory || primary_memory == 0)
errx(1, "not enough memory available");
+
+   if (noaction)
+   exit(0);
+
+   pri = md_read("pri");
+   if (pri == NULL)
+   err(1, "unable to get PRI");
+   hvmd = md_read("hv.md");
+   if (hvmd == NULL)
+   err(1, "unable to get Hypervisor MD");
+
+   pri_init(pri);
+   pri_alloc_memory(hv_membase, hv_memsize);
 
hvmd_init(hvmd);
primary = primary_init();

Re: Correcty reloading unresolved host in syslogd @Conf lines

[Todd C . Miller]

On Fri, 22 May 2020 21:29:43 +0200, Alexander Bluhm wrote:

> On Fri, May 22, 2020 at 07:38:30AM -0600, Todd C. Miller wrote:
> > I'm a little confused by the protocol handling in cfline.
> >
> > if (strcmp(proto, "udp") == 0) {
> > if (fd_udp == -1)
> > proto = "udp6";
> > if (fd_udp6 == -1)
> > proto = "udp4";
> > ipproto = proto;
> > }
> >
> > Doesn't that mean that in the default case if a syslog server is
> > not reachable, proto will end up being set to "udp4" and not "udp"?
> > If so, then your diff will only retry udp4 on SIGHUP instead of
> > both udp4 and udp6.
>
> What do you mean by "not reachable"?  As we do not connect(2) and
> ignore most errors of sendto(2), syslogd(8) knows nothing about
> reachabiliy.  I guess you mean "if DNS lookup fails".

Sorry, yes, I meant if the DNS lookup fails.

> fd_udp and fd_udp6 should never become -1 as we cannot reopen them.
> If fd_udp6 is -1 we have to restrict ourselves to "udp4".  But it
> is better to move this code out of the big if else block.  Then we
> get the "no udp4" warning if something went wrong.
>
> There was another problem with my diff.  If DNS server switches
> between A and  answers after SIGHUP, the wrong socket has been
> closed.  It is better to close the sockets based only on configuration,
> not on runtime DNS.  Note that when the config file changes, syslogd
> re-execs itself and we start with fresh sockets.
>
> New diff, move the send_udp = 1 a bit up to the config logic.

OK millert@

 - todd

Re: Correcty reloading unresolved host in syslogd @Conf lines

[Alexander Bluhm]

On Fri, May 22, 2020 at 07:38:30AM -0600, Todd C. Miller wrote:
> I'm a little confused by the protocol handling in cfline.
>
>   if (strcmp(proto, "udp") == 0) {
>   if (fd_udp == -1)
>   proto = "udp6";
>   if (fd_udp6 == -1)
>   proto = "udp4";
>   ipproto = proto;
>   }
>
> Doesn't that mean that in the default case if a syslog server is
> not reachable, proto will end up being set to "udp4" and not "udp"?
> If so, then your diff will only retry udp4 on SIGHUP instead of
> both udp4 and udp6.

What do you mean by "not reachable"?  As we do not connect(2) and
ignore most errors of sendto(2), syslogd(8) knows nothing about
reachabiliy.  I guess you mean "if DNS lookup fails".

fd_udp and fd_udp6 should never become -1 as we cannot reopen them.
If fd_udp6 is -1 we have to restrict ourselves to "udp4".  But it
is better to move this code out of the big if else block.  Then we
get the "no udp4" warning if something went wrong.

There was another problem with my diff.  If DNS server switches
between A and  answers after SIGHUP, the wrong socket has been
closed.  It is better to close the sockets based only on configuration,
not on runtime DNS.  Note that when the config file changes, syslogd
re-execs itself and we start with fresh sockets.

New diff, move the send_udp = 1 a bit up to the config logic.

ok?

bluhm

Index: usr.sbin/syslogd/syslogd.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.262
diff -u -p -r1.262 syslogd.c
--- usr.sbin/syslogd/syslogd.c  5 Jul 2019 13:23:27 -   1.262
+++ usr.sbin/syslogd/syslogd.c  22 May 2020 18:21:23 -
@@ -853,20 +853,6 @@ main(int argc, char *argv[])
event_add(ev_udp, NULL);
if (fd_udp6 != -1)
event_add(ev_udp6, NULL);
-   } else {
-   /*
-* If generic UDP file descriptors are used neither
-* for receiving nor for sending, close them.  Then
-* there is no useless *.514 in netstat.
-*/
-   if (fd_udp != -1 && !send_udp) {
-   close(fd_udp);
-   fd_udp = -1;
-   }
-   if (fd_udp6 != -1 && !send_udp6) {
-   close(fd_udp6);
-   fd_udp6 = -1;
-   }
}
for (i = 0; i < nbind; i++)
if (fd_bind[i] != -1)
@@ -2416,6 +2402,7 @@ init(void)
s = 0;
strlcpy(progblock, "*", sizeof(progblock));
strlcpy(hostblock, "*", sizeof(hostblock));
+   send_udp = send_udp6 = 0;
while (getline(&cline, &s, cf) != -1) {
/*
 * check for end-of-section, comments, strip off trailing
@@ -2508,6 +2495,22 @@ init(void)
Initialized = 1;
dropped_warn(&init_dropped, "during initialization");

+   if (SecureMode) {
+   /*
+* If generic UDP file descriptors are used neither
+* for receiving nor for sending, close them.  Then
+* there is no useless *.514 in netstat.
+*/
+   if (fd_udp != -1 && !send_udp) {
+   close(fd_udp);
+   fd_udp = -1;
+   }
+   if (fd_udp6 != -1 && !send_udp6) {
+   close(fd_udp6);
+   fd_udp6 = -1;
+   }
+   }
+
if (Debug) {
SIMPLEQ_FOREACH(f, &Files, f_next) {
for (i = 0; i <= LOG_NFACILITIES; i++)
@@ -2704,20 +2707,24 @@ cfline(char *line, char *progblock, char
}
if (proto == NULL)
proto = "udp";
-   ipproto = proto;
if (strcmp(proto, "udp") == 0) {
if (fd_udp == -1)
proto = "udp6";
if (fd_udp6 == -1)
proto = "udp4";
-   ipproto = proto;
+   }
+   ipproto = proto;
+   if (strcmp(proto, "udp") == 0) {
+   send_udp = send_udp6 = 1;
} else if (strcmp(proto, "udp4") == 0) {
+   send_udp = 1;
if (fd_udp == -1) {
log_warnx("no udp4 \"%s\"",
f->f_un.f_forw.f_loghost);
break;
}
} else if (strcmp(proto, "udp6") == 0) {
+   send_udp6 = 1;
if (fd_udp6 == -1) {
log_warnx("no udp6 \"%s\"",
f->f_un.f_forw.f_loghost);
@@ -2761,11 +2768,9 @@ cfline(char *line, char *progblock, char

Re: userland clock_gettime proof of concept

[Paul Irofti]

Hi,

Here is another iteration of the diff. It addresses some of the issues
and opens a discussion for ohers.

Fixed.

  - find_timekeep() called 3 times, call only once (deraadt@)
 -> this was leftover code, removed from everywhere except the
wrapper

  - atomic read time (deraadt@)
 o generation mechanism like the timehands (kettenis@)
   SOLUTION:
  kernel: gen mechanism is already used by the functions called,
  added seq timekeep counter
  userland: added gen-like mechanism based on seq
   PROBLEMS:
  might need to change a bit  after high-res clock addition

  - versioning mechanism for shared page (kettenis@)
-> added two bytes for major and minor at page start

  - structure may need _ or __ to avoid potential collision (deraadt@)
-> prefixed with __ as found in other sys headers

Discussions.

  - /sbin/init init_main.c!start_init() map page? (deraadt@)
-> that is not the problem, the page should be mapped even there
   by the sys_execve() call

  - bikesheding
o struct timekeep naming (deraadt@)
o AT_TIMEKEEP at 2000 (kettenis@)
-> I will leave that for last

  - I do not understand the endianess issues when copying structs but
that might no longer be part of the next diff, see bellow (deraadt@)

Main topic.

High resolution time sources are mandatory with this diff (deraadt@)
and CLOCK_MONOTONIC > nanouptime > binuptime > bintemaddfrac >
tc_delta(th) should be reproduced in userland (kettenis@).
 
I know about the need for high resolution time sources, I was the one
that added back TSC after it was disabled in favor of HPET last year...

The way I see it, we would need to either duplicate or expose some of
the microtime(9) functions to userland plus the CPU skews in order to
achieve our goal.

Basically we would replace the timespec structs in the shared page with
a timehands struct and get that updated through tc_windup() and then
proceed to adjust based on rdtsc offsets until we get the next update.

This will require a bit of tweaking to the current "locking" mechanism,
but I think that will be done quickly.

Does that sound good? Do you want it done another way?

Thank you for the feedback so far,
Paul


diff --git lib/libc/asr/asr.c lib/libc/asr/asr.c
index cd056c85719..2b25d49f32a 100644
--- lib/libc/asr/asr.c
+++ lib/libc/asr/asr.c
@@ -196,11 +196,11 @@ poll_intrsafe(struct pollfd *fds, nfds_t nfds, int 
timeout)
struct timespec pollstart, pollend, elapsed;
int r;
 
-   if (clock_gettime(CLOCK_MONOTONIC, &pollstart))
+   if (WRAP(clock_gettime)(CLOCK_MONOTONIC, &pollstart))
return -1;
 
while ((r = poll(fds, 1, timeout)) == -1 && errno == EINTR) {
-   if (clock_gettime(CLOCK_MONOTONIC, &pollend))
+   if (WRAP(clock_gettime)(CLOCK_MONOTONIC, &pollend))
return -1;
timespecsub(&pollend, &pollstart, &elapsed);
timeout -= elapsed.tv_sec * 1000 + elapsed.tv_nsec / 100;
@@ -418,7 +418,7 @@ asr_check_reload(struct asr *asr)
asr->a_rtime = 0;
}
 
-   if (clock_gettime(CLOCK_MONOTONIC, &ts) == -1)
+   if (WRAP(clock_gettime)(CLOCK_MONOTONIC, &ts) == -1)
return;
 
if ((ts.tv_sec - asr->a_rtime) < RELOAD_DELAY && asr->a_rtime != 0)
diff --git lib/libc/crypt/bcrypt.c lib/libc/crypt/bcrypt.c
index 82de8fa33b7..02fd3013cc1 100644
--- lib/libc/crypt/bcrypt.c
+++ lib/libc/crypt/bcrypt.c
@@ -248,9 +248,9 @@ _bcrypt_autorounds(void)
char buf[_PASSWORD_LEN];
int duration;
 
-   clock_gettime(CLOCK_THREAD_CPUTIME_ID, &before);
+   WRAP(clock_gettime)(CLOCK_THREAD_CPUTIME_ID, &before);
bcrypt_newhash("testpassword", r, buf, sizeof(buf));
-   clock_gettime(CLOCK_THREAD_CPUTIME_ID, &after);
+   WRAP(clock_gettime)(CLOCK_THREAD_CPUTIME_ID, &after);
 
duration = after.tv_sec - before.tv_sec;
duration *= 100;
diff --git lib/libc/gen/times.c lib/libc/gen/times.c
index 02e4dd44b5c..36841810d1b 100644
--- lib/libc/gen/times.c
+++ lib/libc/gen/times.c
@@ -52,7 +52,7 @@ times(struct tms *tp)
return ((clock_t)-1);
tp->tms_cutime = CONVTCK(ru.ru_utime);
tp->tms_cstime = CONVTCK(ru.ru_stime);
-   if (clock_gettime(CLOCK_MONOTONIC, &ts) == -1)
+   if (WRAP(clock_gettime)(CLOCK_MONOTONIC, &ts) == -1)
return ((clock_t)-1);
return (ts.tv_sec * CLK_TCK + ts.tv_nsec / (10 / CLK_TCK));
 }
diff --git lib/libc/gen/timespec_get.c lib/libc/gen/timespec_get.c
index 520a5954025..845cbe80356 100644
--- lib/libc/gen/timespec_get.c
+++ lib/libc/gen/timespec_get.c
@@ -37,7 +37,7 @@ timespec_get(struct timespec *ts, int base)
 {
switch (base) {
case TIME_UTC:
-   if (clock_gettime(CLOCK_REALTIME, ts) == -1)
+   if (WRAP(clock_gettime)(CLOCK_REALTIME, ts) == -1)
retu

Re: locale thread support

[Marc Espie]

On Fri, May 22, 2020 at 02:31:38PM +0200, Mark Kettenis wrote:
> > Date: Fri, 22 May 2020 13:50:44 +0200
> > From: Marc Espie 
> > 
> > I've had to neuter some setlocale() in mlt, since our code is
> > definitely NOT thread-safe.  No biggie, since we do not have support
> > for LC_NUMERIC right now.
> 
> Hmm, setlocale() is explicitly mentionded as not thread-safe in POSIX.
> 
> > I think we might want the thread-specific functions, namely stuff like
> > strtod_l, or sprintf_l  and friends.
> 
> That is .  which isn't actually standardized.  But there is
> a de-facto standard set by Apple.  Most OSes have an implementation by
> now and our libc++ needs it (and uses a stub implementation for now).
> The Darwin and/or FreeBSD man pages are probably the best
> documentation for these interfaces.
> 
> > Even if they do not do anything specific right now with a locale object,
> > at least they would prevent re-entrency issues.
> 
> Yes, implementing these shouldn't be hard.  We already have wctype_l()
> and iswctype_l() (which are standardized) which do care about the
> locale.  But everything else probably doesn't so these functions can
> be simple wrappers around the non-_l functions that simply ignore the
> locale.  It's just work but really doesn't require sepcific skills.
> I'm sure Ingo will be able to give some constructive feedback once a
> diff exists.

Looks like for the header part we can mostly borrow from FreeBSD, and I gather
the libc part shouldn't be that hard (minus locale support of course).

I'd wait to see what Ingo thinks about that.

Re: vmm timer for linux guests

[Renato Aguiar]

Hi Sivaram and David,

Here is my patch for Linux 4.19: 
https://gitlab.com/renatoaguiar/linux/-/commit/325afccf1e2d156be745f811411327e22bbd4c20


I also tried same patch with Linux 5.4 
, 
but for some reason 4.19 seemed more stable.


Regards,

--
Renato Aguiar

Re: fix pppx(4) with net/ifq.c rev 1.38

[Vitaliy Makkoveev]

On Fri, May 22, 2020 at 07:57:13AM +1000, David Gwynne wrote:
> On Wed, May 20, 2020 at 05:42:35PM +0300, Vitaliy Makkoveev wrote:
> > I got splassert with pppx(4) and net/ifq.c rev 1.38 raised by
> > NET_ASSERT_LOCKED() in netinet/ip_output.c:113 and underlaying routines.
> > 
> > net/ifq.c rev 1.38 is not in snapshot yet so you need to checkout and
> > build kernel to reproduce.
> > 
> >  dmesg begin 
> > 
> > splassert: ip_output: want 2 have 0
> > Starting stack trace...
> > ip_output(fd801f8c8800,0,0,0,0,0) at ip_output+0x8f
> > pipex_l2tp_output(fd801f8c8800,8e787808) at
> > pipex_l2tp_output+0x21d
> > pipex_ppp_output(fd801f8c8800,8e787808,21) at
> > pipex_ppp_output+0xda
> > pppx_if_start(8e787268) at pppx_if_start+0x83
> > if_qstart_compat(8e7874e0) at if_qstart_compat+0x2e
> > ifq_serialize(8e7874e0,8e7875b0) at ifq_serialize+0x103
> > taskq_thread(81f3df30) at taskq_thread+0x4d
> > end trace frame: 0x0, count: 250
> > End of stack trace.
> > splassert: ipsp_spd_lookup: want 2 have 0
> > Starting stack trace...
> > ipsp_spd_lookup(fd801f8c8800,2,14,8e726c9c,2,0) at
> > ipsp_spd_lookup+0x80
> > ip_output_ipsec_lookup(fd801f8c8800,14,8e726c9c,0,1) at
> > ip_output_ipsec_lookup+0x4d
> > ip_output(fd801f8c8800,0,0,0,0,0) at ip_output+0x4fa
> > pipex_l2tp_output(fd801f8c8800,8e787808) at
> > pipex_l2tp_output+0x21d
> > pipex_ppp_output(fd801f8c8800,8e787808,21) at
> > pipex_ppp_output+0xda
> > pppx_if_start(8e787268) at pppx_if_start+0x83
> > if_qstart_compat(8e7874e0) at if_qstart_compat+0x2e
> > ifq_serialize(8e7874e0,8e7875b0) at ifq_serialize+0x103
> > taskq_thread(81f3df30) at taskq_thread+0x4d
> > end trace frame: 0x0, count: 248
> > End of stack trace.
> > splassert: spd_table_get: want 2 have 0
> > 
> >  dmesg end 
> > 
> > 1. `pxi_if' owned by struct pppx_if has IFXF_MPSAFE flag unset
> >   1.1 pppx(4) sets IFQ_SET_MAXLEN(&ifp->if_snd, 1) at net/if_pppx.c:866
> > 2. pppx_if_output() is called under NET_LOCK()
> > 3. pppx_if_output() calls if_enqueue() at net/if_pppx.c:1123
> > 4. pppx(4) doesn't set `ifp->if_enqueue' so if_enqueue() calls
> > if_enqueue_ifq() at net/if.c:709 (which is set in net/if.c:639)
> > 5. if_enqueue_ifq() calls ifq_start() at net/if.c:734
> > 6. ifq_start() we a still under NET_LOCK() here
> > 
> > 6.a. in net/ifq.c rev 1.37 ifq_start() checks "ifq_len(ifq) >=
> > min(ifp->if_txmit, ifq->ifq_maxlen)" and this was always true because
> > (1.1) so we always call ifq_run_start() which calls ifq_serialize().
> > 
> > ifq_serialize() will call if_qstart_compat() which calls
> > pppx_if_start() which calls pipex_ppp_output() etc while we still
> > holding NET_LOCK() so the assertions I reported above are not raised.
> > 
> > 6.b. net/ifq.c rev 1.38 introduce checks of IFXF_MPSAFE flag. so we are
> > always going to net/ifq.c:132 where we adding out task to `systq'
> > referenced by `ifq_softnet' (ifq_softnet set to `systq' at
> > net/ifq.c:199).
> > 
> > taskq_thread() doesn't grab NET_LOCK() so after net/ifq.c rev 1.38
> > ifq_serialize() and underlaying pppx_if_start() call without NET_LOCK()
> > and corresponding asserts raised.
> > 
> > The problem I pointed is not in net/ifq.c rev 1.38 but in pppx(4).
> > `if_start' routines should grab NET_LOCK() by themself if it is required
> > but pppx_if_start() and pppac_start() did't do that. pppac_start() has
> > no underlaying NET_ASSERT_LOCKED() so the pppx(4) is the only case were
> > the problem is shown.
> > 
> > Since NET_LOCK() is required by pipex(4), diff below adds it to
> > pppx_if_start() and pppac_start().
> > 
> > After net/ifq.c rev 1.38 pppx_if_start() will newer be called from
> > pppx_if_output() but from `systq' only so I don't add lock/unlock
> > dances around if_enqueue() at net/if_pppx.c:1123.
> > 
> > Diff tested for both pppx(4) and pppac(4) cases.
> 
> thanks for the detailed analysis. i wondered how the ifq change
> triggered this exactly, and your mail makes it clear.
> 
> however, pppx/pppac/pipex are not the first or only drivers in the tree
> that encapsulate a packet in IP from their if_start routine and send it
> out with the network stack. the way this has been solved in every other
> driver has been to call ip{,6}_send to transmit the packet instead
> of ip{,6}_output.
> 
> can you try the following diff?
>

I tested this diff and it works for me. But the problem I pointed is
about pipex(4) locking.

pipex(4) requires NET_LOCK() be grabbed not only for underlaying
ip{,6}_output() but for itself too. But since pppac_start() has
unpredictable behavior I suggested to make it predictable [1].

With net/ifq.c rev 1.37 and rev 1.39 pppx_if_start() routine will be
allways called under NET_LOCK(), but pppac_start() will not. It depends
on packet count stored in `if_snd' (look at net/ifq.c:125).

With net/ifq.c rev 1.38 pppx_if

Re: locale thread support

[Marc Espie]

On Fri, May 22, 2020 at 04:02:31PM +0200, Mark Kettenis wrote:
> > From: "Todd C. Miller" 
> > Date: Fri, 22 May 2020 07:23:55 -0600
> > 
> > On Fri, 22 May 2020 14:57:11 +0200, Marc Espie wrote:
> > 
> > > From a security standpoint, is there a "cheap" way to make setlocale 
> > > abort()
> > > instead of trying to do a double free on  when running in a race 
> > > condition ?
> > 
> > We could use _THREAD_PRIVATE_MUTEX as we do in other parts of libc.
> 
> That might eliminate two threads racing eachoither in setlocale(), but
> it wouldn't stop threads that actually access the locale from
> use-after-free type bugs.  Unless you use the lock there as well.  But
> that could have a major performance impact.

Well, I'd be happy to have an abort() in case a race is detected.

As we said, this is 100% unsupported behavior! but it's reasonably hard to 
detect.

Maybe a simple way to poison setlocale is it's called from two different threads
in a given process ?... I have zero idea.

My point is NOT to make broken code work, but to make it obvious it's broken!
because right now, it's a sporadic race... and even though this is complicated, 
I
fear it's a security issue waiting to happen.

Re: locale thread support

[Mark Kettenis]

> From: "Todd C. Miller" 
> Date: Fri, 22 May 2020 07:23:55 -0600
> 
> On Fri, 22 May 2020 14:57:11 +0200, Marc Espie wrote:
> 
> > From a security standpoint, is there a "cheap" way to make setlocale abort()
> > instead of trying to do a double free on  when running in a race condition ?
> 
> We could use _THREAD_PRIVATE_MUTEX as we do in other parts of libc.

That might eliminate two threads racing eachoither in setlocale(), but
it wouldn't stop threads that actually access the locale from
use-after-free type bugs.  Unless you use the lock there as well.  But
that could have a major performance impact.

Re: Correcty reloading unresolved host in syslogd @Conf lines

[Todd C . Miller]

On Fri, 22 May 2020 12:08:28 +0200, Alexander Bluhm wrote:

> On Wed, May 20, 2020 at 09:29:54PM -0400, sven falempin wrote:
> > ? Will it goes into base this time ?
>
> I need an OK from a developer.  Anyone?

I'm a little confused by the protocol handling in cfline.

if (strcmp(proto, "udp") == 0) {
if (fd_udp == -1)
proto = "udp6";
if (fd_udp6 == -1)
proto = "udp4";
ipproto = proto;
}

Doesn't that mean that in the default case if a syslog server is
not reachable, proto will end up being set to "udp4" and not "udp"?
If so, then your diff will only retry udp4 on SIGHUP instead of
both udp4 and udp6.

 - todd

Re: locale thread support

[Todd C . Miller]

On Fri, 22 May 2020 14:57:11 +0200, Marc Espie wrote:

> From a security standpoint, is there a "cheap" way to make setlocale abort()
> instead of trying to do a double free on  when running in a race condition ?

We could use _THREAD_PRIVATE_MUTEX as we do in other parts of libc.

 - todd

Re: vmm timer for linux guests

[Sivaram Gowkanapalli]

Hello Dave,

Thanks for sharing this. I have been using your hacked up version of 
virtio_vmmci to keep the timer in sync all this time.

btw, would you recommend a linux distro other than alpine for vmm? I need to 
run an Oracle Java app which needs glibc, hence, I cannot use alpine. I tried 
tiny core but had trouble with the install in my initial attempts. I have not 
dug into the issue yet. Just want to check if there is a better option instead.

I am using devuan and it has been working fine in the vm.

Thanks again

Re: locale thread support

[Marc Espie]

On Fri, May 22, 2020 at 02:31:38PM +0200, Mark Kettenis wrote:
> > Date: Fri, 22 May 2020 13:50:44 +0200
> > From: Marc Espie 
> > 
> > I've had to neuter some setlocale() in mlt, since our code is
> > definitely NOT thread-safe.  No biggie, since we do not have support
> > for LC_NUMERIC right now.
> 
> Hmm, setlocale() is explicitly mentionded as not thread-safe in POSIX.

>From a security standpoint, is there a "cheap" way to make setlocale abort()
instead of trying to do a double free on  when running in a race condition ?

> > I think we might want the thread-specific functions, namely stuff like
> > strtod_l, or sprintf_l  and friends.
> 
> That is .  which isn't actually standardized.  But there is
> a de-facto standard set by Apple.  Most OSes have an implementation by
> now and our libc++ needs it (and uses a stub implementation for now).
> The Darwin and/or FreeBSD man pages are probably the best
> documentation for these interfaces.
> 
> > Even if they do not do anything specific right now with a locale object,
> > at least they would prevent re-entrency issues.
> 
> Yes, implementing these shouldn't be hard.  We already have wctype_l()
> and iswctype_l() (which are standardized) which do care about the
> locale.  But everything else probably doesn't so these functions can
> be simple wrappers around the non-_l functions that simply ignore the
> locale.  It's just work but really doesn't require sepcific skills.
> I'm sure Ingo will be able to give some constructive feedback once a
> diff exists.


> > Note that the current issue is a time-bomb, because it is a race, so
> > it does only manifest itself as a double free in freegl...
> > 
> > uselocale is fine, but it is not on windows, so highly portable code
> > tends to prefer strtod_l...
> 
> Which is funny since strtod_l is even less standard than uselocale() ;).

Well, I had a look at the current code, they changed it yet again, so it's
probably going to fail in newer, more funny ways.

Re: locale thread support

[Mark Kettenis]

> Date: Fri, 22 May 2020 13:50:44 +0200
> From: Marc Espie 
> 
> I've had to neuter some setlocale() in mlt, since our code is
> definitely NOT thread-safe.  No biggie, since we do not have support
> for LC_NUMERIC right now.

Hmm, setlocale() is explicitly mentionded as not thread-safe in POSIX.

> I think we might want the thread-specific functions, namely stuff like
> strtod_l, or sprintf_l  and friends.

That is .  which isn't actually standardized.  But there is
a de-facto standard set by Apple.  Most OSes have an implementation by
now and our libc++ needs it (and uses a stub implementation for now).
The Darwin and/or FreeBSD man pages are probably the best
documentation for these interfaces.

> Even if they do not do anything specific right now with a locale object,
> at least they would prevent re-entrency issues.

Yes, implementing these shouldn't be hard.  We already have wctype_l()
and iswctype_l() (which are standardized) which do care about the
locale.  But everything else probably doesn't so these functions can
be simple wrappers around the non-_l functions that simply ignore the
locale.  It's just work but really doesn't require sepcific skills.
I'm sure Ingo will be able to give some constructive feedback once a
diff exists.

> Note that the current issue is a time-bomb, because it is a race, so
> it does only manifest itself as a double free in freegl...
> 
> uselocale is fine, but it is not on windows, so highly portable code
> tends to prefer strtod_l...

Which is funny since strtod_l is even less standard than uselocale() ;).

Re: locale thread support

[Joerg Sonnenberger]

On Fri, May 22, 2020 at 01:50:44PM +0200, Marc Espie wrote:
> uselocale is fine, but it is not on windows, so highly portable code tends to
> prefer strtod_l...

The problem with uselocale is two fold and why I explicitly decided to not
implement it in NetBSD:
(1) It can come at a significant performance cost as all locale-aware
operations need to access thread-specific memory. While that is moderate
fast on x86, other architectures are not so nice. Even on x86, it can
easily result a factor of two or three for things like the ctype.h
macros.
(2) It can easily POLA for code that different threads have differnt
locales. There is a lot of existing code written that don't know how to
deal with.

Add that the explicit *_l functions are the preferred mechanism for
implementing the primitives in higher languages and it becomes a
no-brainer.

Joerg

locale thread support

[Marc Espie]

I've had to neuter some setlocale() in mlt, since our code is definitely NOT
thread-safe.  No biggie, since we do not have support for LC_NUMERIC right now.

I think we might want the thread-specific functions, namely stuff like
strtod_l, or sprintf_l  and friends.

Even if they do not do anything specific right now with a locale object,
at least they would prevent re-entrency issues.

Note that the current issue is a time-bomb, because it is a race, so it does 
only
manifest itself as  a double free in freegl...


uselocale is fine, but it is not on windows, so highly portable code tends to
prefer strtod_l...

Re: vmm timer for linux guests

[Dave Voutila]

On Fri, May 22, 2020 at 6:15 AM Pratik Vyas  wrote:
> Hi Sivaram and Renato,
>
> I have been looking at this issue this week with Dave.  (funnily Dave
> and I independently also decided to write a linux patch to attach
> pvclock and debugging these crashes).

My hacky "vmm-clock" implementation is here if you'd like to compare
notes: https://git.sr.ht/~voutilad/linux/log/linux-5.4-obsd

My goal is to strip down the implementation to the bare minimum needed
for an OpenBSD guest similar to what I did with my vmmci(4)
implementation: https://github.com/voutilad/virtio_vmmci

One thing I've found is (at least with busybox's top(1)), the
accounting for cpu utilization is wrong if you just wire up the
kvm-clock clocksource.

>
> If you have your linux vm with kvm-clock around, can you try this rather
> crude patch and see if it still crashes?  This might also fix the '100%
> cpu when using alpine via console' problem.

>From my quick testing, with both the standard Linux 5.4 kernel with
Alpine 3.11 and my 5.4.40 fork, it resolves the CPU utilization issue
from the serial console, but doesn't resolve the serial console lockup
when spamming the return key. I haven't dug into why yet.

> For ref, this is my linux side patch to attach kvm-clock http://ix.io/2lzK
>
> @@ -96,7 +103,7 @@ ns8250_init(int fd, uint32_t vmid)
>  */
> com1_dev.pause_ct = (com1_dev.baudrate / 8) / 1000 * 10;
>
> -   event_set(&com1_dev.event, com1_dev.fd, EV_READ | EV_PERSIST,
> +   event_set(&com1_dev.event, com1_dev.fd, EV_READ,
> com_rcv_event, (void *)(intptr_t)vmid);

pd@ I think you're missing the analogous code in ns8250_restore().

-- 
Dave Voutila

Re: vmm timer for linux guests

[Sivaram Gowkanapalli]

Hello Renato,

Could you please share the linux patches? We can try with them too.

Thanks


From: Renato Aguiar [ren...@renatoaguiar.net]
Sent: Thursday, May 21, 2020 3:55 PM
To: tech@openbsd.org
Cc: Sivaram Gowkanapalli
Subject: Re: vmm timer for linux guests

Hi Sivaram,

I'm the author of the e-mail thread that you mentioned. After
feedback I got from OpenBSD community, I created a patch for Linux
to enable kvm-clock when booting on VMM. It managed to keep clock
in sync, but I experienced random crashes in vmd after some time
running the VM.

Multiple fixes have been merged to vmm/vmd since then, so I'm
planning to give it another try with OpenBSD 6.7.

If you are interested, I can share the patch with you.

Regards,

--
Renato Aguiar

Re: vmm timer for linux guests

[Sivaram Gowkanapalli]

Hello Pratik,

> For ref, this is my linux side patch to attach kvm-clock http://ix.io/2lzK

Thanks, will try the below patches.

Re: diff: uvm: fix unitialized var and simplify code in km_alloc()

[Alexander Bluhm]

On Wed, May 20, 2020 at 11:44:57AM +0200, Jan Klemkow wrote:
> The function km_alloc() returns the uninitialized local variable sva if
> pgl is empty.  It seems to be not possible in the current condition of
> the code, but I'm not sure if this is guaranteed.  Thus, I would prefer
> to initialize sva with zero.

I think initializing sva to 0 is better than returning some stack
memory if something goes wrong.

> It also seems to be unnecessary to loop over the whole pagelist to find
> the first element.  The marco pmap_map_direct() just does some
> calculations and the value of va is discarded.

It is not a macro on other architectures.  You must make sure that
it has no side effects everywhere.  I would not touch this.

> I build and run the code on amd64 without any issue and regress/sys/uvm
> also doesn't show any problems with that diff.

Only testing amd64 is not enough for such a diff.

bluhm

> Index: uvm/uvm_km.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_km.c,v
> retrieving revision 1.136
> diff -u -p -r1.136 uvm_km.c
> --- uvm/uvm_km.c  18 Feb 2020 12:13:40 -  1.136
> +++ uvm/uvm_km.c  20 May 2020 06:17:41 -
> @@ -816,7 +816,7 @@ km_alloc(size_t sz, const struct kmem_va
>   paddr_t pla_align;
>   int pla_flags;
>   int pla_maxseg;
> - vaddr_t va, sva;
> + vaddr_t va, sva = 0;
>
>   KASSERT(sz == round_page(sz));
>
> @@ -851,11 +851,8 @@ km_alloc(size_t sz, const struct kmem_va
>* allocations.
>*/
>   if (kv->kv_singlepage || kp->kp_maxseg == 1) {
> - TAILQ_FOREACH(pg, &pgl, pageq) {
> - va = pmap_map_direct(pg);
> - if (pg == TAILQ_FIRST(&pgl))
> - sva = va;
> - }
> + if ((pg = TAILQ_FIRST(&pgl)) != NULL)
> + sva = pmap_map_direct(pg);
>   return ((void *)sva);
>   }
>  #endif

Re: vmm timer for linux guests

[Pratik Vyas]

* Renato Aguiar  [2020-05-21 12:55:45 -0700]:


Hi Sivaram,

I'm the author of the e-mail thread that you mentioned. After feedback 
I got from OpenBSD community, I created a patch for Linux to enable 
kvm-clock when booting on VMM. It managed to keep clock in sync, but I 
experienced random crashes in vmd after some time running the VM.


Multiple fixes have been merged to vmm/vmd since then, so I'm planning 
to give it another try with OpenBSD 6.7.


If you are interested, I can share the patch with you.

Regards,

--
Renato Aguiar



Hi Sivaram and Renato,

I have been looking at this issue this week with Dave.  (funnily Dave
and I independently also decided to write a linux patch to attach
pvclock and debugging these crashes).

If you have your linux vm with kvm-clock around, can you try this rather
crude patch and see if it still crashes?  This might also fix the '100%
cpu when using alpine via console' problem.

For ref, this is my linux side patch to attach kvm-clock http://ix.io/2lzK

--
Pratik

diff --git a/usr.sbin/vmd/ns8250.c b/usr.sbin/vmd/ns8250.c
index 497e6fad550..33f1a371c16 100644
--- a/usr.sbin/vmd/ns8250.c
+++ b/usr.sbin/vmd/ns8250.c
@@ -36,6 +36,8 @@
 
 extern char *__progname;

 struct ns8250_dev com1_dev;
+struct event read_delay_ev;
+struct timeval read_delay_tv;
 
 static void com_rcv_event(int, short, void *);

 static void com_rcv(struct ns8250_dev *, uint32_t, uint32_t);
@@ -61,6 +63,11 @@ ratelimit(int fd, short type, void *arg)
vcpu_deassert_pic_irq(com1_dev.vmid, 0, com1_dev.irq);
 }
 
+static void

+arm_read(int fd, short type, void *arg) {
+   event_add(&com1_dev.event, NULL);
+}
+
 void
 ns8250_init(int fd, uint32_t vmid)
 {
@@ -96,7 +103,7 @@ ns8250_init(int fd, uint32_t vmid)
 */
com1_dev.pause_ct = (com1_dev.baudrate / 8) / 1000 * 10;
 
-	event_set(&com1_dev.event, com1_dev.fd, EV_READ | EV_PERSIST,

+   event_set(&com1_dev.event, com1_dev.fd, EV_READ,
com_rcv_event, (void *)(intptr_t)vmid);
 
 	/*

@@ -112,6 +119,9 @@ ns8250_init(int fd, uint32_t vmid)
timerclear(&com1_dev.rate_tv);
com1_dev.rate_tv.tv_usec = 1;
evtimer_set(&com1_dev.rate, ratelimit, NULL);
+   read_delay_tv.tv_usec = 1;
+   evtimer_set(&read_delay_ev, arm_read,
+   (void *)(intptr_t)vmid);
 }
 
 static void

@@ -131,6 +141,7 @@ com_rcv_event(int fd, short kind, void *arg)
 */
if (com1_dev.rcv_pending) {
mutex_unlock(&com1_dev.mutex);
+   evtimer_add(&read_delay_ev, &read_delay_tv);
return;
}
 
@@ -146,6 +157,7 @@ com_rcv_event(int fd, short kind, void *arg)

vcpu_deassert_pic_irq((uintptr_t)arg, 0, com1_dev.irq);
}
}
+   event_add(&com1_dev.event, NULL);
 
 	mutex_unlock(&com1_dev.mutex);

 }

Re: Correcty reloading unresolved host in syslogd @Conf lines

[Alexander Bluhm]

On Wed, May 20, 2020 at 09:29:54PM -0400, sven falempin wrote:
> ? Will it goes into base this time ?

I need an OK from a developer.  Anyone?

bluhm

> On Mon, May 18, 2020 at 5:31 AM Alexander Bluhm 
> > Index: usr.sbin/syslogd/syslogd.c
> > ===
> > RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
> > retrieving revision 1.262
> > diff -u -p -r1.262 syslogd.c
> > --- usr.sbin/syslogd/syslogd.c  5 Jul 2019 13:23:27 -   1.262
> > +++ usr.sbin/syslogd/syslogd.c  9 Feb 2020 20:25:20 -
> > @@ -853,20 +853,6 @@ main(int argc, char *argv[])
> > event_add(ev_udp, NULL);
> > if (fd_udp6 != -1)
> > event_add(ev_udp6, NULL);
> > -   } else {
> > -   /*
> > -* If generic UDP file descriptors are used neither
> > -* for receiving nor for sending, close them.  Then
> > -* there is no useless *.514 in netstat.
> > -*/
> > -   if (fd_udp != -1 && !send_udp) {
> > -   close(fd_udp);
> > -   fd_udp = -1;
> > -   }
> > -   if (fd_udp6 != -1 && !send_udp6) {
> > -   close(fd_udp6);
> > -   fd_udp6 = -1;
> > -   }
> > }
> > for (i = 0; i < nbind; i++)
> > if (fd_bind[i] != -1)
> > @@ -2416,6 +2402,7 @@ init(void)
> > s = 0;
> > strlcpy(progblock, "*", sizeof(progblock));
> > strlcpy(hostblock, "*", sizeof(hostblock));
> > +   send_udp = send_udp6 = 0;
> > while (getline(&cline, &s, cf) != -1) {
> > /*
> >  * check for end-of-section, comments, strip off trailing
> > @@ -2508,6 +2495,22 @@ init(void)
> > Initialized = 1;
> > dropped_warn(&init_dropped, "during initialization");
> >
> > +   if (SecureMode) {
> > +   /*
> > +* If generic UDP file descriptors are used neither
> > +* for receiving nor for sending, close them.  Then
> > +* there is no useless *.514 in netstat.
> > +*/
> > +   if (fd_udp != -1 && !send_udp) {
> > +   close(fd_udp);
> > +   fd_udp = -1;
> > +   }
> > +   if (fd_udp6 != -1 && !send_udp6) {
> > +   close(fd_udp6);
> > +   fd_udp6 = -1;
> > +   }
> > +   }
> > +
> > if (Debug) {
> > SIMPLEQ_FOREACH(f, &Files, f_next) {
> > for (i = 0; i <= LOG_NFACILITIES; i++)
> > @@ -2755,6 +2758,13 @@ cfline(char *line, char *progblock, char
> > sizeof(f->f_un.f_forw.f_addr)) != 0) {
> > log_warnx("bad hostname \"%s\"",
> > f->f_un.f_forw.f_loghost);
> > +   /* DNS lookup may work after SIGHUP, keep sockets
> > */
> > +   if (strcmp(proto, "udp") == 0)
> > +   send_udp = send_udp6 = 1;
> > +   else if (strcmp(proto, "udp4") == 0)
> > +   send_udp = 1;
> > +   else if (strcmp(proto, "udp6") == 0)
> > +   send_udp6 = 1;
> > break;
> > }
> > f->f_file = -1;
> >

Re: amdgpio(4): acpi_attach_args resources

[Mark Kettenis]

> From: James Hastings 
> Date: Fri, 22 May 2020 00:55:16 -0400 (EDT)
> 
> stop parsing _CRS and use resources from struct acpi_attach_args.

Thanks, but I already have diffs for all the ACPI gpio drivers in my
tree, waiting for an ok from those slackers who call themselves
OpenBSD developers.

> Index: dev/acpi/amdgpio.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/amdgpio.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 amdgpio.c
> --- dev/acpi/amdgpio.c26 Jan 2020 00:11:42 -  1.2
> +++ dev/acpi/amdgpio.c21 May 2020 04:31:52 -
> @@ -55,11 +55,7 @@ struct amdgpio_softc {
>  
>   bus_space_tag_t sc_memt;
>   bus_space_handle_t sc_memh;
> - bus_addr_t sc_addr;
> - bus_size_t sc_size;
>  
> - int sc_irq;
> - int sc_irq_flags;
>   void *sc_ih;
>  
>   int sc_npins;
> @@ -85,7 +81,6 @@ const char *amdgpio_hids[] = {
>   NULL
>  };
>  
> -int  amdgpio_parse_resources(int, union acpi_resource *, void *);
>  int  amdgpio_read_pin(void *, int);
>  void amdgpio_write_pin(void *, int, int);
>  void amdgpio_intr_establish(void *, int, int, int (*)(), void *);
> @@ -106,13 +101,22 @@ amdgpio_attach(struct device *parent, st
>  {
>   struct acpi_attach_args *aaa = aux;
>   struct amdgpio_softc *sc = (struct amdgpio_softc *)self;
> - struct aml_value res;
>   int64_t uid;
>  
>   sc->sc_acpi = (struct acpi_softc *)parent;
>   sc->sc_node = aaa->aaa_node;
>   printf(": %s", sc->sc_node->name);
>  
> + if (aaa->aaa_naddr < 1) {
> + printf(", no registers\n");
> + return;
> + }
> +
> + if (aaa->aaa_nirq < 1) {
> + printf(", no interrupt\n");
> + return;
> + }
> +
>   if (aml_evalinteger(sc->sc_acpi, sc->sc_node, "_UID", 0, NULL, &uid)) {
>   printf(", can't find uid\n");
>   return;
> @@ -129,19 +133,6 @@ amdgpio_attach(struct device *parent, st
>   return;
>   }
>  
> - if (aml_evalname(sc->sc_acpi, sc->sc_node, "_CRS", 0, NULL, &res)) {
> - printf(", can't find registers\n");
> - return;
> - }
> -
> - aml_parse_resource(&res, amdgpio_parse_resources, sc);
> - aml_freevalue(&res);
> - printf(" addr 0x%lx/0x%lx", sc->sc_addr, sc->sc_size);
> - if (sc->sc_addr == 0 || sc->sc_size == 0) {
> - printf("\n");
> - return;
> - }
> -
>   sc->sc_pin_ih = mallocarray(sc->sc_npins, sizeof(*sc->sc_pin_ih),
>   M_DEVBUF, M_NOWAIT | M_ZERO);
>   if (sc->sc_pin_ih == NULL) {
> @@ -149,17 +140,18 @@ amdgpio_attach(struct device *parent, st
>   return;
>   }
>  
> - printf(" irq %d", sc->sc_irq);
> + printf(" addr 0x%llx/0x%llx", aaa->aaa_addr[0], aaa->aaa_size[0]);
> + printf(" irq %d", aaa->aaa_irq[0]);
>  
>   sc->sc_memt = aaa->aaa_memt;
> - if (bus_space_map(sc->sc_memt, sc->sc_addr, sc->sc_size, 0,
> + if (bus_space_map(sc->sc_memt, aaa->aaa_addr[0], aaa->aaa_size[0], 0,
>   &sc->sc_memh)) {
>   printf(", can't map registers\n");
>   goto free;
>   }
>  
> - sc->sc_ih = acpi_intr_establish(sc->sc_irq, sc->sc_irq_flags, IPL_BIO,
> - amdgpio_intr, sc, sc->sc_dev.dv_xname);
> + sc->sc_ih = acpi_intr_establish(aaa->aaa_irq[0], aaa->aaa_irq_flags[0],
> + IPL_BIO, amdgpio_intr, sc, sc->sc_dev.dv_xname);
>   if (sc->sc_ih == NULL) {
>   printf(", can't establish interrupt\n");
>   goto unmap;
> @@ -177,32 +169,9 @@ amdgpio_attach(struct device *parent, st
>   return;
>  
>  unmap:
> - bus_space_unmap(sc->sc_memt, sc->sc_memh, sc->sc_size);
> + bus_space_unmap(sc->sc_memt, sc->sc_memh, aaa->aaa_size[0]);
>  free:
>   free(sc->sc_pin_ih, M_DEVBUF, sc->sc_npins * sizeof(*sc->sc_pin_ih));
> -}
> -
> -int
> -amdgpio_parse_resources(int crsidx, union acpi_resource *crs, void *arg)
> -{
> - struct amdgpio_softc *sc = arg;
> - int type = AML_CRSTYPE(crs);
> -
> - switch (type) {
> - case LR_MEM32FIXED:
> - sc->sc_addr = crs->lr_m32fixed._bas;
> - sc->sc_size = crs->lr_m32fixed._len;
> - break;
> - case LR_EXTIRQ:
> - sc->sc_irq = crs->lr_extirq.irq[0];
> - sc->sc_irq_flags = crs->lr_extirq.flags;
> - break;
> - default:
> - printf(" type 0x%x\n", type);
> - break;
> - }
> -
> - return 0;
>  }
>  
>  int
> 
> 

Re: pledge(2) sndioctl(1)

[Ricardo Mestre]

Hello,

I tried to open the raw device but now it seems I was to sleepy to
figure out that I couldn't access it due to sndiod(8) having the device
opened earlier and therefore coundn't reach that code path.

Here's the audio promise added, but maybe it raises the question again
if these utilities should have direct access to the devices, and also
includes sndiod(8) not running when this is done?

ratchov@ will this change eventually? Otherwise the below keeps that
code path happy.

Index: sndioctl.c
===
RCS file: /cvs/src/usr.bin/sndioctl/sndioctl.c,v
retrieving revision 1.10
diff -u -p -u -r1.10 sndioctl.c
--- sndioctl.c  17 May 2020 05:39:32 -  1.10
+++ sndioctl.c  22 May 2020 07:01:30 -
@@ -948,6 +948,13 @@ main(int argc, char **argv)
fprintf(stderr, "%s: can't open control device\n", devname);
exit(1);
}
+
+   if (pledge("stdio audio", NULL) == -1) {
+   fprintf(stderr, "%s: pledge: %s\n", getprogname(),
+   strerror(errno));
+   exit(1);
+   }
+
if (!sioctl_ondesc(hdl, ondesc, NULL)) {
fprintf(stderr, "%s: can't get device description\n", devname);
exit(1);

On 08:25 Fri 22 May , Sebastien Marie wrote:
> On Fri, May 22, 2020 at 06:57:00AM +0200, Sebastien Marie wrote:
> > On Thu, May 21, 2020 at 11:07:39PM +0100, Ricardo Mestre wrote:
> > > Hi,
> > > 
> > > After the handle sioctl_hdl `hdl' is opened (which in itself requires rw 
> > > fs
> > > access and opening an unix socket) then all operations happen over that 
> > > handle
> > > so the program may be restricted to only "stdio".
> > > 
> > > All options were tested successfully, including the examples from the 
> > > manpage
> > > plus tweaking the audio from an app ($MYBROWSER).
> > 
> > Did you only perform runtime checking ? or also auditing the code source ?
> > 
> > Because depending your hardware and the way sndiod is configured, it isn't
> > necessary the same code path used.
> > 
> > Just one example: with device "snd/0", the sioctl_open() will use
> > _sioctl_aucat_open() for handler initialisation, whereas with "rsnd/0", it 
> > is
> > _sioctl_sun_open().
> > 
> > As all functions called after sioctl_open() are using the handler, you might
> > have tested only a part of the code path.
> > 
> > I didn't look deeper. So it might fine, but it could also break some
> > configurations.
>  
> I looked a bit deeper.
> 
> commit()
>  sioctl_setval()
>[with "rsnd/0"]
>sioctl_sun_setctl()
>  setvol()
>ioctl(hdl->fd, AUDIO_MIXER_WRITE, &ctrl)
> 
> calling ioctl(AUDIO_MIXER_WRITE) should required "audio".
> 
> whereas when using default "snd/0" device, it is calling sioctl_aucat_setctl()
> and it will write(2) a message to sndiod(8) (and "stdio" is enough).
> 
> I agree that "rsnd/0" isn't the standard case (it requires root to read/write 
> to
> /dev), but it means you will broke sndioctl(1) in such cases.
> 
> Thanks.
> -- 
> Sebastien Marie
>