* Re: [PATCH v2] staging: media: ipu7: fix boot_config leak on queue_mem failure
2026-04-17 8:01 ` Dan Carpenter
@ 2026-04-17 8:05 ` Dan Carpenter
2026-04-17 8:46 ` Dan Carpenter
1 sibling, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2026-04-17 8:05 UTC (permalink / raw)
To: Huihui Huang
Cc: Sakari Ailus, Mauro Carvalho Chehab, Bingbu Cao,
Greg Kroah-Hartman, linux-media, linux-staging, linux-kernel
On Fri, Apr 17, 2026 at 11:01:29AM +0300, Dan Carpenter wrote:
> On Fri, Apr 17, 2026 at 03:39:39PM +0800, Huihui Huang wrote:
> > There is a memory leak in drivers/staging/media/ipu7/ipu7-boot.c.
> >
> > In ipu7_boot_init_boot_config(), boot_config is allocated by
> > ipu7_dma_alloc(). If the second ipu7_dma_alloc() for queue_mem fails,
> > the function returns -ENOMEM without freeing the previously allocated
> > boot_config.
> >
> > Add the missing ipu7_dma_free() call before returning on the error
> > path.
> >
> > Signed-off-by: Huihui Huang <hhhuang@smu.edu.sg>
> > ---
> > v2: Reword commit message in imperative mood. Remove unnecessary
> > NULL assignment on the error path.
> > ---
> > drivers/staging/media/ipu7/ipu7-boot.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/staging/media/ipu7/ipu7-boot.c b/drivers/staging/media/ipu7/ipu7-boot.c
> > index d7901ff78b38..495b3e05a9b1 100644
> > --- a/drivers/staging/media/ipu7/ipu7-boot.c
> > +++ b/drivers/staging/media/ipu7/ipu7-boot.c
> > @@ -263,6 +263,8 @@ int ipu7_boot_init_boot_config(struct ipu7_bus_device *adev,
> > GFP_KERNEL, 0);
> > if (!syscom->queue_mem) {
> > dev_err(dev, "Failed to allocate queue memory.\n");
> > + ipu7_dma_free(adev, adev->boot_config_size,
> > + adev->boot_config, adev->boot_config_dma_addr, 0);
> > return -ENOMEM;
>
> Adding a free here leads to a double free. It's the same issue.
> One magical cleanup function in the caller.
>
> I haven't looked at this but I bet there are bugs in the error handling
> since magical cleanup functions are always buggy.
Btw, if you had kept the "adev->boot_config = NULL;" assignment that
you had in v1 then that would have prevented the double free since
ipu7_boot_release_boot_config() tests for that... This information is
not useful to you at this point but I'm sure you will find it
frustrating. :P
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] staging: media: ipu7: fix boot_config leak on queue_mem failure
2026-04-17 8:01 ` Dan Carpenter
2026-04-17 8:05 ` Dan Carpenter
@ 2026-04-17 8:46 ` Dan Carpenter
2026-04-17 14:22 ` [v2] " Huihui Huang
1 sibling, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2026-04-17 8:46 UTC (permalink / raw)
To: Huihui Huang
Cc: Sakari Ailus, Mauro Carvalho Chehab, Bingbu Cao,
Greg Kroah-Hartman, linux-media, linux-staging, linux-kernel
On Fri, Apr 17, 2026 at 11:01:29AM +0300, Dan Carpenter wrote:
> I haven't looked at this but I bet there are bugs in the error handling
> since magical cleanup functions are always buggy.
The error handling in ipu7_fw_isys_init() is frustrating to review but
it works fine.
Imagine that someone were writing this error handling methodically, using
a normal unwind ladder.
https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/
drivers/staging/media/ipu7/ipu7-fw-isys.c
81 int ipu7_fw_isys_init(struct ipu7_isys *isys)
82 {
83 struct syscom_queue_config *queue_configs;
84 struct ipu7_bus_device *adev = isys->adev;
85 struct device *dev = &adev->auxdev.dev;
86 struct ipu7_insys_config *isys_config;
87 struct ipu7_syscom_context *syscom;
88 dma_addr_t isys_config_dma_addr;
89 unsigned int i, num_queues;
90 u32 freq;
91 u8 major;
92 int ret;
93
94 /* Allocate and init syscom context. */
95 syscom = devm_kzalloc(dev, sizeof(struct ipu7_syscom_context),
96 GFP_KERNEL);
97 if (!syscom)
98 return -ENOMEM;
This is our first resource. It's allocated with devm_ so we don't need
to free it. Nothing to free.
99
100 adev->syscom = syscom;
101 syscom->num_input_queues = IPU_INSYS_MAX_INPUT_QUEUES;
102 syscom->num_output_queues = IPU_INSYS_MAX_OUTPUT_QUEUES;
103 num_queues = syscom->num_input_queues + syscom->num_output_queues;
104 queue_configs = devm_kzalloc(dev, FW_QUEUE_CONFIG_SIZE(num_queues),
105 GFP_KERNEL);
This is our second resource. Still devm_. Still nothing to free.
106 if (!queue_configs) {
107 ipu7_fw_isys_release(isys);
And yet we call ipu7_fw_isys_release(). Fortunately it's a no-op.
108 return -ENOMEM;
109 }
110 syscom->queue_configs = queue_configs;
111 queue_configs[IPU_INSYS_OUTPUT_MSG_QUEUE].max_capacity =
112 IPU_ISYS_SIZE_RECV_QUEUE;
113 queue_configs[IPU_INSYS_OUTPUT_MSG_QUEUE].token_size_in_bytes =
114 sizeof(struct ipu7_insys_resp);
115 queue_configs[IPU_INSYS_OUTPUT_LOG_QUEUE].max_capacity =
116 IPU_ISYS_SIZE_LOG_QUEUE;
117 queue_configs[IPU_INSYS_OUTPUT_LOG_QUEUE].token_size_in_bytes =
118 sizeof(struct ipu7_insys_resp);
119 queue_configs[IPU_INSYS_OUTPUT_RESERVED_QUEUE].max_capacity = 0;
120 queue_configs[IPU_INSYS_OUTPUT_RESERVED_QUEUE].token_size_in_bytes = 0;
121
122 queue_configs[IPU_INSYS_INPUT_DEV_QUEUE].max_capacity =
123 IPU_ISYS_MAX_STREAMS;
124 queue_configs[IPU_INSYS_INPUT_DEV_QUEUE].token_size_in_bytes =
125 sizeof(struct ipu7_insys_send_queue_token);
126
127 for (i = IPU_INSYS_INPUT_MSG_QUEUE; i < num_queues; i++) {
128 queue_configs[i].max_capacity = IPU_ISYS_SIZE_SEND_QUEUE;
129 queue_configs[i].token_size_in_bytes =
130 sizeof(struct ipu7_insys_send_queue_token);
131 }
132
133 /* Allocate ISYS subsys config. */
134 isys_config = ipu7_dma_alloc(adev, sizeof(struct ipu7_insys_config),
135 &isys_config_dma_addr, GFP_KERNEL, 0);
136 if (!isys_config) {
137 dev_err(dev, "Failed to allocate isys subsys config.\n");
138 ipu7_fw_isys_release(isys);
Still nothing to free. This is still a no-op.
139 return -ENOMEM;
140 }
141 isys->subsys_config = isys_config;
isys->subsys_config is our first resource that we have to free.
142 isys->subsys_config_dma_addr = isys_config_dma_addr;
143 memset(isys_config, 0, sizeof(struct ipu7_insys_config));
144 isys_config->logger_config.use_source_severity = 0;
145 isys_config->logger_config.use_channels_enable_bitmask = 1;
146 isys_config->logger_config.channels_enable_bitmask =
147 LOGGER_CONFIG_CHANNEL_ENABLE_SYSCOM_BITMASK;
148 isys_config->logger_config.hw_printf_buffer_base_addr = 0U;
149 isys_config->logger_config.hw_printf_buffer_size_bytes = 0U;
150 isys_config->wdt_config.wdt_timer1_us = 0;
151 isys_config->wdt_config.wdt_timer2_us = 0;
152 ret = ipu_buttress_get_isys_freq(adev->isp, &freq);
This doesn't allocate anything, but we would need to free
isys->subsys_config. Ideally, we would have a goto free_subsys_config;
here.
153 if (ret) {
154 dev_err(dev, "Failed to get ISYS frequency.\n");
155 ipu7_fw_isys_release(isys);
But this does free isys->subsys_config. Good.
156 return ret;
157 }
158
159 ipu7_dma_sync_single(adev, isys_config_dma_addr,
160 sizeof(struct ipu7_insys_config));
161
162 major = is_ipu8(adev->isp->hw_ver) ? 2U : 1U;
163 ret = ipu7_boot_init_boot_config(adev, queue_configs, num_queues,
164 freq, isys_config_dma_addr, major);
This allocates one or both of syscom->queue_mem and adev->boot_config.
Ideally it would clean up partial allocations and we would do the same
goto free_subsys_config; here.
165 if (ret)
166 ipu7_fw_isys_release(isys);
But this does free them along with isys->subsys_config. Everything
works.
167
168 return ret;
169 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread