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 { + } +}