From 2b2517e4a583ae484690e2d743865aec3c98f12a Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 14:39:29 +0200 Subject: [PATCH] issue 1841/S7: name the processes still mapping a replaced file and fail the check on a DKMS module not built for an installed kernel, so a machine is never left unable to load its drivers unnoticed --- modules/pacman/cmd/pacman-tools/aftercheck.go | 1 + .../cmd/pacman-tools/aftercheck_test.go | 1 + .../pacman/cmd/pacman-tools/busshapes_test.go | 4 +- modules/pacman/cmd/pacman-tools/mapped.go | 173 ++++++++++++++++++ .../pacman/cmd/pacman-tools/mapped_test.go | 131 +++++++++++++ modules/pacman/cmd/pacman-tools/shapes.go | 4 + .../cmd/pacman-tools/transaction_test.go | 6 + modules/pacman/cmd/pacman-tools/upgrades.go | 2 + 8 files changed, 320 insertions(+), 2 deletions(-) create mode 100644 modules/pacman/cmd/pacman-tools/mapped.go create mode 100644 modules/pacman/cmd/pacman-tools/mapped_test.go diff --git a/modules/pacman/cmd/pacman-tools/aftercheck.go b/modules/pacman/cmd/pacman-tools/aftercheck.go index 33d6abc9..d7a4c05b 100644 --- a/modules/pacman/cmd/pacman-tools/aftercheck.go +++ b/modules/pacman/cmd/pacman-tools/aftercheck.go @@ -50,6 +50,7 @@ func (u *Upgrades) AfterCheck(run Run, rec *Record) Check { } c.RunningKernel, c.InstalledKernel, c.RestartNeeded = u.restartNeeded() c.Passed = succeeded && healthy && len(c.NewlyFailed) == 0 + u.afterCheckII(&c) return c } diff --git a/modules/pacman/cmd/pacman-tools/aftercheck_test.go b/modules/pacman/cmd/pacman-tools/aftercheck_test.go index 445da005..5410f8bb 100644 --- a/modules/pacman/cmd/pacman-tools/aftercheck_test.go +++ b/modules/pacman/cmd/pacman-tools/aftercheck_test.go @@ -35,6 +35,7 @@ func checkMachine(failed, linuxNow string) *Machine { m := machine(byLine(map[string]Ran{ "systemctl list-units --state=failed --plain --no-legend --no-pager": {Stdout: failed}, "pacman -Qq": {Stdout: "bash\nlinux\nlinux-firmware\nlinux-lts\n"}, + "dkms status": {Status: 127, Err: "ENOENT"}, "pacman -Qlq -- linux": {Stdout: "/usr/lib/modules/" + linuxNow + "/vmlinuz\n"}, "pacman -Qlq -- linux-firmware": {Stdout: "/usr/lib/firmware/\n"}, "pacman -Qlq -- linux-lts": {Stdout: "/usr/lib/modules/6.18.54-2-lts/vmlinuz\n"}, diff --git a/modules/pacman/cmd/pacman-tools/busshapes_test.go b/modules/pacman/cmd/pacman-tools/busshapes_test.go index b6ed161a..afabca1b 100644 --- a/modules/pacman/cmd/pacman-tools/busshapes_test.go +++ b/modules/pacman/cmd/pacman-tools/busshapes_test.go @@ -26,8 +26,8 @@ var busFields = map[string][]string{ "Step": {"step", "at", "why", "held", "unit", "ignored", "plan", "exit", "error", "changes", "check"}, "HeldKernel": {"package", "version", "module", "bound"}, "Changes": {"added", "removed", "changed"}, - "Check": {"passed", "pacnew", "pacsave", "news", "news_error", "mapped", "newly_failed", "restart_needed", "running_kernel", - "installed_kernel", "unbuilt"}, + "Check": {"passed", "pacnew", "pacsave", "news", "news_error", "mapped", "mapped_error", "newly_failed", "restart_needed", "running_kernel", + "installed_kernel", "unbuilt", "dkms_error"}, "NewsTitle": {"title", "link", "published"}, "Mapped": {"unit", "container", "executable"}, "DKMSBuild": {"module", "version", "kernel"}, diff --git a/modules/pacman/cmd/pacman-tools/mapped.go b/modules/pacman/cmd/pacman-tools/mapped.go new file mode 100644 index 00000000..64e18af3 --- /dev/null +++ b/modules/pacman/cmd/pacman-tools/mapped.go @@ -0,0 +1,173 @@ +package main + +// The after-upgrade check, its second part (novox/hq ADR 0508 §4): the processes still mapping a file the upgrade +// replaced, named by their unit or container and their executable's path, never their command line or +// environment; and whether every DKMS module is built for every installed kernel. Reading another process's +// mappings needs root, so the root helper reads them (ADR 0510's `maps`), with MappedDeleted below. + +import ( + "os" + "path" + "regexp" + "sort" + "strconv" + "strings" +) + +// ProcFS is the process table as MappedDeleted reads it: the machine's /proc, or a stand-in in a test. +type ProcFS interface { + ReadDir(dir string) ([]string, error) + ReadFile(name string) ([]byte, error) + Readlink(name string) (string, error) +} + +// osProc is the machine's own /proc. +type osProc struct{} + +func (osProc) ReadDir(dir string) ([]string, error) { + entries, err := os.ReadDir(dir) + var names []string + for _, e := range entries { + names = append(names, e.Name()) + } + return names, err +} +func (osProc) ReadFile(name string) ([]byte, error) { return os.ReadFile(name) } +func (osProc) Readlink(name string) (string, error) { return os.Readlink(name) } + +// replacedPrefixes are where a package's files are: a mapping deleted elsewhere (a temporary file, shared +// memory) is no file an upgrade replaced. +var replacedPrefixes = []string{"/usr/", "/opt/"} + +var containerScope = regexp.MustCompile(`(?:docker|libpod|crio|cri-containerd)-([0-9a-f]{12})[0-9a-f]*\.scope$`) + +// cgroupOwner names a process's unit or container from its cgroup line `0::/system.slice/x.service`. +func cgroupOwner(cgroup string) (unit, container string) { + for _, l := range strings.Split(cgroup, "\n") { + _, p, ok := strings.Cut(l, "::") + if !ok { + continue + } + parts := strings.Split(strings.Trim(p, "/"), "/") + for i := len(parts) - 1; i >= 0; i-- { + if g := containerScope.FindStringSubmatch(parts[i]); g != nil { + return "", g[1] + } + if i > 0 && parts[i-1] == "docker" && len(parts[i]) >= 12 { + return "", parts[i][:12] + } + } + for i := len(parts) - 1; i >= 0; i-- { + if strings.HasSuffix(parts[i], ".service") || strings.HasSuffix(parts[i], ".scope") { + return parts[i], "" + } + } + } + return "", "" +} + +// MappedDeleted reads the process table under procRoot for processes mapping a deleted package file, each named +// once by its unit or container and its executable, sorted. +func MappedDeleted(fs ProcFS, procRoot string) ([]Mapped, error) { + names, err := fs.ReadDir(procRoot) + if err != nil { + return nil, err + } + seen := map[Mapped]bool{} + out := []Mapped{} + for _, name := range names { + if _, err := strconv.Atoi(name); err != nil { + continue + } + dir := path.Join(procRoot, name) + maps, err := fs.ReadFile(dir + "/maps") + if err != nil { + continue // the process ended meanwhile + } + replaced := false + for _, l := range strings.Split(string(maps), "\n") { + if !strings.HasSuffix(l, " (deleted)") { + continue + } + f := strings.Fields(strings.TrimSuffix(l, " (deleted)")) + if len(f) < 6 { + continue + } + for _, p := range replacedPrefixes { + if strings.HasPrefix(f[5], p) { + replaced = true + } + } + } + if !replaced { + continue + } + exe, _ := fs.Readlink(dir + "/exe") + cgroup, _ := fs.ReadFile(dir + "/cgroup") + unit, container := cgroupOwner(string(cgroup)) + m := Mapped{Unit: unit, Container: container, Executable: strings.TrimSuffix(exe, " (deleted)")} + if !seen[m] { + seen[m] = true + out = append(out, m) + } + } + sort.Slice(out, func(i, j int) bool { + a, b := out[i], out[j] + if a.Unit+a.Container != b.Unit+b.Container { + return a.Unit+a.Container < b.Unit+b.Container + } + return a.Executable < b.Executable + }) + return out, nil +} + +// unbuiltDKMS is every DKMS module built for some kernel and not for an installed one its own bound allows. +func (m *Machine) unbuiltDKMS() ([]DKMSBuild, error) { + modules, err := m.DKMSModules() + if err != nil { + return nil, err + } + kernels, err := m.installedKernels() + if err != nil { + return nil, err + } + out := []DKMSBuild{} + for _, mod := range modules { + if len(mod.Kernels) == 0 { + continue // added and never built: not a module this machine uses + } + bound := m.dkmsBound(mod) + for _, k := range kernels { + if contains(mod.Kernels, k) { + continue + } + if held, _ := heldBy(bound, k); held != "" { + continue + } + out = append(out, DKMSBuild{Module: mod.Module, Version: mod.Version, Kernel: k}) + } + } + return out, nil +} + +// afterCheckII fills the check's second part and judges it: a DKMS module not built for a kernel fails it; +// a process still mapping a replaced file is said and does not. +func (u *Upgrades) afterCheckII(c *Check) { + if u.Root == nil { + c.MappedError = "no root helper to read the processes' mapped files" + } else if mapped, err := u.Root.Maps(); err != nil { + c.MappedError = err.Error() + } else { + c.Mapped = mapped + } + unbuilt, err := u.Machine.unbuiltDKMS() + if err != nil { + c.DKMSError = err.Error() + c.Passed = false + return + } + c.Unbuilt = unbuilt + if len(unbuilt) > 0 { + c.Passed = false + } +} diff --git a/modules/pacman/cmd/pacman-tools/mapped_test.go b/modules/pacman/cmd/pacman-tools/mapped_test.go new file mode 100644 index 00000000..dc5f5574 --- /dev/null +++ b/modules/pacman/cmd/pacman-tools/mapped_test.go @@ -0,0 +1,131 @@ +package main + +// The after-upgrade check, its second part (novox/hq ADR 0508 §4; acceptance 1841/AC5): processes still mapping a +// replaced file, by unit or container and executable, never a command line or environment; DKMS built for every +// installed kernel. + +import ( + "errors" + "strings" + "testing" +) + +// standInProc is a process table in memory; it records every file read, so nothing else is read. +type standInProc struct { + files map[string]string + links map[string]string + read []string +} + +func (p *standInProc) ReadDir(dir string) ([]string, error) { + seen := map[string]bool{} + var names []string + for f := range p.files { + if rest, ok := strings.CutPrefix(f, dir+"/"); ok { + n, _, _ := strings.Cut(rest, "/") + if !seen[n] { + seen[n] = true + names = append(names, n) + } + } + } + return names, nil +} + +func (p *standInProc) ReadFile(name string) ([]byte, error) { + p.read = append(p.read, name) + if s, ok := p.files[name]; ok { + return []byte(s), nil + } + return nil, errNoFile +} + +func (p *standInProc) Readlink(name string) (string, error) { + if s, ok := p.links[name]; ok { + return s, nil + } + return "", errNoFile +} + +const libcDeleted = "7f2a1c000000-7f2a1c028000 r--p 00000000 103:02 5513 /usr/lib/libc.so.6 (deleted)\n" + +func TestAProcessMappingAReplacedFileIsNamedByItsUnitOrContainerAndExecutableOnly(t *testing.T) { + p := &standInProc{files: map[string]string{ + "/proc/1/maps": libcDeleted, + "/proc/1/cgroup": "0::/init.scope\n", + "/proc/200/maps": "5600-5700 r-xp 00000000 103:02 77 /usr/bin/sshd\n" + strings.Replace(libcDeleted, "libc.so.6", "libcrypto.so.3", 1), + "/proc/200/cgroup": "0::/system.slice/sshd.service\n", + "/proc/201/maps": libcDeleted, + "/proc/201/cgroup": "0::/system.slice/sshd.service\n", + "/proc/300/maps": libcDeleted, + "/proc/300/cgroup": "0::/system.slice/docker-0123456789abcdef0123456789abcdef.scope\n", + "/proc/400/maps": "7f00-7f10 rw-s 00000000 00:1a 9 /dev/shm/made-up (deleted)\n", + "/proc/400/cgroup": "0::/system.slice/made-up.service\n", + "/proc/500/maps": "5600-5700 r-xp 00000000 103:02 77 /usr/bin/made-up\n", + "/proc/500/cmdline": "made-up --password=secret", + "/proc/500/environ": "TOKEN=secret", + "/proc/self/maps": libcDeleted, + }, links: map[string]string{ + "/proc/1/exe": "/usr/lib/systemd/systemd", "/proc/200/exe": "/usr/bin/sshd (deleted)", "/proc/201/exe": "/usr/bin/sshd (deleted)", + "/proc/300/exe": "/usr/bin/made-up-server", + }} + got, err := MappedDeleted(p, "/proc") + if err != nil { + t.Fatal(err) + } + var said []string + for _, m := range got { + said = append(said, m.Unit+"|"+m.Container+"|"+m.Executable) + } + if strings.Join(said, " ") != "|0123456789ab|/usr/bin/made-up-server init.scope||/usr/lib/systemd/systemd sshd.service||/usr/bin/sshd" { + t.Errorf("%v", said) + } + for _, r := range p.read { + if strings.HasSuffix(r, "/cmdline") || strings.HasSuffix(r, "/environ") { + t.Errorf("read %s: a process is named by its unit and executable, never its command line or environment", r) + } + } +} + +func TestADKMSModuleNotBuiltForAnInstalledKernelFailsTheCheck(t *testing.T) { + m := checkMachine("", "7.2.9-arch1-1") + table := map[string]Ran{ + "systemctl list-units --state=failed --plain --no-legend --no-pager": {}, + "pacman -Qq": {Stdout: "linux\nlinux-lts\n"}, + "pacman -Qlq -- linux": {Stdout: "/usr/lib/modules/7.2.9-arch1-1/vmlinuz\n"}, + "pacman -Qlq -- linux-lts": {Stdout: "/usr/lib/modules/6.18.54-2-lts/vmlinuz\n"}, + "dkms status": {Stdout: "zfs/2.4.4, 6.18.54-2-lts, x86_64: installed\nzfs/2.4.4, 7.2.8-arch1-2, x86_64: installed\n" + + "made-up-driver/1.0, 6.18.54-2-lts, x86_64: installed\nxpad/0.4: added\n"}, + } + m.Run = byLine(table, nil) + read := m.ReadFile + m.ReadFile = func(path string) ([]byte, error) { + if path == "/usr/src/made-up-driver-1.0/dkms.conf" { + return []byte("BUILD_EXCLUSIVE_KERNEL=\"^6\\.\"\n"), nil + } + return read(path) + } + u := checkUpgrades(t, m) + u.Root = &fakeRoot{mapped: []Mapped{{Unit: "sshd.service", Executable: "/usr/bin/sshd"}}} + c := u.AfterCheck(checkedRun(StepStarted, StepFinished), &Record{}) + if len(c.Unbuilt) != 1 || c.Unbuilt[0] != (DKMSBuild{Module: "zfs", Version: "2.4.4", Kernel: "7.2.9-arch1-1"}) || c.Passed { + t.Errorf("zfs not built for the new kernel fails the check (made-up-driver excludes it, xpad was never built): %+v", c) + } + if len(c.Mapped) != 1 || c.Mapped[0].Unit != "sshd.service" { + t.Errorf("mapped: %+v", c.Mapped) + } + + table["dkms status"] = Ran{Stdout: "zfs/2.4.4, 6.18.54-2-lts, x86_64: installed\nzfs/2.4.4, 7.2.9-arch1-1, x86_64: installed\n"} + if c := u.AfterCheck(checkedRun(StepStarted, StepFinished), &Record{}); !c.Passed || len(c.Mapped) != 1 { + t.Errorf("built for every kernel passes; a mapped file is said and does not fail it: %+v", c) + } + + u.Root = &fakeRoot{mapsErr: errors.New("the root helper's socket is not there")} + if c := u.AfterCheck(checkedRun(StepStarted, StepFinished), &Record{}); !strings.Contains(c.MappedError, "socket") { + t.Errorf("unread mappings are said: %+v", c) + } + table["dkms status"] = Ran{Status: 1, Stderr: "dkms: permission denied\n"} + if c := u.AfterCheck(checkedRun(StepStarted, StepFinished), &Record{}); c.Passed || c.DKMSError == "" { + t.Errorf("unread DKMS builds do not pass: %+v", c) + } +} diff --git a/modules/pacman/cmd/pacman-tools/shapes.go b/modules/pacman/cmd/pacman-tools/shapes.go index 4da17688..3c364538 100644 --- a/modules/pacman/cmd/pacman-tools/shapes.go +++ b/modules/pacman/cmd/pacman-tools/shapes.go @@ -235,6 +235,8 @@ type Check struct { NewsError string `json:"news_error,omitempty"` // The units and containers whose processes still map a file the upgrade replaced. Mapped []Mapped `json:"mapped"` + // Why the mapped files could not be read, when they could not. + MappedError string `json:"mapped_error,omitempty"` // The units failed now that were not failed in the record. NewlyFailed []string `json:"newly_failed"` // Whether a restart is needed: the running kernel is not the installed version of its package. @@ -243,6 +245,8 @@ type Check struct { InstalledKernel string `json:"installed_kernel"` // The DKMS modules not built for an installed kernel. Unbuilt []DKMSBuild `json:"unbuilt"` + // Why the DKMS builds could not be read, when they could not; the check then does not pass. + DKMSError string `json:"dkms_error,omitempty"` } // NewsTitle is one item of the distribution's news, without its text. diff --git a/modules/pacman/cmd/pacman-tools/transaction_test.go b/modules/pacman/cmd/pacman-tools/transaction_test.go index fe0c47ec..ad2cc04a 100644 --- a/modules/pacman/cmd/pacman-tools/transaction_test.go +++ b/modules/pacman/cmd/pacman-tools/transaction_test.go @@ -18,6 +18,8 @@ type fakeRoot struct { started []string fail error journal []string + mapped []Mapped + mapsErr error } func (f *fakeRoot) Transaction(kind, run string, ignore []string) error { @@ -30,6 +32,8 @@ func (f *fakeRoot) Transaction(kind, run string, ignore []string) error { func (f *fakeRoot) Journal(unit string, n int) ([]string, error) { return f.journal, nil } +func (f *fakeRoot) Maps() ([]Mapped, error) { return f.mapped, f.mapsErr } + // keeperBed is one node's keeper over states in memory and a machine of fakes: a unit's state, the lock and // pacman's log are set by the test as the transaction goes. type keeperBed struct { @@ -75,6 +79,8 @@ func newKeeperBed(t *testing.T) *keeperBed { return Ran{} case line == "dkms status": return Ran{Stdout: zfsStatus} + case line == "pacman -Qq": + return Ran{Stdout: "bash\nlinux\n"} case line == "pacman -Qlq -- linux": return files("/usr/lib/modules/7.2.8-arch1-2/vmlinuz") case line == "pacman -Qlq -- linux-headers": diff --git a/modules/pacman/cmd/pacman-tools/upgrades.go b/modules/pacman/cmd/pacman-tools/upgrades.go index 8f58f8e8..567dacc5 100644 --- a/modules/pacman/cmd/pacman-tools/upgrades.go +++ b/modules/pacman/cmd/pacman-tools/upgrades.go @@ -41,6 +41,8 @@ type RootActs interface { Transaction(kind, run string, ignore []string) error // Journal is the last n lines a transaction unit logged. Journal(unit string, n int) ([]string, error) + // Maps is every process still mapping a replaced file, by its unit or container and its executable. + Maps() ([]Mapped, error) } // TransactionUnit is the unit a run's transaction runs as: named by the run, so a keeper started again finds it.