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.
> 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 30 May 2023 15:37:41 -0000
> @@ -221,6 +221,9 @@ struct wg_peer {
>
> SLIST_ENTRY(wg_peer) p_start_list;
> int p_start_onlist;
> +
> + struct mutex p_description_mtx;
> + char p_description[IFDESCRSIZE];
> };
>
> struct wg_softc {
> @@ -275,6 +278,8 @@ int wg_peer_get_sockaddr(struct wg_peer
> void wg_peer_clear_src(struct wg_peer *);
> void wg_peer_get_endpoint(struct wg_peer *, struct wg_endpoint *);
> void wg_peer_counters_add(struct wg_peer *, uint64_t, uint64_t);
> +void wg_peer_set_description(struct wg_peer *, const char *);
> +void wg_peer_get_description(struct wg_peer *, char *);
>
> int wg_aip_add(struct wg_softc *, struct wg_peer *, struct wg_aip_io *);
> struct wg_peer *
> @@ -407,6 +412,9 @@ wg_peer_create(struct wg_softc *sc, uint
> peer->p_counters_tx = 0;
> peer->p_counters_rx = 0;
>
> + mtx_init(&peer->p_description_mtx, IPL_NET);
> + strlcpy(peer->p_description, "", IFDESCRSIZE);
> +
> mtx_init(&peer->p_endpoint_mtx, IPL_NET);
> bzero(&peer->p_endpoint, sizeof(peer->p_endpoint));
>
> @@ -581,6 +589,22 @@ wg_peer_counters_add(struct wg_peer *pee
> mtx_leave(&peer->p_counters_mtx);
> }
>
> +void
> +wg_peer_set_description(struct wg_peer *peer, const char *description)
> +{
> + mtx_enter(&peer->p_description_mtx);
> + strlcpy(peer->p_description, description, IFDESCRSIZE);
> + mtx_leave(&peer->p_description_mtx);
> +}
> +
> +void
> +wg_peer_get_description(struct wg_peer *peer, char *description)
> +{
> + mtx_enter(&peer->p_description_mtx);
> + strlcpy(description, peer->p_description, IFDESCRSIZE);
> + mtx_leave(&peer->p_description_mtx);
> +}
> +
> int
> wg_aip_add(struct wg_softc *sc, struct wg_peer *peer, struct wg_aip_io *d)
> {
> @@ -2320,6 +2344,9 @@ wg_ioctl_set(struct wg_softc *sc, struct
> }
> }
>
> + if (peer_o.p_flags & WG_PEER_SET_DESCRIPTION)
> + wg_peer_set_description(peer, peer_o.p_description);
> +
> 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 +2456,8 @@ wg_ioctl_get(struct wg_softc *sc, struct
> aip_count++;
> }
> peer_o.p_aips_count = aip_count;
> +
> + wg_peer_get_description(peer, peer_o.p_description);
>
> if ((ret = copyout(&peer_o, peer_p, sizeof(peer_o))) != 0)
> goto unlock_and_ret_size;
--
:wq Claudio
