ksh "clear-screen" for vi mode
Hi everyone, I would like to add a clear screen feature for the vi mode of ksh. I have seen that it has been done for the emacs mode last year and wanned to adapt that code to vi. But it isn't quite a success for now because it's hard for me to get were the clear screen operation is actually written in emacs.c and it is also probably due to the fact that I'm not really used to C. Here is a sum up of my understanding of it : It's clear that the important function is x_redraw, but more precisely according to my understanding of it, it clears the screen when x_bs is called which itself calls x_adjust and I guess that x_adjust is eventually clearing the screen, but I'm not quite sure that this is what is happening. So here are my two main questions : 1. Do you think it is a good approach? Or should I try to understand how to clear the screen by reading another code which is maybe simpler? 2. Am I understanding the clear-screen feature of emacs? Thanks in advance for all your responses, and advices, Best, Rhylx
diff: pfctl: error message for nonexisting rtable
Hi,
When pf rule with a "on rdomain n" with nonexisting rdomain n causes
/etc/pf.conf:XXX: rdomain n does not exist
error. But with a "rtable n" with nonexisting rtable n will cause
pfctl: DIOCADDRULE: Device busy
error. It is hard to find the cause by this error message.
/etc/pf.conf:XXX: rtable n does not exist
is better.
ok?
Make pfctl check if the rtable really exists when parsing the config.
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 04:54:39 -
@@ -393,6 +393,7 @@ u_int16_t parseicmpspec(char *, sa_famil
int kw_casecmp(const void *, const void *);
int map_tos(char *string, 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 +1218,10 @@ antispoof_opt: LABEL label {
yyerror("invalid rtable id");
YYERROR;
}
+ else if (rtable_exists($2) != 1) {
+ yyerror("rtable %lld does not exist", $2);
+ YYERROR;
+ }
antispoof_opts.rtableid = $2;
}
;
@@ -2001,6 +2006,10 @@ filter_opt : USER uids {
yyerror("invalid rtable id");
YYERROR;
}
+ else if (rtable_exists($2) != 1) {
+ yyerror("rtable %lld does not exist", $2);
+ YYERROR;
+ }
filter_opts.rtableid = $2;
}
| DIVERTTO STRING PORT portplain {
@@ -5899,6 +5908,36 @@ rdomain_exists(u_int rdomain)
}
/* rdomain is a table, but not an rdomain */
return 0;
+}
+
+int
+rtable_exists(u_int rtable)
+{
+ size_t len;
+ struct rt_tableinfo info;
+ int mib[6];
+ static u_int found[RT_TABLEID_MAX+1];
+
+ if (found[rtable] == 1)
+ return 1;
+
+ mib[0] = CTL_NET;
+ mib[1] = PF_ROUTE;
+ mib[2] = 0;
+ mib[3] = 0;
+ mib[4] = NET_RT_TABLE;
+ mib[5] = rtable;
+
+ len = sizeof(info);
+ if (sysctl(mib, 6, , , NULL, 0) == -1) {
+ if (errno == ENOENT) {
+ /* table nonexistent */
+ return 0;
+ }
+ err(1, "%s", __func__);
+ }
+ found[rtable] = 1;
+ return 1;
}
int
Re: smtpd: document "pki" option for relay delivery in smtpd.conf(5)
On Sun, 13 Sep 2020 20:45:35 +0800, Nick Gasson wrote: > I struggled a bit to configure smtpd to relay to a remote server that > requires SSL client certificates. The solution is to just add a "pki > host.example.org" option, but "pki" is not listed as a valid option for > the relay delivery method, even though the parser accepts it. Looks good to me. Anyone else want to OK this? - todd
Re: rtm_getifa: Never ignore RTA_IFP
On 2020-09-13 12:40, Claudio Jeker wrote: > On Sun, Sep 13, 2020 at 11:28:11AM -0400, Demi M. Obenour wrote: >> Resending without quoted-printable encoding, in case that helps. >> --- >> If an RTM_ADD command on a routing socket includes an RTA_IFA sockaddr, >> and that sockaddr is an exact match for one of the interfaces in the >> relevant routing domain, any RTA_IFP sockaddr in the same message is >> ignored. If there are multiple interfaces with the same IP address, >> this can cause packets to be sent out the wrong interface. Fix this >> by skipping the exact-match check on RTA_IFA if an RTA_IFP sockaddr >> was sent. >> >> The RTA_IFP sockaddr was also being ignored if there was no interface >> with the requested index. Return ENXIO to userspace instead. >> >> The bug can easily be seen by running the following commands as root >> on a machine where vether0 and vether1 do not exist, and on which >> the subnet 192.0.2.0/24 (reserved for documentation) is not in use: >> >> # ifconfig vether0 destroy 2>/dev/null >> # ifconfig vether1 destroy 2>/dev/null >> # dummy_mac=fe:ff:ff:ff:ff:ff dummy_ip=192.0.2.5 >> # ifconfig vether0 create lladdr "$dummy_mac" >> # ifconfig vether1 create lladdr "$dummy_mac" >> # ifconfig vether0 inet "$dummy_ip" prefixlen 32 >> # route -n delete "$dummy_ip/32" "$dummy_ip" >> # ifconfig vether1 inet "$dummy_ip" prefixlen 32 >> # route -n delete "$dummy_ip/32" "$dummy_ip" >> # route -n add -inet 192.0.2.6 -static -iface -llinfo -link \ >> vether1 -ifp vether1 -inet -ifa "$dummy_ip" >> # route -n show -inet >> >> On a system with an unpatched kernel, the final command will show >> that the route to 192.0.2.6 is via vether0. If this patch has been >> applied, the route to 192.0.2.6 will be (correctly) via vether1. >> > > Can you please explain why you think this fix is needed. Looking at the > code above my first reaction is don't do that. The configuration with the > same IP on multiple interface you do is something I would generally not > recommend. > It seems like you try to do a poor mans link aggregation and should > instead use trunk(4) or aggr(4). Thanks for your response. Indeed, having the same IP address on multiple interfaces is not (contrary to what I thought at first) common practice, so I will explain why I want to support it. The short answer is that when OpenBSD is used as a layer 3 IPv4 router with a significant number of ports, it is far simpler to assign a single IP to the OpenBSD machine, rather than having to assign a separate subnet for each connection. Since there is technically one network per interface, using the same IP address for all of the ports saves a significant number of IP addresses. With one IP address per machine, each machine only needs to be assigned a single IP address, no matter how many machines it is connected to. Furthermore, this address does not depend on what machines the router is connected to, which makes automatic IP address assignment much simpler. The long answer involves a discussion of QubesOS network topology, and I have omitted it in the interest of brevity. However, I would be more than happy to include it upon request. On Linux, forcing a route to use a particular interface and source address is trivial: ``` # ip route replace to "$subnet" dev "$iface" src "$src" ``` However, I was not able to find an OpenBSD equivalent for that command. The closest I was able to find was ``` route -n add -iface -inet -static "$subnet" -link "$iface" \ -ifp "$iface" -ifa "$src" ``` but it doesn’t work if there are multiple interfaces that have $src as their address. It is also worth noting that I was actually not using route(8) directly, but rather using a custom C program and routing sockets; this allowed me to specify the destination MAC address at the same time. As it happens, `route -n add -iface -inet -static "$subnet" -link "$iface"` works. However, it would not be sufficient if there were multiple IP addresses assigned to $iface and I needed to select which one to use. It also doesn’t let me specify a MAC address, although I might be able to do so in C. Finally, when I pass `-ifp "$iface"` to route(8), I expect that the route will go through $iface. If the kernel cannot ensure this, it should return an error to userspace, rather than silently ignoring what I told it to do. Thanks for your time and attention. Sincerely, Demi
Re: go/rust vs uvm_map_inentry()
On Sun, Sep 13, 2020 at 04:49:48PM +0200, Sebastien Marie wrote:
> On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> > I'm no longer able to reproduce the corruption while building lang/go
> > with the diff below. Something relevant to threading change in go since
> > march?
> >
> > Can someone try this diff and tell me if go and/or rust still fail?
>
> quickly tested with rustc build (nightly here), and it is failing at random
> places (not always at the same) with memory errors (signal 11, compiler ICE
> signal 6...)
>
A first hint.
With the help of deraadt@, it was found that disabling
uvm_map_inentry() call in usertrap() is enough to avoid the crashes.
To be clear, I am using the following diff:
diff 3e16148d8fe176d83ff415f6c03a79618da4401e /data/semarie/repos/openbsd/src
blob - 7f195a5309280943e0138953c61fffcb6a80c6bf
file + sys/arch/amd64/conf/GENERIC.MP
--- sys/arch/amd64/conf/GENERIC.MP
+++ sys/arch/amd64/conf/GENERIC.MP
@@ -4,6 +4,8 @@ include "arch/amd64/conf/GENERIC"
option MULTIPROCESSOR
#optionMP_LOCKDEBUG
-#optionWITNESS
+option WITNESS
+
+pseudo-device dt
cpu* at mainbus?
blob - fc23bc67e305a1a1edc7d6f08ecb982dccdc4a45
file + sys/uvm/uvm_map.c
--- sys/uvm/uvm_map.c
+++ sys/uvm/uvm_map.c
@@ -1893,16 +1893,16 @@ uvm_map_inentry(struct proc *p, struct p_inentry *ie,
boolean_t ok = TRUE;
if (uvm_map_inentry_recheck(serial, addr, ie)) {
- KERNEL_LOCK();
ok = uvm_map_inentry_fix(p, ie, addr, fn, serial);
if (!ok) {
+ KERNEL_LOCK();
printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid,
addr, ie->ie_start, ie->ie_end);
p->p_p->ps_acflag |= AMAP;
sv.sival_ptr = (void *)PROC_PC(p);
trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv);
+ KERNEL_UNLOCK();
}
- KERNEL_UNLOCK();
}
return (ok);
}
blob - 4a4c6275aa766fe2e4f5c9d913d1257f41a9d578
file + sys/arch/amd64/amd64/trap.c
--- sys/arch/amd64/amd64/trap.c
+++ sys/arch/amd64/amd64/trap.c
@@ -343,10 +343,12 @@ usertrap(struct trapframe *frame)
p->p_md.md_regs = frame;
refreshcreds(p);
+#if 0
if (!uvm_map_inentry(p, >p_spinentry, PROC_STACK(p),
"[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
uvm_map_inentry_sp, p->p_vmspace->vm_map.sserial))
goto out;
+#endif
switch (type) {
case T_PROTFLT: /* protection fault */
Thanks.
--
Sebastien Marie
Re: trunk: keep interface up on port removal
On 2020/09/13 14:47, Klemens Nanni wrote:
> 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:
This was in 2015 btw.
> 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?
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.
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.
Re: rtm_getifa: Never ignore RTA_IFP
On Sun, Sep 13, 2020 at 11:28:11AM -0400, Demi M. Obenour wrote:
> Resending without quoted-printable encoding, in case that helps.
> ---
> If an RTM_ADD command on a routing socket includes an RTA_IFA sockaddr,
> and that sockaddr is an exact match for one of the interfaces in the
> relevant routing domain, any RTA_IFP sockaddr in the same message is
> ignored. If there are multiple interfaces with the same IP address,
> this can cause packets to be sent out the wrong interface. Fix this
> by skipping the exact-match check on RTA_IFA if an RTA_IFP sockaddr
> was sent.
>
> The RTA_IFP sockaddr was also being ignored if there was no interface
> with the requested index. Return ENXIO to userspace instead.
>
> The bug can easily be seen by running the following commands as root
> on a machine where vether0 and vether1 do not exist, and on which
> the subnet 192.0.2.0/24 (reserved for documentation) is not in use:
>
> # ifconfig vether0 destroy 2>/dev/null
> # ifconfig vether1 destroy 2>/dev/null
> # dummy_mac=fe:ff:ff:ff:ff:ff dummy_ip=192.0.2.5
> # ifconfig vether0 create lladdr "$dummy_mac"
> # ifconfig vether1 create lladdr "$dummy_mac"
> # ifconfig vether0 inet "$dummy_ip" prefixlen 32
> # route -n delete "$dummy_ip/32" "$dummy_ip"
> # ifconfig vether1 inet "$dummy_ip" prefixlen 32
> # route -n delete "$dummy_ip/32" "$dummy_ip"
> # route -n add -inet 192.0.2.6 -static -iface -llinfo -link \
> vether1 -ifp vether1 -inet -ifa "$dummy_ip"
> # route -n show -inet
>
> On a system with an unpatched kernel, the final command will show
> that the route to 192.0.2.6 is via vether0. If this patch has been
> applied, the route to 192.0.2.6 will be (correctly) via vether1.
>
Can you please explain why you think this fix is needed. Looking at the
code above my first reaction is don't do that. The configuration with the
same IP on multiple interface you do is something I would generally not
recommend.
It seems like you try to do a poor mans link aggregation and should
instead use trunk(4) or aggr(4).
> ---
> sys/net/rtsock.c | 26 --
> 1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git sys/net/rtsock.c sys/net/rtsock.c
> index fa84ddc25e5..dc8446bd78f 100644
> --- sys/net/rtsock.c
> +++ sys/net/rtsock.c
> @@ -1235,7 +1235,8 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
> struct sockaddr_dl *sdl;
>
> sdl = satosdl(info->rti_info[RTAX_IFP]);
> - ifp = if_get(sdl->sdl_index);
> + if ((ifp = if_get(sdl->sdl_index)) == NULL)
> + return (ENXIO);
> }
>
> #ifdef IPSEC
> @@ -1246,11 +1247,19 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int
> rtid)
>* enc0.
>*/
> if (info->rti_info[RTAX_DST] &&
> - info->rti_info[RTAX_DST]->sa_family == PF_KEY)
> + info->rti_info[RTAX_DST]->sa_family == PF_KEY) {
> info->rti_ifa = enc_getifa(rtid, 0);
> +
> + if (info->rti_ifa != NULL && ifp != NULL &&
> + ifp != info->rti_ifa->ifa_ifp) {
> + if_put(ifp);
> + return (EADDRNOTAVAIL);
> + }
> + }
> #endif
>
> - if (info->rti_ifa == NULL && info->rti_info[RTAX_IFA] != NULL)
> + if (info->rti_ifa == NULL && ifp == NULL &&
> + info->rti_info[RTAX_IFA] != NULL)
> info->rti_ifa = ifa_ifwithaddr(info->rti_info[RTAX_IFA], rtid);
>
> if (info->rti_ifa == NULL) {
> @@ -1273,10 +1282,15 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int
> rtid)
> sa, sa, rtid);
> }
>
> - if_put(ifp);
> -
> - if (info->rti_ifa == NULL)
> + if (info->rti_ifa == NULL) {
> + if_put(ifp);
> return (ENETUNREACH);
> + }
> +
> + if (ifp != NULL && ifp != info->rti_ifa->ifa_ifp)
> + panic("rtm_getifa: returned route to wrong interface");
> +
> + if_put(ifp);
>
> return (0);
> }
>
>
>
--
:wq Claudio
Re: go/rust vs uvm_map_inentry()
> Date: Sun, 13 Sep 2020 17:54:18 +0200 > From: Martin Pieuchot > > On 13/09/20(Sun) 16:54, Mark Kettenis wrote: > > > Date: Sun, 13 Sep 2020 16:49:48 +0200 > > > From: Sebastien Marie > > > > > > On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote: > > > > I'm no longer able to reproduce the corruption while building lang/go > > > > with the diff below. Something relevant to threading change in go since > > > > march? > > > > > > > > Can someone try this diff and tell me if go and/or rust still fail? > > > > > > quickly tested with rustc build (nightly here), and it is failing at > > > random places (not always at the same) with memory errors (signal > > > 11, compiler ICE signal 6...) > > > > Is it failing when you don't have tracing enabled and not failing when > > the tracing is disabled perhaps? > > It is failing even without tracing. Sorry, I meant is it failing even with tracing?
Re: go/rust vs uvm_map_inentry()
On Sun, Sep 13, 2020 at 09:15:15AM -0600, Theo de Raadt wrote: > crashes -- but without any kernel printfs? crashes and no kernel printfs -- Sebastien Marie
Re: go/rust vs uvm_map_inentry()
On 13/09/20(Sun) 16:54, Mark Kettenis wrote: > > Date: Sun, 13 Sep 2020 16:49:48 +0200 > > From: Sebastien Marie > > > > On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote: > > > I'm no longer able to reproduce the corruption while building lang/go > > > with the diff below. Something relevant to threading change in go since > > > march? > > > > > > Can someone try this diff and tell me if go and/or rust still fail? > > > > quickly tested with rustc build (nightly here), and it is failing at > > random places (not always at the same) with memory errors (signal > > 11, compiler ICE signal 6...) > > Is it failing when you don't have tracing enabled and not failing when > the tracing is disabled perhaps? It is failing even without tracing.
rtm_getifa: Never ignore RTA_IFP
Resending without quoted-printable encoding, in case that helps.
---
If an RTM_ADD command on a routing socket includes an RTA_IFA sockaddr,
and that sockaddr is an exact match for one of the interfaces in the
relevant routing domain, any RTA_IFP sockaddr in the same message is
ignored. If there are multiple interfaces with the same IP address,
this can cause packets to be sent out the wrong interface. Fix this
by skipping the exact-match check on RTA_IFA if an RTA_IFP sockaddr
was sent.
The RTA_IFP sockaddr was also being ignored if there was no interface
with the requested index. Return ENXIO to userspace instead.
The bug can easily be seen by running the following commands as root
on a machine where vether0 and vether1 do not exist, and on which
the subnet 192.0.2.0/24 (reserved for documentation) is not in use:
# ifconfig vether0 destroy 2>/dev/null
# ifconfig vether1 destroy 2>/dev/null
# dummy_mac=fe:ff:ff:ff:ff:ff dummy_ip=192.0.2.5
# ifconfig vether0 create lladdr "$dummy_mac"
# ifconfig vether1 create lladdr "$dummy_mac"
# ifconfig vether0 inet "$dummy_ip" prefixlen 32
# route -n delete "$dummy_ip/32" "$dummy_ip"
# ifconfig vether1 inet "$dummy_ip" prefixlen 32
# route -n delete "$dummy_ip/32" "$dummy_ip"
# route -n add -inet 192.0.2.6 -static -iface -llinfo -link \
vether1 -ifp vether1 -inet -ifa "$dummy_ip"
# route -n show -inet
On a system with an unpatched kernel, the final command will show
that the route to 192.0.2.6 is via vether0. If this patch has been
applied, the route to 192.0.2.6 will be (correctly) via vether1.
---
sys/net/rtsock.c | 26 --
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git sys/net/rtsock.c sys/net/rtsock.c
index fa84ddc25e5..dc8446bd78f 100644
--- sys/net/rtsock.c
+++ sys/net/rtsock.c
@@ -1235,7 +1235,8 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
struct sockaddr_dl *sdl;
sdl = satosdl(info->rti_info[RTAX_IFP]);
- ifp = if_get(sdl->sdl_index);
+ if ((ifp = if_get(sdl->sdl_index)) == NULL)
+ return (ENXIO);
}
#ifdef IPSEC
@@ -1246,11 +1247,19 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
* enc0.
*/
if (info->rti_info[RTAX_DST] &&
- info->rti_info[RTAX_DST]->sa_family == PF_KEY)
+ info->rti_info[RTAX_DST]->sa_family == PF_KEY) {
info->rti_ifa = enc_getifa(rtid, 0);
+
+ if (info->rti_ifa != NULL && ifp != NULL &&
+ ifp != info->rti_ifa->ifa_ifp) {
+ if_put(ifp);
+ return (EADDRNOTAVAIL);
+ }
+ }
#endif
- if (info->rti_ifa == NULL && info->rti_info[RTAX_IFA] != NULL)
+ if (info->rti_ifa == NULL && ifp == NULL &&
+ info->rti_info[RTAX_IFA] != NULL)
info->rti_ifa = ifa_ifwithaddr(info->rti_info[RTAX_IFA], rtid);
if (info->rti_ifa == NULL) {
@@ -1273,10 +1282,15 @@ rtm_getifa(struct rt_addrinfo *info, unsigned int rtid)
sa, sa, rtid);
}
- if_put(ifp);
-
- if (info->rti_ifa == NULL)
+ if (info->rti_ifa == NULL) {
+ if_put(ifp);
return (ENETUNREACH);
+ }
+
+ if (ifp != NULL && ifp != info->rti_ifa->ifa_ifp)
+ panic("rtm_getifa: returned route to wrong interface");
+
+ if_put(ifp);
return (0);
}
Re: go/rust vs uvm_map_inentry()
> From: "Theo de Raadt"
> Date: Sun, 13 Sep 2020 08:56:04 -0600
>
> Sebastien Marie wrote:
>
> > On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> > > I'm no longer able to reproduce the corruption while building lang/go
> > > with the diff below. Something relevant to threading change in go since
> > > march?
> > >
> > > Can someone try this diff and tell me if go and/or rust still fail?
> >
> > quickly tested with rustc build (nightly here), and it is failing at random
> > places (not always at the same) with memory errors (signal 11, compiler ICE
> > signal 6...)
>
> Ah, so that is a firm no. Totally busted.
>
> Clearly uvm_map_inentry_fix() obviously needs the KERNEL_LOCK in the
> presence of threads, I guess one thread can get into here while another
> is changing the map.
>
> The first check in uvm_map_inentry_fix does two checks against the map,
> but the map is not locked:
>
> if (addr < map->min_offset || addr >= map->max_offset)
No that should work; min_offset and max_offset are immutable after exec.
> > > Index: uvm/uvm_map.c
> > > ===
> > > RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> > > retrieving revision 1.266
> > > diff -u -p -r1.266 uvm_map.c
> > > --- uvm/uvm_map.c 12 Sep 2020 17:08:50 - 1.266
> > > +++ uvm/uvm_map.c 13 Sep 2020 10:12:25 -
> > > @@ -1893,16 +1893,16 @@ uvm_map_inentry(struct proc *p, struct p
> > > boolean_t ok = TRUE;
> > >
> > > if (uvm_map_inentry_recheck(serial, addr, ie)) {
> > > - KERNEL_LOCK();
> > > ok = uvm_map_inentry_fix(p, ie, addr, fn, serial);
> > > if (!ok) {
> > > + KERNEL_LOCK();
> > > printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid,
> > > addr, ie->ie_start, ie->ie_end);
> > > p->p_p->ps_acflag |= AMAP;
> > > sv.sival_ptr = (void *)PROC_PC(p);
> > > trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv);
> > > + KERNEL_UNLOCK();
> > > }
> > > - KERNEL_UNLOCK();
> > > }
> > > return (ok);
> > > }
> > >
> >
> > --
> > Sebastien Marie
> >
>
>
Re: go/rust vs uvm_map_inentry()
We need to know which one is hitting a broken counter party -- is it the stack checker, or the syscall checker.
Re: go/rust vs uvm_map_inentry()
Sebastien Marie wrote:
> On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> > I'm no longer able to reproduce the corruption while building lang/go
> > with the diff below. Something relevant to threading change in go since
> > march?
> >
> > Can someone try this diff and tell me if go and/or rust still fail?
>
> quickly tested with rustc build (nightly here), and it is failing at random
> places (not always at the same) with memory errors (signal 11, compiler ICE
> signal 6...)
Ah, so that is a firm no. Totally busted.
Clearly uvm_map_inentry_fix() obviously needs the KERNEL_LOCK in the
presence of threads, I guess one thread can get into here while another
is changing the map.
The first check in uvm_map_inentry_fix does two checks against the map,
but the map is not locked:
if (addr < map->min_offset || addr >= map->max_offset)
>
>
> > Index: uvm/uvm_map.c
> > ===
> > RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> > retrieving revision 1.266
> > diff -u -p -r1.266 uvm_map.c
> > --- uvm/uvm_map.c 12 Sep 2020 17:08:50 - 1.266
> > +++ uvm/uvm_map.c 13 Sep 2020 10:12:25 -
> > @@ -1893,16 +1893,16 @@ uvm_map_inentry(struct proc *p, struct p
> > boolean_t ok = TRUE;
> >
> > if (uvm_map_inentry_recheck(serial, addr, ie)) {
> > - KERNEL_LOCK();
> > ok = uvm_map_inentry_fix(p, ie, addr, fn, serial);
> > if (!ok) {
> > + KERNEL_LOCK();
> > printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid,
> > addr, ie->ie_start, ie->ie_end);
> > p->p_p->ps_acflag |= AMAP;
> > sv.sival_ptr = (void *)PROC_PC(p);
> > trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv);
> > + KERNEL_UNLOCK();
> > }
> > - KERNEL_UNLOCK();
> > }
> > return (ok);
> > }
> >
>
> --
> Sebastien Marie
>
Re: go/rust vs uvm_map_inentry()
> Date: Sun, 13 Sep 2020 16:49:48 +0200
> From: Sebastien Marie
>
> On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> > I'm no longer able to reproduce the corruption while building lang/go
> > with the diff below. Something relevant to threading change in go since
> > march?
> >
> > Can someone try this diff and tell me if go and/or rust still fail?
>
> quickly tested with rustc build (nightly here), and it is failing at
> random places (not always at the same) with memory errors (signal
> 11, compiler ICE signal 6...)
Is it failing when you don't have tracing enabled and not failing when
the tracing is disabled perhaps?
> > Index: uvm/uvm_map.c
> > ===
> > RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> > retrieving revision 1.266
> > diff -u -p -r1.266 uvm_map.c
> > --- uvm/uvm_map.c 12 Sep 2020 17:08:50 - 1.266
> > +++ uvm/uvm_map.c 13 Sep 2020 10:12:25 -
> > @@ -1893,16 +1893,16 @@ uvm_map_inentry(struct proc *p, struct p
> > boolean_t ok = TRUE;
> >
> > if (uvm_map_inentry_recheck(serial, addr, ie)) {
> > - KERNEL_LOCK();
> > ok = uvm_map_inentry_fix(p, ie, addr, fn, serial);
> > if (!ok) {
> > + KERNEL_LOCK();
> > printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid,
> > addr, ie->ie_start, ie->ie_end);
> > p->p_p->ps_acflag |= AMAP;
> > sv.sival_ptr = (void *)PROC_PC(p);
> > trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv);
> > + KERNEL_UNLOCK();
> > }
> > - KERNEL_UNLOCK();
> > }
> > return (ok);
> > }
> >
>
> --
> Sebastien Marie
>
>
Re: go/rust vs uvm_map_inentry()
On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> I'm no longer able to reproduce the corruption while building lang/go
> with the diff below. Something relevant to threading change in go since
> march?
>
> Can someone try this diff and tell me if go and/or rust still fail?
quickly tested with rustc build (nightly here), and it is failing at random
places (not always at the same) with memory errors (signal 11, compiler ICE
signal 6...)
> Index: uvm/uvm_map.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> retrieving revision 1.266
> diff -u -p -r1.266 uvm_map.c
> --- uvm/uvm_map.c 12 Sep 2020 17:08:50 - 1.266
> +++ uvm/uvm_map.c 13 Sep 2020 10:12:25 -
> @@ -1893,16 +1893,16 @@ uvm_map_inentry(struct proc *p, struct p
> boolean_t ok = TRUE;
>
> if (uvm_map_inentry_recheck(serial, addr, ie)) {
> - KERNEL_LOCK();
> ok = uvm_map_inentry_fix(p, ie, addr, fn, serial);
> if (!ok) {
> + KERNEL_LOCK();
> printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid,
> addr, ie->ie_start, ie->ie_end);
> p->p_p->ps_acflag |= AMAP;
> sv.sival_ptr = (void *)PROC_PC(p);
> trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv);
> + KERNEL_UNLOCK();
> }
> - KERNEL_UNLOCK();
> }
> return (ok);
> }
>
--
Sebastien Marie
Re: pppoe: move softc list out of NET_LOCK() into new pppoe lock
Hello Klemens.
pppoe(4) input path and pppoe(4) config path (I mean clone/destroy)
is always different context. Your diff introduces the new lock which
protects `pppoe_softc_list’ list but what should protect `sc’ you got
from this list after you released `pppoe_lock’?
I mean this dereference is not safe because concurrent thread can
destroy this `sc’ (at least in theory).
> @@ -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;
This time we have multiple locks around input and config paths so I
guess this is the real reason you didn’t caught use after free issue.
Also witness allow us to debug lock order, but not concurrent access.
> On 13 Sep 2020, at 16:12, Klemens Nanni wrote:
>
> 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.c13 Sep 2020 11:00:40 - 1.73
> +++ if_pppoe.c13 Sep 2020 11:31:12 -
> @@ -114,15 +114,18 @@ struct pppoetag {
> #define PPPOE_DISC_MAXPADI 4 /* retry PADI four times
> (quickly) */
> #define PPPOE_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
smtpd: document "pki" option for relay delivery in smtpd.conf(5)
Hi, I struggled a bit to configure smtpd to relay to a remote server that requires SSL client certificates. The solution is to just add a "pki host.example.org" option, but "pki" is not listed as a valid option for the relay delivery method, even though the parser accepts it. Index: smtpd.conf.5 === RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v retrieving revision 1.251 diff -u -p -u -p -r1.251 smtpd.conf.5 --- smtpd.conf.527 Aug 2020 08:58:30 - 1.251 +++ smtpd.conf.513 Sep 2020 12:37:03 - @@ -280,6 +280,14 @@ and .Dq smtps protocols for authentication. Server certificates for those protocols are verified by default. +.It Cm pki Ar pkiname +For secure connections, +use the certificate associated with +.Ar pkiname +(declared in a +.Ic pki +directive) +to prove the client's identity to the remote mail server. .It Cm srs When relaying a mail resulting from a forward, use the Sender Rewriting Scheme to rewrite sender address. -- Thanks, Nick
Re: pppoe: move softc list out of NET_LOCK() into new pppoe lock
On 13/09/20(Sun) 15:12, Klemens Nanni wrote: > 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? I doubt it is, see below: > 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.c13 Sep 2020 11:00:40 - 1.73 > +++ if_pppoe.c13 Sep 2020 11:31:12 - > @@ -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); This introduce a new sleeping point in the packet processing path, something we are avoiding thanks to the NET_LOCK(). Plus this use of a lock alone is insufficient to guarantee the integrity of `sc' because it is used outside of the dance. The NET_LOCK() is fine here, please concentrate on the KERNEL_LOCK() that's where contention happens.
go/rust vs uvm_map_inentry()
I'm no longer able to reproduce the corruption while building lang/go
with the diff below. Something relevant to threading change in go since
march?
Can someone try this diff and tell me if go and/or rust still fail?
Index: uvm/uvm_map.c
===
RCS file: /cvs/src/sys/uvm/uvm_map.c,v
retrieving revision 1.266
diff -u -p -r1.266 uvm_map.c
--- uvm/uvm_map.c 12 Sep 2020 17:08:50 - 1.266
+++ uvm/uvm_map.c 13 Sep 2020 10:12:25 -
@@ -1893,16 +1893,16 @@ uvm_map_inentry(struct proc *p, struct p
boolean_t ok = TRUE;
if (uvm_map_inentry_recheck(serial, addr, ie)) {
- KERNEL_LOCK();
ok = uvm_map_inentry_fix(p, ie, addr, fn, serial);
if (!ok) {
+ KERNEL_LOCK();
printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid,
addr, ie->ie_start, ie->ie_end);
p->p_p->ps_acflag |= AMAP;
sv.sival_ptr = (void *)PROC_PC(p);
trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv);
+ KERNEL_UNLOCK();
}
- KERNEL_UNLOCK();
}
return (ok);
}
pppoe: move softc list out of NET_LOCK() into new pppoe lock
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
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
On 2020/09/13 13:23, 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). > It's not in CVS log because it was tested and didn't work so wasn't committed. I don't know if there's precedent for documenting everything somebody might try but fails in a code comment, feels like the tree would be littered with possibly outdated comments if that's done regularly?
Re: trunk: keep interface up on port removal
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: trunk: keep interface up on port removal
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 > Ah, that seems bad. And in the presence of port-sec or mac security on switches, this could provide a poor experience? Is this the right time to change things?
pckbc: extend the PS/2 interface tests
Not all PS/2-like controllers accept the AUXECHO command, and the test that
pckbc applies in order to check for the presence of the auxiliary interface
may yield false negatives, even on newer hardware (see
https://marc.info/?l=openbsd-misc=158413132831425=2
).
This patch adds an alternative test. It is applied if AUXECHO fails, and
attempts to toggle and reset the KC8_MDISABLE flag by issuing an AUXDISABLE
and AUXENABLE command, which should only succeed if there's an AUX interface.
(The test has a branch which makes it work regardless of the initial state
of the flag; that's unnecessary at present but might be useful in a future
version.)
OK?
Index: pckbc.c
===
RCS file: /cvs/src/sys/dev/ic/pckbc.c,v
retrieving revision 1.53
diff -u -p -r1.53 pckbc.c
--- pckbc.c 30 Nov 2019 18:18:34 - 1.53
+++ pckbc.c 13 Sep 2020 09:27:10 -
@@ -295,6 +295,32 @@ pckbc_attach_slot(struct pckbc_softc *sc
return (found);
}
+int
+pckbc_auxcheck(struct pckbc_internal *t)
+{
+ bus_space_tag_t iot = t->t_iot;
+ bus_space_handle_t ioh_c = t->t_ioh_c;
+ int res = -1;
+
+ if ((t->t_cmdbyte & KC8_MDISABLE) == 0) {
+ if (pckbc_send_cmd(iot, ioh_c, KBC_AUXDISABLE)
+ && pckbc_get8042cmd(t))
+ res = (t->t_cmdbyte & KC8_MDISABLE);
+ pckbc_send_cmd(iot, ioh_c, KBC_AUXENABLE);
+ } else {
+ if (pckbc_send_cmd(iot, ioh_c, KBC_AUXENABLE)
+ && pckbc_get8042cmd(t))
+ res = (~t->t_cmdbyte & KC8_MDISABLE);
+ pckbc_send_cmd(iot, ioh_c, KBC_AUXDISABLE);
+ }
+ if (res == -1)
+ printf("kbc: auxcheck: error\n");
+ else
+ printf("kbc: auxcheck: %d\n", (res > 0));
+ pckbc_get8042cmd(t);
+ return (res > 0);
+}
+
void
pckbc_attach(struct pckbc_softc *sc, int flags)
{
@@ -410,13 +436,17 @@ pckbc_attach(struct pckbc_softc *sc, int
*/
DPRINTF("kbc: aux echo: %x\n", res);
t->t_haveaux = 1;
- if (pckbc_attach_slot(sc, PCKBC_AUX_SLOT, 0))
- cmdbits |= KC8_MENABLE;
}
#ifdef PCKBCDEBUG
else
printf("kbc: aux echo test failed\n");
#endif
+
+ if (res == -1)
+ t->t_haveaux = pckbc_auxcheck(t);
+
+ if (t->t_haveaux && pckbc_attach_slot(sc, PCKBC_AUX_SLOT, 0))
+ cmdbits |= KC8_MENABLE;
#if defined(__i386__) || defined(__amd64__)
if (haskbd == 0 && !ISSET(flags, PCKBCF_FORCE_KEYBOARD_PRESENT)) {
Re: pppoe: start documenting locks
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.
Re: trunk: keep interface up on port removal
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
Re: pppoe: start documenting locks
On 13/09/20(Sun) 10:05, Klemens Nanni wrote:
>
> 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?
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.
> 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.
First we should remove the KRENEL_LOCK() from around pppoeintr(). The
NET_LOCK() is not an issue right now.
One comment below, either way ok mpi@
> 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.c28 Jul 2020 09:52:32 - 1.70
> +++ if_pppoe.c20 Aug 2020 15:27:09 -
> @@ -114,27 +115,34 @@ struct pppoetag {
> #define PPPOE_DISC_MAXPADI 4 /* retry PADI four times
> (quickly) */
> #define PPPOE_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.
> + * 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 */
>
Re: trunk: keep interface up on port removal
This has been tried before, I forget what but there were problems -- Sent from a phone, apologies for poor formatting. On 12 September 2020 21:16:31 Alexander Bluhm wrote: OK bluhm@ On Sat, Sep 12, 2020 at 05:49:52PM +0200, Klemens Nanni wrote: 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)
RFC: kern.video.record
Since I recently opened my big fat mouth and suggested that
"kern.video.record" (analogous to kern.audio.record) might be a good idea, I
decided to put together a quick prototype (heavily based on the
kern.audio.record code). This at least roughly works for me but raises some
questions such as:
* Is uvideo the only driver that can capture video? [I imagine not, but I
don't really know.]
* I've taken the same approach as kern.audio.record which is to keep on
handing out data even when kern.video.record=0 *but* it's harder to work
out what data we should hand out. At the moment we just pass on whatever
happened to be in the buffer beforehand (which is clearly not a good
idea in the long term, but proves the point for now). For uncompressed
video, handing over (say) an entirely black image is probably easy; for
compressed video we'd have to think about whether we also want to
manipulate frame headers etc. It would probably be easier to simply
intercept the "opening video" event but that would mean that if
something is already streaming video, setting kern.video.record=0 would
allow it to keep recording.
Comments welcome, including "this is a terrible idea full stop"!
Laurie
Index: sbin/sysctl/sysctl.c
===
RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
retrieving revision 1.252
diff -u -p -r1.252 sysctl.c
--- sbin/sysctl/sysctl.c15 Jul 2020 07:13:56 - 1.252
+++ sbin/sysctl/sysctl.c13 Sep 2020 08:08:57 -
@@ -130,6 +130,7 @@ struct ctlname machdepname[] = CTL_MACHD
#endif
struct ctlname ddbname[] = CTL_DDB_NAMES;
struct ctlname audioname[] = CTL_KERN_AUDIO_NAMES;
+struct ctlname videoname[] = CTL_KERN_VIDEO_NAMES;
struct ctlname witnessname[] = CTL_KERN_WITNESS_NAMES;
char names[BUFSIZ];
int lastused;
@@ -219,6 +220,7 @@ void print_sensor(struct sensor *);
int sysctl_chipset(char *, char **, int *, int, int *);
#endif
int sysctl_audio(char *, char **, int *, int, int *);
+int sysctl_video(char *, char **, int *, int, int *);
int sysctl_witness(char *, char **, int *, int, int *);
void vfsinit(void);
@@ -517,6 +519,11 @@ parse(char *string, int flags)
if (len < 0)
return;
break;
+ case KERN_VIDEO:
+ len = sysctl_video(string, , mib, flags, );
+ if (len < 0)
+ return;
+ break;
case KERN_WITNESS:
len = sysctl_witness(string, , mib, flags, );
if (len < 0)
@@ -1766,6 +1773,7 @@ struct list shmlist = { shmname, KERN_SH
struct list watchdoglist = { watchdogname, KERN_WATCHDOG_MAXID };
struct list tclist = { tcname, KERN_TIMECOUNTER_MAXID };
struct list audiolist = { audioname, KERN_AUDIO_MAXID };
+struct list videolist = { audioname, KERN_VIDEO_MAXID };
struct list witnesslist = { witnessname, KERN_WITNESS_MAXID };
/*
@@ -2813,6 +2821,25 @@ sysctl_audio(char *string, char **bufpp,
return (-1);
mib[2] = indx;
*typep = audiolist.list[indx].ctl_type;
+ return (3);
+}
+
+/*
+ * Handle video support
+ */
+int
+sysctl_video(char *string, char **bufpp, int mib[], int flags, int *typep)
+{
+ int indx;
+
+ if (*bufpp == NULL) {
+ listall(string, );
+ return (-1);
+ }
+ if ((indx = findname(string, "third", bufpp, )) == -1)
+ return (-1);
+ mib[2] = indx;
+ *typep = videolist.list[indx].ctl_type;
return (3);
}
Index: sys/dev/usb/uvideo.c
===
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
retrieving revision 1.209
diff -u -p -r1.209 uvideo.c
--- sys/dev/usb/uvideo.c31 Jul 2020 10:49:33 - 1.209
+++ sys/dev/usb/uvideo.c13 Sep 2020 08:08:58 -
@@ -386,6 +386,12 @@ struct uvideo_devs {
#define uvideo_lookup(v, p) \
((struct uvideo_devs *)usb_lookup(uvideo_devs, v, p))
+/*
+ * Global flag to control if video recording is enabled when the
+ * kern.video.record setting is set to 1.
+ */
+int video_record_enable = 0;
+
int
uvideo_enable(void *v)
{
@@ -2210,7 +2216,8 @@ uvideo_vs_decode_stream_header(struct uv
/* save sample */
sample_len = frame_size - sh->bLength;
if ((fb->offset + sample_len) <= fb->buf_size) {
- bcopy(frame + sh->bLength, fb->buf + fb->offset, sample_len);
+ if (video_record_enable)
+ bcopy(frame + sh->bLength, fb->buf + fb->offset,
sample_len);
fb->offset += sample_len;
}
@@ -2299,7 +2306,8 @@ uvideo_vs_decode_stream_header_isight(st
/* save sample */
sample_len = frame_size;
if ((fb->offset + sample_len) <= fb->buf_size) {
-
pppoe: start documenting locks
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 */
