Commit 4dba12881f88 ("dm zoned: support arbitrary number of devices") made the pointers to additional zoned devices to be stored in a dynamically allocated dmz->ddev array. However, this array is not freed.
Free it when cleaning up zoned device information inside dmz_put_zoned_device(). Assigning NULL to dmz->ddev elements doesn't make sense there as they are not supposed to be reused later and the whole dmz target structure is being cleaned anyway.
Found by Linux Verification Center (linuxtesting.org).
Fixes: 4dba12881f88 ("dm zoned: support arbitrary number of devices") Cc: stable@vger.kernel.org Signed-off-by: Fedor Pchelkin pchelkin@ispras.ru --- drivers/md/dm-zoned-target.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c index ad8e670a2f9b..e25cd9db6275 100644 --- a/drivers/md/dm-zoned-target.c +++ b/drivers/md/dm-zoned-target.c @@ -753,12 +753,10 @@ static void dmz_put_zoned_device(struct dm_target *ti) struct dmz_target *dmz = ti->private; int i;
- for (i = 0; i < dmz->nr_ddevs; i++) { - if (dmz->ddev[i]) { + for (i = 0; i < dmz->nr_ddevs; i++) + if (dmz->ddev[i]) dm_put_device(ti, dmz->ddev[i]); - dmz->ddev[i] = NULL; - } - } + kfree(dmz->ddev); }
static int dmz_fixup_devices(struct dm_target *ti)
On 9/20/23 12:51, Fedor Pchelkin wrote:
Commit 4dba12881f88 ("dm zoned: support arbitrary number of devices") made the pointers to additional zoned devices to be stored in a dynamically allocated dmz->ddev array. However, this array is not freed.
Free it when cleaning up zoned device information inside dmz_put_zoned_device(). Assigning NULL to dmz->ddev elements doesn't make sense there as they are not supposed to be reused later and the whole dmz target structure is being cleaned anyway.
Found by Linux Verification Center (linuxtesting.org).
Fixes: 4dba12881f88 ("dm zoned: support arbitrary number of devices") Cc: stable@vger.kernel.org Signed-off-by: Fedor Pchelkin pchelkin@ispras.ru
drivers/md/dm-zoned-target.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c index ad8e670a2f9b..e25cd9db6275 100644 --- a/drivers/md/dm-zoned-target.c +++ b/drivers/md/dm-zoned-target.c @@ -753,12 +753,10 @@ static void dmz_put_zoned_device(struct dm_target *ti) struct dmz_target *dmz = ti->private; int i;
- for (i = 0; i < dmz->nr_ddevs; i++) {
if (dmz->ddev[i]) {
- for (i = 0; i < dmz->nr_ddevs; i++)
if (dmz->ddev[i]) dm_put_device(ti, dmz->ddev[i]);
dmz->ddev[i] = NULL;}- }
- kfree(dmz->ddev); }
static int dmz_fixup_devices(struct dm_target *ti)
Hmm. I'm not that happy with it; dmz_put_zoned_device() is using dm_target as an argument, whereas all of the functions surrounding the call sites is using the dmz_target directly.
Mind to modify the function to use 'struct dmz_target' as an argument?
Cheers,
Hannes
On 23/09/20 04:06PM, Hannes Reinecke wrote:
On 9/20/23 12:51, Fedor Pchelkin wrote:
Commit 4dba12881f88 ("dm zoned: support arbitrary number of devices") made the pointers to additional zoned devices to be stored in a dynamically allocated dmz->ddev array. However, this array is not freed.
Free it when cleaning up zoned device information inside dmz_put_zoned_device(). Assigning NULL to dmz->ddev elements doesn't make sense there as they are not supposed to be reused later and the whole dmz target structure is being cleaned anyway.
Found by Linux Verification Center (linuxtesting.org).
Fixes: 4dba12881f88 ("dm zoned: support arbitrary number of devices") Cc: stable@vger.kernel.org Signed-off-by: Fedor Pchelkin pchelkin@ispras.ru
drivers/md/dm-zoned-target.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c index ad8e670a2f9b..e25cd9db6275 100644 --- a/drivers/md/dm-zoned-target.c +++ b/drivers/md/dm-zoned-target.c @@ -753,12 +753,10 @@ static void dmz_put_zoned_device(struct dm_target *ti) struct dmz_target *dmz = ti->private; int i;
- for (i = 0; i < dmz->nr_ddevs; i++) {
if (dmz->ddev[i]) {
- for (i = 0; i < dmz->nr_ddevs; i++)
if (dmz->ddev[i]) dm_put_device(ti, dmz->ddev[i]);
dmz->ddev[i] = NULL;}- }
- kfree(dmz->ddev); } static int dmz_fixup_devices(struct dm_target *ti)
Hmm. I'm not that happy with it; dmz_put_zoned_device() is using dm_target as an argument, whereas all of the functions surrounding the call sites is using the dmz_target directly.
Mind to modify the function to use 'struct dmz_target' as an argument?
dm_target is required inside dmz_put_zoned_device() for dm_put_device() calls. I can't see a way for referencing it via dmz_target. Do you mean passing additional second argument like dmz_put_zoned_device(struct dmz_target *dmz, struct dm_target *ti) ?
BTW, I also think it can be renamed to dmz_put_zoned_devices().
Cheers,
Hannes
Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman
On Wed, Sep 20 2023 at 10:35P -0400, Fedor Pchelkin pchelkin@ispras.ru wrote:
On 23/09/20 04:06PM, Hannes Reinecke wrote:
On 9/20/23 12:51, Fedor Pchelkin wrote:
Commit 4dba12881f88 ("dm zoned: support arbitrary number of devices") made the pointers to additional zoned devices to be stored in a dynamically allocated dmz->ddev array. However, this array is not freed.
Free it when cleaning up zoned device information inside dmz_put_zoned_device(). Assigning NULL to dmz->ddev elements doesn't make sense there as they are not supposed to be reused later and the whole dmz target structure is being cleaned anyway.
Found by Linux Verification Center (linuxtesting.org).
Fixes: 4dba12881f88 ("dm zoned: support arbitrary number of devices") Cc: stable@vger.kernel.org Signed-off-by: Fedor Pchelkin pchelkin@ispras.ru
drivers/md/dm-zoned-target.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c index ad8e670a2f9b..e25cd9db6275 100644 --- a/drivers/md/dm-zoned-target.c +++ b/drivers/md/dm-zoned-target.c @@ -753,12 +753,10 @@ static void dmz_put_zoned_device(struct dm_target *ti) struct dmz_target *dmz = ti->private; int i;
- for (i = 0; i < dmz->nr_ddevs; i++) {
if (dmz->ddev[i]) {
- for (i = 0; i < dmz->nr_ddevs; i++)
if (dmz->ddev[i]) dm_put_device(ti, dmz->ddev[i]);
dmz->ddev[i] = NULL;}- }
- kfree(dmz->ddev); } static int dmz_fixup_devices(struct dm_target *ti)
Hmm. I'm not that happy with it; dmz_put_zoned_device() is using dm_target as an argument, whereas all of the functions surrounding the call sites is using the dmz_target directly.
Mind to modify the function to use 'struct dmz_target' as an argument?
dm_target is required inside dmz_put_zoned_device() for dm_put_device() calls. I can't see a way for referencing it via dmz_target. Do you mean passing additional second argument like dmz_put_zoned_device(struct dmz_target *dmz, struct dm_target *ti) ?
No, what you did is fine. Not sure what Hannes is saying given only passing dm_target has symmetry with dm_get_zoned_device (and dmz_fixup_devices).
BTW, I also think it can be renamed to dmz_put_zoned_devices().
I've renamed like you suggested and added a newline to dmz_put_zoned_devices() and staged this fix in linux-next for upstream inclusion before 6.6 final releases.
Mike
On 23/09/20 01:54PM, Mike Snitzer wrote:
On Wed, Sep 20 2023 at 10:35P -0400, Fedor Pchelkin pchelkin@ispras.ru wrote:
On 23/09/20 04:06PM, Hannes Reinecke wrote:
On 9/20/23 12:51, Fedor Pchelkin wrote:
Commit 4dba12881f88 ("dm zoned: support arbitrary number of devices") made the pointers to additional zoned devices to be stored in a dynamically allocated dmz->ddev array. However, this array is not freed.
Free it when cleaning up zoned device information inside dmz_put_zoned_device(). Assigning NULL to dmz->ddev elements doesn't make sense there as they are not supposed to be reused later and the whole dmz target structure is being cleaned anyway.
Found by Linux Verification Center (linuxtesting.org).
Fixes: 4dba12881f88 ("dm zoned: support arbitrary number of devices") Cc: stable@vger.kernel.org Signed-off-by: Fedor Pchelkin pchelkin@ispras.ru
drivers/md/dm-zoned-target.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c index ad8e670a2f9b..e25cd9db6275 100644 --- a/drivers/md/dm-zoned-target.c +++ b/drivers/md/dm-zoned-target.c @@ -753,12 +753,10 @@ static void dmz_put_zoned_device(struct dm_target *ti) struct dmz_target *dmz = ti->private; int i;
- for (i = 0; i < dmz->nr_ddevs; i++) {
if (dmz->ddev[i]) {
- for (i = 0; i < dmz->nr_ddevs; i++)
if (dmz->ddev[i]) dm_put_device(ti, dmz->ddev[i]);
dmz->ddev[i] = NULL;}- }
- kfree(dmz->ddev); } static int dmz_fixup_devices(struct dm_target *ti)
Hmm. I'm not that happy with it; dmz_put_zoned_device() is using dm_target as an argument, whereas all of the functions surrounding the call sites is using the dmz_target directly.
Mind to modify the function to use 'struct dmz_target' as an argument?
dm_target is required inside dmz_put_zoned_device() for dm_put_device() calls. I can't see a way for referencing it via dmz_target. Do you mean passing additional second argument like dmz_put_zoned_device(struct dmz_target *dmz, struct dm_target *ti) ?
No, what you did is fine. Not sure what Hannes is saying given only passing dm_target has symmetry with dm_get_zoned_device (and dmz_fixup_devices).
BTW, I also think it can be renamed to dmz_put_zoned_devices().
I've renamed like you suggested and added a newline to dmz_put_zoned_devices() and staged this fix in linux-next for upstream inclusion before 6.6 final releases.
Okay, thanks.
Mike
linux-stable-mirror@lists.linaro.org
-
Fedor Pchelkin -
Hannes Reinecke -
Mike Snitzer