| Message ID | 20210503110555.24001-3-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Fix Transaction leaks | expand |
03.05.2021 14:05, Kevin Wolf wrote: > Like other error paths, this one needs to call tran_finalize() and clean > up the BlockReopenQueue, too. We don't need the "abort" loop on that path. And clean-up of BlockReopenQueue is at "cleanup:" label. So I'd prefer Peter's suggestion (my "[PATCH 2/6] block: bdrv_reopen_multiple(): fix leak of tran object") > > Fixes: CID 1452772 > Fixes: 72373e40fbc7e4218061a8211384db362d3e7348 > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index 5c0ced6238..69615fabd1 100644 > --- a/block.c > +++ b/block.c > @@ -4052,7 +4052,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp) > ret = bdrv_flush(bs_entry->state.bs); > if (ret < 0) { > error_setg_errno(errp, -ret, "Error flushing drive"); > - goto cleanup; > + goto abort; > } > } > >
Am 03.05.2021 um 13:40 hat Vladimir Sementsov-Ogievskiy geschrieben: > 03.05.2021 14:05, Kevin Wolf wrote: > > Like other error paths, this one needs to call tran_finalize() and clean > > up the BlockReopenQueue, too. > > We don't need the "abort" loop on that path. And clean-up of > BlockReopenQueue is at "cleanup:" label. The cleanup of the BlockReopenQueue itself is there, but not of all fields in it. Specifically, these lines are needed: qobject_unref(bs_entry->state.explicit_options); qobject_unref(bs_entry->state.options); The references are taken in bdrv_reopen_queue_child() and either used in commit or released on abort. Doing nothing with them leaks them. > So I'd prefer Peter's suggestion (my "[PATCH 2/6] block: > bdrv_reopen_multiple(): fix leak of tran object") I don't like it because I think every call that doesn't end in a commit, should jump to the abort label to make sure that everything that remains unused because of this is correctly cleaned up. Kevin
03.05.2021 15:41, Kevin Wolf wrote: > Am 03.05.2021 um 13:40 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 03.05.2021 14:05, Kevin Wolf wrote: >>> Like other error paths, this one needs to call tran_finalize() and clean >>> up the BlockReopenQueue, too. >> >> We don't need the "abort" loop on that path. And clean-up of >> BlockReopenQueue is at "cleanup:" label. > > The cleanup of the BlockReopenQueue itself is there, but not of all > fields in it. Specifically, these lines are needed: > > qobject_unref(bs_entry->state.explicit_options); > qobject_unref(bs_entry->state.options); > > The references are taken in bdrv_reopen_queue_child() and either used in > commit or released on abort. Doing nothing with them leaks them. Oops. Somehow I decided they are set in _prepare. > >> So I'd prefer Peter's suggestion (my "[PATCH 2/6] block: >> bdrv_reopen_multiple(): fix leak of tran object") > > I don't like it because I think every call that doesn't end in a commit, > should jump to the abort label to make sure that everything that remains > unused because of this is correctly cleaned up. > agree now.. Still, don't we miss these two qobject_unref() calls on success path?
Am 03.05.2021 um 15:09 hat Vladimir Sementsov-Ogievskiy geschrieben: > 03.05.2021 15:41, Kevin Wolf wrote: > > Am 03.05.2021 um 13:40 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > 03.05.2021 14:05, Kevin Wolf wrote: > > > > Like other error paths, this one needs to call tran_finalize() and clean > > > > up the BlockReopenQueue, too. > > > > > > We don't need the "abort" loop on that path. And clean-up of > > > BlockReopenQueue is at "cleanup:" label. > > > > The cleanup of the BlockReopenQueue itself is there, but not of all > > fields in it. Specifically, these lines are needed: > > > > qobject_unref(bs_entry->state.explicit_options); > > qobject_unref(bs_entry->state.options); > > > > The references are taken in bdrv_reopen_queue_child() and either used in > > commit or released on abort. Doing nothing with them leaks them. > > Oops. Somehow I decided they are set in _prepare. > > > > > > So I'd prefer Peter's suggestion (my "[PATCH 2/6] block: > > > bdrv_reopen_multiple(): fix leak of tran object") > > > > I don't like it because I think every call that doesn't end in a commit, > > should jump to the abort label to make sure that everything that remains > > unused because of this is correctly cleaned up. > > > > > agree now.. > > Still, don't we miss these two qobject_unref() calls on success path? No, they are put to use in bdrv_reopen_commit(): qobject_unref(bs->explicit_options); qobject_unref(bs->options); bs->explicit_options = reopen_state->explicit_options; bs->options = reopen_state->options; Kevin
03.05.2021 14:05, Kevin Wolf wrote: > Like other error paths, this one needs to call tran_finalize() and clean > up the BlockReopenQueue, too. > > Fixes: CID 1452772 > Fixes: 72373e40fbc7e4218061a8211384db362d3e7348 > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index 5c0ced6238..69615fabd1 100644 > --- a/block.c > +++ b/block.c > @@ -4052,7 +4052,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp) > ret = bdrv_flush(bs_entry->state.bs); > if (ret < 0) { > error_setg_errno(errp, -ret, "Error flushing drive"); > - goto cleanup; > + goto abort; > } > } > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
03.05.2021 17:33, Kevin Wolf wrote: > Am 03.05.2021 um 15:09 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 03.05.2021 15:41, Kevin Wolf wrote: >>> Am 03.05.2021 um 13:40 hat Vladimir Sementsov-Ogievskiy geschrieben: >>>> 03.05.2021 14:05, Kevin Wolf wrote: >>>>> Like other error paths, this one needs to call tran_finalize() and clean >>>>> up the BlockReopenQueue, too. >>>> >>>> We don't need the "abort" loop on that path. And clean-up of >>>> BlockReopenQueue is at "cleanup:" label. >>> >>> The cleanup of the BlockReopenQueue itself is there, but not of all >>> fields in it. Specifically, these lines are needed: >>> >>> qobject_unref(bs_entry->state.explicit_options); >>> qobject_unref(bs_entry->state.options); >>> >>> The references are taken in bdrv_reopen_queue_child() and either used in >>> commit or released on abort. Doing nothing with them leaks them. >> >> Oops. Somehow I decided they are set in _prepare. >> >>> >>>> So I'd prefer Peter's suggestion (my "[PATCH 2/6] block: >>>> bdrv_reopen_multiple(): fix leak of tran object") >>> >>> I don't like it because I think every call that doesn't end in a commit, >>> should jump to the abort label to make sure that everything that remains >>> unused because of this is correctly cleaned up. >>> >> >> >> agree now.. >> >> Still, don't we miss these two qobject_unref() calls on success path? > > No, they are put to use in bdrv_reopen_commit(): > > qobject_unref(bs->explicit_options); > qobject_unref(bs->options); > > bs->explicit_options = reopen_state->explicit_options; > bs->options = reopen_state->options; > > Kevin > OK then. I should have checked myself :\
diff --git a/block.c b/block.c index 5c0ced6238..69615fabd1 100644 --- a/block.c +++ b/block.c @@ -4052,7 +4052,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp) ret = bdrv_flush(bs_entry->state.bs); if (ret < 0) { error_setg_errno(errp, -ret, "Error flushing drive"); - goto cleanup; + goto abort; } }
Like other error paths, this one needs to call tran_finalize() and clean up the BlockReopenQueue, too. Fixes: CID 1452772 Fixes: 72373e40fbc7e4218061a8211384db362d3e7348 Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)