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 <csnider@mirantis.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

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 {

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
 }

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 {

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

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.

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")
 	}

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.

daemon/libnetwork/internal/nftables/testdata/TestTableUseAfterClose.golden

@@ -0,0 +1,4 @@
+table ip this_is_a_table {
+	chain this_is_a_chain {
+	}
+}