On Wed, May 31, 2023 at 02:07:17PM +0000, Klemens Nanni wrote:
> On Wed, May 31, 2023 at 10:27:13AM +0200, Claudio Jeker wrote:
> > On Tue, May 30, 2023 at 11:56:01PM +0000, Klemens Nanni wrote:
> > > On Tue, May 23, 2023 at 07:13:28PM +0000, Klemens Nanni wrote:
> > > > On Sat, Jan 14, 2023 at 02:28:27PM +0000, Stuart Henderson wrote:
> > > > > On 2023/01/12 04:49, Mikolaj Kucharski wrote:
> > > > > > Hi,
> > > > > >
> > > > > > Is there anything else which I can do, to help this diff reviwed and
> > > > > > increase the chance of getting in?
> > > > > >
> > > > > > Thread at https://marc.info/?t=163478298600001&r=1&w=2
> > > > > >
> > > > > > Last version of the diff at
> > > > > > https://marc.info/?l=openbsd-tech&m=167185582521873&q=mbox
> > > > >
> > > > > Inlining that for a few comments, otherwise it's ok sthen
> > > >
> > > > wgdescr[iption] would be consistent with the existing descr[iption].
> > > > At least my keep typing the trailing "r"...
> > > >
> > > > Then '-wgdescr' and 'wgdescr ""' work and are implemented exactly like
> > > > te inteface description equivalents.
> > > >
> > > > I could use this now in a new VPN setup, so here's a polished diff,
> > > > with the above, missing ifconfig.8 bits written and other nits inline.
> > > >
> > > > As Theo suggested, I'd drop the wg.4 and leave it to ifconfig.8 proper.
> > > >
> > > > Feedback?
> > > >
> > > > Either way, net/wireguard-tools needs a bump/rebuild.
> > >
> > > Updated diff at the end, grabbing the new per-description mutex also for
> > > reading, not just writing it.
> > >
> > > I did not run into an issue with the first two diffs, but other peer
> > > properties have their own mutex as well and they're consistently used
> > > for all accesses, as I'd expect, so protect new description properly.
> > >
> > > Also fixed ifconfig.8's wireguard synopsis bits.
> > >
> > > Anyone?
> >
> > This mutex makes very little sense to me.
> > Access to this field is already serialized by the sc->sc_lock rwlock
> > so there is no need for this mutex.
>
> Right, description is only read/written through the ioctl, whereas other
> stuff under its own mutex, e.g. peer rx/tx counters, need to serialise
> packet processing/actual counting with reading out from the ioctl layer.
>
> I did not look close enough, but lazily grabbed the original diff's mutex
> in all places.
>
> Hopefully the last diff.
Two comments below. With them fixed OK claudio@
> Index: sys/net/if_wg.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_wg.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 if_wg.c
> --- sys/net/if_wg.c 30 May 2023 08:30:01 -0000 1.27
> +++ sys/net/if_wg.c 31 May 2023 12:41:50 -0000
> @@ -221,6 +221,8 @@ struct wg_peer {
>
> SLIST_ENTRY(wg_peer) p_start_list;
> int p_start_onlist;
> +
> + char p_description[IFDESCRSIZE];
> };
>
> struct wg_softc {
> @@ -407,6 +409,8 @@ wg_peer_create(struct wg_softc *sc, uint
> peer->p_counters_tx = 0;
> peer->p_counters_rx = 0;
>
> + strlcpy(peer->p_description, "", IFDESCRSIZE);
> +
> mtx_init(&peer->p_endpoint_mtx, IPL_NET);
> bzero(&peer->p_endpoint, sizeof(peer->p_endpoint));
>
> @@ -581,6 +585,11 @@ wg_peer_counters_add(struct wg_peer *pee
> mtx_leave(&peer->p_counters_mtx);
> }
>
> +void
> +wg_peer_get_description(struct wg_peer *peer, char *description)
> +{
> +}
> +
Empty function does nothing.
> int
> wg_aip_add(struct wg_softc *sc, struct wg_peer *peer, struct wg_aip_io *d)
> {
> @@ -2320,6 +2329,10 @@ wg_ioctl_set(struct wg_softc *sc, struct
> }
> }
>
> + if (peer_o.p_flags & WG_PEER_SET_DESCRIPTION)
> + strlcpy(peer->p_description, peer_o.p_description,
> + IFDESCRSIZE);
> +
> aip_p = &peer_p->p_aips[0];
> for (j = 0; j < peer_o.p_aips_count; j++) {
> if ((ret = copyin(aip_p, &aip_o, sizeof(aip_o))) != 0)
> @@ -2429,6 +2442,8 @@ wg_ioctl_get(struct wg_softc *sc, struct
> aip_count++;
> }
> peer_o.p_aips_count = aip_count;
> +
> + strlcpy(peer_o.p_description, peer->p_description, IFDESCRSIZE);
>
> if ((ret = copyout(&peer_o, peer_p, sizeof(peer_o))) != 0)
> goto unlock_and_ret_size;
> Index: sys/net/if_wg.h
> ===================================================================
> RCS file: /cvs/src/sys/net/if_wg.h,v
> retrieving revision 1.4
> diff -u -p -r1.4 if_wg.h
> --- sys/net/if_wg.h 22 Jun 2020 12:20:44 -0000 1.4
> +++ sys/net/if_wg.h 29 May 2023 22:20:02 -0000
> @@ -61,6 +61,7 @@ struct wg_aip_io {
> #define WG_PEER_REPLACE_AIPS (1 << 4)
> #define WG_PEER_REMOVE (1 << 5)
> #define WG_PEER_UPDATE (1 << 6)
> +#define WG_PEER_SET_DESCRIPTION (1 << 7)
>
> #define p_sa p_endpoint.sa_sa
> #define p_sin p_endpoint.sa_sin
> @@ -80,6 +81,7 @@ struct wg_peer_io {
> uint64_t p_txbytes;
> uint64_t p_rxbytes;
> struct timespec p_last_handshake; /* nanotime */
> + char p_description[IFDESCRSIZE];
> size_t p_aips_count;
> struct wg_aip_io p_aips[];
> };
> Index: sbin/ifconfig/ifconfig.8
> ===================================================================
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
> retrieving revision 1.395
> diff -u -p -r1.395 ifconfig.8
> --- sbin/ifconfig/ifconfig.8 16 May 2023 14:32:54 -0000 1.395
> +++ sbin/ifconfig/ifconfig.8 30 May 2023 23:38:33 -0000
> @@ -2316,6 +2316,7 @@ Packets on a VLAN interface without a ta
> .Op Fl wgpeerall
> .Oo
> .Oo Fl Oc Ns Cm wgpeer Ar publickey
> +.Op Oo Fl Oc Ns Cm wgdescr Ns Oo Cm iption Oc Ar value
> .Op Cm wgaip Ar allowed-ip_address/prefix
> .Op Cm wgendpoint Ar peer_address port
> .Op Cm wgpka Ar interval
> @@ -2383,6 +2384,13 @@ Peer configuration options, which apply
> immediately preceding them,
> are as follows:
> .Bl -tag -width Ds
> +.Tg wgdescription
> +.It Cm wgdescr Ns Oo Cm iption Oc Ar value
> +Set the peer's description.
> +This can be used to label peers in situations where they may
> +otherwise be difficult to distinguish.
> +.It Cm -wgdescr Ns Op Cm iption
> +Clear the peer description.
> .It Cm wgaip Ar allowed-ip_address/prefix
> Set the peer's IPv4 or IPv6
> .Ar allowed-ip_address
> Index: sbin/ifconfig/ifconfig.c
> ===================================================================
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> retrieving revision 1.464
> diff -u -p -r1.464 ifconfig.c
> --- sbin/ifconfig/ifconfig.c 16 May 2023 14:32:54 -0000 1.464
> +++ sbin/ifconfig/ifconfig.c 29 May 2023 22:20:02 -0000
> @@ -351,6 +351,7 @@ void transceiverdump(const char *, int);
>
> /* WG */
> void setwgpeer(const char *, int);
> +void setwgpeerdesc(const char *, int);
> void setwgpeerep(const char *, const char *);
> void setwgpeeraip(const char *, int);
> void setwgpeerpsk(const char *, int);
> @@ -360,6 +361,7 @@ void setwgkey(const char *, int);
> void setwgrtable(const char *, int);
>
> void unsetwgpeer(const char *, int);
> +void unsetwgpeerdesc(const char *, int);
> void unsetwgpeerpsk(const char *, int);
> void unsetwgpeerall(const char *, int);
>
> @@ -619,6 +621,8 @@ const struct cmd {
> { "sffdump", 0, 0, transceiverdump },
>
> { "wgpeer", NEXTARG, A_WIREGUARD, setwgpeer},
> + { "wgdescription", NEXTARG, A_WIREGUARD, setwgpeerdesc},
> + { "wgdescr", NEXTARG, A_WIREGUARD, setwgpeerdesc},
> { "wgendpoint", NEXTARG2, A_WIREGUARD, NULL, setwgpeerep},
> { "wgaip", NEXTARG, A_WIREGUARD, setwgpeeraip},
> { "wgpsk", NEXTARG, A_WIREGUARD, setwgpeerpsk},
> @@ -627,7 +631,8 @@ const struct cmd {
> { "wgkey", NEXTARG, A_WIREGUARD, setwgkey},
> { "wgrtable", NEXTARG, A_WIREGUARD, setwgrtable},
> { "-wgpeer", NEXTARG, A_WIREGUARD, unsetwgpeer},
> - { "-wgpsk", 0, A_WIREGUARD, unsetwgpeerpsk},
> + { "-wgdescription", 0, A_WIREGUARD, unsetwgpeerdesc},
> + { "-wgdescr", 0, A_WIREGUARD, unsetwgpeerdesc},
> { "-wgpeerall", 0, A_WIREGUARD, unsetwgpeerall},
>
> #else /* SMALL */
> @@ -5736,6 +5741,15 @@ setwgpeer(const char *peerkey_b64, int p
> }
>
> void
> +setwgpeerdesc(const char *descr, int param)
> +{
> + if (wg_peer == NULL)
> + errx(1, "wgdescr: wgpeer not set");
> + wg_peer->p_flags |= WG_PEER_SET_DESCRIPTION;
> + strlcpy(wg_peer->p_description, descr, IFDESCRSIZE);
> +}
> +
> +void
> setwgpeeraip(const char *aip, int param)
> {
> int res;
> @@ -5839,6 +5853,15 @@ unsetwgpeer(const char *peerkey_b64, int
> }
>
> void
> +unsetwgpeerdesc(const char *descr, int param)
> +{
> + if (wg_peer == NULL)
> + errx(1, "wgdescr: wgpeer not set");
> + wg_peer->p_flags |= WG_PEER_SET_DESCRIPTION;
> + strlcpy(wg_peer->p_description, "", IFDESCRSIZE);
> +}
> +
> +void
> unsetwgpeerpsk(const char *value, int param)
> {
> if (wg_peer == NULL)
> @@ -5907,6 +5930,9 @@ wg_status(int ifaliases)
> b64_ntop(wg_peer->p_public, WG_KEY_LEN,
> key, sizeof(key));
> printf("\twgpeer %s\n", key);
> +
> + if (strlen(wg_peer->p_description))
> + printf("\t\twgdescr: %s\n",
> wg_peer->p_description);
Looks like a line that is too long but please double check.
>
> if (wg_peer->p_flags & WG_PEER_HAS_PSK)
> printf("\t\twgpsk (present)\n");
>
--
:wq Claudio
