Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] block: 3-way deadlock between vdev_mutex, q_usage_counter, and queue freeze - [High] block: virtio-blk mempool-less dynamic allocation in bio submission path - [Medium] virtio_blk: TOCTOU race in ctrl_vq timeout causes memory leak - [Medium] block: memory leak of crypto profile on virtblk_probe error path --
commit d627f18e8794bdd76b3e4d25da68c27a6fd40f71 Author: Linlin Zhang <[email protected]> virtio_blk: add inline encryption support This commit adds support for the virtio-blk inline encryption feature. It allows offloading per-I/O encryption to the host's inline crypto engine instead of performing it in software in the guest. > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > index 2499ae2fb9e22..6fd23b45868a1 100644 > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c [ ... ] > @@ -985,7 +1051,7 @@ static void virtblk_ctrlq_callback(struct virtqueue *vq) > */ > if (abandoned) { > kfree(creq->compl); > - kfree(creq); > + kfree_sensitive(creq); > } else { > complete(&creq->compl->done); > } [Severity: Medium] Does this modification leave a time-of-check to time-of-use (TOCTOU) memory leak unaddressed? If a timeout expires in virtblk_ctrl_vq_request(), the waiter thread prepares to mark the request abandoned. If the device interrupt fires concurrently and virtblk_ctrlq_callback() reads abandoned as false before the waiter updates it, the callback will complete the request without freeing it. The waiter then sets abandoned to true and returns -ETIMEDOUT. Callers like virtblk_crypto_keyslot_program() receive -ETIMEDOUT and skip their local cleanup, assuming the background mechanism will free it. Neither path frees the memory. [ ... ] > @@ -1067,11 +1133,566 @@ static int virtblk_ctrl_vq_request(struct virtio_blk > *vblk, [ ... ] > +static int virtblk_crypto_keyslot_program(struct blk_crypto_profile *profile, > + const struct blk_crypto_key *key, > + unsigned int slot) > +{ > + struct virtio_blk *vblk = virtblk_from_profile(profile); > + struct scatterlist type_sg, out_req_sg, status_sg, *sgs[3]; > + struct virtblk_ctrl_request *creq; > + int err; > + > + mutex_lock(&vblk->vdev_mutex); [Severity: High] Can this create an AB-BA-C deadlock with queue freezing? virtblk_crypto_keyslot_program() acquires vdev_mutex. Since this runs as a block crypto profile callback during bio submission, the q_usage_counter read reference is already held by the submitting thread. If a concurrent teardown initiates a queue freeze via blk_mq_freeze_queue(), it blocks waiting for existing readers to finish. Concurrently, if virtblk_report_zones() has acquired vdev_mutex and attempts to allocate a request (blocking on the freeze), a 3-way deadlock occurs where virtblk_report_zones blocks on the freeze, the freeze blocks on the bio submission, and the bio submission blocks on vdev_mutex. > + if (!vblk->vdev) { > + err = -ENXIO; > + goto out_unlock; > + } > + > + /* > + * GFP_NOIO: this callback runs on the bio-submission path, which > + * memory reclaim can reach while writing back dirty pages to this > + * same device; GFP_KERNEL here could recurse into that same reclaim > + * and self-deadlock. > + */ > + creq = kzalloc_obj(*creq, GFP_NOIO); [Severity: High] Is it safe to use a mempool-less dynamic allocation here in the synchronous bio submission path? Under severe memory pressure, kzalloc_obj() (and kmalloc_obj() in virtblk_ctrl_vq_request) can fail and return -ENOMEM. This causes the block layer to immediately fail the bio with BLK_STS_RESOURCE instead of requeuing it, potentially dropping page writebacks and breaking forward-progress guarantees. [ ... ] > @@ -1764,6 +2387,28 @@ static int virtblk_probe(struct virtio_device *vdev) [ ... ] > + } else { > + err = virtblk_init_crypto(vblk); > + if (!err) { > + if (!blk_crypto_register(&vblk->profile, > vblk->disk->queue)) > + dev_warn(&vdev->dev, > + "failed to register inline > crypto profile\n"); > + } else { > + dev_warn(&vdev->dev, > + "inline crypto init failed: %d, > continuing without inline crypto support\n", > + err); > + } > + } > } > > err = device_add_disk(&vdev->dev, vblk->disk, virtblk_attr_groups); [Severity: Medium] Does this error path leak the crypto profile allocations? If device_add_disk() fails, the probe function jumps to out_cleanup_disk. Because device_add_disk() failed, the disk's GD_ADDED flag isn't set, meaning put_disk() will skip the .free_disk callback (virtblk_free_disk). As a result, virtblk_destroy_crypto() is never called, leaking the dynamically allocated profile.slots and profile.slot_hashtable arrays. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
