Re: [PATCH] freebsd: Implement selection of FIB (routing table) for tunneled packets

["Jason A. Donenfeld" <Jason@zx2c4.com>]

Hi Frank,

On Sat, Mar 20, 2021 at 11:15 AM Frank Behrens <frank@harz.behrens.de> wrote:
>
> Hi Jason,
>
> thanks for your response.
>
> Am 19.03.2021 schrieb Jason A. Donenfeld:
> > In other words, you have push access to all branches beginning with fb/ .
> That works, thanks. Meanwhile I pushed my branch to fb/fib.
>
> > Right now we have the `wg set wg0 fwmark ...` mapped to
> > SO_USER_COOKIE, as I'm sure you saw there. But maybe FIB would be a
> > better thing to use for that? We could adjust wireguard-go to do the
> > same with the tuntap ioctl.
> I believe we have different, orthogonal things:
>
> 1. The selection of routing table (fib) for received, decrypted packets.
> -> Already implemented in wg_deliver_in() #2098 and controlled
> by "ifconfig wg0 fib 1"
>
> 2. The selection of routing table for outgoing, encrypted packets.
> -> That is addressed by my patch and controlled by
> "ifconfig wg0 tunnelfib 1". Maybe wg(8) should receive also
> an option for that purpose, if other OS use equivalent functions.
>
> 3. The setting of special marks, useable in packet filter/firewall
> processing. I guess, that is the meaning for "wg.. fwmark". I'm not
> sure, how best to implement that for FreeBSD. For ipfw(4) there is some
> functionality using socket cookies, as already implemented. For pf(4)
> packet filter the documentation mentions mbuf_tags(9). Apparently
> we need some input from a FreeBSD packet filter developer.

It looks like user_cookie is independently useful for use in ipfw, so
I'll keep fwmark mapped to that. But your tunnelfib patch is _also_
useful, so I'll apply this patch.

Some bugs/questions, though:

+static void
+wg_socket_setfib(struct wg_softc *sc)
+{
+ struct wg_socket *so = &sc->sc_socket;
+ struct socket *so4, *so6;
+ struct sockopt sopt;
+ int rc;
+
+ so4 = atomic_load_ptr(&so->so_so4);
+ so6 = atomic_load_ptr(&so->so_so6);

These are epoch-protected members. So the line above that should have
NET_EPOCH_ASSERT().

+ if (so4) {
+ rc = sosetopt(so4, &sopt);
+ MPASS(rc == 0);
+ }
+ if (so6) {
+ rc = sosetopt(so6, &sopt);
+ MPASS(rc == 0);
+ }

Rather than MPASS, this error should be passed back to the caller and
reported back to userspace, in the same way that we account for errors
with socket binding.

+ case SIOCSTUNFIB:
+ if ((ret = priv_check(curthread, PRIV_NET_SETIFFIB)) != 0)
+ break;

This priv check should be relative to our stored creds that control
the socket right? There are some jail considerations to think about
here.

+ if (ifr->ifr_fib >= rt_numfibs) {
+ ret = EINVAL;
+ } else {
+ sc->sc_fibnum = ifr->ifr_fib;
+ wg_socket_setfib(sc);
+ }

There are two ways to implement this. Either we check for 0 <= ifr_fib
< rt_numfibs, and then directly assign it to so->so_fibnum, like we do
for user_cookie, and avoid sosockopt. This sounds simplest and I
wouldn't mind doing that. Or we use sosockopt, but the bounds testing
remains in sosockopt and not duplicated here, and we rely on
wg_socket_setfib bubbling the error back up. Let's pick one of these
approaches, rather than combining them.


struct wg_softc {
LIST_ENTRY(wg_softc) sc_entry;
struct ifnet *sc_ifp;
int sc_flags;
+ u_int sc_fibnum;

Shouldn't this be added to `struct wg_socket`, alongside the
so_user_cookie member there, and managed in the same way?

Jason