* [bug report] dma-buf: call dma_buf_stats_setup after dmabuf is in valid list
@ 2022-05-16 6:49 Dan Carpenter
2022-05-16 7:13 ` Charan Teja Kalla
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2022-05-16 6:49 UTC (permalink / raw)
To: quic_charante; +Cc: dri-devel
Hello Charan Teja Reddy,
The patch ef3a6b70507a: "dma-buf: call dma_buf_stats_setup after
dmabuf is in valid list" from May 10, 2022, leads to the following
Smatch static checker warning:
drivers/dma-buf/dma-buf.c:569 dma_buf_export()
warn: '&dmabuf->list_node' not removed from list
drivers/dma-buf/dma-buf.c
538 file = dma_buf_getfile(dmabuf, exp_info->flags);
539 if (IS_ERR(file)) {
540 ret = PTR_ERR(file);
541 goto err_dmabuf;
542 }
543
544 file->f_mode |= FMODE_LSEEK;
545 dmabuf->file = file;
546
547 mutex_init(&dmabuf->lock);
548 INIT_LIST_HEAD(&dmabuf->attachments);
549
550 mutex_lock(&db_list.lock);
551 list_add(&dmabuf->list_node, &db_list.head);
Added to the list
552 mutex_unlock(&db_list.lock);
553
554 ret = dma_buf_stats_setup(dmabuf);
555 if (ret)
556 goto err_sysfs;
Goto
557
558 return dmabuf;
559
560 err_sysfs:
561 /*
562 * Set file->f_path.dentry->d_fsdata to NULL so that when
563 * dma_buf_release() gets invoked by dentry_ops, it exits
564 * early before calling the release() dma_buf op.
565 */
566 file->f_path.dentry->d_fsdata = NULL;
567 fput(file);
568 err_dmabuf:
569 kfree(dmabuf);
dmabuf is freed, but it's still on the list so it leads to a use after
free.
570 err_module:
571 module_put(exp_info->owner);
572 return ERR_PTR(ret);
573 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] dma-buf: call dma_buf_stats_setup after dmabuf is in valid list
2022-05-16 6:49 [bug report] dma-buf: call dma_buf_stats_setup after dmabuf is in valid list Dan Carpenter
@ 2022-05-16 7:13 ` Charan Teja Kalla
2022-05-16 7:18 ` Christian König
0 siblings, 1 reply; 4+ messages in thread
From: Charan Teja Kalla @ 2022-05-16 7:13 UTC (permalink / raw)
To: Dan Carpenter, Christian König; +Cc: T.J. Mercier, dri-devel
++ Adding Christian
On 5/16/2022 12:19 PM, Dan Carpenter wrote:
> Hello Charan Teja Reddy,
>
> The patch ef3a6b70507a: "dma-buf: call dma_buf_stats_setup after
> dmabuf is in valid list" from May 10, 2022, leads to the following
> Smatch static checker warning:
>
> drivers/dma-buf/dma-buf.c:569 dma_buf_export()
> warn: '&dmabuf->list_node' not removed from list
>
> drivers/dma-buf/dma-buf.c
> 538 file = dma_buf_getfile(dmabuf, exp_info->flags);
> 539 if (IS_ERR(file)) {
> 540 ret = PTR_ERR(file);
> 541 goto err_dmabuf;
> 542 }
> 543
> 544 file->f_mode |= FMODE_LSEEK;
> 545 dmabuf->file = file;
> 546
> 547 mutex_init(&dmabuf->lock);
> 548 INIT_LIST_HEAD(&dmabuf->attachments);
> 549
> 550 mutex_lock(&db_list.lock);
> 551 list_add(&dmabuf->list_node, &db_list.head);
>
> Added to the list
>
> 552 mutex_unlock(&db_list.lock);
> 553
> 554 ret = dma_buf_stats_setup(dmabuf);
> 555 if (ret)
> 556 goto err_sysfs;
>
> Goto
>
> 557
> 558 return dmabuf;
> 559
> 560 err_sysfs:
> 561 /*
> 562 * Set file->f_path.dentry->d_fsdata to NULL so that when
> 563 * dma_buf_release() gets invoked by dentry_ops, it exits
> 564 * early before calling the release() dma_buf op.
> 565 */
> 566 file->f_path.dentry->d_fsdata = NULL;
> 567 fput(file);
> 568 err_dmabuf:
> 569 kfree(dmabuf);
>
> dmabuf is freed, but it's still on the list so it leads to a use after
> free.
This seems to be a false positive. On closing the file @line no:567, it
ends up calling dma_buf_file_release() which does remove dmabuf from its
list.
static int dma_buf_file_release(struct inode *inode, struct file *file) {
......
mutex_lock(&db_list.lock);
list_del(&dmabuf->list_node);
mutex_unlock(&db_list.lock);
......
}
>
> 570 err_module:
> 571 module_put(exp_info->owner);
> 572 return ERR_PTR(ret);
> 573 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] dma-buf: call dma_buf_stats_setup after dmabuf is in valid list
2022-05-16 7:13 ` Charan Teja Kalla
@ 2022-05-16 7:18 ` Christian König
2022-05-16 8:47 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Christian König @ 2022-05-16 7:18 UTC (permalink / raw)
To: Charan Teja Kalla, Dan Carpenter; +Cc: T.J. Mercier, dri-devel
Am 16.05.22 um 09:13 schrieb Charan Teja Kalla:
> ++ Adding Christian
>
> On 5/16/2022 12:19 PM, Dan Carpenter wrote:
>> Hello Charan Teja Reddy,
>>
>> The patch ef3a6b70507a: "dma-buf: call dma_buf_stats_setup after
>> dmabuf is in valid list" from May 10, 2022, leads to the following
>> Smatch static checker warning:
>>
>> drivers/dma-buf/dma-buf.c:569 dma_buf_export()
>> warn: '&dmabuf->list_node' not removed from list
>>
>> drivers/dma-buf/dma-buf.c
>> 538 file = dma_buf_getfile(dmabuf, exp_info->flags);
>> 539 if (IS_ERR(file)) {
>> 540 ret = PTR_ERR(file);
>> 541 goto err_dmabuf;
>> 542 }
>> 543
>> 544 file->f_mode |= FMODE_LSEEK;
>> 545 dmabuf->file = file;
>> 546
>> 547 mutex_init(&dmabuf->lock);
>> 548 INIT_LIST_HEAD(&dmabuf->attachments);
>> 549
>> 550 mutex_lock(&db_list.lock);
>> 551 list_add(&dmabuf->list_node, &db_list.head);
>>
>> Added to the list
>>
>> 552 mutex_unlock(&db_list.lock);
>> 553
>> 554 ret = dma_buf_stats_setup(dmabuf);
>> 555 if (ret)
>> 556 goto err_sysfs;
>>
>> Goto
>>
>> 557
>> 558 return dmabuf;
>> 559
>> 560 err_sysfs:
>> 561 /*
>> 562 * Set file->f_path.dentry->d_fsdata to NULL so that when
>> 563 * dma_buf_release() gets invoked by dentry_ops, it exits
>> 564 * early before calling the release() dma_buf op.
>> 565 */
>> 566 file->f_path.dentry->d_fsdata = NULL;
>> 567 fput(file);
>> 568 err_dmabuf:
>> 569 kfree(dmabuf);
>>
>> dmabuf is freed, but it's still on the list so it leads to a use after
>> free.
> This seems to be a false positive. On closing the file @line no:567, it
> ends up calling dma_buf_file_release() which does remove dmabuf from its
> list.
Yeah, correct as far as I can see. The checker just can't see that the
fput will cleanup the list.
Regards,
Christian.
>
> static int dma_buf_file_release(struct inode *inode, struct file *file) {
> ......
> mutex_lock(&db_list.lock);
> list_del(&dmabuf->list_node);
> mutex_unlock(&db_list.lock);
> ......
> }
>
>> 570 err_module:
>> 571 module_put(exp_info->owner);
>> 572 return ERR_PTR(ret);
>> 573 }
>>
>> regards,
>> dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] dma-buf: call dma_buf_stats_setup after dmabuf is in valid list
2022-05-16 7:18 ` Christian König
@ 2022-05-16 8:47 ` Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2022-05-16 8:47 UTC (permalink / raw)
To: Christian König; +Cc: Charan Teja Kalla, T.J. Mercier, dri-devel
On Mon, May 16, 2022 at 09:18:55AM +0200, Christian König wrote:
> > > 557
> > > 558 return dmabuf;
> > > 559
> > > 560 err_sysfs:
> > > 561 /*
> > > 562 * Set file->f_path.dentry->d_fsdata to NULL so that when
> > > 563 * dma_buf_release() gets invoked by dentry_ops, it exits
> > > 564 * early before calling the release() dma_buf op.
> > > 565 */
> > > 566 file->f_path.dentry->d_fsdata = NULL;
> > > 567 fput(file);
> > > 568 err_dmabuf:
> > > 569 kfree(dmabuf);
> > >
> > > dmabuf is freed, but it's still on the list so it leads to a use after
> > > free.
> > This seems to be a false positive. On closing the file @line no:567, it
> > ends up calling dma_buf_file_release() which does remove dmabuf from its
> > list.
>
> Yeah, correct as far as I can see. The checker just can't see that the fput
> will cleanup the list.
Yep. Thanks!
I hope that that Smatch will be better at parsing the fput() by the end
of the year but right now it doesn't work at all.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-16 8:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-16 6:49 [bug report] dma-buf: call dma_buf_stats_setup after dmabuf is in valid list Dan Carpenter
2022-05-16 7:13 ` Charan Teja Kalla
2022-05-16 7:18 ` Christian König
2022-05-16 8:47 ` Dan Carpenter
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.