| Message ID | 20231225044356.626900-3-masahisa.kojima@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | fix and refactoring of efi_disk.c | expand |
On 12/25/23 05:43, Masahisa Kojima wrote: > efi_delete_handle() calls efi_purge_handle(), then it finally > frees the efi handle. > Both diskobj and handle variables in efi_disk_remove() have > the same pointer, we can not access diskobj->dp after calling > efi_delete_handle(). > > This commit saves the struct efi_device_path pointer before > calling efi_delete_handle(). This commit also fixes the > missing free for volume member in struct efi_disk_obj. > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > --- > lib/efi_loader/efi_disk.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > index a2f8b531a3..415d8601ba 100644 > --- a/lib/efi_loader/efi_disk.c > +++ b/lib/efi_loader/efi_disk.c > @@ -701,7 +701,9 @@ int efi_disk_remove(void *ctx, struct event *event) > struct udevice *dev = event->data.dm.dev; > efi_handle_t handle; > struct blk_desc *desc; > + struct efi_device_path *dp = NULL; > struct efi_disk_obj *diskobj = NULL; > + struct efi_simple_file_system_protocol *volume = NULL; > efi_status_t ret; > > if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) > @@ -722,14 +724,18 @@ int efi_disk_remove(void *ctx, struct event *event) > return 0; > } > > + if (diskobj) { diskobj = container_of(handle, struct efi_disk_obj, header); can be replaced by diskobj = handle We should check the handle immediately after dev_tag_get_ptr() and return 0 if the handle == NULL. > + dp = diskobj->dp; > + volume = diskobj->volume; > + } > + > ret = efi_delete_handle(handle); We must not delete the handle in case of UCLASS_EFI_LOADER. Instead of calling efi_delete_handle we should uninstall all protocols that we have installed. If no protocol is left the handle will go away. To make the protocols to delete tractable they should be opened with BY_DRIVER. When a partition is removed we must close the protocol interfaces opened with EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER (cf. efi_disk_add_dev()). Best regards Heinrich > /* Do not delete DM device if there are still EFI drivers attached. */ > if (ret != EFI_SUCCESS) > return -1; > > - if (diskobj) > - efi_free_pool(diskobj->dp); > - > + efi_free_pool(dp); > + free(volume); > dev_tag_del(dev, DM_TAG_EFI); > > return 0;
Hi Heinrich, On Mon, 25 Dec 2023 at 19:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 12/25/23 05:43, Masahisa Kojima wrote: > > efi_delete_handle() calls efi_purge_handle(), then it finally > > frees the efi handle. > > Both diskobj and handle variables in efi_disk_remove() have > > the same pointer, we can not access diskobj->dp after calling > > efi_delete_handle(). > > > > This commit saves the struct efi_device_path pointer before > > calling efi_delete_handle(). This commit also fixes the > > missing free for volume member in struct efi_disk_obj. > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > --- > > lib/efi_loader/efi_disk.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > index a2f8b531a3..415d8601ba 100644 > > --- a/lib/efi_loader/efi_disk.c > > +++ b/lib/efi_loader/efi_disk.c > > @@ -701,7 +701,9 @@ int efi_disk_remove(void *ctx, struct event *event) > > struct udevice *dev = event->data.dm.dev; > > efi_handle_t handle; > > struct blk_desc *desc; > > + struct efi_device_path *dp = NULL; > > struct efi_disk_obj *diskobj = NULL; > > + struct efi_simple_file_system_protocol *volume = NULL; > > efi_status_t ret; > > > > if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) > > @@ -722,14 +724,18 @@ int efi_disk_remove(void *ctx, struct event *event) > > return 0; > > } > > > > + if (diskobj) { > > > diskobj = container_of(handle, struct efi_disk_obj, header); > > can be replaced by > > diskobj = handle > > We should check the handle immediately after dev_tag_get_ptr() and > return 0 if the handle == NULL. OK, I will try to improve the efi_disk_remove() function. > > > + dp = diskobj->dp; > > + volume = diskobj->volume; > > + } > > + > > ret = efi_delete_handle(handle); > > We must not delete the handle in case of UCLASS_EFI_LOADER. > > Instead of calling efi_delete_handle we should uninstall all protocols > that we have installed. If no protocol is left the handle will go away. > > To make the protocols to delete tractable they should be opened with > BY_DRIVER. efi_delete_handle() calls efi_remove_all_protocols(), and frees the efi handle if no protocol is left on the handle. So I think efi_delete_handle() does all the required processes mentioned above. > > When a partition is removed we must close the protocol interfaces opened > with EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER (cf. efi_disk_add_dev()). OK, I will fix this. Thanks, Masahisa Kojima > > Best regards > > Heinrich > > > /* Do not delete DM device if there are still EFI drivers attached. */ > > if (ret != EFI_SUCCESS) > > return -1; > > > > - if (diskobj) > > - efi_free_pool(diskobj->dp); > > - > > + efi_free_pool(dp); > > + free(volume); > > dev_tag_del(dev, DM_TAG_EFI); > > > > return 0; >
Am 26. Dezember 2023 02:13:08 MEZ schrieb Masahisa Kojima <masahisa.kojima@linaro.org>: >Hi Heinrich, > >On Mon, 25 Dec 2023 at 19:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> On 12/25/23 05:43, Masahisa Kojima wrote: >> > efi_delete_handle() calls efi_purge_handle(), then it finally >> > frees the efi handle. >> > Both diskobj and handle variables in efi_disk_remove() have >> > the same pointer, we can not access diskobj->dp after calling >> > efi_delete_handle(). >> > >> > This commit saves the struct efi_device_path pointer before >> > calling efi_delete_handle(). This commit also fixes the >> > missing free for volume member in struct efi_disk_obj. >> > >> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> >> > --- >> > lib/efi_loader/efi_disk.c | 12 +++++++++--- >> > 1 file changed, 9 insertions(+), 3 deletions(-) >> > >> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c >> > index a2f8b531a3..415d8601ba 100644 >> > --- a/lib/efi_loader/efi_disk.c >> > +++ b/lib/efi_loader/efi_disk.c >> > @@ -701,7 +701,9 @@ int efi_disk_remove(void *ctx, struct event *event) >> > struct udevice *dev = event->data.dm.dev; >> > efi_handle_t handle; >> > struct blk_desc *desc; >> > + struct efi_device_path *dp = NULL; >> > struct efi_disk_obj *diskobj = NULL; >> > + struct efi_simple_file_system_protocol *volume = NULL; >> > efi_status_t ret; >> > >> > if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) >> > @@ -722,14 +724,18 @@ int efi_disk_remove(void *ctx, struct event *event) >> > return 0; >> > } >> > >> > + if (diskobj) { >> >> >> diskobj = container_of(handle, struct efi_disk_obj, header); >> >> can be replaced by >> >> diskobj = handle >> >> We should check the handle immediately after dev_tag_get_ptr() and >> return 0 if the handle == NULL. > >OK, I will try to improve the efi_disk_remove() function. > >> >> > + dp = diskobj->dp; >> > + volume = diskobj->volume; >> > + } >> > + >> > ret = efi_delete_handle(handle); >> >> We must not delete the handle in case of UCLASS_EFI_LOADER. >> >> Instead of calling efi_delete_handle we should uninstall all protocols >> that we have installed. If no protocol is left the handle will go away. >> >> To make the protocols to delete tractable they should be opened with >> BY_DRIVER. > >efi_delete_handle() calls efi_remove_all_protocols(), and frees the efi handle >if no protocol is left on the handle. >So I think efi_delete_handle() does all the required processes mentioned above. Given an EFI application like iPXE that provides a handle with the block IO protocol implemented by the application: Why should U-Boot remove the block IO protocol or the device path protocol when the application calls DisconnectController()? Best regards Heinrich > >> >> When a partition is removed we must close the protocol interfaces opened >> with EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER (cf. efi_disk_add_dev()). > >OK, I will fix this. > >Thanks, >Masahisa Kojima > >> >> Best regards >> >> Heinrich >> >> > /* Do not delete DM device if there are still EFI drivers attached. */ >> > if (ret != EFI_SUCCESS) >> > return -1; >> > >> > - if (diskobj) >> > - efi_free_pool(diskobj->dp); >> > - >> > + efi_free_pool(dp); >> > + free(volume); >> > dev_tag_del(dev, DM_TAG_EFI); >> > >> > return 0; >>
Hi Heinrich, On Tue, 26 Dec 2023 at 10:43, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > Am 26. Dezember 2023 02:13:08 MEZ schrieb Masahisa Kojima <masahisa.kojima@linaro.org>: > >Hi Heinrich, > > > >On Mon, 25 Dec 2023 at 19:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> On 12/25/23 05:43, Masahisa Kojima wrote: > >> > efi_delete_handle() calls efi_purge_handle(), then it finally > >> > frees the efi handle. > >> > Both diskobj and handle variables in efi_disk_remove() have > >> > the same pointer, we can not access diskobj->dp after calling > >> > efi_delete_handle(). > >> > > >> > This commit saves the struct efi_device_path pointer before > >> > calling efi_delete_handle(). This commit also fixes the > >> > missing free for volume member in struct efi_disk_obj. > >> > > >> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > >> > --- > >> > lib/efi_loader/efi_disk.c | 12 +++++++++--- > >> > 1 file changed, 9 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > >> > index a2f8b531a3..415d8601ba 100644 > >> > --- a/lib/efi_loader/efi_disk.c > >> > +++ b/lib/efi_loader/efi_disk.c > >> > @@ -701,7 +701,9 @@ int efi_disk_remove(void *ctx, struct event *event) > >> > struct udevice *dev = event->data.dm.dev; > >> > efi_handle_t handle; > >> > struct blk_desc *desc; > >> > + struct efi_device_path *dp = NULL; > >> > struct efi_disk_obj *diskobj = NULL; > >> > + struct efi_simple_file_system_protocol *volume = NULL; > >> > efi_status_t ret; > >> > > >> > if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) > >> > @@ -722,14 +724,18 @@ int efi_disk_remove(void *ctx, struct event *event) > >> > return 0; > >> > } > >> > > >> > + if (diskobj) { > >> > >> > >> diskobj = container_of(handle, struct efi_disk_obj, header); > >> > >> can be replaced by > >> > >> diskobj = handle > >> > >> We should check the handle immediately after dev_tag_get_ptr() and > >> return 0 if the handle == NULL. > > > >OK, I will try to improve the efi_disk_remove() function. > > > >> > >> > + dp = diskobj->dp; > >> > + volume = diskobj->volume; > >> > + } > >> > + > >> > ret = efi_delete_handle(handle); > >> > >> We must not delete the handle in case of UCLASS_EFI_LOADER. > >> > >> Instead of calling efi_delete_handle we should uninstall all protocols > >> that we have installed. If no protocol is left the handle will go away. > >> > >> To make the protocols to delete tractable they should be opened with > >> BY_DRIVER. > > > >efi_delete_handle() calls efi_remove_all_protocols(), and frees the efi handle > >if no protocol is left on the handle. > >So I think efi_delete_handle() does all the required processes mentioned above. > > Given an EFI application like iPXE that provides a handle with the block IO protocol implemented by the application: Why should U-Boot remove the block IO protocol or the device path protocol when the application calls DisconnectController()? Yes, but the current implementation does the same. So in the case of UCLASS_EFI_LOADER, we need to immediately return without calling efi_delete_handle(), is my understanding correct? I checked lib/efi_selftest/efi_selftest_block_device.c implementation, the teadown() process uninstalls the all protocol_interface then the EFI handle goes away if no protocol is left. Thanks, Masahisa Kojima > > Best regards > > Heinrich > > > > >> > >> When a partition is removed we must close the protocol interfaces opened > >> with EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER (cf. efi_disk_add_dev()). > > > >OK, I will fix this. > > > >Thanks, > >Masahisa Kojima > > > >> > >> Best regards > >> > >> Heinrich > >> > >> > /* Do not delete DM device if there are still EFI drivers attached. */ > >> > if (ret != EFI_SUCCESS) > >> > return -1; > >> > > >> > - if (diskobj) > >> > - efi_free_pool(diskobj->dp); > >> > - > >> > + efi_free_pool(dp); > >> > + free(volume); > >> > dev_tag_del(dev, DM_TAG_EFI); > >> > > >> > return 0; > >>
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index a2f8b531a3..415d8601ba 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -701,7 +701,9 @@ int efi_disk_remove(void *ctx, struct event *event) struct udevice *dev = event->data.dm.dev; efi_handle_t handle; struct blk_desc *desc; + struct efi_device_path *dp = NULL; struct efi_disk_obj *diskobj = NULL; + struct efi_simple_file_system_protocol *volume = NULL; efi_status_t ret; if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&handle)) @@ -722,14 +724,18 @@ int efi_disk_remove(void *ctx, struct event *event) return 0; } + if (diskobj) { + dp = diskobj->dp; + volume = diskobj->volume; + } + ret = efi_delete_handle(handle); /* Do not delete DM device if there are still EFI drivers attached. */ if (ret != EFI_SUCCESS) return -1; - if (diskobj) - efi_free_pool(diskobj->dp); - + efi_free_pool(dp); + free(volume); dev_tag_del(dev, DM_TAG_EFI); return 0;
efi_delete_handle() calls efi_purge_handle(), then it finally frees the efi handle. Both diskobj and handle variables in efi_disk_remove() have the same pointer, we can not access diskobj->dp after calling efi_delete_handle(). This commit saves the struct efi_device_path pointer before calling efi_delete_handle(). This commit also fixes the missing free for volume member in struct efi_disk_obj. Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> --- lib/efi_loader/efi_disk.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)