Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] block: use-after-free of bdev in blkdev_open() when bdev_open() fails - [High] block: unpinned gendisk pointer in f_dma_io_tag allows tag ABA reuse --
commit 8ffc425965529fcad78b514404dc0182c1ca1bfb Author: Pavel Begunkov <[email protected]> block: set dma-buf-io tag to block files This commit sets filp->f_dma_io_tag to bdev->bd_disk in blkdev_open() so that dma-buf-io contexts can be shared across files and partitions belonging to the same block device. > diff --git a/block/fops.c b/block/fops.c > index e5c27cc35ca1f..2b10b835e99dc 100644 > --- a/block/fops.c > +++ b/block/fops.c > @@ -686,6 +686,7 @@ static int blkdev_open(struct inode *inode, struct file > *filp) > ret = bdev_open(bdev, mode, filp->private_data, NULL, filp); > if (ret) > blkdev_put_no_open(bdev); > + filp->f_dma_io_tag = (unsigned long)bdev->bd_disk; [Severity: High] Can this trigger a use-after-free of bdev when bdev_open() fails? In blkdev_open(), blkdev_get_no_open() acquires a reference to bdev->bd_device and drops the temporary blockdev_superblock inode reference. If bdev_open() returns an error (for example, when racing with disk or partition removal, or when bdev_disk_changed() drops partitions), blkdev_put_no_open(bdev) calls put_device(&bdev->bd_device) and drops blkdev_open()'s only reference to bdev: block/fops.c:blkdev_open() bdev = blkdev_get_no_open(inode->i_rdev, true); ... ret = bdev_open(bdev, mode, filp->private_data, NULL, filp); if (ret) blkdev_put_no_open(bdev); put_device(&bdev->bd_device); disk_release() / part_release() bdev_drop(bdev); filp->f_dma_io_tag = (unsigned long)bdev->bd_disk; Dereferencing bdev->bd_disk unconditionally after blkdev_put_no_open(bdev) reads from bdev after it may already have been freed. Should blkdev_open() return early on error, or set f_dma_io_tag inside bdev_open() alongside the other bdev_file fields? [Severity: High] Also, is it safe to use the raw bdev->bd_disk pointer as f_dma_io_tag without pinning the gendisk for the lifetime of the dma_buf_io_ctx? When a dmabuf is registered via __io_register_dmabuf(), dma_buf_io_ctx_create() stores file->f_dma_io_tag in ctx->importer_tag and calls into the block layer: dma_buf_io_ctx_create() ctx->importer_tag = file->f_dma_io_tag; blkdev_init_dma_buf_io_ctx() nvme_init_dma_buf_io_ctx() __nvme_init_dma_buf_io_ctx() nvme_init_io_ctx_locked() nvme_init_io_ctx_locked() only takes a reference to struct nvme_ctrl via nvme_get_ctrl(ctrl); neither blkdev_init_dma_buf_io_ctx() nor nvme_init_dma_buf_io_ctx() pins bdev->bd_disk or struct nvme_ns. Once the registration file is closed (or dropped in __io_register_dmabuf()), if the NVMe namespace is removed while the controller stays live (such as via nvme_ns_remove()), del_gendisk() and put_disk() can free the struct gendisk while ctx remains active with ctx->is_dead still false. If a new struct gendisk is later allocated at the same address for a different NVMe controller or a non-NVMe block device, opening that device will assign the same address to filp->f_dma_io_tag. Subsequent I/O using the registered dmabuf will pass dma_buf_io_file_compatible() and dispatch a REQ_DMABUF bio carrying nvme_dmabuf_map to the unrelated block device, where a non-NVMe driver will interpret bio->bi_io_vec (aliased with bio->bi_dmabuf_map) as a bio_vec array. Could this cause cross-device DMA or type confusion? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
