* [bug report] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs
@ 2025-05-26 9:01 Dan Carpenter
2025-05-26 9:26 ` Shuai Xue
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2025-05-26 9:01 UTC (permalink / raw)
To: Shuai Xue; +Cc: dmaengine
Hello Shuai Xue,
Commit 3fd2f4bc010c ("dmaengine: idxd: fix memory leak in error
handling path of idxd_setup_wqs") from Apr 4, 2025 (linux-next),
leads to the following Smatch static checker warning:
drivers/dma/idxd/init.c:246 idxd_setup_wqs()
error: uninitialized symbol 'conf_dev'.
drivers/dma/idxd/init.c
177 static int idxd_setup_wqs(struct idxd_device *idxd)
178 {
179 struct device *dev = &idxd->pdev->dev;
180 struct idxd_wq *wq;
181 struct device *conf_dev;
182 int i, rc;
183
184 idxd->wqs = kcalloc_node(idxd->max_wqs, sizeof(struct idxd_wq *),
185 GFP_KERNEL, dev_to_node(dev));
186 if (!idxd->wqs)
187 return -ENOMEM;
188
189 idxd->wq_enable_map = bitmap_zalloc_node(idxd->max_wqs, GFP_KERNEL, dev_to_node(dev));
190 if (!idxd->wq_enable_map) {
191 rc = -ENOMEM;
192 goto err_bitmap;
193 }
194
195 for (i = 0; i < idxd->max_wqs; i++) {
196 wq = kzalloc_node(sizeof(*wq), GFP_KERNEL, dev_to_node(dev));
197 if (!wq) {
198 rc = -ENOMEM;
199 goto err;
On this error path we either free an uninitialized variable or we
double free conf_dev.
200 }
201
202 idxd_dev_set_type(&wq->idxd_dev, IDXD_DEV_WQ);
203 conf_dev = wq_confdev(wq);
204 wq->id = i;
205 wq->idxd = idxd;
206 device_initialize(wq_confdev(wq));
207 conf_dev->parent = idxd_confdev(idxd);
208 conf_dev->bus = &dsa_bus_type;
209 conf_dev->type = &idxd_wq_device_type;
210 rc = dev_set_name(conf_dev, "wq%d.%d", idxd->id, wq->id);
211 if (rc < 0)
212 goto err;
When we're cleaning up loops what I recommend is that we clean up the
partial iterations before the goto.
if (rc < 0) {
put_device(conf_dev);
kfree(wq);
goto unwind_loop;
}
That's sort of how the code was written originally but it missed some
frees.
213
214 mutex_init(&wq->wq_lock);
215 init_waitqueue_head(&wq->err_queue);
216 init_completion(&wq->wq_dead);
217 init_completion(&wq->wq_resurrect);
218 wq->max_xfer_bytes = WQ_DEFAULT_MAX_XFER;
219 idxd_wq_set_max_batch_size(idxd->data->type, wq, WQ_DEFAULT_MAX_BATCH);
220 wq->enqcmds_retries = IDXD_ENQCMDS_RETRIES;
221 wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev));
222 if (!wq->wqcfg) {
223 rc = -ENOMEM;
224 goto err;
225 }
Same:
if (!wq->wqcfg) {
put_device(conf_dev);
kfree(wq);
rc = -ENOMEM;
goto unwind_loop;
}
226
227 if (idxd->hw.wq_cap.op_config) {
228 wq->opcap_bmap = bitmap_zalloc(IDXD_MAX_OPCAP_BITS, GFP_KERNEL);
229 if (!wq->opcap_bmap) {
230 rc = -ENOMEM;
231 goto err_opcap_bmap;
232 }
if (!wq->wqcfg) {
kfree(wq->wqcfg);
put_device(conf_dev);
kfree(wq);
rc = -ENOMEM;
goto unwind_loop;
}
233 bitmap_copy(wq->opcap_bmap, idxd->opcap_bmap, IDXD_MAX_OPCAP_BITS);
234 }
235 mutex_init(&wq->uc_lock);
236 xa_init(&wq->upasid_xa);
237 idxd->wqs[i] = wq;
238 }
239
Imagine if we add another two allocations here.
foo = alloc();
if (!foo)
goto err;
bar = alloc();
if (!bar)
goto free_foo;
240 return 0;
241
Then we have to do a little bunny hop.
free_foo:
free(foo);
goto err; // <-- bunny hop
err_opcap_bmap:
kfree(wq->wqcfg);
People often get confused and forget the bunny hop.
242 err_opcap_bmap:
243 kfree(wq->wqcfg);
244
245 err:
--> 246 put_device(conf_dev);
247 kfree(wq);
248
249 while (--i >= 0) {
250 wq = idxd->wqs[i];
251 if (idxd->hw.wq_cap.op_config)
252 bitmap_free(wq->opcap_bmap);
253 kfree(wq->wqcfg);
254 conf_dev = wq_confdev(wq);
255 put_device(conf_dev);
256 kfree(wq);
257
258 }
259 bitmap_free(idxd->wq_enable_map);
260
261 err_bitmap:
262 kfree(idxd->wqs);
263
264 return rc;
265 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs
2025-05-26 9:01 [bug report] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs Dan Carpenter
@ 2025-05-26 9:26 ` Shuai Xue
2025-05-28 2:22 ` Vinicius Costa Gomes
0 siblings, 1 reply; 3+ messages in thread
From: Shuai Xue @ 2025-05-26 9:26 UTC (permalink / raw)
To: Dan Carpenter, Dave Jiang, Colin King (gmail); +Cc: dmaengine
在 2025/5/26 17:01, Dan Carpenter 写道:
> Hello Shuai Xue,
>
> Commit 3fd2f4bc010c ("dmaengine: idxd: fix memory leak in error
> handling path of idxd_setup_wqs") from Apr 4, 2025 (linux-next),
> leads to the following Smatch static checker warning:
>
> drivers/dma/idxd/init.c:246 idxd_setup_wqs()
> error: uninitialized symbol 'conf_dev'.
>
> drivers/dma/idxd/init.c
> 177 static int idxd_setup_wqs(struct idxd_device *idxd)
> 178 {
> 179 struct device *dev = &idxd->pdev->dev;
> 180 struct idxd_wq *wq;
> 181 struct device *conf_dev;
> 182 int i, rc;
> 183
> 184 idxd->wqs = kcalloc_node(idxd->max_wqs, sizeof(struct idxd_wq *),
> 185 GFP_KERNEL, dev_to_node(dev));
> 186 if (!idxd->wqs)
> 187 return -ENOMEM;
> 188
> 189 idxd->wq_enable_map = bitmap_zalloc_node(idxd->max_wqs, GFP_KERNEL, dev_to_node(dev));
> 190 if (!idxd->wq_enable_map) {
> 191 rc = -ENOMEM;
> 192 goto err_bitmap;
> 193 }
> 194
> 195 for (i = 0; i < idxd->max_wqs; i++) {
> 196 wq = kzalloc_node(sizeof(*wq), GFP_KERNEL, dev_to_node(dev));
> 197 if (!wq) {
> 198 rc = -ENOMEM;
> 199 goto err;
>
> On this error path we either free an uninitialized variable or we
> double free conf_dev.
Hi, Dan,
Thanks for reporting this bug:)
It has reported by Colin but he forgot to cc mailing list.
And I sent a patch to fix it:
https://lore.kernel.org/lkml/19668a72-c523-42ab-87ac-990a4baac214@intel.com/
>
> 200 }
> 201
> 202 idxd_dev_set_type(&wq->idxd_dev, IDXD_DEV_WQ);
> 203 conf_dev = wq_confdev(wq);
> 204 wq->id = i;
> 205 wq->idxd = idxd;
> 206 device_initialize(wq_confdev(wq));
> 207 conf_dev->parent = idxd_confdev(idxd);
> 208 conf_dev->bus = &dsa_bus_type;
> 209 conf_dev->type = &idxd_wq_device_type;
> 210 rc = dev_set_name(conf_dev, "wq%d.%d", idxd->id, wq->id);
> 211 if (rc < 0)
> 212 goto err;
>
> When we're cleaning up loops what I recommend is that we clean up the
> partial iterations before the goto.
>
> if (rc < 0) {
> put_device(conf_dev);
> kfree(wq);
> goto unwind_loop;
> }
>
> That's sort of how the code was written originally but it missed some
> frees.
>
>
> 213
> 214 mutex_init(&wq->wq_lock);
> 215 init_waitqueue_head(&wq->err_queue);
> 216 init_completion(&wq->wq_dead);
> 217 init_completion(&wq->wq_resurrect);
> 218 wq->max_xfer_bytes = WQ_DEFAULT_MAX_XFER;
> 219 idxd_wq_set_max_batch_size(idxd->data->type, wq, WQ_DEFAULT_MAX_BATCH);
> 220 wq->enqcmds_retries = IDXD_ENQCMDS_RETRIES;
> 221 wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev));
> 222 if (!wq->wqcfg) {
> 223 rc = -ENOMEM;
> 224 goto err;
> 225 }
>
> Same:
>
> if (!wq->wqcfg) {
> put_device(conf_dev);
> kfree(wq);
> rc = -ENOMEM;
> goto unwind_loop;
> }
>
> 226
> 227 if (idxd->hw.wq_cap.op_config) {
> 228 wq->opcap_bmap = bitmap_zalloc(IDXD_MAX_OPCAP_BITS, GFP_KERNEL);
> 229 if (!wq->opcap_bmap) {
> 230 rc = -ENOMEM;
> 231 goto err_opcap_bmap;
> 232 }
>
> if (!wq->wqcfg) {
> kfree(wq->wqcfg);
> put_device(conf_dev);
> kfree(wq);
> rc = -ENOMEM;
> goto unwind_loop;
> }
>
>
> 233 bitmap_copy(wq->opcap_bmap, idxd->opcap_bmap, IDXD_MAX_OPCAP_BITS);
> 234 }
> 235 mutex_init(&wq->uc_lock);
> 236 xa_init(&wq->upasid_xa);
> 237 idxd->wqs[i] = wq;
> 238 }
> 239
>
> Imagine if we add another two allocations here.
>
> foo = alloc();
> if (!foo)
> goto err;
> bar = alloc();
> if (!bar)
> goto free_foo;
>
>
> 240 return 0;
> 241
>
> Then we have to do a little bunny hop.
>
> free_foo:
> free(foo);
> goto err; // <-- bunny hop
>
> err_opcap_bmap:
> kfree(wq->wqcfg);
>
> People often get confused and forget the bunny hop.
I think so, this is the way I used in the original patch I send.
but reviewer Markus point to move common free code to additional
jump targets.
https://lore.kernel.org/lkml/98327a4d-7684-4908-9d67-5dfcaa229ae1@web.de/
I feel free to change back to clean up the partial iterations before the goto.
@Dave, which way do you like?
(Dave is on vacation, I can send a new patch if he prefer the latter way)
>
> 242 err_opcap_bmap:
> 243 kfree(wq->wqcfg);
> 244
> 245 err:
> --> 246 put_device(conf_dev);
> 247 kfree(wq);
> 248
> 249 while (--i >= 0) {
> 250 wq = idxd->wqs[i];
> 251 if (idxd->hw.wq_cap.op_config)
> 252 bitmap_free(wq->opcap_bmap);
> 253 kfree(wq->wqcfg);
> 254 conf_dev = wq_confdev(wq);
> 255 put_device(conf_dev);
> 256 kfree(wq);
> 257
> 258 }
> 259 bitmap_free(idxd->wq_enable_map);
> 260
> 261 err_bitmap:
> 262 kfree(idxd->wqs);
> 263
> 264 return rc;
> 265 }
>
> regards,
> dan carpenter
Thanks.
Regards
Shuai
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs
2025-05-26 9:26 ` Shuai Xue
@ 2025-05-28 2:22 ` Vinicius Costa Gomes
0 siblings, 0 replies; 3+ messages in thread
From: Vinicius Costa Gomes @ 2025-05-28 2:22 UTC (permalink / raw)
To: Shuai Xue, Dan Carpenter, Dave Jiang, Colin King (gmail); +Cc: dmaengine
Hi Shuai,
Shuai Xue <xueshuai@linux.alibaba.com> writes:
> 在 2025/5/26 17:01, Dan Carpenter 写道:
>> Hello Shuai Xue,
>>
>> Commit 3fd2f4bc010c ("dmaengine: idxd: fix memory leak in error
>> handling path of idxd_setup_wqs") from Apr 4, 2025 (linux-next),
>> leads to the following Smatch static checker warning:
>>
>> drivers/dma/idxd/init.c:246 idxd_setup_wqs()
>> error: uninitialized symbol 'conf_dev'.
>>
>> drivers/dma/idxd/init.c
>> 177 static int idxd_setup_wqs(struct idxd_device *idxd)
>> 178 {
>> 179 struct device *dev = &idxd->pdev->dev;
>> 180 struct idxd_wq *wq;
>> 181 struct device *conf_dev;
>> 182 int i, rc;
>> 183
>> 184 idxd->wqs = kcalloc_node(idxd->max_wqs, sizeof(struct idxd_wq *),
>> 185 GFP_KERNEL, dev_to_node(dev));
>> 186 if (!idxd->wqs)
>> 187 return -ENOMEM;
>> 188
>> 189 idxd->wq_enable_map = bitmap_zalloc_node(idxd->max_wqs, GFP_KERNEL, dev_to_node(dev));
>> 190 if (!idxd->wq_enable_map) {
>> 191 rc = -ENOMEM;
>> 192 goto err_bitmap;
>> 193 }
>> 194
>> 195 for (i = 0; i < idxd->max_wqs; i++) {
>> 196 wq = kzalloc_node(sizeof(*wq), GFP_KERNEL, dev_to_node(dev));
>> 197 if (!wq) {
>> 198 rc = -ENOMEM;
>> 199 goto err;
>>
>> On this error path we either free an uninitialized variable or we
>> double free conf_dev.
>
> Hi, Dan,
>
> Thanks for reporting this bug:)
>
> It has reported by Colin but he forgot to cc mailing list.
> And I sent a patch to fix it:
> https://lore.kernel.org/lkml/19668a72-c523-42ab-87ac-990a4baac214@intel.com/
>
>>
>> 200 }
>> 201
>> 202 idxd_dev_set_type(&wq->idxd_dev, IDXD_DEV_WQ);
>> 203 conf_dev = wq_confdev(wq);
>> 204 wq->id = i;
>> 205 wq->idxd = idxd;
>> 206 device_initialize(wq_confdev(wq));
>> 207 conf_dev->parent = idxd_confdev(idxd);
>> 208 conf_dev->bus = &dsa_bus_type;
>> 209 conf_dev->type = &idxd_wq_device_type;
>> 210 rc = dev_set_name(conf_dev, "wq%d.%d", idxd->id, wq->id);
>> 211 if (rc < 0)
>> 212 goto err;
>>
>> When we're cleaning up loops what I recommend is that we clean up the
>> partial iterations before the goto.
>>
>> if (rc < 0) {
>> put_device(conf_dev);
>> kfree(wq);
>> goto unwind_loop;
>> }
>>
>> That's sort of how the code was written originally but it missed some
>> frees.
>>
>>
>> 213
>> 214 mutex_init(&wq->wq_lock);
>> 215 init_waitqueue_head(&wq->err_queue);
>> 216 init_completion(&wq->wq_dead);
>> 217 init_completion(&wq->wq_resurrect);
>> 218 wq->max_xfer_bytes = WQ_DEFAULT_MAX_XFER;
>> 219 idxd_wq_set_max_batch_size(idxd->data->type, wq, WQ_DEFAULT_MAX_BATCH);
>> 220 wq->enqcmds_retries = IDXD_ENQCMDS_RETRIES;
>> 221 wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev));
>> 222 if (!wq->wqcfg) {
>> 223 rc = -ENOMEM;
>> 224 goto err;
>> 225 }
>>
>> Same:
>>
>> if (!wq->wqcfg) {
>> put_device(conf_dev);
>> kfree(wq);
>> rc = -ENOMEM;
>> goto unwind_loop;
>> }
>>
>> 226
>> 227 if (idxd->hw.wq_cap.op_config) {
>> 228 wq->opcap_bmap = bitmap_zalloc(IDXD_MAX_OPCAP_BITS, GFP_KERNEL);
>> 229 if (!wq->opcap_bmap) {
>> 230 rc = -ENOMEM;
>> 231 goto err_opcap_bmap;
>> 232 }
>>
>> if (!wq->wqcfg) {
>> kfree(wq->wqcfg);
>> put_device(conf_dev);
>> kfree(wq);
>> rc = -ENOMEM;
>> goto unwind_loop;
>> }
>>
>>
>> 233 bitmap_copy(wq->opcap_bmap, idxd->opcap_bmap, IDXD_MAX_OPCAP_BITS);
>> 234 }
>> 235 mutex_init(&wq->uc_lock);
>> 236 xa_init(&wq->upasid_xa);
>> 237 idxd->wqs[i] = wq;
>> 238 }
>> 239
>>
>> Imagine if we add another two allocations here.
>>
>> foo = alloc();
>> if (!foo)
>> goto err;
>> bar = alloc();
>> if (!bar)
>> goto free_foo;
>>
>>
>> 240 return 0;
>> 241
>>
>> Then we have to do a little bunny hop.
>>
>> free_foo:
>> free(foo);
>> goto err; // <-- bunny hop
>>
>> err_opcap_bmap:
>> kfree(wq->wqcfg);
>>
>> People often get confused and forget the bunny hop.
>
> I think so, this is the way I used in the original patch I send.
> but reviewer Markus point to move common free code to additional
> jump targets.
>
> https://lore.kernel.org/lkml/98327a4d-7684-4908-9d67-5dfcaa229ae1@web.de/
>
> I feel free to change back to clean up the partial iterations before the goto.
> @Dave, which way do you like?
>
> (Dave is on vacation, I can send a new patch if he prefer the latter way)
>
I think the solution you proposed is fine. At least to me, it looks clear
enough and seems to fix the problem.
>>
>> 242 err_opcap_bmap:
>> 243 kfree(wq->wqcfg);
>> 244
>> 245 err:
>> --> 246 put_device(conf_dev);
>> 247 kfree(wq);
>> 248
>> 249 while (--i >= 0) {
>> 250 wq = idxd->wqs[i];
>> 251 if (idxd->hw.wq_cap.op_config)
>> 252 bitmap_free(wq->opcap_bmap);
>> 253 kfree(wq->wqcfg);
>> 254 conf_dev = wq_confdev(wq);
>> 255 put_device(conf_dev);
>> 256 kfree(wq);
>> 257
>> 258 }
>> 259 bitmap_free(idxd->wq_enable_map);
>> 260
>> 261 err_bitmap:
>> 262 kfree(idxd->wqs);
>> 263
>> 264 return rc;
>> 265 }
>>
>> regards,
>> dan carpenter
>
>
> Thanks.
>
> Regards
> Shuai
Cheers,
--
Vinicius
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-05-28 2:22 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-26 9:01 [bug report] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs Dan Carpenter
2025-05-26 9:26 ` Shuai Xue
2025-05-28 2:22 ` Vinicius Costa Gomes
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.