From f53e78e0d559f01d2b069048b1b7107e9ec86fe4 Mon Sep 17 00:00:00 2001 From: Avery Pennarun Date: Wed, 25 Mar 2020 03:47:55 -0400 Subject: [PATCH] wgengine: don't lose filter state on filter reconfig. We were abandoning the UDP port LRU every time we got a new packet filter from tailcontrol, which caused return packets to suddenly stop arriving. --- ipn/local.go | 2 +- wgengine/filter/filter.go | 42 +++++++++++++++++++++++----------- wgengine/filter/filter_test.go | 2 +- wgengine/userspace.go | 7 ++++++ wgengine/watchdog.go | 5 ++++ wgengine/wgengine.go | 3 +++ 6 files changed, 46 insertions(+), 15 deletions(-) diff --git a/ipn/local.go b/ipn/local.go index 1ccb461c1..9d3703640 100644 --- a/ipn/local.go +++ b/ipn/local.go @@ -320,7 +320,7 @@ func (b *LocalBackend) updateFilter(netMap *controlclient.NetworkMap) { } else { b.logf("netmap packet filter: (suppressed)\n") } - b.e.SetFilter(filter.New(netMap.PacketFilter)) + b.e.SetFilter(filter.New(netMap.PacketFilter, b.e.GetFilter())) } } diff --git a/wgengine/filter/filter.go b/wgengine/filter/filter.go index ac0fd1282..6a908fcaa 100644 --- a/wgengine/filter/filter.go +++ b/wgengine/filter/filter.go @@ -15,11 +15,14 @@ import ( "tailscale.com/wgengine/packet" ) +type filterState struct { + mu sync.Mutex + lru *lru.Cache +} + type Filter struct { matches Matches - - udpMu sync.Mutex - udplru *lru.Cache + state *filterState } type Response int @@ -66,17 +69,25 @@ var MatchAllowAll = Matches{ } func NewAllowAll() *Filter { - return New(MatchAllowAll) + return New(MatchAllowAll, nil) } func NewAllowNone() *Filter { - return New(nil) + return New(nil, nil) } -func New(matches Matches) *Filter { +func New(matches Matches, shareStateWith *Filter) *Filter { + var state *filterState + if shareStateWith != nil { + state = shareStateWith.state + } else { + state = &filterState{ + lru: lru.New(LRU_MAX), + } + } f := &Filter{ matches: matches, - udplru: lru.New(LRU_MAX), + state: state, } return f } @@ -144,6 +155,10 @@ func (f *Filter) runIn(q *packet.QDecode) (r Response, why string) { switch q.IPProto { case packet.ICMP: // If any port is open to an IP, allow ICMP to it. + // TODO(apenwarr): allow ICMP packets on existing sessions. + // Right now ICMP Echo Response doesn't always work, and + // probably important ICMP responses on TCP sessions + // also get blocked. if matchIPWithoutPorts(f.matches, q) { return Accept, "icmp ok" } @@ -165,9 +180,9 @@ func (f *Filter) runIn(q *packet.QDecode) (r Response, why string) { case packet.UDP: t := tuple{q.SrcIP, q.DstIP, q.SrcPort, q.DstPort} - f.udpMu.Lock() - _, ok := f.udplru.Get(t) - f.udpMu.Unlock() + f.state.mu.Lock() + _, ok := f.state.lru.Get(t) + f.state.mu.Unlock() if ok { return Accept, "udp cached" @@ -182,12 +197,13 @@ func (f *Filter) runIn(q *packet.QDecode) (r Response, why string) { } func (f *Filter) runOut(q *packet.QDecode) (r Response, why string) { + // TODO(apenwarr): create sessions on ICMP Echo Request too. if q.IPProto == packet.UDP { t := tuple{q.DstIP, q.SrcIP, q.DstPort, q.SrcPort} - f.udpMu.Lock() - f.udplru.Add(t, t) - f.udpMu.Unlock() + f.state.mu.Lock() + f.state.lru.Add(t, t) + f.state.mu.Unlock() } return Accept, "ok out" } diff --git a/wgengine/filter/filter_test.go b/wgengine/filter/filter_test.go index 1833e9232..7dc0b099d 100644 --- a/wgengine/filter/filter_test.go +++ b/wgengine/filter/filter_test.go @@ -39,7 +39,7 @@ func TestFilter(t *testing.T) { {SrcIPs: []IP{0}, DstPorts: ippr(0, 443, 443)}, {SrcIPs: []IP{0x99010101, 0x99010102, 0x99030303}, DstPorts: ippr(0x01020304, 999, 999)}, } - acl := New(mm) + acl := New(mm, nil) for _, ent := range []Matches{Matches{mm[0]}, mm} { b, err := json.Marshal(ent) diff --git a/wgengine/userspace.go b/wgengine/userspace.go index 2092af963..6029ce8f7 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -36,6 +36,7 @@ type userspaceEngine struct { router Router magicConn *magicsock.Conn linkMon *monitor.Mon + filt *filter.Filter wgLock sync.Mutex // serializes all wgdev operations lastReconfig string @@ -380,7 +381,13 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, dnsDomains []string) error return nil } +func (e *userspaceEngine) GetFilter() *filter.Filter { + return e.filt +} + func (e *userspaceEngine) SetFilter(filt *filter.Filter) { + e.filt = filt + var filtin, filtout func(b []byte) device.FilterResult if filt == nil { e.logf("wgengine: nil filter provided; no access restrictions.\n") diff --git a/wgengine/watchdog.go b/wgengine/watchdog.go index 90bfc2344..bb2f95ff2 100644 --- a/wgengine/watchdog.go +++ b/wgengine/watchdog.go @@ -63,6 +63,11 @@ func (e *watchdogEngine) watchdog(name string, fn func()) { func (e *watchdogEngine) Reconfig(cfg *wgcfg.Config, dnsDomains []string) error { return e.watchdogErr("Reconfig", func() error { return e.wrap.Reconfig(cfg, dnsDomains) }) } +func (e *watchdogEngine) GetFilter() *filter.Filter { + var x *filter.Filter + e.watchdog("GetFilter", func() { x = e.wrap.GetFilter() }) + return x +} func (e *watchdogEngine) SetFilter(filt *filter.Filter) { e.watchdog("SetFilter", func() { e.wrap.SetFilter(filt) }) } diff --git a/wgengine/wgengine.go b/wgengine/wgengine.go index a3bcb9a86..2f215ad34 100644 --- a/wgengine/wgengine.go +++ b/wgengine/wgengine.go @@ -101,6 +101,9 @@ type Engine interface { // sends an updated network map. Reconfig(cfg *wgcfg.Config, dnsDomains []string) error + // GetFilter returns the current packet filter, if any + GetFilter() *filter.Filter + // SetFilter updates the packet filter. SetFilter(*filter.Filter)