Re: [PATCH] cdrom: modernize locking, memory allocation, and fix checkpatch warnings

[Jedrzej Sz]

Hi Phil,
Thank you for the prompt and thorough review.

You are completely right on all points.

(1) & (2): I agree. The formatting changes, moving to __func__, and
general code styling are essentially churn for a legacy driver.
It creates unnecessary noise for git blame and stable backports
without solving a functional issue.
I will drop all cosmetic patches. However eager I am to apply them,
their presence would generate more noise than necessary.

(3): Excellent point regarding the error return values (-ENOSYS to
-ENOTTY). Changing the ABI return codes risks breaking legacy
userspace tooling that expects the old behavior.
My QEMU testing did not cover the full ecosystem of user-space CD/DVD
tools, so introducing this risk is unacceptable. I am dropping this
patch as well.

Given your feedback and my own deeper architectural analysis of the
locking changes (which introduced theoretical TOCTOU risks and
potential hardware lockouts during slow I/O operations), I am
discarding the bulk of this series.

I would like to submit a heavily slimmed down, single-patch v2 that
focuses solely on a real, undeniable issue: memory safety. I plan to
modernize the changer info allocations by replacing the obsolete
`kmalloc_obj()` pattern with `kzalloc(struct_size(...), ...)` to
provide inherent protection against integer overflow vulnerabilities
during allocation. I will prepare and thoroughly test this single,
surgical patch and send it as v2 shortly.

Thank you again for steering me in the right direction.


Best regards,
Jędrzej Szoszorek


śr., 3 cze 2026 o 19:34 Phillip Potter <phil@xxxxxxxxxxxxxxxx> napisał(a):
>
> On Tue, Jun 02, 2026 at 05:07:18PM +0200, Jedrzej-Sz wrote:
> > From: Jędrzej Szoszorek <jedrzej.szoszo@xxxxxxxxx>
> >
> > This patch introduces several modernizations and fixes to the legacy cdrom driver to align it with current kernel standards:
> >
> >     Concurrency: Added cdrom_events_lock spinlock to protect vfs_events and ioctl_events from data races. Wrapped use_count modifications in cdrom_open() and cdrom_release() with cdrom_mutex.
> >
> >     Memory Safety: Replaced obsolete and unsafe allocation methods with kzalloc() and struct_size() to prevent potential buffer overflows when allocating flexible arrays (e.g., struct cdrom_changer_info).
> >
> >     Sysctl API: Refactored cdrom_sysctl_info() to use dynamic memory allocation for the info buffer instead of a hardcoded string array, preventing truncation with multiple drives. Additionally, removed the empty {} sentinel at the end of cdrom_table to fix runtime warnings with the modern register_sysctl API.
> >
> >     Error Handling: Replaced inappropriate -ENOSYS returns in ioctl paths with -ENOTTY, as expected by current kernel design patterns.
> >
> >     Includes: Migrated from <asm/unaligned.h> to <linux/unaligned.h> to resolve compilation errors on modern mainline trees, updating unaligned memory access macros accordingly.
> >
> >     Code Style: Fixed numerous formatting issues, block comment alignments, trailing whitespaces, and replaced hardcoded function names in debug logs with __func__ to ensure checkpatch.pl compliance.
> >
> > Signed-off-by: Jędrzej Szoszorek <jedrzej.szoszo@xxxxxxxxx>
> > ---
> > Note: During the review and refactoring of this legacy driver, I used
> > an AI assistant (Gemini) to help identify outdated API patterns (like
> > missing event locks, legacy allocations, and sysctl sentinels). All
> > suggestions were manually audited, verified against current kernel APIs,
> > and successfully tested in a minimal QEMU environment.
> >
>
> Dear Jędrzej,
>
> Thank you for the patch. In its current form however, I don't feel like
> it's something I can effectively review. Few comments from me:
>
> (1) This is a huge patch. 700+ insertions and 600+ removals. If you're
>     serious about the changes, please split them up.
>
> (2) I'm not convinced of the value of the changes, for example just
>     reformatting the contributors list. It doesn't add much value as far
>     as I can see, whilst of course being structurally correct. It is
>     'churn' technically speaking.
>
> (3) If you are changing error values and other such things, I'm curious
>     to know how these changes have been tested, and if they introduce
>     regressions in calling code? When this is such a big refactor,
>     "successfully tested in a minimal QEMU environment." doesn't cut it
>     for me at present.
>
> Given the legacy/widespread nature of the driver, whilst I'm not
> explicitly anti-AI, I would hope that all these changes are well
> understood. In particular, that they solve real problems/issues without
> introducing regressions. If that's the case, then I'm happy to take a
> look at a slimmed down and revised set of patches that effect equivalent
> changes to the driver.
>
> All the best,
> Phil