Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [Critical] DMA Use-After-Free due to freeing buffers while GSP is still active on timeout -- commit ad8609a0c320b89583cef07a17ca5ce5c85167ea Author: Alexandre Courbot <[email protected]> gpu: nova-core: store Fsp instance in Gpu The Fsp instance was only used in the Hopper+ boot path, and consequently built locally (and immediately dropped) in it. This worked well as a temporary measure, but the FSP is a GPU sub-device, so its lifetime should match the GPU rather than a single boot invocation. > diff --git a/drivers/gpu/nova-core/gsp/hal/gh100.rs > b/drivers/gpu/nova-core/gsp/hal/gh100.rs > index 2805a35abb798..de786871c8ec4 100644 > --- a/drivers/gpu/nova-core/gsp/hal/gh100.rs > +++ b/drivers/gpu/nova-core/gsp/hal/gh100.rs [ ... ] > @@ -150,7 +146,7 @@ fn boot( > KBox::new(FspUnloadBundle, GFP_KERNEL)? as KBox<dyn UnloadBundle> > ); > > - let mut fsp = Fsp::wait_secure_boot(dev, bar, chipset)?; > + let fsp = ctx.fsp.as_mut().ok_or(ENODEV)?; > > let args = FmcBootArgs::new( > dev, [Severity: Critical] This is a pre-existing issue, but I wanted to ask about the error handling in this function and how it affects DMA buffers. If wait_for_gsp_lockdown_release() times out later in Gh100::boot(), the ? operator causes an early return: let res = fsp.boot_fmc(dev, fb_layout, &args); wait_for_gsp_lockdown_release(dev, gsp_falcon, args.boot_params_dma_handle())?; When this happens, args is dropped, and the error propagates back to Gsp::boot(), which bypasses the unload_guard creation and immediately drops wpr_meta: let unload_bundle = hal .boot(&self, &mut ctx, &fb_layout, &wpr_meta)? Dropping these structures frees their underlying DMA memory. However, since the timeout occurred, the GSP hardware is not forcefully halted. A similar issue appears during the unload path in FspUnloadBundle::run(). If a timeout occurs while waiting for RISC-V to halt, the code explicitly detects that the engine is still active but returns an error without forcefully halting it: read_poll_timeout( || Ok(ctx.gsp_falcon.is_riscv_active()), |&active| !active, Delta::from_millis(10), Delta::from_secs(5), ) .map(|_| ()) .inspect_err(|_| dev_err!(ctx.dev(), "GSP falcon failed to halt\n")) Since the error propagates and allows the DMA memory to be freed while the engine continues executing, could the active GSP hardware write to the freed host memory and cause memory corruption? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
