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

> On 30 Dec 2024, at 19:32, Stuart Henderson <stu@spacehopper.org> wrote:
> 
> 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.

He needs this macro unrolling to use newly introduced wg_log_ip().

> 
>>> +		{
>>> +			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?

1. peer->p_endpoint should be protected with p_endpoint_mtx mutex(9).
2. Do we need to output ip addresses? I mean outside wg(4) too. If
so, isn’t it better to teach kprintf() something like '%pI{4,6}’
format?