[PATCH 0/4 v2] Reconnect controllers on failues
Hi, This is a resend of [0] trying to reconnect drivers gracefully in case of a protocol removal failure.
Changes since v1: - Fix an existing memory leak when uninstalling a protocol. The opened protocol information was never released - Drop a patch that was incorrectly trying to free protocols on the handle that were not associated with the driver being released - Rewrite the sefltests and instead of installing a second driver handle on the controller handle, force the controller to return EFI_DEVICE_ERROR on the first removal. When that happens all the children, that are disconnected before the controller, must be reconnected - Reconnect all controllers on the handle if after disconnecting drivers and closing protocols opened by GET_PROTOCOL, TEST_PROTOCOL, HANDLE_PROTOCOL opened protocols are still present in the handle
[0] https://lore.kernel.org/u-boot/20230615143941.416924-1-ilias.apalodimas@lina...
Regards /Ilias
Ilias Apalodimas (4): efi_loader: reconnect drivers on failure efi_loader: check the status of disconnected drivers efi_loader: fix the return codes of UninstallProtocol efi_selftests: add extra testcases on controller handling
lib/efi_loader/efi_boottime.c | 43 ++++++++++++++++---- lib/efi_selftest/efi_selftest_controllers.c | 44 +++++++++++++++++++-- 2 files changed, 77 insertions(+), 10 deletions(-)
-- 2.40.1
efi_disconnect_controller() doesn't reconnect drivers in case of failure. Reconnect the disconnected drivers properly
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- lib/efi_loader/efi_boottime.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 5006c0e1e4af..d44562d8f4e0 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -97,6 +97,12 @@ static efi_status_t EFIAPI efi_disconnect_controller( efi_handle_t driver_image_handle, efi_handle_t child_handle);
+static +efi_status_t EFIAPI efi_connect_controller(efi_handle_t controller_handle, + efi_handle_t *driver_image_handle, + struct efi_device_path *remain_device_path, + bool recursive); + /* Called on every callback entry */ int __efi_entry_check(void) { @@ -1298,7 +1304,7 @@ static efi_status_t efi_disconnect_all_drivers const efi_guid_t *protocol, efi_handle_t child_handle) { - efi_uintn_t number_of_drivers; + efi_uintn_t number_of_drivers, tmp; efi_handle_t *driver_handle_buffer; efi_status_t r, ret;
@@ -1308,15 +1314,30 @@ static efi_status_t efi_disconnect_all_drivers return ret; if (!number_of_drivers) return EFI_SUCCESS; - ret = EFI_NOT_FOUND; + + tmp = number_of_drivers; while (number_of_drivers) { - r = EFI_CALL(efi_disconnect_controller( + ret = EFI_CALL(efi_disconnect_controller( handle, driver_handle_buffer[--number_of_drivers], child_handle)); - if (r == EFI_SUCCESS) - ret = r; + if (ret != EFI_SUCCESS) + goto reconnect; + } + + free(driver_handle_buffer); + return ret; + +reconnect: + /* Reconnect all disconnected drivers */ + for (; number_of_drivers < tmp; number_of_drivers++) { + r = EFI_CALL(efi_connect_controller(handle, + &driver_handle_buffer[number_of_drivers], + NULL, true)); + if (r != EFI_SUCCESS) + EFI_PRINT("Failed to reconnect controller\n"); } + free(driver_handle_buffer); return ret; } -- 2.40.1
On 6/20/23 08:19, Ilias Apalodimas wrote:
efi_disconnect_controller() doesn't reconnect drivers in case of failure. Reconnect the disconnected drivers properly
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
efi_uninstall_protocol() calls efi_disconnect_all_drivers() but never checks the return value. Instead it tries to identify protocols that are still open after closing the ones that were opened with EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL, EFI_OPEN_PROTOCOL_GET_PROTOCOL and EFI_OPEN_PROTOCOL_TEST_PROTOCOL.
Instead of doing that, check the return value early and exit if disconnecting the drivers failed. Also reconnect all the drivers of a handle if protocols are still found on the handle after disconnecting controllers and closing the remaining protocols.
While at it fix a memory leak and properly free the opened protocol information when closing a protocol.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- lib/efi_loader/efi_boottime.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index d44562d8f4e0..bfcd913dfad9 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1374,17 +1374,23 @@ static efi_status_t efi_uninstall_protocol if (r != EFI_SUCCESS) goto out; /* Disconnect controllers */ - efi_disconnect_all_drivers(efiobj, protocol, NULL); + r = efi_disconnect_all_drivers(efiobj, protocol, NULL); + if (r != EFI_SUCCESS) { + r = EFI_ACCESS_DENIED; + goto out; + } /* Close protocol */ list_for_each_entry_safe(item, pos, &handler->open_infos, link) { if (item->info.attributes == EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL || item->info.attributes == EFI_OPEN_PROTOCOL_GET_PROTOCOL || item->info.attributes == EFI_OPEN_PROTOCOL_TEST_PROTOCOL) - list_del(&item->link); + efi_delete_open_info(item); } + /* if agents didn't close the protocols properly */ if (!list_empty(&handler->open_infos)) { r = EFI_ACCESS_DENIED; + EFI_CALL(efi_connect_controller(handle, NULL, NULL, true)); goto out; } r = efi_remove_protocol(handle, protocol, protocol_interface); -- 2.40.1
Am 20. Juni 2023 08:19:29 MESZ schrieb Ilias Apalodimas ilias.apalodimas@linaro.org:
efi_uninstall_protocol() calls efi_disconnect_all_drivers() but never checks the return value. Instead it tries to identify protocols that are still open after closing the ones that were opened with EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL, EFI_OPEN_PROTOCOL_GET_PROTOCOL and EFI_OPEN_PROTOCOL_TEST_PROTOCOL.
Instead of doing that, check the return value early and exit if disconnecting the drivers failed. Also reconnect all the drivers of a handle if protocols are still found on the handle after disconnecting controllers and closing the remaining protocols.
We talk about a single protocol but possibly multiple open prototocol information records.
Othrwise looks good.
Best regards
Heinrich
While at it fix a memory leak and properly free the opened protocol information when closing a protocol.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
lib/efi_loader/efi_boottime.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index d44562d8f4e0..bfcd913dfad9 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1374,17 +1374,23 @@ static efi_status_t efi_uninstall_protocol if (r != EFI_SUCCESS) goto out; /* Disconnect controllers */
- efi_disconnect_all_drivers(efiobj, protocol, NULL);
- r = efi_disconnect_all_drivers(efiobj, protocol, NULL);
- if (r != EFI_SUCCESS) {
r = EFI_ACCESS_DENIED;goto out;- } /* Close protocol */ list_for_each_entry_safe(item, pos, &handler->open_infos, link) { if (item->info.attributes == EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL || item->info.attributes == EFI_OPEN_PROTOCOL_GET_PROTOCOL || item->info.attributes == EFI_OPEN_PROTOCOL_TEST_PROTOCOL)
list_del(&item->link);
efi_delete_open_info(item);- /* if agents didn't close the protocols properly */ if (!list_empty(&handler->open_infos)) { r = EFI_ACCESS_DENIED;
EFI_CALL(efi_connect_controller(handle, NULL, NULL, true));-- 2.40.1
On 6/20/23 08:19, Ilias Apalodimas wrote:
efi_uninstall_protocol() calls efi_disconnect_all_drivers() but never checks the return value. Instead it tries to identify protocols that are still open after closing the ones that were opened with EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL, EFI_OPEN_PROTOCOL_GET_PROTOCOL and EFI_OPEN_PROTOCOL_TEST_PROTOCOL.
Instead of doing that, check the return value early and exit if disconnecting the drivers failed. Also reconnect all the drivers of a handle if protocols are still found on the handle after disconnecting controllers and closing the remaining protocols.
While at it fix a memory leak and properly free the opened protocol information when closing a protocol.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Up to now we did not check the return value of DisconnectController. A previous patch is fixing that taking into account what happened during the controller disconnect. But that check takes place before our code is trying to figure out if the interface exists to begin with. In case a driver is not allowed to unbind -- e.g returning EFI_DEVICE_ERROR, we will end up returning that error instead of EFI_NOT_FOUND.
Add an extra check on the top of the function to make sure the protocol interface exists before trying to disconnect any drivers
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- lib/efi_loader/efi_boottime.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index bfcd913dfad9..d75a3336e3f1 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1373,6 +1373,8 @@ static efi_status_t efi_uninstall_protocol r = efi_search_protocol(handle, protocol, &handler); if (r != EFI_SUCCESS) goto out; + if (handler->protocol_interface != protocol_interface) + return EFI_NOT_FOUND; /* Disconnect controllers */ r = efi_disconnect_all_drivers(efiobj, protocol, NULL); if (r != EFI_SUCCESS) { -- 2.40.1
We recently fixed a few issues wrt to controller handling. Add a few test cases to cover the new code. - return EFI_DEVICE_ERROR the first time the protocol interface of the controller is uninstalled, after all the children have been disconnected. This should make the drivers reconnect - add tests to verify controllers are reconnected when uninstalling a protocol fails - add tests to make sure EFI_NOT_FOUND is returned if a non existent interface is being removed
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- lib/efi_selftest/efi_selftest_controllers.c | 44 +++++++++++++++++++-- 1 file changed, 41 insertions(+), 3 deletions(-)
diff --git a/lib/efi_selftest/efi_selftest_controllers.c b/lib/efi_selftest/efi_selftest_controllers.c index 79bc86fb0c3a..513c9868fda9 100644 --- a/lib/efi_selftest/efi_selftest_controllers.c +++ b/lib/efi_selftest/efi_selftest_controllers.c @@ -28,6 +28,7 @@ static efi_guid_t guid_child_controller = static efi_handle_t handle_controller; static efi_handle_t handle_child_controller[NUMBER_OF_CHILD_CONTROLLERS]; static efi_handle_t handle_driver; +static bool allow_removal;
/* * Count child controllers @@ -85,8 +86,8 @@ static efi_status_t EFIAPI supported( controller_handle, EFI_OPEN_PROTOCOL_BY_DRIVER); switch (ret) { case EFI_ACCESS_DENIED: - case EFI_ALREADY_STARTED: return ret; + case EFI_ALREADY_STARTED: case EFI_SUCCESS: break; default: @@ -124,8 +125,8 @@ static efi_status_t EFIAPI start( controller_handle, EFI_OPEN_PROTOCOL_BY_DRIVER); switch (ret) { case EFI_ACCESS_DENIED: - case EFI_ALREADY_STARTED: return ret; + case EFI_ALREADY_STARTED: case EFI_SUCCESS: break; default: @@ -238,6 +239,9 @@ static efi_status_t EFIAPI stop( if (ret != EFI_SUCCESS) efi_st_error("Cannot free buffer\n");
+ if (!allow_removal) + return EFI_DEVICE_ERROR; + /* Detach driver from controller */ ret = boottime->close_protocol( controller_handle, &guid_controller, @@ -342,6 +346,7 @@ static int execute(void) return EFI_ST_FAILURE; } /* Destroy remaining child controllers and disconnect controller */ + allow_removal = true; ret = boottime->disconnect_controller(handle_controller, NULL, NULL); if (ret != EFI_SUCCESS) { efi_st_error("Failed to disconnect controller\n"); @@ -393,7 +398,40 @@ static int execute(void) efi_st_error("Number of children %u != %u\n", (unsigned int)count, NUMBER_OF_CHILD_CONTROLLERS); } - /* Uninstall controller protocol */ + + allow_removal = false; + /* Try to uninstall controller protocol using the wrong interface */ + ret = boottime->uninstall_protocol_interface(handle_controller, + &guid_controller, + &interface1); + if (ret != EFI_NOT_FOUND) { + efi_st_error("Interface not checked when uninstalling protocol\n"); + return EFI_ST_FAILURE; + } + + /* + * Uninstall a protocol while Disconnect controller won't + * allow it. + */ + ret = boottime->uninstall_protocol_interface(handle_controller, + &guid_controller, + &interface2); + if (ret != EFI_ACCESS_DENIED) { + efi_st_error("Uninstall protocol interface failed\n"); + return EFI_ST_FAILURE; + } + /* + * Check number of child controllers and make sure children have + * been reconnected + */ + ret = count_child_controllers(handle_controller, &guid_controller, + &count); + if (ret != EFI_SUCCESS || count != NUMBER_OF_CHILD_CONTROLLERS) { + efi_st_error("Number of children %u != %u\n", + (unsigned int)count, NUMBER_OF_CHILD_CONTROLLERS); + } + + allow_removal = true; ret = boottime->uninstall_protocol_interface(handle_controller, &guid_controller, &interface2); -- 2.40.1
participants (2)
-
Heinrich Schuchardt -
Ilias Apalodimas