From 558df8d6dea9863538b0e2e542dca90e0aaff6c4 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Tue, 28 Jul 2026 16:14:27 -0400 Subject: [PATCH] d/libn/i/nftables: make Table a reference type Table.Close() had a value receiver, so setting t.t = nil only modified the callee's copy of the handle. The caller's Table was left looking valid: t, _ := nftables.NewTable(...) t.Close() t.IsValid() // true Worse, Close() only dropped the nftables handle, and nftApply() opens a new one whenever it finds none. So Apply() and Reload() on a closed table silently reopened a handle and carried on updating the ruleset. At the root of it, a Table looked like a plain value but behaved like a reference, and nothing stopped it from being copied. So embed table in Table by value and hand out *Table instead. The Table/table split is still needed - table's fields have to be exported for text/template - but reference semantics are now visible at every call site, and they're enforced: because table contains a sync.Mutex, "go vet" reports both a copy of a Table and a method or function that takes one by value, so the shape of this bug is no longer expressible. Close() therefore can't invalidate the table by clearing a pointer, it has to record the state. Add a closed flag, and refuse to open a new nftables handle for a table that's been closed. Apply() checked neither for a closed table nor for a nil *Table, which would have panicked. It now reports an error, checking the closed state with applyLock held so that it can't race with Close(), and before the in-memory table is touched so that a rejected update isn't recorded as applied. The invalid table is now a nil *Table rather than a zero-value Table, which also removes the need for consumers to return an empty Table alongside an error. That made it obvious that the nftabler was leaking the table it had just created when it gave up on setting up IPv6, so close it. Signed-off-by: Cory Snider Co-Authored-By: Claude Opus 5 --- .../bridge/internal/nftabler/network.go | 4 +- .../bridge/internal/nftabler/nftabler.go | 22 ++-- .../drivers/bridge/internal/nftabler/port.go | 2 +- .../drivers/overlay/encryption_nft_linux.go | 8 +- daemon/libnetwork/drivers/overlay/overlay.go | 2 +- .../internal/nftables/nftables_linux.go | 110 +++++++++++++----- .../internal/nftables/nftables_linux_test.go | 91 +++++++++++++-- .../testdata/TestTableUseAfterClose.golden | 4 + 8 files changed, 187 insertions(+), 56 deletions(-) create mode 100644 daemon/libnetwork/internal/nftables/testdata/TestTableUseAfterClose.golden diff --git a/daemon/libnetwork/drivers/bridge/internal/nftabler/network.go b/daemon/libnetwork/drivers/bridge/internal/nftabler/network.go index 0f4c9663d3..4ef375bdbd 100644 --- a/daemon/libnetwork/drivers/bridge/internal/nftabler/network.go +++ b/daemon/libnetwork/drivers/bridge/internal/nftabler/network.go @@ -49,7 +49,7 @@ func (nft *Nftabler) NewNetwork(ctx context.Context, nc firewaller.NetworkConfig return n, nil } -func (n *network) configure(ctx context.Context, table nftables.Table, conf firewaller.NetworkConfigFam) (*nftables.Modifier, error) { +func (n *network) configure(ctx context.Context, table *nftables.Table, conf firewaller.NetworkConfigFam) (*nftables.Modifier, error) { if !conf.Prefix.IsValid() { return nil, nil } @@ -233,7 +233,7 @@ func (n *network) ReapplyNetworkLevelRules(ctx context.Context) error { } func (n *network) DelNetworkLevelRules(ctx context.Context) error { - remove := func(t nftables.Table, remover *nftables.Modifier) { + remove := func(t *nftables.Table, remover *nftables.Modifier) { if remover != nil { ctx := log.WithLogger(ctx, log.G(ctx).WithFields(log.Fields{"bridge": n.config.IfName})) if err := t.Apply(ctx, *remover); err != nil { diff --git a/daemon/libnetwork/drivers/bridge/internal/nftabler/nftabler.go b/daemon/libnetwork/drivers/bridge/internal/nftabler/nftabler.go index d7e97fbf91..675aaecc36 100644 --- a/daemon/libnetwork/drivers/bridge/internal/nftabler/nftabler.go +++ b/daemon/libnetwork/drivers/bridge/internal/nftabler/nftabler.go @@ -47,14 +47,20 @@ const ( type Nftabler struct { config firewaller.Config cleaner firewaller.FirewallCleaner - table4 nftables.Table - table6 nftables.Table + table4 *nftables.Table + table6 *nftables.Table } // NewNftabler creates a new Nftabler instance, initializing the nftables tables. // Call Close() on the returned Nftabler to release resources when done. -func NewNftabler(ctx context.Context, config firewaller.Config) (*Nftabler, error) { +func NewNftabler(ctx context.Context, config firewaller.Config) (_ *Nftabler, retErr error) { nft := &Nftabler{config: config} + defer func() { + if retErr != nil { + // Don't leave the tables set up so-far open. + _ = nft.Close() + } + }() if nft.config.IPv4 { var err error @@ -81,11 +87,11 @@ func (nft *Nftabler) Close() error { return errors.Join(nft.table4.Close(), nft.table6.Close()) } -func (nft *Nftabler) init(ctx context.Context, family nftables.Family) (nftables.Table, error) { +func (nft *Nftabler) init(ctx context.Context, family nftables.Family) (*nftables.Table, error) { // Instantiate the table. table, err := nftables.NewTable(family, dockerTable) if err != nil { - return table, err + return nil, err } tm := nftables.Modifier{} @@ -210,15 +216,17 @@ func (nft *Nftabler) init(ctx context.Context, family nftables.Family) (nftables } if err := table.Apply(ctx, tm); err != nil { + // The table's no use to anyone now, don't hold its resources open. + _ = table.Close() if family == nftables.IPv4 { - return nftables.Table{}, err + return nil, err } // Perhaps the kernel has no IPv6 support. It won't be possible to create IPv6 // networks without enabling ip6_tables in the kernel, or disabling ip6tables in // the daemon config. But, allow the daemon to start because IPv4 will work. So, // log the problem, and continue. log.G(ctx).WithError(err).Warn("ip6tables is enabled, but cannot set up IPv6 nftables table") - return nftables.Table{}, nil + return nil, nil } return table, nil } diff --git a/daemon/libnetwork/drivers/bridge/internal/nftabler/port.go b/daemon/libnetwork/drivers/bridge/internal/nftabler/port.go index 7bd54dfb1b..d03538a4b7 100644 --- a/daemon/libnetwork/drivers/bridge/internal/nftabler/port.go +++ b/daemon/libnetwork/drivers/bridge/internal/nftabler/port.go @@ -58,7 +58,7 @@ func splitByContainerFam(pbs []types.PortBinding) ([]types.PortBinding, []types. return pbs4, pbs6 } -func (n *network) setPerPortRules(ctx context.Context, pbs []types.PortBinding, table nftables.Table, ipv nftables.Family, unprotected, enable bool) error { +func (n *network) setPerPortRules(ctx context.Context, pbs []types.PortBinding, table *nftables.Table, ipv nftables.Family, unprotected, enable bool) error { tm := nftables.Modifier{} updater := tm.Create if !enable { diff --git a/daemon/libnetwork/drivers/overlay/encryption_nft_linux.go b/daemon/libnetwork/drivers/overlay/encryption_nft_linux.go index be3711390f..073df618af 100644 --- a/daemon/libnetwork/drivers/overlay/encryption_nft_linux.go +++ b/daemon/libnetwork/drivers/overlay/encryption_nft_linux.go @@ -22,7 +22,7 @@ const ( ) // ensureOverlayEncNftTable returns the overlay encryption nft table, running one-time setup on first use. -func (d *driver) ensureOverlayEncNftTable(ctx context.Context) (nftables.Table, error) { +func (d *driver) ensureOverlayEncNftTable(ctx context.Context) (*nftables.Table, error) { d.overlayEncNftInitMu.Lock() defer d.overlayEncNftInitMu.Unlock() if d.overlayEncNftTable.IsValid() { @@ -31,7 +31,7 @@ func (d *driver) ensureOverlayEncNftTable(ctx context.Context) (nftables.Table, v6, err := d.isIPv6Transport() if err != nil { - return nftables.Table{}, err + return nil, err } fam := nftables.IPv4 if v6 { @@ -39,7 +39,7 @@ func (d *driver) ensureOverlayEncNftTable(ctx context.Context) (nftables.Table, } t, err := nftables.NewTable(fam, nftOverlayTable) if err != nil { - return nftables.Table{}, err + return nil, err } tm := nftables.Modifier{} @@ -87,7 +87,7 @@ func (d *driver) ensureOverlayEncNftTable(ctx context.Context) (nftables.Table, if err := t.Apply(ctx, tm); err != nil { _ = t.Close() - return nftables.Table{}, err + return nil, err } d.overlayEncNftTable = t diff --git a/daemon/libnetwork/drivers/overlay/overlay.go b/daemon/libnetwork/drivers/overlay/overlay.go index 7618633710..980a71cf7e 100644 --- a/daemon/libnetwork/drivers/overlay/overlay.go +++ b/daemon/libnetwork/drivers/overlay/overlay.go @@ -45,7 +45,7 @@ type driver struct { keys []*key overlayEncNftInitMu sync.Mutex - overlayEncNftTable nftables.Table + overlayEncNftTable *nftables.Table // mu must be held when accessing the fields which follow it // in the struct definition. diff --git a/daemon/libnetwork/internal/nftables/nftables_linux.go b/daemon/libnetwork/internal/nftables/nftables_linux.go index a8330a3873..08cc87b4fb 100644 --- a/daemon/libnetwork/internal/nftables/nftables_linux.go +++ b/daemon/libnetwork/internal/nftables/nftables_linux.go @@ -55,6 +55,7 @@ import ( "strconv" "strings" "sync" + "sync/atomic" "text/template" "time" @@ -259,9 +260,9 @@ func RunCmd(ctx context.Context, nftCmd []byte) error { ////////////////////////////// // Tables -// table is the internal representation of an nftables table. -// Its elements need to be exported for use by text/template, but they should only be -// manipulated via exported methods. +// table is the internal representation of an nftables table, embedded in a +// [Table]. Its elements need to be exported for use by text/template, but they +// should only be manipulated via [Table]'s methods. type table struct { Name string Family Family @@ -275,11 +276,29 @@ type table struct { applyLock sync.Mutex nftHandle *nftCtx // applyLock must be held to access + // created is set by [NewTable] and never modified afterwards. It tells a + // real table apart from the zero value of a [Table], which is not usable. + created bool + // closed is set by [Table.Close] with applyLock held. It may be read without + // the lock, but a check that must not race with Close needs to hold it. + closed atomic.Bool } +var ( + // errInvalidTable is returned by operations on a [Table] that didn't come + // from [NewTable]. + errInvalidTable = errors.New("invalid table") + // errTableClosed is returned by operations on a [Table] that has been closed. + errTableClosed = errors.New("nftables table is closed") +) + // nftApply executes the nftables commands in nftCmd. // Acquire t.applyLock before calling this function. func (t *table) nftApply(ctx context.Context, nftCmd []byte) error { + // Don't open a new handle for a table that's been closed. + if t.closed.Load() { + return errTableClosed + } if t.nftHandle == nil { h, err := newNftCtx() if err != nil { @@ -290,17 +309,35 @@ func (t *table) nftApply(ctx context.Context, nftCmd []byte) error { return t.nftHandle.Apply(ctx, nftCmd) } -// Table is a handle for an nftables table. +// checkUsable returns an error describing why t can't be used - it didn't come +// from [NewTable], or it has been closed. +// +// Acquire t.applyLock before calling, unless a stale answer will do: without the +// lock, the table may be closed by the time the caller acts on a nil return. +func (t *table) checkUsable() error { + if !t.created { + return errInvalidTable + } + if t.closed.Load() { + return errTableClosed + } + return nil +} + +// Table is a handle for an nftables table. Create one using [NewTable], and +// release it using [Table.Close]. type Table struct { - t *table + t table } -// IsValid returns true if t is a valid reference to a table. -func (t Table) IsValid() bool { - return t.t != nil +// IsValid returns true if t refers to a usable table, that is, if it was +// returned by [NewTable] and has not been closed. +func (t *Table) IsValid() bool { + return t != nil && t.t.checkUsable() == nil } -// NewTable creates a new nftables table and returns a [Table] +// NewTable creates a new nftables table and returns a [Table]. Close it to +// release the resources it holds. // // See https://wiki.nftables.org/wiki-nftables/index.php/Configuring_tables // @@ -316,45 +353,47 @@ func (t Table) IsValid() bool { // // To fully delete an underlying nftables table, if one already exists, // use [Table.Reload] after creating the table. -func NewTable(family Family, name string) (Table, error) { - t := Table{ - t: &table{ +func NewTable(family Family, name string) (*Table, error) { + return &Table{ + t: table{ Name: name, Family: family, Maps: map[string]*nftMap{}, Sets: map[string]*set{}, Chains: map[string]*chain{}, MustFlush: true, + created: true, }, - } - return t, nil + }, nil } // Close releases resources associated with the table. It does not modify or delete // the underlying nftables table. -func (t Table) Close() error { - if t.IsValid() { - t.t.applyLock.Lock() - defer t.t.applyLock.Unlock() - if t.t.nftHandle != nil { - t.t.nftHandle.Close() - t.t.nftHandle = nil - } - t.t = nil +func (t *Table) Close() error { + if !t.IsValid() { + return nil } + t.t.applyLock.Lock() + defer t.t.applyLock.Unlock() + if t.t.nftHandle != nil { + t.t.nftHandle.Close() + t.t.nftHandle = nil + } + t.t.closed.Store(true) return nil } // Name returns the name of the table, or an empty string if t is not valid. -func (t Table) Name() string { +func (t *Table) Name() string { if !t.IsValid() { return "" } return t.t.Name } -// Family returns the address family of the nftables table described by [TableRef]. -func (t Table) Family() Family { +// Family returns the address family of the nftables table, or an empty string if +// t is not valid. +func (t *Table) Family() Family { if !t.IsValid() { return "" } @@ -364,7 +403,7 @@ func (t Table) Family() Family { // SetBaseChainPolicy sets the default policy for a base chain. The update // is applied immediately, unlike creation/deletion of objects via a [Modifier] // which are not applied until [Modifier.Apply] is called. -func (t Table) SetBaseChainPolicy(ctx context.Context, chainName string, policy BaseChainPolicy) error { +func (t *Table) SetBaseChainPolicy(ctx context.Context, chainName string, policy BaseChainPolicy) error { if !t.IsValid() { return errors.New("invalid table") } @@ -454,8 +493,17 @@ func (t *Table) Apply(ctx context.Context, tm ...Modifier) (retErr error) { if !Enabled() { return errors.New("nftables is not enabled") } + if t == nil { + return errInvalidTable + } t.t.applyLock.Lock() defer t.t.applyLock.Unlock() + // Check under the lock, so that the table can't be closed between here and + // the update. Bail out before touching the in-memory table, an update that + // can't be applied to nftables must not be recorded as applied. + if err := t.t.checkUsable(); err != nil { + return err + } var rollback []command defer func() { @@ -463,7 +511,7 @@ func (t *Table) Apply(ctx context.Context, tm ...Modifier) (retErr error) { return } for _, c := range slices.Backward(rollback) { - if _, err := c.rollback(ctx, t.t); err != nil { + if _, err := c.rollback(ctx, &t.t); err != nil { log.G(ctx).WithError(err).Error("Failed to roll back nftables updates") } } @@ -473,7 +521,7 @@ func (t *Table) Apply(ctx context.Context, tm ...Modifier) (retErr error) { // Apply tm's updates to the Table. for _, tmm := range tm { for _, cmd := range tmm.cmds { - applied, err := cmd.apply(ctx, t.t) + applied, err := cmd.apply(ctx, &t.t) if err != nil { return fmt.Errorf("rule from %s:%d: %w", cmd.callerFile, cmd.callerLine, err) } @@ -485,7 +533,7 @@ func (t *Table) Apply(ctx context.Context, tm ...Modifier) (retErr error) { // Update nftables. var buf bytes.Buffer - if err := incrementalUpdateTempl.Execute(&buf, t.t); err != nil { + if err := incrementalUpdateTempl.Execute(&buf, &t.t); err != nil { return fmt.Errorf("failed to execute template nft ruleset: %w", err) } @@ -515,7 +563,7 @@ func (t *Table) Apply(ctx context.Context, tm ...Modifier) (retErr error) { } // Reload deletes the table, then re-creates it, atomically. -func (t Table) Reload(ctx context.Context) error { +func (t *Table) Reload(ctx context.Context) error { if !Enabled() { return errors.New("nftables is not enabled") } diff --git a/daemon/libnetwork/internal/nftables/nftables_linux_test.go b/daemon/libnetwork/internal/nftables/nftables_linux_test.go index f8c376eef5..1192d95a9a 100644 --- a/daemon/libnetwork/internal/nftables/nftables_linux_test.go +++ b/daemon/libnetwork/internal/nftables/nftables_linux_test.go @@ -32,18 +32,18 @@ func testSetup(t *testing.T) func() { } } -func applyAndCheck(t *testing.T, goldenFilename string, tbl Table, tm ...Modifier) { +func applyAndCheck(t *testing.T, goldenFilename string, tbl *Table, tm ...Modifier) { t.Helper() - err := tbl.Apply(context.Background(), tm...) + err := tbl.Apply(t.Context(), tm...) assert.Check(t, err) res := icmd.RunCommand("nft", "list", "table", string(tbl.Family()), tbl.Name()) res.Assert(t, icmd.Success) golden.Assert(t, res.Combined(), goldenFilename) } -func reloadAndCheck(t *testing.T, goldenFilename string, tbl Table) { +func reloadAndCheck(t *testing.T, goldenFilename string, tbl *Table) { t.Helper() - err := tbl.Reload(context.Background()) + err := tbl.Reload(t.Context()) assert.Check(t, err) res := icmd.RunCommand("nft", "list", "table", string(tbl.Family()), tbl.Name()) res.Assert(t, icmd.Success) @@ -65,6 +65,77 @@ func TestTable(t *testing.T) { applyAndCheck(t, t.Name()+"/created6.golden", tbl6, Modifier{}) } +func TestTableClose(t *testing.T) { + // No nftables setup needed, closing a table that's never been applied doesn't + // run any "nft" commands. + tbl, err := NewTable(IPv4, "this_is_a_table") + assert.NilError(t, err) + assert.Assert(t, tbl.IsValid()) + + // Consumers hold references to the table, closing it must invalidate them all. + ref := tbl + + assert.NilError(t, tbl.Close()) + assert.Check(t, !tbl.IsValid(), "closed table should not be valid") + assert.Check(t, !ref.IsValid(), "reference to a closed table should not be valid") + + // Close is idempotent, and a nil *Table can be closed. + assert.NilError(t, tbl.Close()) + var nilTbl *Table + assert.NilError(t, nilTbl.Close()) + assert.Check(t, !nilTbl.IsValid()) + + // A Table that didn't come from NewTable is not usable either. + var zeroVal Table + assert.Check(t, !zeroVal.IsValid()) + assert.NilError(t, zeroVal.Close()) + + assert.Check(t, is.Equal(tbl.Name(), "")) + assert.Check(t, is.Equal(tbl.Family(), Family(""))) +} + +func TestTableUseAfterClose(t *testing.T) { + defer testSetup(t)() + + tbl, err := NewTable(IPv4, "this_is_a_table") + assert.NilError(t, err) + + const chainName = "this_is_a_chain" + var tm Modifier + tm.Create(Chain{Name: chainName}) + assert.NilError(t, tbl.Apply(t.Context(), tm)) + + ref := tbl + assert.NilError(t, tbl.Close()) + + // Operations on a closed table must be refused, rather than transparently + // opening a new handle to the underlying nftables table. + var tm2 Modifier + tm2.Create(Rule{Chain: chainName, Rule: []string{"counter"}}) + assert.Check(t, is.ErrorIs(tbl.Apply(context.Background(), tm2), errTableClosed)) + assert.Check(t, is.ErrorIs(ref.Apply(context.Background(), tm2), errTableClosed)) + assert.Check(t, is.Error(tbl.Reload(context.Background()), "invalid table")) + assert.Check(t, is.Error(tbl.SetBaseChainPolicy(context.Background(), chainName, BaseChainPolicyDrop), + "invalid table")) + assert.Check(t, is.Nil(tbl.t.nftHandle), "no nftables handle should have been opened") + + // The table itself is left alone by Close, but the rejected update must not + // have reached it. (The table's name and family can't be read from the closed + // handle.) + res := icmd.RunCommand("nft", "list", "table", string(IPv4), "this_is_a_table") + res.Assert(t, icmd.Success) + golden.Assert(t, res.Combined(), t.Name()+".golden") + + // A nil *Table, and a Table that didn't come from NewTable, must be refused + // rather than panicking on their unusable innards. + var nilTbl *Table + assert.Check(t, is.ErrorIs(nilTbl.Apply(context.Background(), tm2), errInvalidTable)) + assert.Check(t, is.Error(nilTbl.Reload(context.Background()), "invalid table")) + + unusable := &Table{} + assert.Check(t, is.ErrorIs(unusable.Apply(context.Background(), tm2), errInvalidTable)) +} + func TestChain(t *testing.T) { defer testSetup(t)() @@ -157,7 +228,7 @@ func TestIgnoreExist(t *testing.T) { tmErr := Modifier{} tmErr.Create(Rule{Chain: chainName, Rule: []string{"counter"}, IgnoreExist: true}) tmErr.Create(Rule{Chain: chainName}) - err = tbl.Apply(context.Background(), tm) + err = tbl.Apply(t.Context(), tm) assert.Check(t, err != nil, "Expected an error") // Reload, to flush table state. @@ -172,7 +243,7 @@ func TestIgnoreExist(t *testing.T) { tmReDel := Modifier{} tmReDel.Delete(Rule{Chain: chainName, Rule: []string{"counter"}, IgnoreExist: true}) tmReDel.Create(Rule{Chain: chainName}) - err = tbl.Apply(context.Background(), tmReDel) + err = tbl.Apply(t.Context(), tmReDel) assert.Check(t, err != nil, "Expected an error") // Reload, to flush table state. @@ -286,7 +357,7 @@ func TestReload(t *testing.T) { deleteTable() // Reconstruct the nftables table. - err = tbl.Reload(context.Background()) + err = tbl.Reload(t.Context()) assert.Check(t, err) res := icmd.RunCommand("nft", "list", "table", string(tbl.Family()), tbl.Name()) res.Assert(t, icmd.Success) @@ -319,7 +390,7 @@ func TestApplyMultipleModifiers(t *testing.T) { tm2.Create(Rule{Chain: "bogus", Rule: []string{"counter"}}) tm2.Create(Rule{Chain: chainName, Rule: []string{"reject"}}) - err = tbl.Apply(context.Background(), tm1, tm2) + err = tbl.Apply(t.Context(), tm1, tm2) assert.Check(t, err != nil, "Expected an error") // Verify the apply was a no-op: the table should not exist yet. @@ -356,7 +427,7 @@ func TestNetdevChain(t *testing.T) { applyAndCheck(t, t.Name()+"/created.golden", tbl, tm) icmd.RunCommand("nft", "flush", "ruleset").Assert(t, icmd.Success) - err = tbl.Reload(context.Background()) + err = tbl.Reload(t.Context()) assert.Check(t, err) res := icmd.RunCommand("nft", "list", "table", string(tbl.Family()), tbl.Name()) res.Assert(t, icmd.Success) @@ -739,7 +810,7 @@ func TestValidation(t *testing.T) { assert.NilError(t, err) defer tbl.Close() tm := Modifier{cmds: tc.cmds} - err = tbl.Apply(context.Background(), tm) + err = tbl.Apply(t.Context(), tm) assert.Check(t, err != nil, "expected error containing '%s'", tc.expErr) assert.Check(t, is.ErrorContains(err, tc.expErr)) // Check the table wasn't created. diff --git a/daemon/libnetwork/internal/nftables/testdata/TestTableUseAfterClose.golden b/daemon/libnetwork/internal/nftables/testdata/TestTableUseAfterClose.golden new file mode 100644 index 0000000000..148748dc0c --- /dev/null +++ b/daemon/libnetwork/internal/nftables/testdata/TestTableUseAfterClose.golden @@ -0,0 +1,4 @@ +table ip this_is_a_table { + chain this_is_a_chain { + } +}