From 7be9ccbcb98c4c0411595fde47a3df1ae5fd87e4 Mon Sep 17 00:00:00 2001 From: Tuomas Katila Date: Wed, 9 Sep 2026 11:08:44 +0300 Subject: [PATCH] recovery: align xpu-smi use in reset and reflash Jobs Don't assume xpu-smi's location in the image and use shell to find it. Signed-off-by: Tuomas Katila --- RECOVERY.md | 4 + config/deployments/xpum/xpum-reset-job.yaml | 12 +- .../controller/gpurecoveryplan_controller.go | 26 ++- .../gpurecoveryplan_controller_test.go | 153 +++++++++++++++--- .../controller/gpurecoveryplan_helpers.go | 49 ++++-- 5 files changed, 202 insertions(+), 42 deletions(-) diff --git a/RECOVERY.md b/RECOVERY.md index 48ec54c..6d021ca 100644 --- a/RECOVERY.md +++ b/RECOVERY.md @@ -43,6 +43,10 @@ authorise the heavier one. Recovery Jobs run `xpu-smi` from the image in `spec.xpuSmi`, privileged, pinned to the affected node, and are retained after completion for post-mortem diagnostics (they are removed when the event is). +The command is run as `/bin/sh -c "xpu-smi …"`, so an image supplied through `spec.xpuSmi.image` has +to provide a shell and have `xpu-smi` on `PATH`. The operator deliberately does not assume an install +path: where the binary lives is the image's business. + ### Reset types | Type | Command | Notes | diff --git a/config/deployments/xpum/xpum-reset-job.yaml b/config/deployments/xpum/xpum-reset-job.yaml index 05500a9..d817b7e 100644 --- a/config/deployments/xpum/xpum-reset-job.yaml +++ b/config/deployments/xpum/xpum-reset-job.yaml @@ -6,6 +6,10 @@ # The operator fills in: spec.template.spec.nodeName, containers[resetter].image, # and containers[resetter].args with the appropriate xpu-smi reset command. # +# The command is /bin/sh -c and the reset is a single command line in args, so +# xpu-smi is resolved on PATH: where the image keeps its binary is the image's +# business, not the operator's. +# # Supported commands (set by the operator from the event's recoveryType): # sbr: xpu-smi config -d --reset # slot: xpu-smi config -d --coldreset @@ -39,12 +43,10 @@ spec: - name: resetter image: intel/gpu-fwupdater-mock:devel # replaced by the operator (spec.xpuSmi.image) imagePullPolicy: IfNotPresent - command: [ "/usr/local/bin/xpu-smi" ] + command: [ "/bin/sh", "-c" ] args: - - "config" - - "-d" - - "0000:00:00.0" # replaced by the operator - - "--reset" # replaced by the operator + # Replaced by the operator with the reset the event calls for. + - "xpu-smi config -d 0000:00:00.0 --reset" resources: requests: cpu: 50m diff --git a/internal/controller/gpurecoveryplan_controller.go b/internal/controller/gpurecoveryplan_controller.go index 2ae2180..e7c9ae6 100644 --- a/internal/controller/gpurecoveryplan_controller.go +++ b/internal/controller/gpurecoveryplan_controller.go @@ -332,6 +332,17 @@ func (r *GPURecoveryPlanReconciler) syncRecoveryEventsFromSlices(ctx context.Con continue } + // The attribute is a free-form string the DRA driver writes, and it reaches the shell + // command line of a privileged root container. A device the operator cannot name a + // PCI address for is one it cannot recover either way, so the shape is required here + // rather than escaped later. + if !validDeviceBDF(bdf) { + klog.Warningf("ResourceSlice %s device %s has %s %q, which is not a PCI address, skipping", + slice.Name, dev.Name, deviceAttrBDF, bdf) + + continue + } + key := deviceKey{node: nodeName, bdf: bdf} for _, taint := range dev.Taints { @@ -983,8 +994,8 @@ func (r *GPURecoveryPlanReconciler) createRecoveryJob(ctx context.Context, plan func (r *GPURecoveryPlanReconciler) createResetJob(ctx context.Context, plan *intelv1a1.GPURecoveryPlan, evt *intelv1a1.RecoveryEvent) error { rt := evt.RecoveryType.Type - args := recoveryTypeToArgs(evt.GPUBDF, rt) - if args == nil { + cmd := buildResetCommand(evt.GPUBDF, rt) + if cmd == "" { klog.Warningf("GPURecoveryPlan %s: unsupported recovery type %s for event %s; skipping", plan.Name, rt, evt.ID) return nil @@ -993,10 +1004,13 @@ func (r *GPURecoveryPlanReconciler) createResetJob(ctx context.Context, plan *in job := deployments.XpuManagerResetJob() jobName := r.prepareRecoveryJob(job, plan, evt) - // Inject the xpu-smi image and the reset command from the plan and the event. + // Inject the xpu-smi image and the reset command from the plan and the event. The template's + // command is /bin/sh -c, as the reflash template's is, so the reset is one argument: a command + // line, not an argv. Going through a shell is what lets xpu-smi be found on PATH, rather than + // the operator having to know where the image the plan names keeps its binary. if c := containerByName(job.Spec.Template.Spec.Containers, resetJobContainer); c != nil { applyXpuSmiImage(c, plan) - c.Args = args + c.Args = []string{cmd} } if err := r.Create(ctx, job); err != nil { @@ -1058,8 +1072,8 @@ func (r *GPURecoveryPlanReconciler) createReflashJob(ctx context.Context, plan * // The template's command is /bin/sh -c, so the flash is one argument: a command line, not an // argv. Overwriting args rather than command keeps the shell, which the template needs - // anyway. - c.Args = buildFDOFlashCommand(evt.GPUBDF, fw.File) + // anyway, and leaves xpu-smi to be found on PATH inside whichever image the plan names. + c.Args = []string{buildFDOFlashCommand(evt.GPUBDF, fw.File)} } if err := r.Create(ctx, job); err != nil { diff --git a/internal/controller/gpurecoveryplan_controller_test.go b/internal/controller/gpurecoveryplan_controller_test.go index 7523f93..970c7ee 100644 --- a/internal/controller/gpurecoveryplan_controller_test.go +++ b/internal/controller/gpurecoveryplan_controller_test.go @@ -860,6 +860,46 @@ var _ = Describe("GPURecoveryPlan Controller", func() { Expect(r.syncRecoveryEventsFromSlices(ctx, p)).To(Succeed()) Expect(p.Status.Events).To(BeEmpty()) }) + + // A BDF that is not a PCI address is dropped at the same point, and for a sharper reason: + // it would otherwise be interpolated into the command line of a privileged root container. + // Detection is the only place the value enters the plan, so this is what lets both command + // builders interpolate it without escaping. + It("should skip a tainted device whose pciAddress is not a PCI address", func() { + slice := &resv1.ResourceSlice{ + ObjectMeta: metav1.ObjectMeta{Name: "slice-bad-bdf"}, + Spec: resv1.ResourceSliceSpec{ + Driver: "gpu.intel.com", + NodeName: ptr.To("node-bad-bdf"), + Pool: resv1.ResourcePool{Name: "pool-bad-bdf", ResourceSliceCount: 1}, + Devices: []resv1.Device{{ + Name: "dev-bad-bdf", + Attributes: map[resv1.QualifiedName]resv1.DeviceAttribute{ + deviceAttrDeviceID: {StringValue: ptr.To("0xabcd")}, + deviceAttrBDF: {StringValue: ptr.To("0000:02:00.0; touch /tmp/pwned")}, + }, + Taints: []resv1.DeviceTaint{ + {Key: deviceTaintKeyReset, Effect: resv1.DeviceTaintEffectNoSchedule}, + }, + }}, + }, + } + Expect(k8sClient.Create(ctx, slice)).To(Succeed()) + DeferCleanup(func() { + _ = k8sClient.Delete(ctx, slice) + }) + + r := newTestReconciler() + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: "plan-bad-bdf"}, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DeviceID: "0xabcd", DefaultResetType: intelv1a1.RecoveryTypeSlot, + }, + } + + Expect(r.syncRecoveryEventsFromSlices(ctx, p)).To(Succeed()) + Expect(p.Status.Events).To(BeEmpty()) + }) }) Context("Helper: taintToDeviceNeed", func() { @@ -2231,8 +2271,8 @@ var _ = Describe("GPURecoveryPlan Controller", func() { Expect(evt.RecoveryType.Type).To(Equal(intelv1a1.RecoveryTypeSlot)) Expect(evt.RecoveryType.SuggestedType).To(Equal(intelv1a1.RecoveryTypeSBR)) - Expect(recoveryTypeToArgs("0000:02:00.0", evt.RecoveryType.Type)).To( - ContainElement("--coldreset"), "an override must change the command actually run") + Expect(buildResetCommand("0000:02:00.0", evt.RecoveryType.Type)).To( + ContainSubstring("--coldreset"), "an override must change the command actually run") }) It("should be a no-op when the approval has no override", func() { @@ -2816,7 +2856,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { }) }) - Context("Helper: recoveryTypeToArgs", func() { + Context("Helper: buildResetCommand", func() { const bdf = "0000:02:00.0" // Every reset type in the enum must map to a distinct xpu-smi invocation, or an admin's @@ -2831,32 +2871,75 @@ var _ = Describe("GPURecoveryPlan Controller", func() { seen := map[string]intelv1a1.RecoveryType{} for _, rt := range resetTypes { - args := recoveryTypeToArgs(bdf, rt) - Expect(args).NotTo(BeEmpty(), "reset type %q must map to a command", rt) + cmd := buildResetCommand(bdf, rt) + Expect(cmd).NotTo(BeEmpty(), "reset type %q must map to a command", rt) - key := strings.Join(args, " ") - Expect(seen).NotTo(HaveKey(key), + Expect(seen).NotTo(HaveKey(cmd), "reset types %q and %q share the command %q, so an override between them is a no-op", - seen[key], rt, key) + seen[cmd], rt, cmd) - seen[key] = rt + seen[cmd] = rt } }) + // The binary is named, not pathed: the plan can point spec.xpuSmi.image at any image that + // has xpu-smi on PATH, which is not the same set of images as those that keep it in + // /usr/local/bin. + It("should invoke xpu-smi by name rather than by path", func() { + Expect(buildResetCommand(bdf, intelv1a1.RecoveryTypeSlot)).To(HavePrefix("xpu-smi ")) + }) + It("should address the BDF the event names", func() { - Expect(recoveryTypeToArgs("0000:af:00.0", intelv1a1.RecoveryTypeSBR)). - To(ContainElement("0000:af:00.0")) + Expect(buildResetCommand("0000:af:00.0", intelv1a1.RecoveryTypeSBR)). + To(ContainSubstring("0000:af:00.0")) }) - It("should return nil for reflash, which is not an xpu-smi reset", func() { - Expect(recoveryTypeToArgs(bdf, intelv1a1.RecoveryTypeReflash)).To(BeNil()) + It("should return no command for reflash, which is not an xpu-smi reset", func() { + Expect(buildResetCommand(bdf, intelv1a1.RecoveryTypeReflash)).To(BeEmpty()) }) - It("should return nil for a type outside the enum", func() { - Expect(recoveryTypeToArgs(bdf, intelv1a1.RecoveryType("flr"))).To(BeNil()) + It("should return no command for a type outside the enum", func() { + Expect(buildResetCommand(bdf, intelv1a1.RecoveryType("flr"))).To(BeEmpty()) }) }) + // Both command lines interpolate the BDF into a string /bin/sh parses, so the shape of the + // ResourceSlice attribute they come from is what stands between a free-form driver-written + // string and a privileged root shell. + Context("Helper: validDeviceBDF", func() { + DescribeTable("should accept the addresses the kernel prints", + func(bdf string) { + Expect(validDeviceBDF(bdf)).To(BeTrue(), "%q is a PCI address", bdf) + }, + Entry("domain zero", "0000:02:00.0"), + Entry("non-zero domain", "0001:af:00.0"), + // lspci prints PCI addresses in uppercase hex, so this is a likely spelling rather + // than a pathological one. + Entry("uppercase hex", "0000:AF:00.0"), + Entry("highest function", "0000:00:1f.7"), + ) + + DescribeTable("should reject anything else", + func(bdf string) { + Expect(validDeviceBDF(bdf)).To(BeFalse(), "%q is not a PCI address", bdf) + }, + Entry("empty", ""), + Entry("no domain", "02:00.0"), + Entry("no function", "0000:02:00"), + Entry("function out of range", "0000:02:00.8"), + Entry("non-hex digits", "0000:0g:00.0"), + Entry("unexpected separators", "0000/02_00,0"), + Entry("trailing separator", "0000:02:00.0:"), + Entry("leading separator", ":02:00.0"), + Entry("trailing newline", "0000:02:00.0\n"), + // The one that matters: a shell metacharacter must not reach a command line run as + // root in a privileged container. + Entry("shell command substitution", "0000:02:00.0; touch /tmp/pwned"), + Entry("shell pipeline", "0000:02:00.0 | sh"), + Entry("backticks", "`id`"), + ) + }) + // The deadline is the only clock running once a recovery Job exists: nothing in the reconcile // gives up on an in-progress event, so an xpu-smi that hangs on a card that has stopped answering // would hold the node's drain taint and the event's state indefinitely. @@ -3190,8 +3273,10 @@ var _ = Describe("GPURecoveryPlan Controller", func() { Expect(resetter).NotTo(BeNil()) Expect(resetter.Image).To(Equal("registry/xpu-smi:v1")) Expect(resetter.ImagePullPolicy).To(Equal(core.PullAlways)) - // The BDF has to reach the command line, not just the event. - Expect(resetter.Args).To(Equal([]string{"config", "-d", "0000:02:00.0", "--coldreset"})) + // The BDF has to reach the command line, not just the event. One argument, because the + // template runs /bin/sh -c: see the reflash Job's specs for what splitting it costs. + Expect(resetter.Command).To(Equal([]string{"/bin/sh", "-c"})) + Expect(resetter.Args).To(Equal([]string{"xpu-smi config -d 0000:02:00.0 --coldreset"})) }) It("should give the Job the operator's own pull secret", func() { @@ -3264,6 +3349,28 @@ var _ = Describe("GPURecoveryPlan Controller", func() { }) }) + // Both createResetJob and createReflashJob hand their container exactly one argument, so both + // templates have to keep a shell as their command. This is a contract between Go and YAML that + // nothing else checks: a template switched back to running the binary directly would exec a whole + // command line as argv[0] and fail inside a pod with a "no such file" naming the entire string. + // + // Going through a shell is also what keeps the path to xpu-smi out of the operator. The plan names + // an image; where that image keeps its binary is its own business, so the command line invokes + // xpu-smi by name and lets PATH resolve it. + Context("Recovery Job templates: the shell contract", func() { + DescribeTable("should run xpu-smi through a shell, one command line at a time", + func(job *batch.Job, containerName string) { + c := containerByName(job.Spec.Template.Spec.Containers, containerName) + Expect(c).NotTo(BeNil()) + Expect(c.Command).To(Equal([]string{"/bin/sh", "-c"})) + Expect(c.Args).To(HaveLen(1), + "the template's own args stand in for what the operator writes, so they must be one command line too") + }, + Entry("reset", deployments.XpuManagerResetJob(), resetJobContainer), + Entry("reflash", deployments.XpuManagerFWUpdateJob(), reflashJobContainer), + ) + }) + // A card in survivability mode has firmware that no reset can fix, so the recovery is to write a // known-good image over it. That is a different Job from a reset — a firmware image, an // initContainer that stages it, and a shell command line rather than an argv — and every part the @@ -3359,9 +3466,13 @@ var _ = Describe("GPURecoveryPlan Controller", func() { Expect(flashC.ImagePullPolicy).To(Equal(core.PullAlways)) // The template runs /bin/sh -c, so the flash has to stay one command line: replacing the - // command with an argv, as a reset does, would leave the shell nothing to run. + // command with an argv, as a reset does, would leave the shell nothing to run. The length + // is the assertion that matters — sh -c takes its command from the first operand and turns + // the rest into $0, $1, …, so a flash split across arguments runs a bare "xpu-smi" and + // silently flashes nothing. Expect(flashC.Command).To(Equal([]string{"/bin/sh", "-c"})) - Expect(flashC.Args).To(Equal(buildFDOFlashCommand(reflashBDF, reflashFile))) + Expect(flashC.Args).To(HaveLen(1)) + Expect(flashC.Args[0]).To(Equal(buildFDOFlashCommand(reflashBDF, reflashFile))) // An admin who has to check what was flashed onto which card reads this, not the pod log. Expect(p.Status.Messages).To(ContainElement(SatisfyAll( @@ -3403,7 +3514,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { Expect(mountedAt(copyC, reflashStagingDir)).NotTo(BeEmpty()) Expect(mountedAt(flashC, reflashStagingDir)).To(Equal(mountedAt(copyC, reflashStagingDir))) - Expect(strings.Join(buildFDOFlashCommand(reflashBDF, reflashFile), " ")).To( + Expect(buildFDOFlashCommand(reflashBDF, reflashFile)).To( ContainSubstring(reflashStagingDir + "/" + reflashFile)) Expect(firmwareImagePath(reflashFile)).To(Equal(firmwareImageDir + "/" + reflashFile)) }) @@ -3412,7 +3523,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { // survivability mode: xpu-smi otherwise declines to write an image it judges no newer than // what is on the device, and what is on the device is exactly what has to go. It("should force an unattended FDO flash", func() { - Expect(strings.Join(buildFDOFlashCommand("0000:02:00.0", "fw.bin"), " ")).To( + Expect(buildFDOFlashCommand("0000:02:00.0", "fw.bin")).To( Equal("xpu-smi updatefw -d 0000:02:00.0 -t FDO -f /update/fw.bin -y --force")) }) diff --git a/internal/controller/gpurecoveryplan_helpers.go b/internal/controller/gpurecoveryplan_helpers.go index e58014e..ee157fa 100644 --- a/internal/controller/gpurecoveryplan_helpers.go +++ b/internal/controller/gpurecoveryplan_helpers.go @@ -21,6 +21,7 @@ import ( "crypto/sha256" "encoding/hex" "fmt" + "regexp" "strings" "time" "unicode/utf8" @@ -219,26 +220,54 @@ func hashSegment(s string) string { return hex.EncodeToString(sum[:])[:idHashLen] } -// recoveryTypeToArgs returns the xpu-smi command-line arguments that carry out the given reset -// against the given BDF. Returns nil for a type xpu-smi has no reset for, reflash above all: it -// writes firmware rather than resetting the device, so it is a different Job entirely. -func recoveryTypeToArgs(bdf string, rt intelv1a1.RecoveryType) []string { +// deviceBDFPattern is the shape the kernel prints a PCI address in ("%04x:%02x:%02x.%d", function +// 0-7), accepting either hex case because lspci and some drivers spell the bus uppercase. +// +// Both recovery command lines interpolate the BDF into a string a privileged root container hands +// to /bin/sh, and it arrives from a free-form ResourceSlice attribute rather than from a validated +// API field, so it is checked on the way in — see validDeviceBDF's callers. +var deviceBDFPattern = regexp.MustCompile(`^[0-9a-fA-F]{4}:[0-9a-fA-F]{2}:[0-9a-fA-F]{2}\.[0-7]$`) + +// validDeviceBDF reports whether bdf is a PCI address the recovery Jobs can be handed. +// +// Deliberately an allow-list of the one shape that is ever correct, not a deny-list of shell +// metacharacters, for the same reason firmwareFileNamePattern is: the value ends up in a command +// line inside a privileged root container, where anything unanticipated is worse than a device the +// operator declines to recover and says so about. +func validDeviceBDF(bdf string) bool { + return deviceBDFPattern.MatchString(bdf) +} + +// buildResetCommand returns the shell command line that carries out the given reset against the +// given BDF. Returns "" for a type xpu-smi has no reset for, reflash above all: it writes firmware +// rather than resetting the device, so it is a different Job entirely. +// +// One string rather than an argv, as buildFDOFlashCommand is and for the same reason — see there. +func buildResetCommand(bdf string, rt intelv1a1.RecoveryType) string { switch rt { case intelv1a1.RecoveryTypeSBR: - return []string{"config", "-d", bdf, "--reset"} + return fmt.Sprintf("xpu-smi config -d %s --reset", bdf) case intelv1a1.RecoveryTypeSlot: - return []string{"config", "-d", bdf, "--coldreset"} + return fmt.Sprintf("xpu-smi config -d %s --coldreset", bdf) case intelv1a1.RecoveryTypeAMC: - return []string{"amc", "--gpureset", "-d", bdf, "-y"} + return fmt.Sprintf("xpu-smi amc --gpureset -d %s -y", bdf) default: - return nil + return "" } } // buildFDOFlashCommand returns the shell command line that reflashes one GPU from a firmware file the // fw-copy initContainer has staged in the shared volume. -func buildFDOFlashCommand(bdf, file string) []string { - return []string{"xpu-smi", "updatefw", "-d", bdf, "-t", "FDO", "-f", fmt.Sprintf("%s/%s", reflashStagingDir, file), "-y", "--force"} +// +// One string, not an argv: the container it lands on runs /bin/sh -c, which takes the command from +// its first operand only and turns the rest into positional parameters. As an argv this flashed +// nothing — the shell ran a bare "xpu-smi" and everything after it became $0, $1 and so on. +// +// Both interpolated values are shape-checked before they can reach here: the webhook allow-lists +// spec.firmware.file down to [a-zA-Z0-9._-] (firmwareFileNamePattern) and detection drops any device +// whose PCI address is not a BDF (validDeviceBDF). +func buildFDOFlashCommand(bdf, file string) string { + return fmt.Sprintf("xpu-smi updatefw -d %s -t FDO -f %s/%s -y --force", bdf, reflashStagingDir, file) } // firmwareImagePath returns path to the firmware file inside the firmware image.