Re: [bug report] dma-buf: call dma_buf_stats_setup after dmabuf is in valid list
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
Re: [bug report] dma-buf: call dma_buf_stats_setup after dmabuf is in valid list
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: '>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(>lock);
548 INIT_LIST_HEAD(>attachments);
549
550 mutex_lock(_list.lock);
551 list_add(>list_node, _list.head);
Added to the list
552 mutex_unlock(_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(_list.lock);
list_del(>list_node);
mutex_unlock(_list.lock);
..
}
570 err_module:
571 module_put(exp_info->owner);
572 return ERR_PTR(ret);
573 }
regards,
dan carpenter
Re: [bug report] dma-buf: call dma_buf_stats_setup after dmabuf is in valid list
++ 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: '>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(>lock);
>548 INIT_LIST_HEAD(>attachments);
>549
>550 mutex_lock(_list.lock);
>551 list_add(>list_node, _list.head);
>
> Added to the list
>
>552 mutex_unlock(_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(_list.lock);
list_del(>list_node);
mutex_unlock(_list.lock);
..
}
>
>570 err_module:
>571 module_put(exp_info->owner);
>572 return ERR_PTR(ret);
>573 }
>
> regards,
> dan carpenter
