From: Stuart Henderson <stu@spacehopper.org>
Subject: Re: wg(4) logging enhancement - revised patch
To: Vitaliy Makkoveev <otto@bsdbox.dev>
Cc: Lloyd <ng2d68@proton.me>, "tech@openbsd.org" <tech@openbsd.org>
Date: Mon, 30 Dec 2024 16:32:07 +0000

On 2024/12/30 06:56, Vitaliy Makkoveev wrote:
> Please, rework it according style(9).
> 
> > On 30 Dec 2024, at 01:29, Lloyd <ng2d68@proton.me> wrote:
> > 
> > Some tweaks made to the log level - moves chatter to DEBUG with security-relevant messages at INFO and WARNING levels. Either way wg(4) will no longer dump bluetext to the console. Tested with both IPv4 and IPv6 endpoints.
> > 
> > --- if_wg.c.orig	Thu Oct 31 12:33:11 2024
> > +++ if_wg.c	Sun Dec 29 20:47:57 2024
> > @@ -30,6 +30,8 @@
> > #include <sys/percpu.h>
> > #include <sys/ioctl.h>
> > #include <sys/mbuf.h>
> > +#include <sys/syslog.h>
> > +#include <sys/time.h>
> > 
> > #include <net/if.h>
> > #include <net/if_var.h>
> > @@ -70,9 +72,6 @@
> > #define NEW_HANDSHAKE_TIMEOUT	(REKEY_TIMEOUT + KEEPALIVE_TIMEOUT)
> > #define UNDERLOAD_TIMEOUT	1
> > 
> > -#define DPRINTF(sc, str, ...) do { if (ISSET((sc)->sc_if.if_flags, IFF_DEBUG))\
> > -    printf("%s: " str, (sc)->sc_if.if_xname, ##__VA_ARGS__); } while (0)
> > -
> > #define CONTAINER_OF(ptr, type, member) ({			\
> > 	const __typeof( ((type *)0)->member ) *__mptr = (ptr);	\
> > 	(type *)( (char *)__mptr - offsetof(type,member) );})
> > @@ -371,6 +370,7 @@ void	wg_down(struct wg_softc *);
> > int	wg_clone_create(struct if_clone *, int);
> > int	wg_clone_destroy(struct ifnet *);
> > void	wgattach(int);
> > +void wg_log_ip(struct wg_endpoint *);
> > 
> > uint64_t	peer_counter = 0;
> > struct pool	wg_aip_pool;
> > @@ -448,7 +448,12 @@ wg_peer_create(struct wg_softc *sc, uint8_t public[WG_
> > 	sc->sc_peer_num++;
> > 	rw_exit_write(&sc->sc_peer_lock);
> > 
> > -	DPRINTF(sc, "Peer %llu created\n", peer->p_id);
> > +	if ( ISSET(sc->sc_if.if_flags, IFF_DEBUG) )
> > +	{

Rather than sprinkling ISSET(...IFF_DEBUG) all over the place and adding
extra indented blocks, using something analogous to the DPRINTF macro
would be easier to read, and probably reduce the number of times it
overflows to another line.

> > +		{
> > +			log(LOG_INFO,
> > +				"%s: Handshake for peer %llu (",
> > +				peer->p_sc->sc_if.if_xname,
> > +				peer->p_id);
> > +			wg_log_ip(&(peer->p_endpoint));
> > +			addlog(") did not complete after %d seconds, retrying (try %d)\n",
> > +				REKEY_TIMEOUT, t->t_handshake_retries + 1);
> > +		}
> > 
> > -		DPRINTF(peer->p_sc, "Handshake for peer %llu did not complete "
> > -		    "after %d seconds, retrying (try %d)\n", peer->p_id,
> > -		    REKEY_TIMEOUT, t->t_handshake_retries + 1);
> > 		wg_peer_clear_src(peer);
> > 		wg_timers_run_send_initiation(t, 1);

I don't know wg(4) particularly well, but are there any concerns
regarding locking with things like this - i.e. 1, accessing information
from peer->p_endpoint, and 2. building up the log entry using addlog?