Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

many: handlers and backend for kernel-modules components #13512

Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions kernel/kernel.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ import (
"os"
"path/filepath"
"regexp"
"strings"

"github.com/snapcore/snapd/snap"
"gopkg.in/yaml.v2"
)

Expand Down Expand Up @@ -77,3 +79,23 @@ func ReadInfo(kernelSnapRootDir string) (*Info, error) {
}
return InfoFromKernelYaml(content)
}

// KernelVersionFromPlaceInfo returns the kernel version for a mounted kernel
// snap (this would be the output if "uname -r" for a running kernel).
func KernelVersionFromPlaceInfo(spi snap.PlaceInfo) (string, error) {
systemMapPathPrefix := filepath.Join(spi.MountDir(), "System.map-")
matchPath := systemMapPathPrefix + "*"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of guessig, could we read the snap.yaml and use the version declared there, assuming we can rely on the version being set and matching what would be found if we chopped the file names into pieces.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately uname -r is not matching the version in snap.yaml and even if it did we might find non-Canonical kernels not following our conventions.

matches, err := filepath.Glob(matchPath)
if err != nil {
// could be only ErrBadPattern, should not really happen
return "", fmt.Errorf("internal error: %w", err)
}
if len(matches) != 1 {
return "", fmt.Errorf("number of matches for %s is %d", matchPath, len(matches))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe:

unexpected number of matches (%d) for kernel %s ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

}
version := strings.TrimPrefix(matches[0], systemMapPathPrefix)
if version == "" {
return "", fmt.Errorf("kernel version not set in %s", matches[0])
}
return version, nil
}
66 changes: 58 additions & 8 deletions kernel/kernel_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@ import (

. "gopkg.in/check.v1"

"github.com/snapcore/snapd/dirs"
"github.com/snapcore/snapd/kernel"
"github.com/snapcore/snapd/snap"
"github.com/snapcore/snapd/testutil"
)

func makeMockKernel(c *C, kernelYaml string, filesWithContent map[string]string) string {
Expand All @@ -47,9 +50,18 @@ func makeMockKernel(c *C, kernelYaml string, filesWithContent map[string]string)
return kernelRootDir
}

type kernelYamlTestSuite struct{}
type kernelTestSuite struct {
testutil.BaseTest
}

var _ = Suite(&kernelTestSuite{})

var _ = Suite(&kernelYamlTestSuite{})
func (s *kernelTestSuite) SetUpTest(c *C) {
s.BaseTest.SetUpTest(c)

dirs.SetRootDir(c.MkDir())
s.AddCleanup(func() { dirs.SetRootDir("") })
}

func TestCommand(t *testing.T) { TestingT(t) }

Expand All @@ -67,19 +79,19 @@ assets:
non-#alphanumeric:
`)

func (s *kernelYamlTestSuite) TestInfoFromKernelYamlSad(c *C) {
func (s *kernelTestSuite) TestInfoFromKernelYamlSad(c *C) {
ki, err := kernel.InfoFromKernelYaml([]byte("foo"))
c.Check(err, ErrorMatches, "(?m)cannot parse kernel metadata: .*")
c.Check(ki, IsNil)
}

func (s *kernelYamlTestSuite) TestInfoFromKernelYamlBadName(c *C) {
func (s *kernelTestSuite) TestInfoFromKernelYamlBadName(c *C) {
ki, err := kernel.InfoFromKernelYaml(mockInvalidKernelYaml)
c.Check(err, ErrorMatches, `invalid asset name "non-#alphanumeric", please use only alphanumeric characters and dashes`)
c.Check(ki, IsNil)
}

func (s *kernelYamlTestSuite) TestInfoFromKernelYamlHappy(c *C) {
func (s *kernelTestSuite) TestInfoFromKernelYamlHappy(c *C) {
ki, err := kernel.InfoFromKernelYaml(mockKernelYaml)
c.Check(err, IsNil)
c.Check(ki, DeepEquals, &kernel.Info{
Expand All @@ -95,13 +107,13 @@ func (s *kernelYamlTestSuite) TestInfoFromKernelYamlHappy(c *C) {
})
}

func (s *kernelYamlTestSuite) TestReadKernelYamlOptional(c *C) {
func (s *kernelTestSuite) TestReadKernelYamlOptional(c *C) {
ki, err := kernel.ReadInfo("this-path-does-not-exist")
c.Check(err, IsNil)
c.Check(ki, DeepEquals, &kernel.Info{})
}

func (s *kernelYamlTestSuite) TestReadKernelYamlSad(c *C) {
func (s *kernelTestSuite) TestReadKernelYamlSad(c *C) {
mockKernelSnapRoot := c.MkDir()
kernelYamlPath := filepath.Join(mockKernelSnapRoot, "meta/kernel.yaml")
err := os.MkdirAll(filepath.Dir(kernelYamlPath), 0755)
Expand All @@ -114,7 +126,7 @@ func (s *kernelYamlTestSuite) TestReadKernelYamlSad(c *C) {
c.Check(ki, IsNil)
}

func (s *kernelYamlTestSuite) TestReadKernelYamlHappy(c *C) {
func (s *kernelTestSuite) TestReadKernelYamlHappy(c *C) {
mockKernelSnapRoot := c.MkDir()
kernelYamlPath := filepath.Join(mockKernelSnapRoot, "meta/kernel.yaml")
err := os.MkdirAll(filepath.Dir(kernelYamlPath), 0755)
Expand All @@ -136,3 +148,41 @@ func (s *kernelYamlTestSuite) TestReadKernelYamlHappy(c *C) {
},
})
}

func (s *kernelTestSuite) TestKernelVersionFromPlaceInfo(c *C) {
spi := snap.MinimalPlaceInfo("pc-kernel", snap.R(1))

c.Assert(os.MkdirAll(spi.MountDir(), 0755), IsNil)

// No map file
ver, err := kernel.KernelVersionFromPlaceInfo(spi)
c.Check(err, ErrorMatches, `number of matches for .* is 0`)
c.Check(ver, Equals, "")

// Create file so kernel version can be found
c.Assert(os.WriteFile(filepath.Join(
spi.MountDir(), "System.map-5.15.0-78-generic"), []byte{}, 0644), IsNil)
ver, err = kernel.KernelVersionFromPlaceInfo(spi)
c.Check(err, IsNil)
c.Check(ver, Equals, "5.15.0-78-generic")

// Too many matches
c.Assert(os.WriteFile(filepath.Join(
spi.MountDir(), "System.map-6.8.0-71-generic"), []byte{}, 0644), IsNil)
ver, err = kernel.KernelVersionFromPlaceInfo(spi)
c.Check(err, ErrorMatches, `number of matches for .* is 2`)
c.Check(ver, Equals, "")
}

func (s *kernelTestSuite) TestKernelVersionFromPlaceInfoNotSetInFile(c *C) {
spi := snap.MinimalPlaceInfo("pc-kernel", snap.R(1))

c.Assert(os.MkdirAll(spi.MountDir(), 0755), IsNil)

// Create bad file name
c.Assert(os.WriteFile(filepath.Join(
spi.MountDir(), "System.map-"), []byte{}, 0644), IsNil)
ver, err := kernel.KernelVersionFromPlaceInfo(spi)
c.Check(err, ErrorMatches, `kernel version not set in .*System\.map\-`)
c.Check(ver, Equals, "")
}
2 changes: 2 additions & 0 deletions overlord/snapstate/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ type managerBackend interface {
StartServices(svcs []*snap.AppInfo, disabledSvcs []string, meter progress.Meter, tm timings.Measurer) error
StopServices(svcs []*snap.AppInfo, reason snap.ServiceStopReason, meter progress.Meter, tm timings.Measurer) error
QueryDisabledServices(info *snap.Info, pb progress.Meter) ([]string, error)
SetupKernelModulesComponent(cpi snap.ContainerPlaceInfo, cref naming.ComponentRef, kernelVersion string, meter progress.Meter) (err error)
UndoSetupKernelModulesComponent(cpi snap.ContainerPlaceInfo, cref naming.ComponentRef, kernelVersion string, meter progress.Meter) error

// the undoers for install
UndoSetupSnap(s snap.PlaceInfo, typ snap.Type, installRecord *backend.InstallRecord, dev snap.Device, meter progress.Meter) error
Expand Down
8 changes: 8 additions & 0 deletions overlord/snapstate/backend/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,11 @@ func MockMkdirAllChown(f func(string, os.FileMode, sys.UserID, sys.GroupID) erro
mkdirAllChown = old
}
}

func MockRunDepmod(f func(baseDir, kernelVersion string) error) func() {
old := runDepmod
runDepmod = f
return func() {
runDepmod = old
}
}
217 changes: 217 additions & 0 deletions overlord/snapstate/backend/kernel_modules_components.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,217 @@
// -*- Mode: Go; indent-tabs-mode: t -*-

/*
* Copyright (C) 2024 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

package backend

import (
"fmt"
"os"
"path/filepath"

"github.com/snapcore/snapd/dirs"
"github.com/snapcore/snapd/logger"
"github.com/snapcore/snapd/osutil"
"github.com/snapcore/snapd/progress"
"github.com/snapcore/snapd/snap"
"github.com/snapcore/snapd/snap/naming"
"github.com/snapcore/snapd/systemd"
)

type NoKernelDriversError struct {
cref naming.ComponentRef
kernelVersion string
}

func (e NoKernelDriversError) Error() string {
return fmt.Sprintf("%s does not contain firmware or components for %s",
e.cref, e.kernelVersion)
}

func cleanupMount(mountDir string, meter progress.Meter) error {
mountDir = filepath.Join(dirs.GlobalRootDir, mountDir)
// this also ensures that the mount unit stops
if err := removeMountUnit(mountDir, meter); err != nil {
return err
}

if err := os.RemoveAll(mountDir); err != nil {
return err
}

return nil
Comment on lines +53 to +57
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: could be shortened if you wanted to

Suggested change
if err := os.RemoveAll(mountDir); err != nil {
return err
}
return nil
return os.RemoveAll(mountDir)

}

type kernelModulesCleanupParts struct {
compMountDir string
modulesMountDir string
rerunDepmod bool
}

func cleanupKernelModulesSetup(parts *kernelModulesCleanupParts, kernelVersion string, meter progress.Meter) error {
if parts.modulesMountDir != "" {
if err := cleanupMount(parts.modulesMountDir, meter); err != nil {
return err
}
if parts.rerunDepmod {
if err := runDepmod("/usr", kernelVersion); err != nil {
return err
}
}
}

if parts.compMountDir != "" {
if err := cleanupMount(parts.compMountDir, meter); err != nil {
return err
}
}

return nil
}

func checkKernelModulesCompContent(mountDir, kernelVersion string) (bool, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there isn't much in the way of checking, sounds more like discoverKernelModulesComContent?

hasModules := osutil.IsDirectory(filepath.Join(mountDir, "modules", kernelVersion))
hasFirmware := osutil.IsDirectory(filepath.Join(mountDir, "firmware"))
return hasModules, hasFirmware
}

func componentMountPoint(componentName, kernelVersion string) string {
return filepath.Join("/run/mnt/kernel-modules/", kernelVersion, componentName)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still wonder if it would be beneficial or not to include the revision in this one

}

func modulesMountPoint(componentName, kernelVersion string) string {
return filepath.Join("/usr/lib/modules", kernelVersion, "updates", componentName)
}

// SetupKernelModulesComponent creates and starts mount units for
// kernel-modules components.
func (b Backend) SetupKernelModulesComponent(cpi snap.ContainerPlaceInfo, cref naming.ComponentRef, kernelVersion string, meter progress.Meter) (err error) {
var sysd systemd.Systemd
if b.preseed {
sysd = systemd.NewEmulationMode(dirs.GlobalRootDir)
} else {
sysd = systemd.New(systemd.SystemMode, meter)
}

// Restore state if something goes wrong
var cleanOnFailure kernelModulesCleanupParts
defer func() {
if err == nil {
return
}
if err := cleanupKernelModulesSetup(&cleanOnFailure, kernelVersion, meter); err != nil {
logger.Noticef("while cleaning up a failed kernel-modules set-up: %v", err)
}
}()
Comment on lines +112 to +120
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we keep doing this here? or should we make it a problem of the caller, which might decide to do a switch back instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point as indeed in some cases we want to restore the old state and we might not want to do previously a clean-up.


// Check that the kernel-modules component really has
// something we must mount on early boot.
hasModules, hasFirmware := checkKernelModulesCompContent(cpi.MountDir(), kernelVersion)
if !hasModules && !hasFirmware {
return &NoKernelDriversError{cref: cref, kernelVersion: kernelVersion}
}

// Mount the component itself (we need it early, so the mount in /snap cannot
// be used).
componentMount := componentMountPoint(cref.ComponentName, kernelVersion)
addMountUnitOptions := &systemd.MountUnitOptions{
MountUnitType: systemd.BeforeDriversLoadMountUnit,
Lifetime: systemd.Persistent,
Description: fmt.Sprintf("Mount unit for kernel-modules component %s", cref),
What: cpi.MountFile(),
Where: componentMount,
Fstype: "squashfs",
Options: []string{"nodev,ro,x-gdu.hide,x-gvfs-hide"},
}
_, err = sysd.EnsureMountUnitFileWithOptions(addMountUnitOptions)
if err != nil {
return fmt.Errorf("cannot create mount in %q: %w", componentMount, err)
}
cleanOnFailure.compMountDir = componentMount

if hasModules {
// systemd automatically works out dependencies on the "what"
// path too so this mount happens after the component one.
modulesDir := modulesMountPoint(cref.ComponentName, kernelVersion)
addMountUnitOptions = &systemd.MountUnitOptions{
MountUnitType: systemd.BeforeDriversLoadMountUnit,
Lifetime: systemd.Persistent,
Description: fmt.Sprintf("Mount unit for modules from %s", cref.String()),
What: filepath.Join(componentMount, "modules", kernelVersion),
Where: modulesDir,
Fstype: "none",
Options: []string{"bind"},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we record somewhere a discussion of why using a bind mount and not a symlink for this?

}
_, err = sysd.EnsureMountUnitFileWithOptions(addMountUnitOptions)
if err != nil {
return fmt.Errorf("cannot create mount in %q: %w", modulesDir, err)
}
cleanOnFailure.modulesMountDir = modulesDir

// Rebuild modinfo files
if err := runDepmod("/usr", kernelVersion); err != nil {
return err
}
cleanOnFailure.rerunDepmod = true
}

if hasFirmware {
// TODO create recursively symlinks in
// /usr/lib/firmware/updates while checking for conflicts with
// existing files.
}

return nil
}

var runDepmod = runDepmodImpl

func runDepmodImpl(baseDir, kernelVersion string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we have at least some tests using testutil.MockCmd to test this?

logger.Debugf("running depmod on %q for kernel %s", baseDir, kernelVersion)
stdout, stderr, err := osutil.RunSplitOutput("depmod", "-b", baseDir, kernelVersion)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw. do you need to pass -e to get more useful error information maybe?

logger.Debugf("depmod stderr:\n%s\n\ndepmod stdout:\n%s",
string(stderr), string(stdout))
if err != nil {
return osutil.OutputErrCombine(stdout, stderr, err)
}
return nil
}

// UndoSetupKernelModulesComponent undoes the work of SetupKernelModulesComponent
func (b Backend) UndoSetupKernelModulesComponent(cpi snap.ContainerPlaceInfo, cref naming.ComponentRef, kernelVersion string, meter progress.Meter) error {
hasModules, hasFirmware := checkKernelModulesCompContent(cpi.MountDir(), kernelVersion)
var partsToClean kernelModulesCleanupParts
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably good to have a comment about why rerunDepmod is not set

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a comment a bit below related to depmod - not 100% sure if that is what you meant though.

if hasFirmware {
// TODO remove recursively symlinks in
// /usr/lib/firmware/updates (set var in kernelModulesCleanupParts)
}

// Remove created mount units
if hasModules {
partsToClean.modulesMountDir =
modulesMountPoint(cref.ComponentName, kernelVersion)
partsToClean.rerunDepmod = true
}

if hasModules || hasFirmware {
partsToClean.compMountDir =
componentMountPoint(cref.ComponentName, kernelVersion)
}

return cleanupKernelModulesSetup(&partsToClean, kernelVersion, meter)
}
Loading
Loading