Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Initialization function returns positive value `1` on failure, breaking module load semantics and causing subsequent `registered_fb` array corruption on unload. - [High] `maxinefb_fix.smem_start` is initialized with a virtual address (`CKSEG1ADDR`) instead of the hardware physical address, breaking user-space `mmap()`. -- --- Patch [1]: [RFC PATCH] fbdev: maxinefb: fix build issues --- commit e391418fa2f21f8c420eccb513c031674ef184c6 Author: Randy Dunlap <[email protected]> fbdev: maxinefb: fix build issues This commit fixes build errors and warnings for the maxinefb driver on 32-bit and 64-bit MACH_DECSTATION. It updates KSEG1ADDR to CKSEG1ADDR and adds the static keyword to missing prototypes. > diff --git a/drivers/video/fbdev/maxinefb.c b/drivers/video/fbdev/maxinefb.c > index 52528eb4dfb41..8aa1389e2d4c8 100644 > --- a/drivers/video/fbdev/maxinefb.c > +++ b/drivers/video/fbdev/maxinefb.c [ ... ] > @@ -111,7 +98,7 @@ static const struct fb_ops maxinefb_ops = { > .fb_setcolreg = maxinefb_setcolreg, > }; > > -int __init maxinefb_init(void) > +static int __init maxinefb_init(void) [Severity: High] This is a pre-existing issue, but does returning 1 on failure in this function cause issues with module load semantics? In maxinefb_init(): if (register_framebuffer(&fb_info) < 0) return 1; If register_framebuffer() fails, returning a positive integer causes the kernel to treat the module as successfully loaded because the module init framework expects negative error codes for failure. Later, when the module is unloaded, maxinefb_exit() calls unregister_framebuffer() on the unregistered fb_info. Since it was never successfully registered, its node field is 0. Does this cause the generic do_unregister_framebuffer() to blindly execute registered_fb[0] = NULL, corrupting the kernel's framebuffer state by deleting the reference to whatever valid framebuffer occupied node 0? > diff --git a/include/video/maxinefb.h b/include/video/maxinefb.h > index 6aeb4acca2bdb..2b66b32f355cf 100644 > --- a/include/video/maxinefb.h > +++ b/include/video/maxinefb.h [ ... ] > @@ -13,13 +13,13 @@ > /* > * IMS332 video controller register base address > */ > -#define MAXINEFB_IMS332_ADDRESS KSEG1ADDR(0x1c140000) > +#define MAXINEFB_IMS332_ADDRESS CKSEG1ADDR(0x1c140000) > > /* > * Begin of DECstation 5000/xx onboard framebuffer memory, default resolution > * is 1024x768x8 > */ > -#define DS5000_xx_ONBOARD_FBMEM_START KSEG1ADDR(0x0a000000) > +#define DS5000_xx_ONBOARD_FBMEM_START CKSEG1ADDR(0x0a000000) [Severity: High] This is a pre-existing issue, but does assigning a virtual address to maxinefb_fix.smem_start break user-space mmap()? In maxinefb_init(): fb_start = DS5000_xx_ONBOARD_FBMEM_START; ... maxinefb_fix.smem_start = fb_start; The smem_start field is passed to vm_iomap_memory() by the generic framebuffer mmap handler (fb_mmap()). Because vm_iomap_memory() strictly expects a physical address, does passing a KSEG1 virtual address set up user-space page tables pointing to an invalid physical address? This could trigger an invalid physical memory access and a kernel panic when user-space accesses the mapped memory. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
