Hi,
On Tue, 29 Mar 2022 17:28:23 +0900
Yuichiro NAITO <naito.yuich...@gmail.com> wrote:
> There is one thing I'm worrying about.
> Ifconfig doesn't show wgrtable value with your patch.
> In my use case as follows, it seems that setting `wgrtable 1` is
> ignored.
>
> ```
> # 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 wg0
> wg0: flags=80c3<UP,BROADCAST,RUNNING,NOARP,MULTICAST> mtu 1420
> index 6 priority 0 llprio 3
> wgport 7111
> wgpubkey e/CYTG1RGqT4jmrY0Fom8cAdtOWP7F/gBVwamyINRlg=
> groups: wg
> ```
Thank you for pointing this out.
In this case, wg0 is binding 7111/udp on rdomain 0. So I have
supposed ignoring "wgrtable 1" is correct. But if we configure
wgrtable when creating,
% doas ifconfig wg0 create wgport 7111 wgrtable 1 wgkey `openssl rand -base64
32` up
% doas ifconfig wg0
wg0: flags=80c3<UP,BROADCAST,RUNNING,NOARP,MULTICAST> mtu 1420
index 13 priority 0 llprio 3
wgport 7111
wgrtable 1
wgpubkey /4v4hsi426MsVZojJ0rwRvk8kK0jSckjcU2Z1L/k5W8=
groups: wg
%
It displays "wgrtable 1". And actually
% netstat -T0 -naf inet | grep 7111
% netstat -T1 -naf inet | grep 7111
udp 0 0 *.7111 *.*
%
it binds 7111/udp on rtable 1.
So I start wondering why binding 7111/udp on table 1 fails with
EADDRINUSE when 7111/udp on rtable 0 is used.
> On 3/28/22 15:59, YASUOKA Masahiko wrote:
>> On Mon, 28 Mar 2022 15:20:02 +0900
>> Yuichiro NAITO <naito.yuich...@gmail.com> wrote:
>>> 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?
>> Yes. There is nothing to do, since the command is to bind the same
>> port, protocol, and domain of prevous.
>> The code seems to do such the skip already, but it lacks consideration
>> for rtable_l2(rtable) != rtable case.
>>
>>> 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)
>>>
>
> --
> Yuichiro NAITO (naito.yuich...@gmail.com)
