[PATCH] dmaengine: Free channel and device IDs correctly

[PATCH] dmaengine: Free channel and device IDs correctly

[Changyul Lee]

Commit 0821009445a8 ("dmaengine: fix channel index enumeration") added
device->chan_ida and moved freeing device->dev_id from chan_dev_release()
to dma_async_device_unregister(). Since then, two paths handle these IDs
incorrectly.

__dma_async_device_channel_unregister() frees chan->chan_id before
device_unregister() removes the channel's sysfs entry. A channel
registered on the same device in that window can get the same ID, and
its device_register() fails:

  sysfs: cannot create duplicate filename '/devices/platform/...
  kobject: kobject_add_internal failed for dma0chan0 with -EEXIST, ...

idxd can hit this when one wq is unbound while another wq of the same
device is bound, because it holds only the per-wq lock.

When dma_async_device_register() fails after registering at least one
channel, its err_out path unregisters the channel devices but frees
neither the channel IDs nor device->dev_id. The next DMA device then
gets the next "dma<N>" number, skipping the leaked one.

In __dma_async_device_channel_unregister(), free chan_id after
device_unregister(). In the err_out path, add the missing frees: each
chan_id after device_unregister(), and dev_id after the loop.

Fixes: 0821009445a8 ("dmaengine: fix channel index enumeration")
Signed-off-by: Changyul Lee <lcy8047@gmail.com>
---

Notes:
    Found with a static analysis tool we are developing.
    
    Tested in QEMU (x86_64) with two test modules:
    
    - Unregister one channel and register another on the same dma_device
      from a second thread. A class_interface remove_dev() callback on the
      dma class starts the second thread while device_del() of the first
      channel is in progress, to make the window deterministic. Without
      this patch the new channel gets ID 0 and fails with -EEXIST. With it,
      the new channel gets ID 1 and is registered.
    
    - Register a dma_device whose second channel fails to register (both
      channels request the same static channel ID). Without this patch
      chan_ida is not empty afterwards and the next dma_device gets dev_id
      1. With it, chan_ida is empty and dev_id 0 is reused.
    
    Not tested on idxd hardware.

 drivers/dma/dmaengine.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 34be53487a21..6f9dcc735af5 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -1171,8 +1171,8 @@ static void __dma_async_device_channel_unregister(struct dma_device *device,
 	device->chancnt--;
 	chan->chan_dev->chan = NULL;
 	mutex_unlock(&dma_list_mutex);
-	ida_free(&device->chan_ida, chan->chan_id);
 	device_unregister(dmaengine_chan_dev(chan));
+	ida_free(&device->chan_ida, chan->chan_id);
 	free_percpu(chan->local);
 }
 
@@ -1307,8 +1307,10 @@ int dma_async_device_register(struct dma_device *device)
 		chan->chan_dev->chan = NULL;
 		mutex_unlock(&dma_list_mutex);
 		device_unregister(dmaengine_chan_dev(chan));
+		ida_free(&device->chan_ida, chan->chan_id);
 		free_percpu(chan->local);
 	}
+	ida_free(&dma_ida, device->dev_id);
 	return rc;
 }
 EXPORT_SYMBOL(dma_async_device_register);

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.43.0