Re: remove net.inet6.ip6.soiikey from userland
On 2023-05-20 19:37 +02, Paul de Weerd wrote: > On Sat, May 20, 2023 at 05:33:11PM +0200, Florian Obser wrote: > | In case this turns out to be useful for unlocking work in the kernel. > | > | It's a minimum diff, if we want to go this way we probably want to move > | init_soiikey() to the engine process and stop bouncing through the main > | process when an interface changes. > | > | This changes behaviour: in -current we can change the sysctl and down/up > | an interface to read the new value, with this diff that no longer > | works. slaacd will (and can) only read the file on startup. > | > | This has consequences for the installer: slaacd starts before > | /mnt/etc/soii.key is available in the upgrade case. Which in turn means > | that we get a different IPv6 address in the installer than in the > | running system. I don't know how big of an issue that is. > > Can't speak for others, but my use case for soii-addresses is for > incoming connections - outgoing ones should use the temporary privacy > addressed. So for the installer it doesn't matter. > > My guess is that this goes for many (most? all?) users of > soii-addresses, so it should be safe to not have those in the > installer during upgrades. I'm worried that someone runs a completely locked down network. They installed a server, figured out the random but stable IP address and added it to their firewall and only allow communication from known IP addresses. They will be fine if they use sysupgrade(8). The installed base is probably smaller than 6. > > Paul > -- In my defence, I have been left unsupervised.
Re: remove net.inet6.ip6.soiikey from userland
On Sat, May 20, 2023 at 05:33:11PM +0200, Florian Obser wrote:
| In case this turns out to be useful for unlocking work in the kernel.
|
| It's a minimum diff, if we want to go this way we probably want to move
| init_soiikey() to the engine process and stop bouncing through the main
| process when an interface changes.
|
| This changes behaviour: in -current we can change the sysctl and down/up
| an interface to read the new value, with this diff that no longer
| works. slaacd will (and can) only read the file on startup.
|
| This has consequences for the installer: slaacd starts before
| /mnt/etc/soii.key is available in the upgrade case. Which in turn means
| that we get a different IPv6 address in the installer than in the
| running system. I don't know how big of an issue that is.
Can't speak for others, but my use case for soii-addresses is for
incoming connections - outgoing ones should use the temporary privacy
addressed. So for the installer it doesn't matter.
My guess is that this goes for many (most? all?) users of
soii-addresses, so it should be safe to not have those in the
installer during upgrades.
Paul
| diff --git distrib/miniroot/install.sub distrib/miniroot/install.sub
| index d3d944bf2ca..aa84e4808f2 100644
| --- distrib/miniroot/install.sub
| +++ distrib/miniroot/install.sub
| @@ -2620,9 +2620,6 @@ enable_network() {
| echo "127.0.0.1\tlocalhost" >/tmp/i/hosts
| echo "::1\t\tlocalhost" >>/tmp/i/hosts
|
| - _f=/mnt/etc/soii.key
| - [[ -f $_f ]] && sysctl "net.inet6.ip6.soiikey=$(<$_f)"
| -
| enable_ifs
| }
|
| diff --git distrib/special/sysctl/sysctl.c distrib/special/sysctl/sysctl.c
| index 9156d5f455c..7aedb6dd55b 100644
| --- distrib/special/sysctl/sysctl.c
| +++ distrib/special/sysctl/sysctl.c
| @@ -99,39 +99,6 @@ pstring(struct var *v)
| return (1);
| }
|
| -int
| -parse_hex_char(char ch)
| -{
| - if (ch >= '0' && ch <= '9')
| - return (ch - '0');
| -
| - ch = tolower((unsigned char)ch);
| - if (ch >= 'a' && ch <= 'f')
| - return (ch - 'a' + 10);
| -
| - return (-1);
| -}
| -
| -int
| -set_soii_key(char *src)
| -{
| - uint8_t key[SOIIKEY_LEN];
| - int mib[4] = {CTL_NET, PF_INET6, IPPROTO_IPV6, IPV6CTL_SOIIKEY};
| - int i, c;
| -
| - for(i = 0; i < SOIIKEY_LEN; i++) {
| - if ((c = parse_hex_char(src[2 * i])) == -1)
| - return (-1);
| - key[i] = c << 4;
| - if ((c = parse_hex_char(src[2 * i + 1])) == -1)
| - return (-1);
| - key[i] |= c;
| - }
| -
| - return sysctl(mib, sizeof(mib) / sizeof(mib[0]), NULL, NULL, key,
| - SOIIKEY_LEN);
| -}
| -
| int
| main(int argc, char *argv[])
| {
| @@ -159,17 +126,6 @@ main(int argc, char *argv[])
|
| while (argc--) {
| name = *argv++;
| - /*
| - * strlen("net.inet6.ip6.soiikey="
| - * "") == 54
| - * strlen("net.inet6.ip6.soiikey=") == 22
| - */
| - if (strlen(name) == 54 && strncmp(name,
| - "net.inet6.ip6.soiikey=", 22) == 0) {
| - set_soii_key(name + 22);
| - continue;
| - }
| -
| for (i = 0; i < sizeof(vars)/sizeof(vars[0]); i++) {
| if (strcmp(name, vars[i].name) == 0) {
| (vars[i].print)(&vars[i]);
| diff --git etc/netstart etc/netstart
| index af4866f909e..105d5a977cf 100644
| --- etc/netstart
| +++ etc/netstart
| @@ -360,13 +360,6 @@ if ifconfig lo0 inet6 >/dev/null 2>&1; then
| IP6KERNEL=true
| fi
|
| -# Load key material for the generation of IPv6 Semantically Opaque Interface
| -# Identifiers (SOII) used for SLAAC addresses.
| -if $IP6KERNEL && ! $PRINT_ONLY; then
| - [[ -f /etc/soii.key ]] &&
| - sysctl -q "net.inet6.ip6.soiikey=$( /etc/soii.key &&
| - chmod 600 /etc/soii.key && sysctl -q \
| - "net.inet6.ip6.soiikey=$(
| #include
| #include
| +#include
| #include
| -#include
| #include
| #include
|
| @@ -34,6 +34,7 @@
| #include
| #include
|
| +#include
| #include
| #include
| #include
| @@ -76,7 +77,8 @@ voidconfigure_gateway(struct imsg_configure_dfr *,
uint8_t);
| void add_gateway(struct imsg_configure_dfr *);
| void delete_gateway(struct imsg_configure_dfr *);
| void send_rdns_proposal(struct imsg_propose_rdns *);
| -int get_soiikey(uint8_t *);
| +int parse_hex_char(char);
| +void init_soiikey(void);
|
| static int main_imsg_send_ipc_sockets(struct imsgbuf *, struct imsgbuf *);
| int main_imsg_compose_frontend(int, int, void *, uint16_t);
| @@ -89,6 +91,7 @@ pid_tfrontend_pid;
| pid_t engine_pid;
|
| int routesock, ioctl_sock, rtm_seq = 0;
| +uint8_t soiikey[
remove net.inet6.ip6.soiikey from userland
In case this turns out to be useful for unlocking work in the kernel.
It's a minimum diff, if we want to go this way we probably want to move
init_soiikey() to the engine process and stop bouncing through the main
process when an interface changes.
This changes behaviour: in -current we can change the sysctl and down/up
an interface to read the new value, with this diff that no longer
works. slaacd will (and can) only read the file on startup.
This has consequences for the installer: slaacd starts before
/mnt/etc/soii.key is available in the upgrade case. Which in turn means
that we get a different IPv6 address in the installer than in the
running system. I don't know how big of an issue that is.
diff --git distrib/miniroot/install.sub distrib/miniroot/install.sub
index d3d944bf2ca..aa84e4808f2 100644
--- distrib/miniroot/install.sub
+++ distrib/miniroot/install.sub
@@ -2620,9 +2620,6 @@ enable_network() {
echo "127.0.0.1\tlocalhost" >/tmp/i/hosts
echo "::1\t\tlocalhost" >>/tmp/i/hosts
- _f=/mnt/etc/soii.key
- [[ -f $_f ]] && sysctl "net.inet6.ip6.soiikey=$(<$_f)"
-
enable_ifs
}
diff --git distrib/special/sysctl/sysctl.c distrib/special/sysctl/sysctl.c
index 9156d5f455c..7aedb6dd55b 100644
--- distrib/special/sysctl/sysctl.c
+++ distrib/special/sysctl/sysctl.c
@@ -99,39 +99,6 @@ pstring(struct var *v)
return (1);
}
-int
-parse_hex_char(char ch)
-{
- if (ch >= '0' && ch <= '9')
- return (ch - '0');
-
- ch = tolower((unsigned char)ch);
- if (ch >= 'a' && ch <= 'f')
- return (ch - 'a' + 10);
-
- return (-1);
-}
-
-int
-set_soii_key(char *src)
-{
- uint8_t key[SOIIKEY_LEN];
- int mib[4] = {CTL_NET, PF_INET6, IPPROTO_IPV6, IPV6CTL_SOIIKEY};
- int i, c;
-
- for(i = 0; i < SOIIKEY_LEN; i++) {
- if ((c = parse_hex_char(src[2 * i])) == -1)
- return (-1);
- key[i] = c << 4;
- if ((c = parse_hex_char(src[2 * i + 1])) == -1)
- return (-1);
- key[i] |= c;
- }
-
- return sysctl(mib, sizeof(mib) / sizeof(mib[0]), NULL, NULL, key,
- SOIIKEY_LEN);
-}
-
int
main(int argc, char *argv[])
{
@@ -159,17 +126,6 @@ main(int argc, char *argv[])
while (argc--) {
name = *argv++;
- /*
-* strlen("net.inet6.ip6.soiikey="
-* "") == 54
-* strlen("net.inet6.ip6.soiikey=") == 22
-*/
- if (strlen(name) == 54 && strncmp(name,
- "net.inet6.ip6.soiikey=", 22) == 0) {
- set_soii_key(name + 22);
- continue;
- }
-
for (i = 0; i < sizeof(vars)/sizeof(vars[0]); i++) {
if (strcmp(name, vars[i].name) == 0) {
(vars[i].print)(&vars[i]);
diff --git etc/netstart etc/netstart
index af4866f909e..105d5a977cf 100644
--- etc/netstart
+++ etc/netstart
@@ -360,13 +360,6 @@ if ifconfig lo0 inet6 >/dev/null 2>&1; then
IP6KERNEL=true
fi
-# Load key material for the generation of IPv6 Semantically Opaque Interface
-# Identifiers (SOII) used for SLAAC addresses.
-if $IP6KERNEL && ! $PRINT_ONLY; then
- [[ -f /etc/soii.key ]] &&
- sysctl -q "net.inet6.ip6.soiikey=$( /etc/soii.key &&
- chmod 600 /etc/soii.key && sysctl -q \
- "net.inet6.ip6.soiikey=$(
#include
#include
+#include
#include
-#include
#include
#include
@@ -34,6 +34,7 @@
#include
#include
+#include
#include
#include
#include
@@ -76,7 +77,8 @@ void configure_gateway(struct imsg_configure_dfr *, uint8_t);
void add_gateway(struct imsg_configure_dfr *);
void delete_gateway(struct imsg_configure_dfr *);
void send_rdns_proposal(struct imsg_propose_rdns *);
-intget_soiikey(uint8_t *);
+intparse_hex_char(char);
+void init_soiikey(void);
static int main_imsg_send_ipc_sockets(struct imsgbuf *, struct imsgbuf *);
intmain_imsg_compose_frontend(int, int, void *, uint16_t);
@@ -89,6 +91,7 @@ pid_t frontend_pid;
pid_t engine_pid;
int routesock, ioctl_sock, rtm_seq = 0;
+uint8_t soiikey[SLAACD_SOIIKEY_LEN];
void
main_sig_handler(int sig, short event, void *arg)
@@ -189,6 +192,10 @@ main(int argc, char *argv[])
if (getpwnam(SLAACD_USER) == NULL)
errx(1, "unknown user %s", SLAACD_USER);
+#ifndef SMALL
+ init_soiikey();
+#endif /* SMALL */
+
log_init(debug, LOG_DAEMON);
log_setverbose(verbose);
@@ -431,11 +438,9 @@ main_dispatch_frontend(int fd, short event, void *bula)
fatalx("%s: IMSG_UPDATE_IF wrong length: %lu",
__func__, IMSG_DATA_SIZE(imsg)
Virtio fix for testing
Hi,
with help from Aaron Mason, I have found the problem responsible for the
vioscsi panic on oracle cloud and windows qemu. Basically before using any
virt-queues, the guest must set the DRIVER_OK status bit, or the host may
not process the queues. This affects vioscsi(4) and viogpu(4) in openbsd,
but I have changed the code in all virtio drivers to work the same.
This seems to be a common bug in guests and qemu has workarounds in some
code paths, but not in others. I think that is the reason why I could not
reproduce the issue on my own qemu/linux system.
I have only tested this on amd64 / qemu. I would be interested in test
results on other systems, especially on top of vmd and on arm64 with
viogpu.
Aaron, can you please also test with the updated diff?
Thanks in advance.
Cheers,
Stefan
commit d3209e34d77ab7353973efb2a57303f636fff4f4
Author: Stefan Fritsch
Date: Sat May 20 14:18:57 2023 +0200
virtio: Set DRIVER_OK earlier
The DRIVER_OK must be set before using any virt-queues. To allow virtio
device drivers to use the virt-queues in their attach functions, set the
bit there and not in the virtio transport attach function. Only vioscsi
and viogpu really need this, but let's only have one standard way to do
this .
Noticed because of hangs with vioscsi on qemu/windows and in the Oracle
cloud. With much debugging help by Aaron Mason .
Also revert vioscsi.c 1.31 "Temporarily workaround double calls into
vioscsi_req_done()"
diff --git a/sys/dev/fdt/virtio_mmio.c b/sys/dev/fdt/virtio_mmio.c
index 12f5a6cdeb6..682cbd304a1 100644
--- a/sys/dev/fdt/virtio_mmio.c
+++ b/sys/dev/fdt/virtio_mmio.c
@@ -293,7 +293,6 @@ virtio_mmio_attach(struct device *parent, struct device
*self, void *aux)
goto fail_2;
}
- virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
return;
fail_2:
diff --git a/sys/dev/pci/virtio_pci.c b/sys/dev/pci/virtio_pci.c
index 236120cf5a6..747101b8382 100644
--- a/sys/dev/pci/virtio_pci.c
+++ b/sys/dev/pci/virtio_pci.c
@@ -651,7 +651,6 @@ virtio_pci_attach(struct device *parent, struct device
*self, void *aux)
}
printf("%s: %s\n", vsc->sc_dev.dv_xname, intrstr);
- virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
return;
fail_2:
diff --git a/sys/dev/pv/if_vio.c b/sys/dev/pv/if_vio.c
index be53ccaf7c3..5d1ca8d0030 100644
--- a/sys/dev/pv/if_vio.c
+++ b/sys/dev/pv/if_vio.c
@@ -600,6 +600,7 @@ vio_attach(struct device *parent, struct device *self, void
*aux)
timeout_set(&sc->sc_txtick, vio_txtick, &sc->sc_vq[VQTX]);
timeout_set(&sc->sc_rxtick, vio_rxtick, &sc->sc_vq[VQRX]);
+ virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
if_attach(ifp);
ether_ifattach(ifp);
diff --git a/sys/dev/pv/vioblk.c b/sys/dev/pv/vioblk.c
index 093ce9b5a58..9f8a10e0fa2 100644
--- a/sys/dev/pv/vioblk.c
+++ b/sys/dev/pv/vioblk.c
@@ -251,6 +251,7 @@ vioblk_attach(struct device *parent, struct device *self,
void *aux)
saa.saa_quirks = 0;
saa.saa_wwpn = saa.saa_wwnn = 0;
+ virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
config_found(self, &saa, scsiprint);
return;
diff --git a/sys/dev/pv/viocon.c b/sys/dev/pv/viocon.c
index bd96467c810..882f0961ada 100644
--- a/sys/dev/pv/viocon.c
+++ b/sys/dev/pv/viocon.c
@@ -203,6 +203,7 @@ viocon_attach(struct device *parent, struct device *self,
void *aux)
goto err;
}
viocon_rx_fill(sc->sc_ports[0]);
+ virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
return;
err:
diff --git a/sys/dev/pv/viogpu.c b/sys/dev/pv/viogpu.c
index e8a91c34310..06f4388a7b0 100644
--- a/sys/dev/pv/viogpu.c
+++ b/sys/dev/pv/viogpu.c
@@ -235,6 +235,8 @@ viogpu_attach(struct device *parent, struct device *self,
void *aux)
sc->sc_fb_dma_kva, sc->sc_fb_dma_size, NULL, BUS_DMA_NOWAIT) != 0)
goto fb_unmap;
+ virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
+
if (viogpu_create_2d(sc, 1, sc->sc_fb_width, sc->sc_fb_height) != 0)
goto fb_unmap;
diff --git a/sys/dev/pv/viomb.c b/sys/dev/pv/viomb.c
index 8169770553e..a8d408e7392 100644
--- a/sys/dev/pv/viomb.c
+++ b/sys/dev/pv/viomb.c
@@ -220,6 +220,7 @@ viomb_attach(struct device *parent, struct device *self,
void *aux)
sensordev_install(&sc->sc_sensdev);
printf("\n");
+ virtio_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER_OK);
return;
err_dmamap:
bus_dmamap_destroy(vsc->sc_dmat, sc->sc_req.bl_dmamap);
diff --git a/sys/dev/pv/viornd.c b/sys/dev/pv/viornd.c
index 39442bc8391..490e823173b 100644
--- a/sys/dev/pv/viornd.c
+++ b/sys/dev/pv/viornd.c
@@ -138,6 +138,7 @@ viornd_attach(struct device *parent, struct device *self,
void *aux)
timeout_add(&sc->sc_tick, 1);
printf("\n");
+ virtio_s
Re: Fix wrong interface mtu in tcp_mss
On Fri, May 19, 2023 at 07:58:47PM +0200, Jan Klemkow wrote: > Hi, > > We use the wrong interface and mtu in tcp_mss() to calculate the mss if > the destination address points is a local address. In ip_output() we > use the correct interface and its mtu. > > This limits the mss to 1448 if the mtu of the interface it 1500, > instead of using a local 32k mss. > > The bigger issue is: local bulk traffic with the current TSO > implementation is broken. tcp_output() creates TSO packets with an mss > smaller then 32k and ip_output() calls if_output instead of > tcp_if_output_tso() because it fits into the mtu check of lo0. > > This diff takes the same logic to pick the interface in tcp_mss() as its > done in ip_output() and fixes both issues. > > ok? I'm fine with this going in since it emulates the same behaviour as ip_output. For the curious ip6_output() seems to have the same workaround. Now in an ideal world the other issue exposed by this mtu mismatch should also be fixed. We had similar issues with TCP over loopback on multiple occasions. So maybe it is time to fix this for good. Note: In an ideal world lo(4) would do TSO and LRO since it bounces the packet right back at us. > Index: netinet/tcp_input.c > === > RCS file: /cvs/src/sys/netinet/tcp_input.c,v > retrieving revision 1.387 > diff -u -p -r1.387 tcp_input.c > --- netinet/tcp_input.c 14 Mar 2023 00:24:05 - 1.387 > +++ netinet/tcp_input.c 19 May 2023 17:22:47 - > @@ -2805,7 +2805,11 @@ tcp_mss(struct tcpcb *tp, int offer) > if (rt == NULL) > goto out; > > - ifp = if_get(rt->rt_ifidx); > + if (ISSET(rt->rt_flags, RTF_LOCAL)) > + ifp = if_get(rtable_loindex(inp->inp_rtableid)); > + else > + ifp = if_get(rt->rt_ifidx); > + > if (ifp == NULL) > goto out; > > -- :wq Claudio
