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)
>
