Thanks for the explanation.
I understand how your patch works.

I want to ask the goal of your patch.
It seems just removing 'Address already in use' message.
Is my guessing right?

On 3/28/22 14:01, YASUOKA Masahiko wrote:
Hi,

On Mon, 28 Mar 2022 12:12:39 +0900
Yuichiro NAITO <naito.yuich...@gmail.com> wrote:
On 3/27/22 18:25, YASUOKA Masahiko wrote:
Hi,
On Wed, 9 Mar 2022 15:28:44 +0900
Yuichiro NAITO <naito.yuich...@gmail.com> wrote:
I see 'Address already in use' message,
when I change wgrtable for a running wg interface.
It doesn't make sense to me.

It can be reproduced by the following command sequence.

```
# route -T1 add default `cat /etc/mygate`
# ifconfig wg0 create wgport 7111 wgkey `cat /etc/mykey.wg0`
# ifconfig wg0 up
# ifconfig wg0 wgrtable 1
ifconfig: SIOCSWG: Address already in use
```

When I down wg0 interface before changing wgrtable,
It succeeds and no messages are shown.

I investigated the reason why 'Address already in use' is shown.

If wgrtable is specified by ifconfig argument,
`wg_ioctl_set` function in `sys/net/if_wg.c` is called.

And if the wg interface is running, `wg_bind` function is called.
`wg_bind` creates new sockets (IPv4 and 6) and replace them from old
ones.

If only wgrtable is changed, `wg_bind` binds as same port as existing
sockets.
So 'Address already in use' is shown.

Here is a simple patch to close existing sockets before `wg_bind`.
It works for me but I'm not 100% sure this is right fix.

Any other ideas?

```
diff --git a/sys/net/if_wg.c b/sys/net/if_wg.c
index 4dae3e3c976..0159664fb34 100644
--- a/sys/net/if_wg.c
+++ b/sys/net/if_wg.c
@@ -2253,11 +2253,14 @@ wg_ioctl_set(struct wg_softc *sc, struct
wg_data_io *data)
        if (port != sc->sc_udp_port || rtable != sc->sc_udp_rtable) {
                TAILQ_FOREACH(peer, &sc->sc_peer_seq, p_seq_entry)
                        wg_peer_clear_src(peer);

-               if (sc->sc_if.if_flags & IFF_RUNNING)
+               if (sc->sc_if.if_flags & IFF_RUNNING) {
+                       if (port == sc->sc_udp_port)
+                               wg_unbind(sc);
                        if ((ret = wg_bind(sc, &port, &rtable)) != 0)
                                goto error;
+               }

                sc->sc_udp_port = port;
                sc->sc_udp_rtable = rtable;
        }
```
If rdomain 1 exists, the error will not shown.
   # ifconfig vether0 rdomain 1 up
   # ifconfig wg0 create wgport 7111 wgkey `openssl rand -base64 32` up
   # ifconfig wg0 wgrtable 1
   #

Yes, if rdomain 1 is created before `ifconfig wg0 wgrtable 1`,
setting wgrtable succeeds and there is no problem.

In the case which you reported to, it is supposed that rtable 1 exists
but rdomain 1 doesn't exist.
Even when "wgtable 1" is configured, becase there is no dedicated
rdomain, rdomain 0 will be used to bind the UDP port.

Exactly, it's the case that I reported and want to fix.

So what wg(4) should do for this case is "nothing".

I'm a little bit confused.
As you said, I can confirm your patch doesn't set wgrtable in my use
case.
It is not the result that I wanted.

    # ifconfig wg0 create wgport 7111 wgkey `openssl rand -base64 32` up
    -> bind 7111/udp on rdomain 0  (1)

is expected. (1)

    # ifconfig wg0 wgrtable 1
    -> bind 7111/udp on rdomain 0  (2)

is expected, since there is no "domain 1".

If trying to do (1) and (2), then it causes EADDRINUSE since it is to
bind the same port, proto, and domain.  The latest diff is skip (2)
properly.

Previous

   -    if (port != sc->sc_udp_port || rtable != sc->sc_udp_rtable) {

"rtable != sc->sc_udp_rtable" was wrong since rdomain for rtable may
not exist.  This is the cause of EADDRINUSE.


So the diff is updated.
ok?
Index: sys/net/if_wg.c
===================================================================
RCS file: /disk/cvs/openbsd/src/sys/net/if_wg.c,v
retrieving revision 1.22
diff -u -p -r1.22 if_wg.c
--- sys/net/if_wg.c     22 Feb 2022 01:15:02 -0000      1.22
+++ sys/net/if_wg.c     27 Mar 2022 09:17:08 -0000
@@ -2250,7 +2250,8 @@ wg_ioctl_set(struct wg_softc *sc, struct
        else
                rtable = sc->sc_udp_rtable;
   -    if (port != sc->sc_udp_port || rtable != sc->sc_udp_rtable) {
+       if (port != sc->sc_udp_port ||
+           rtable_l2(rtable) != sc->sc_udp_rtable) {
                TAILQ_FOREACH(peer, &sc->sc_peer_seq, p_seq_entry)
                        wg_peer_clear_src(peer);

--
Yuichiro NAITO (naito.yuich...@gmail.com)


--
Yuichiro NAITO (naito.yuich...@gmail.com)

Reply via email to