From: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
[ Upstream commit cc6e8d1ccea792d8550428e0831e3a35b0ccfddc ]
The ivtv driver has a structure named ivtv_open_id that models an open file handle for the device. It embeds a v4l2_fh instance for file handles that correspond to a V4L2 video device, and stores a pointer to that v4l2_fh in struct ivtv_stream to identify which open file handle owns a particular stream.
In addition to video devices, streams can be owned by ALSA PCM devices. Those devices do not make use of the v4l2_fh instance for obvious reasons, but the snd_ivtv_pcm_capture_open() function still initializes a "fake" v4l2_fh for the sole purpose of using it as an open file handle identifier. The v4l2_fh is not properly destroyed when the ALSA PCM device is closed, leading to possible resource leaks.
Fortunately, the v4l2_fh instance pointed to by ivtv_stream is not accessed, only the pointer value is used for comparison. Replace it with a pointer to the ivtv_open_id structure that embeds the v4l2_fh, and don't initialize the v4l2_fh for ALSA PCM devices.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Hans Verkuil hverkuil+cisco@kernel.org Signed-off-by: Sasha Levin sashal@kernel.org ---
LLM Generated explanations, may be completely bogus:
YES
Why this is a good stable backport - Fixes a real bug: ALSA PCM open in ivtv initialized a fake v4l2_fh that was never properly destroyed on success or on “already streaming” early return, causing a kernel memory leak across repeated open/close cycles. - Small, contained change: confines to ivtv driver internals (media: pci: ivtv), with no API/ABI changes to V4L2/ALSA frameworks. - Behavior-preserving: the driver used the v4l2_fh pointer solely as an opaque owner identifier (pointer equality/non-NULL), never dereferencing it. Switching to an ivtv_open_id pointer preserves semantics without allocating any v4l2_fh resources. - Low regression risk: only owner-tracking logic and ALSA open path are touched; no architectural changes; no critical core subsystems affected. - User impact: prevents kernel memory leakage reachable by regular users via the ALSA PCM device node.
What the patch changes (with code evidence) - Stop creating a fake v4l2_fh for ALSA PCM open: - Removes `v4l2_fh_init(&item.fh, &s->vdev)` in ALSA open, avoiding allocation that was never exited in the success/streaming path. - Evidence: drivers/media/pci/ivtv/ivtv-alsa-pcm.c:151 - Switch stream owner tracking from v4l2_fh* to ivtv_open_id*: - Struct change: `struct ivtv_stream` replaces `struct v4l2_fh *fh;` with `struct ivtv_open_id *id;` - Evidence: drivers/media/pci/ivtv/ivtv-driver.h:334 - Adds forward decl for `struct ivtv_open_id;` so the pointer type can be used earlier in the header (part of the change). - Update owner comparisons and checks accordingly: - ivtv_claim_stream now compares `s->id == id` instead of `s->fh == &id->fh`, assigns `s->id = id`, and handles VBI special-case the same way. - Evidence: drivers/media/pci/ivtv/ivtv-fileops.c:42, 46–51, 59 - ivtv_release_stream clears `s->id = NULL` (was `s->fh = NULL`) and uses `s_vbi->id` for “still claimed” checks. - Evidence: drivers/media/pci/ivtv/ivtv-fileops.c:97, 129 - ivtv_read initialization check uses `s->id == NULL` instead of `s->fh == NULL`. - Evidence: drivers/media/pci/ivtv/ivtv-fileops.c:362 - ivtv_stop_capture VBI-internal use case sets `s->id = NULL` instead of `s->fh = NULL`. - Evidence: drivers/media/pci/ivtv/ivtv-fileops.c:834 - ivtv_v4l2_close compares `s->id != id` instead of `s->fh != &id->fh` to detect ownership. - Evidence: drivers/media/pci/ivtv/ivtv-fileops.c:918 - ivtv-irq VBI data handling checks `s->id == NULL` (was `s->fh == NULL`) to decide whether to free buffers if no owner, and uses `s->id` for wakeups. - Evidence: drivers/media/pci/ivtv/ivtv-irq.c:301–312 (check), 333–334 (wake_up) - Remove the now-unneeded v4l2_fh exit on the ALSA open fail path: - Since we no longer init a fake fh, there’s nothing to exit on -EBUSY. - Evidence: drivers/media/pci/ivtv/ivtv-alsa-pcm.c:151 (removal implies corresponding exit removal)
Why this fixes the leak - Before: `snd_ivtv_pcm_capture_open()` created `item.fh` via `v4l2_fh_init` and then: - On successful claim and subsequent returns (including the “already streaming” fast path), there was no matching `v4l2_fh_exit`, leaking internal resources of `v4l2_fh`. See initialization at drivers/media/pci/ivtv/ivtv-alsa-pcm.c:151 and early return in the streaming case just after claim. - After: There is no `v4l2_fh_init` for ALSA opens; the driver tracks ownership with a raw `ivtv_open_id *` pointer whose address is used only as a token (never dereferenced), eliminating the need for any initialization or teardown on the ALSA path.
Risk and side effects - Preserved semantics: s->fh was never dereferenced; it was used only for pointer equality and non-NULL checks. Replacing with s->id keeps those semantics while removing the artificial fh lifecycle. - Internal-only: Changes are contained within ivtv; no external APIs affected. Struct layout changed only internally to the driver. - Concurrency/logic continuity: The VBI special-case claim/release and wakeup conditions are updated consistently to use s->id; read path and IRQ paths keep identical logic, just different pointer type.
Stable criteria check - Important bugfix: addresses a user-visible kernel memory leak. - Minimal, localized change: touches only ivtv, no cross-subsystem churn. - No new features or architectural changes. - Low regression risk, with clear correctness rationale. - No explicit “Cc: stable” tag in the message, but it meets the stable rules for critical bug fixes with minimal risk.
Backport notes - Ensure all s->fh uses in ivtv are converted to s->id. In the current tree, these occur at: - drivers/media/pci/ivtv/ivtv-fileops.c:42, 46–51, 59, 97, 129, 362, 834, 918 - drivers/media/pci/ivtv/ivtv-irq.c:308, 333 - drivers/media/pci/ivtv/ivtv-driver.h:334 (field) - No other ivtv files refer to s->fh; remaining fh usage is via id->fh for V4L2 event/poll mechanisms and is unchanged. - The forward declaration `struct ivtv_open_id;` must be added before `struct ivtv_stream` in ivtv-driver.h to avoid compile errors.
Conclusion - This commit fixes a resource leak in a safe, contained way without changing behavior, and is suitable for backporting to stable kernel trees.
drivers/media/pci/ivtv/ivtv-alsa-pcm.c | 2 -- drivers/media/pci/ivtv/ivtv-driver.h | 3 ++- drivers/media/pci/ivtv/ivtv-fileops.c | 18 +++++++++--------- drivers/media/pci/ivtv/ivtv-irq.c | 4 ++-- 4 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/media/pci/ivtv/ivtv-alsa-pcm.c b/drivers/media/pci/ivtv/ivtv-alsa-pcm.c index 8f346d7da9c8d..269a799ec046c 100644 --- a/drivers/media/pci/ivtv/ivtv-alsa-pcm.c +++ b/drivers/media/pci/ivtv/ivtv-alsa-pcm.c @@ -148,14 +148,12 @@ static int snd_ivtv_pcm_capture_open(struct snd_pcm_substream *substream)
s = &itv->streams[IVTV_ENC_STREAM_TYPE_PCM];
- v4l2_fh_init(&item.fh, &s->vdev); item.itv = itv; item.type = s->type;
/* See if the stream is available */ if (ivtv_claim_stream(&item, item.type)) { /* No, it's already in use */ - v4l2_fh_exit(&item.fh); snd_ivtv_unlock(itvsc); return -EBUSY; } diff --git a/drivers/media/pci/ivtv/ivtv-driver.h b/drivers/media/pci/ivtv/ivtv-driver.h index a6ffa99e16bc6..83818048f7fe4 100644 --- a/drivers/media/pci/ivtv/ivtv-driver.h +++ b/drivers/media/pci/ivtv/ivtv-driver.h @@ -322,6 +322,7 @@ struct ivtv_queue { };
struct ivtv; /* forward reference */ +struct ivtv_open_id;
struct ivtv_stream { /* These first four fields are always set, even if the stream @@ -331,7 +332,7 @@ struct ivtv_stream { const char *name; /* name of the stream */ int type; /* stream type */
- struct v4l2_fh *fh; /* pointer to the streaming filehandle */ + struct ivtv_open_id *id; /* pointer to the streaming ivtv_open_id */ spinlock_t qlock; /* locks access to the queues */ unsigned long s_flags; /* status flags, see above */ int dma; /* can be PCI_DMA_TODEVICE, PCI_DMA_FROMDEVICE or PCI_DMA_NONE */ diff --git a/drivers/media/pci/ivtv/ivtv-fileops.c b/drivers/media/pci/ivtv/ivtv-fileops.c index cfa28d0355863..1ac8d691df5cd 100644 --- a/drivers/media/pci/ivtv/ivtv-fileops.c +++ b/drivers/media/pci/ivtv/ivtv-fileops.c @@ -39,16 +39,16 @@ int ivtv_claim_stream(struct ivtv_open_id *id, int type)
if (test_and_set_bit(IVTV_F_S_CLAIMED, &s->s_flags)) { /* someone already claimed this stream */ - if (s->fh == &id->fh) { + if (s->id == id) { /* yes, this file descriptor did. So that's OK. */ return 0; } - if (s->fh == NULL && (type == IVTV_DEC_STREAM_TYPE_VBI || + if (s->id == NULL && (type == IVTV_DEC_STREAM_TYPE_VBI || type == IVTV_ENC_STREAM_TYPE_VBI)) { /* VBI is handled already internally, now also assign the file descriptor to this stream for external reading of the stream. */ - s->fh = &id->fh; + s->id = id; IVTV_DEBUG_INFO("Start Read VBI\n"); return 0; } @@ -56,7 +56,7 @@ int ivtv_claim_stream(struct ivtv_open_id *id, int type) IVTV_DEBUG_INFO("Stream %d is busy\n", type); return -EBUSY; } - s->fh = &id->fh; + s->id = id; if (type == IVTV_DEC_STREAM_TYPE_VBI) { /* Enable reinsertion interrupt */ ivtv_clear_irq_mask(itv, IVTV_IRQ_DEC_VBI_RE_INSERT); @@ -94,7 +94,7 @@ void ivtv_release_stream(struct ivtv_stream *s) struct ivtv *itv = s->itv; struct ivtv_stream *s_vbi;
- s->fh = NULL; + s->id = NULL; if ((s->type == IVTV_DEC_STREAM_TYPE_VBI || s->type == IVTV_ENC_STREAM_TYPE_VBI) && test_bit(IVTV_F_S_INTERNAL_USE, &s->s_flags)) { /* this stream is still in use internally */ @@ -126,7 +126,7 @@ void ivtv_release_stream(struct ivtv_stream *s) /* was already cleared */ return; } - if (s_vbi->fh) { + if (s_vbi->id) { /* VBI stream still claimed by a file descriptor */ return; } @@ -359,7 +359,7 @@ static ssize_t ivtv_read(struct ivtv_stream *s, char __user *ubuf, size_t tot_co size_t tot_written = 0; int single_frame = 0;
- if (atomic_read(&itv->capturing) == 0 && s->fh == NULL) { + if (atomic_read(&itv->capturing) == 0 && s->id == NULL) { /* shouldn't happen */ IVTV_DEBUG_WARN("Stream %s not initialized before read\n", s->name); return -EIO; @@ -831,7 +831,7 @@ void ivtv_stop_capture(struct ivtv_open_id *id, int gop_end) id->type == IVTV_ENC_STREAM_TYPE_VBI) && test_bit(IVTV_F_S_INTERNAL_USE, &s->s_flags)) { /* Also used internally, don't stop capturing */ - s->fh = NULL; + s->id = NULL; } else { ivtv_stop_v4l2_encode_stream(s, gop_end); @@ -915,7 +915,7 @@ int ivtv_v4l2_close(struct file *filp) v4l2_fh_exit(fh);
/* Easy case first: this stream was never claimed by us */ - if (s->fh != &id->fh) + if (s->id != id) goto close_done;
/* 'Unclaim' this stream */ diff --git a/drivers/media/pci/ivtv/ivtv-irq.c b/drivers/media/pci/ivtv/ivtv-irq.c index 4d63daa01eed2..078d9cd77c710 100644 --- a/drivers/media/pci/ivtv/ivtv-irq.c +++ b/drivers/media/pci/ivtv/ivtv-irq.c @@ -305,7 +305,7 @@ static void dma_post(struct ivtv_stream *s) ivtv_process_vbi_data(itv, buf, 0, s->type); s->q_dma.bytesused += buf->bytesused; } - if (s->fh == NULL) { + if (s->id == NULL) { ivtv_queue_move(s, &s->q_dma, NULL, &s->q_free, 0); return; } @@ -330,7 +330,7 @@ static void dma_post(struct ivtv_stream *s) set_bit(IVTV_F_I_HAVE_WORK, &itv->i_flags); }
- if (s->fh) + if (s->id) wake_up(&s->waitq); }