From 7f2eb1d87a98c563072f0a0e4014c874cc06897c Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 5 May 2021 21:00:49 -0700 Subject: [PATCH] net/tstun: fix TUN log spam when ACLs drop a packet Whenever we dropped a packet due to ACLs, wireguard-go was logging: Failed to write packet to TUN device: packet dropped by filter Instead, just lie to wireguard-go and pretend everything is okay. Fixes #1229 Signed-off-by: Brad Fitzpatrick --- net/tstun/wrap.go | 19 ++++++++++++++----- net/tstun/wrap_test.go | 11 +++++++---- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/net/tstun/wrap.go b/net/tstun/wrap.go index 3c3e13895..1dbe30a1c 100644 --- a/net/tstun/wrap.go +++ b/net/tstun/wrap.go @@ -457,13 +457,22 @@ func (t *Wrapper) filterIn(buf []byte) filter.Response { // like wireguard-go/tun.Device.Write. func (t *Wrapper) Write(buf []byte, offset int) (int, error) { if !t.disableFilter { - res := t.filterIn(buf[offset:]) - if res == filter.DropSilently { + if t.filterIn(buf[offset:]) != filter.Accept { + // If we're not accepting the packet, lie to wireguard-go and pretend + // that everything is okay with a nil error, so wireguard-go + // doesn't log about this Write "failure". + // + // We return len(buf), but the ill-defined wireguard-go/tun.Device.Write + // method doesn't specify how the offset affects the return value. + // In fact, the Linux implementation does one of two different things depending + // on how the /dev/net/tun was created. But fortunately the wireguard-go + // code ignores the int return and only looks at the error: + // + // device/receive.go: _, err = device.tun.device.Write(....) + // + // TODO(bradfitz): fix upstream interface docs, implementation. return len(buf), nil } - if res != filter.Accept { - return 0, ErrFiltered - } } t.noteActivity() diff --git a/net/tstun/wrap_test.go b/net/tstun/wrap_test.go index 66330bb55..c6ce0590d 100644 --- a/net/tstun/wrap_test.go +++ b/net/tstun/wrap_test.go @@ -329,11 +329,14 @@ func TestFilter(t *testing.T) { var filtered bool if tt.dir == in { + // Use the side effect of updating the last + // activity atomic to determine whether the + // data was actually filtered. + // If it stays zero, nothing made it through + // to the wrapped TUN. + atomic.StoreInt64(&tun.lastActivityAtomic, 0) _, err = tun.Write(tt.data, 0) - if err == ErrFiltered { - filtered = true - err = nil - } + filtered = atomic.LoadInt64(&tun.lastActivityAtomic) == 0 } else { chtun.Outbound <- tt.data n, err = tun.Read(buf[:], 0)