Re: [bug report] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs

[Vinicius Costa Gomes <vinicius.gomes@intel.com>]

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