Merge pull request containerd#3364 from apostasie/hardening-filestore

Add a generic abstraction for filesystem based stores

Author: fahedouch

cmd/nerdctl/namespace/namespace.go

@@ -18,7 +18,6 @@ package namespace
 
 import (
 	"fmt"
-	"os"
 	"sort"
 	"strings"
 	"text/tabwriter"
@@ -120,13 +119,10 @@ func namespaceLsAction(cmd *cobra.Command, args []string) error {
 		if err != nil {
 			log.L.Warn(err)
 		} else {
-			volEnts, err := volStore.List(false)
+			numVolumes, err = volStore.Count()
 			if err != nil {
-				if !os.IsNotExist(err) {
-					log.L.Warn(err)
-				}
+				log.L.Warn(err)
 			}
-			numVolumes = len(volEnts)
 		}
 
 		labels, err := client.NamespaceService().Labels(ctx, ns)

pkg/cmd/compose/compose.go

@@ -77,15 +77,8 @@ func New(client *containerd.Client, globalOptions types.GlobalCommandOptions, op
 	if err != nil {
 		return nil, err
 	}
-	options.VolumeExists = func(volName string) (bool, error) {
-		_, volGetErr := volStore.Get(volName, false)
-		if volGetErr == nil {
-			return true, nil
-		} else if errors.Is(volGetErr, errdefs.ErrNotFound) {
-			return false, nil
-		}
-		return false, volGetErr
-	}
+	// FIXME: this is racy. See note in up_volume.go
+	options.VolumeExists = volStore.Exists
 
 	options.ImageExists = func(ctx context.Context, rawRef string) (bool, error) {
 		refNamed, err := referenceutil.ParseAny(rawRef)

pkg/cmd/container/create.go

@@ -59,12 +59,14 @@ import (
 	"github.com/containerd/nerdctl/v2/pkg/platformutil"
 	"github.com/containerd/nerdctl/v2/pkg/referenceutil"
 	"github.com/containerd/nerdctl/v2/pkg/rootlessutil"
+	"github.com/containerd/nerdctl/v2/pkg/store"
 	"github.com/containerd/nerdctl/v2/pkg/strutil"
 )
 
 // Create will create a container.
 func Create(ctx context.Context, client *containerd.Client, args []string, netManager containerutil.NetworkOptionsManager, options types.ContainerCreateOptions) (containerd.Container, func(), error) {
-	// Acquire an exclusive lock on the volume store until we are done to avoid being raced by other volume operations
+	// Acquire an exclusive lock on the volume store until we are done to avoid being raced by any other
+	// volume operations (or any other operation involving volume manipulation)
 	volStore, err := volume.Store(options.GOptions.Namespace, options.GOptions.DataRoot, options.GOptions.Address)
 	if err != nil {
 		return nil, nil, err
@@ -73,7 +75,7 @@ func Create(ctx context.Context, client *containerd.Client, args []string, netMa
 	if err != nil {
 		return nil, nil, err
 	}
-	defer volStore.Unlock()
+	defer volStore.Release()
 
 	// simulate the behavior of double dash
 	newArg := []string{}
@@ -840,7 +842,8 @@ func generateGcFunc(ctx context.Context, container containerd.Container, ns, id,
 			if containerNameStore, errE = namestore.New(dataStore, ns); errE != nil {
 				log.G(ctx).WithError(errE).Warnf("failed to instantiate container name store during cleanup for container %q", id)
 			}
-			if errE = containerNameStore.Release(name, id); errE != nil {
+			// Double-releasing may happen with containers started with --rm, so, ignore NotFound errors
+			if errE := containerNameStore.Release(name, id); errE != nil && !errors.Is(errE, store.ErrNotFound) {
 				log.G(ctx).WithError(errE).Warnf("failed to release container name store for container %q (%s)", name, id)
 			}
 		}

pkg/cmd/container/remove.go

@@ -32,13 +32,14 @@ import (
 
 	"github.com/containerd/nerdctl/v2/pkg/api/types"
 	"github.com/containerd/nerdctl/v2/pkg/clientutil"
-	"github.com/containerd/nerdctl/v2/pkg/cmd/volume"
 	"github.com/containerd/nerdctl/v2/pkg/containerutil"
 	"github.com/containerd/nerdctl/v2/pkg/dnsutil/hostsstore"
 	"github.com/containerd/nerdctl/v2/pkg/idutil/containerwalker"
 	"github.com/containerd/nerdctl/v2/pkg/ipcutil"
 	"github.com/containerd/nerdctl/v2/pkg/labels"
+	"github.com/containerd/nerdctl/v2/pkg/mountutil/volumestore"
 	"github.com/containerd/nerdctl/v2/pkg/namestore"
+	"github.com/containerd/nerdctl/v2/pkg/store"
 )
 
 var _ error = ErrContainerStatus{}
@@ -128,18 +129,10 @@ func RemoveContainer(ctx context.Context, c containerd.Container, globalOptions
 		return err
 	}
 	// Get volume store
-	volStore, err := volume.Store(globalOptions.Namespace, globalOptions.DataRoot, globalOptions.Address)
+	volStore, err := volumestore.New(dataStore, globalOptions.Namespace)
 	if err != nil {
 		return err
 	}
-	// Note: technically, it is not strictly necessary to acquire an exclusive lock on the volume store here.
-	// Worst case scenario, we would fail removing anonymous volumes later on, which is a soft error, and which would
-	// only happen if we concurrently tried to remove the same container.
-	err = volStore.Lock()
-	if err != nil {
-		return err
-	}
-	defer volStore.Unlock()
 	// Decode IPC
 	ipc, err := ipcutil.DecodeIPCLabel(containerLabels[labels.IPC])
 	if err != nil {
@@ -212,24 +205,34 @@ func RemoveContainer(ctx context.Context, c containerd.Container, globalOptions
 
 		// Enforce release name here in case the poststop hook name release fails - soft failure
 		if name != "" {
-			if err = nameStore.Release(name, id); err != nil {
+			// Double-releasing may happen with containers started with --rm, so, ignore NotFound errors
+			if err := nameStore.Release(name, id); err != nil && !errors.Is(err, store.ErrNotFound) {
 				log.G(ctx).WithError(err).Warnf("failed to release container name %s", name)
 			}
 		}
 
-		// De-allocate hosts file - soft failure
-		if err = hostsstore.DeallocHostsFile(dataStore, containerNamespace, id); err != nil {
+		hs, err := hostsstore.New(dataStore, containerNamespace)
+		if err != nil {
+			log.G(ctx).WithError(err).Warnf("failed to instantiate hostsstore for %q", containerNamespace)
+		} else if err = hs.DeallocHostsFile(id); err != nil {
+			// De-allocate hosts file - soft failure
 			log.G(ctx).WithError(err).Warnf("failed to remove hosts file for container %q", id)
 		}
 
 		// Volume removal is not handled by the poststop hook lifecycle because it depends on removeAnonVolumes option
+		// Note that the anonymous volume list has been obtained earlier, without locking the volume store.
+		// Technically, a concurrent operation MAY have deleted these anonymous volumes already at this point, which
+		// would make this operation here "soft fail".
+		// This is not a problem per-se, though we will output a warning in that case.
 		if anonVolumesJSON, ok := containerLabels[labels.AnonymousVolumes]; ok && removeAnonVolumes {
 			var anonVolumes []string
 			if err = json.Unmarshal([]byte(anonVolumesJSON), &anonVolumes); err != nil {
 				log.G(ctx).WithError(err).Warnf("failed to unmarshall anonvolume information for container %q", id)
 			} else {
 				var errs []error
-				_, errs, err = volStore.Remove(anonVolumes)
+				_, errs, err = volStore.Remove(func() ([]string, []error, error) {
+					return anonVolumes, nil, nil
+				})
 				if err != nil || len(errs) > 0 {
 					log.G(ctx).WithError(err).Warnf("failed to remove anonymous volumes %v", anonVolumes)
 				}

pkg/cmd/container/rename.go

@@ -44,7 +44,7 @@ func Rename(ctx context.Context, client *containerd.Client, containerID, newCont
 	if err != nil {
 		return err
 	}
-	hostst, err := hostsstore.NewStore(dataStore)
+	hostst, err := hostsstore.New(dataStore, options.GOptions.Namespace)
 	if err != nil {
 		return err
 	}
@@ -84,7 +84,7 @@ func renameContainer(ctx context.Context, container containerd.Container, newNam
 				labels.Name: name,
 			}
 			namst.Rename(newName, id, name)
-			hostst.Update(ns, id, name)
+			hostst.Update(id, name)
 			container.SetLabels(ctx, lbls)
 		}
 	}()
@@ -93,7 +93,7 @@ func renameContainer(ctx context.Context, container containerd.Container, newNam
 		return err
 	}
 	if runtime.GOOS == "linux" {
-		if err = hostst.Update(ns, id, newName); err != nil {
+		if err = hostst.Update(id, newName); err != nil {
 			log.G(ctx).WithError(err).Warn("failed to update host networking definitions " +
 				"- if your container is using network 'none', this is expected - otherwise, please report this as a bug")
 		}

pkg/cmd/container/run_mount.go

@@ -258,7 +258,7 @@ func generateMountOpts(ctx context.Context, client *containerd.Client, ensuredIm
 
 		log.G(ctx).Debugf("creating anonymous volume %q, for \"VOLUME %s\"",
 			anonVolName, imgVolRaw)
-		anonVol, err := volStore.Create(anonVolName, []string{})
+		anonVol, err := volStore.CreateWithoutLock(anonVolName, []string{})
 		if err != nil {
 			return nil, nil, nil, err
 		}

pkg/cmd/volume/prune.go

@@ -19,10 +19,12 @@ package volume
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	containerd "github.com/containerd/containerd/v2/client"
 
 	"github.com/containerd/nerdctl/v2/pkg/api/types"
+	"github.com/containerd/nerdctl/v2/pkg/inspecttypes/native"
 	"github.com/containerd/nerdctl/v2/pkg/labels"
 )
 
@@ -33,60 +35,50 @@ func Prune(ctx context.Context, client *containerd.Client, options types.VolumeP
 	if err != nil {
 		return err
 	}
-	err = volStore.Lock()
-	if err != nil {
-		return err
-	}
-	defer volStore.Unlock()
 
-	// Get containers and see which volumes are used
-	containers, err := client.Containers(ctx)
-	if err != nil {
-		return err
-	}
+	var toRemove []string // nolint: prealloc
 
-	usedVolumesList, err := usedVolumes(ctx, containers)
-	if err != nil {
-		return err
-	}
-	var removeNames []string // nolint: prealloc
-
-	// Get the list of known volumes from the store
-	volumes, err := volStore.List(false)
-	if err != nil {
-		return err
-	}
+	err = volStore.Prune(func(volumes []*native.Volume) ([]string, error) {
+		// Get containers and see which volumes are used
+		containers, err := client.Containers(ctx)
+		if err != nil {
+			return nil, err
+		}
 
-	// Iterate through the known volumes, making sure we do not remove in-use volumes
-	// but capture as well anon volumes (if --all was passed)
-	for _, volume := range volumes {
-		if _, ok := usedVolumesList[volume.Name]; ok {
-			continue
+		usedVolumesList, err := usedVolumes(ctx, containers)
+		if err != nil {
+			return nil, err
 		}
-		if !options.All {
-			if volume.Labels == nil {
+
+		for _, volume := range volumes {
+			if _, ok := usedVolumesList[volume.Name]; ok {
 				continue
 			}
-			val, ok := (*volume.Labels)[labels.AnonymousVolumes]
-			//skip the named volume and only remove the anonymous volume
-			if !ok || val != "" {
-				continue
+			if !options.All {
+				if volume.Labels == nil {
+					continue
+				}
+				val, ok := (*volume.Labels)[labels.AnonymousVolumes]
+				// skip the named volume and only remove the anonymous volume
+				if !ok || val != "" {
+					continue
+				}
 			}
+			toRemove = append(toRemove, volume.Name)
 		}
-		removeNames = append(removeNames, volume.Name)
-	}
 
-	// Remove the volumes from that list
-	removedNames, _, err := volStore.Remove(removeNames)
+		return toRemove, nil
+	})
+
 	if err != nil {
 		return err
 	}
-	if len(removedNames) > 0 {
+
+	if len(toRemove) > 0 {
 		fmt.Fprintln(options.Stdout, "Deleted Volumes:")
-		for _, name := range removedNames {
-			fmt.Fprintln(options.Stdout, name)
-		}
+		fmt.Fprintln(options.Stdout, strings.Join(toRemove, "\n"))
 		fmt.Fprintln(options.Stdout, "")
 	}
+
 	return nil
 }

pkg/cmd/volume/rm.go

@@ -33,45 +33,38 @@ import (
 )
 
 func Remove(ctx context.Context, client *containerd.Client, volumes []string, options types.VolumeRemoveOptions) error {
-	// Get the volume store and lock it until we are done.
-	// This will prevent racing new containers from being created or removed until we are done with the cleanup of volumes
 	volStore, err := Store(options.GOptions.Namespace, options.GOptions.DataRoot, options.GOptions.Address)
 	if err != nil {
 		return err
 	}
-	err = volStore.Lock()
-	if err != nil {
-		return err
-	}
-	defer volStore.Unlock()
 
-	// Get containers and see which volumes are used
 	containers, err := client.Containers(ctx)
 	if err != nil {
 		return err
 	}
 
-	usedVolumesList, err := usedVolumes(ctx, containers)
-	if err != nil {
-		return err
-	}
-
-	volumeNames := []string{}
-	cannotRemove := []error{}
+	// Note: to avoid racy behavior, this is called by volStore.Remove *inside a lock*
+	removableVolumes := func() (volumeNames []string, cannotRemove []error, err error) {
+		usedVolumesList, err := usedVolumes(ctx, containers)
+		if err != nil {
+			return nil, nil, err
+		}
 
-	for _, name := range volumes {
-		if _, ok := usedVolumesList[name]; ok {
-			cannotRemove = append(cannotRemove, fmt.Errorf("volume %q is in use (%w)", name, errdefs.ErrFailedPrecondition))
-			continue
+		for _, name := range volumes {
+			if _, ok := usedVolumesList[name]; ok {
+				cannotRemove = append(cannotRemove, fmt.Errorf("volume %q is in use (%w)", name, errdefs.ErrFailedPrecondition))
+				continue
+			}
+			volumeNames = append(volumeNames, name)
 		}
-		volumeNames = append(volumeNames, name)
+
+		return volumeNames, cannotRemove, nil
 	}
-	// if err is set, this is a hard filesystem error
-	removedNames, warns, err := volStore.Remove(volumeNames)
+
+	removedNames, cannotRemove, err := volStore.Remove(removableVolumes)
 	if err != nil {
 		return err
 	}
-	cannotRemove = append(cannotRemove, warns...)
 	// Otherwise, output on stdout whatever was successful
 	for _, name := range removedNames {
 		fmt.Fprintln(options.Stdout, name)

pkg/composer/down.go

@@ -128,6 +128,7 @@ func (c *Composer) downVolume(ctx context.Context, shortName string) error {
 	}
 	// shortName is like "db_data", fullName is like "compose-wordpress_db_data"
 	fullName := vol.Name
+	// FIXME: this is racy. See note in up_volume.go
 	volExists, err := c.VolumeExists(fullName)
 	if err != nil {
 		return err

pkg/composer/up_volume.go

@@ -42,9 +42,9 @@ func (c *Composer) upVolume(ctx context.Context, shortName string) error {
 
 	// shortName is like "db_data", fullName is like "compose-wordpress_db_data"
 	fullName := vol.Name
-	// FIXME: this is racy. By the time we get below to creating the volume, there is no guarantee that things are still fine
-	// Furthermore, volStore.Get no longer errors if the volume already exists (docker behavior), so, the purpose of this
-	// call needs to be assessed (it might still error if the name is malformed, or if there is a filesystem error)
+	// FIXME: this is racy. By the time we get below to creating the volume, there is no guarantee that things are still fine.
+	// Furthermore, by the time we are done creating all the volumes, they may very well have been destroyed.
+	// This cannot be fixed without getting rid of the whole "shell-out" approach entirely.
 	volExists, err := c.VolumeExists(fullName)
 	if err != nil {
 		return err