| Message ID | 20231225044356.626900-4-masahisa.kojima@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix and refactoring of efi_disk.c | expand |
On 12/25/23 05:43, Masahisa Kojima wrote: > Current error handling of creating raw disk/partition has > following issues. > - duplicate free for efi handle, efi handle is already freed > in efi_delete_handle() I cannot see where this patch reduces the number of efi_delete_handle() invocations. > - missing free for struct efi_device_path and > struct efi_simple_file_system_protocol in some error paths > > To address those issue, this commit creates the common function > to free the struct efi_disk_obj resources and calls it in case > of error. > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > --- > lib/efi_loader/efi_disk.c | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > index 415d8601ba..d2ac2fab9b 100644 > --- a/lib/efi_loader/efi_disk.c > +++ b/lib/efi_loader/efi_disk.c > @@ -372,6 +372,19 @@ static int efi_fs_exists(struct blk_desc *desc, int part) > return 1; > } > > +static void efi_disk_free_diskobj(struct efi_disk_obj *diskobj) > +{ > + struct efi_device_path *dp = NULL; This NULL value is never used. > + struct efi_simple_file_system_protocol *volume = NULL; ditto > + > + dp = diskobj->dp; > + volume = diskobj->volume; > + > + efi_delete_handle(&diskobj->header); efi_delete_handle() may fail. > + efi_free_pool(dp); The device path may only be freed if it has been uninstalled from the handle. > + free(volume); The simple file protocol interface may only be freed if it has been uninstalled from the handle. > +} > + > /** > * efi_disk_add_dev() - create a handle for a partition or disk > * > @@ -529,9 +542,7 @@ static efi_status_t efi_disk_add_dev( > } > return EFI_SUCCESS; > error: > - efi_delete_handle(&diskobj->header); > - free(diskobj->volume); > - free(diskobj); > + efi_disk_free_diskobj(diskobj); > return ret; > } > > @@ -570,8 +581,7 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) > return ret; > } > if (efi_link_dev(&disk->header, dev)) { > - efi_free_pool(disk->dp); > - efi_delete_handle(&disk->header); > + efi_disk_free_diskobj(disk); > > return -EINVAL; > } > @@ -625,8 +635,7 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) > return -1; > } > if (efi_link_dev(&disk->header, dev)) { > - efi_free_pool(disk->dp); > - efi_delete_handle(&disk->header); > + efi_disk_free_diskobj(disk); In efi_disk_add_dev we have opened a protocol interface. We must close it before removing the partition handle otherwise the disk handle can never be removed. To do this all properly we will need to re-implement this code using proper EFI drivers which expose a driver binding protocol. Please, add a TODO: comment here that closing the protocol is missing. Best regards Heinrich > > return -1; > }
On Mon, 25 Dec 2023 at 19:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 12/25/23 05:43, Masahisa Kojima wrote: > > Current error handling of creating raw disk/partition has > > following issues. > > - duplicate free for efi handle, efi handle is already freed > > in efi_delete_handle() > > I cannot see where this patch reduces the number of efi_delete_handle() > invocations. Duplicate free() calls occur here in the original code: > > - efi_delete_handle(&diskobj->header); > > - free(diskobj->volume); > > - free(diskobj); efi_delete_handle(&diskobj->header) will free the handle(= &diskobj->header, = diskobj). So calling free(diskobj) should be avoided. > > > - missing free for struct efi_device_path and > > struct efi_simple_file_system_protocol in some error paths > > > > To address those issue, this commit creates the common function > > to free the struct efi_disk_obj resources and calls it in case > > of error. > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > --- > > lib/efi_loader/efi_disk.c | 23 ++++++++++++++++------- > > 1 file changed, 16 insertions(+), 7 deletions(-) > > > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c > > index 415d8601ba..d2ac2fab9b 100644 > > --- a/lib/efi_loader/efi_disk.c > > +++ b/lib/efi_loader/efi_disk.c > > @@ -372,6 +372,19 @@ static int efi_fs_exists(struct blk_desc *desc, int part) > > return 1; > > } > > > > +static void efi_disk_free_diskobj(struct efi_disk_obj *diskobj) > > +{ > > + struct efi_device_path *dp = NULL; > > This NULL value is never used. Yes, initialize it with diskobj->dp. > > > + struct efi_simple_file_system_protocol *volume = NULL; > > ditto OK. > > > + > > + dp = diskobj->dp; > > + volume = diskobj->volume; > > + > > + efi_delete_handle(&diskobj->header); > > efi_delete_handle() may fail. I assume this function efi_disk_free_diskobj() is called only from the error path.(I should note as a comment) I think it is better to ignore errors in efi_delete_handle() and proceed to free the other resources. > > > + efi_free_pool(dp); > > The device path may only be freed if it has been uninstalled from the > handle. Sorry, I'm not sure if I understand this comment correctly, but device_path is uninstalled in efi_delete_handle(). > > > + free(volume); > > The simple file protocol interface may only be freed if it has been > uninstalled from the handle. Same as above, all protocols on the handle are uninstalled in efi_delete_handle(). > > > +} > > + > > /** > > * efi_disk_add_dev() - create a handle for a partition or disk > > * > > @@ -529,9 +542,7 @@ static efi_status_t efi_disk_add_dev( > > } > > return EFI_SUCCESS; > > error: > > - efi_delete_handle(&diskobj->header); > > - free(diskobj->volume); > > - free(diskobj); > > + efi_disk_free_diskobj(diskobj); > > return ret; > > } > > > > @@ -570,8 +581,7 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) > > return ret; > > } > > if (efi_link_dev(&disk->header, dev)) { > > - efi_free_pool(disk->dp); > > - efi_delete_handle(&disk->header); > > + efi_disk_free_diskobj(disk); > > > > return -EINVAL; > > } > > @@ -625,8 +635,7 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) > > return -1; > > } > > if (efi_link_dev(&disk->header, dev)) { > > - efi_free_pool(disk->dp); > > - efi_delete_handle(&disk->header); > > + efi_disk_free_diskobj(disk); > > In efi_disk_add_dev we have opened a protocol interface. We must close > it before removing the partition handle otherwise the disk handle can > never be removed. > > To do this all properly we will need to re-implement this code using > proper EFI drivers which expose a driver binding protocol. Please, add a > TODO: comment here that closing the protocol is missing. OK, I will check on this. Thanks, Masahisa Kojima > > Best regards > > Heinrich > > > > > return -1; > > } >
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 415d8601ba..d2ac2fab9b 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -372,6 +372,19 @@ static int efi_fs_exists(struct blk_desc *desc, int part) return 1; } +static void efi_disk_free_diskobj(struct efi_disk_obj *diskobj) +{ + struct efi_device_path *dp = NULL; + struct efi_simple_file_system_protocol *volume = NULL; + + dp = diskobj->dp; + volume = diskobj->volume; + + efi_delete_handle(&diskobj->header); + efi_free_pool(dp); + free(volume); +} + /** * efi_disk_add_dev() - create a handle for a partition or disk * @@ -529,9 +542,7 @@ static efi_status_t efi_disk_add_dev( } return EFI_SUCCESS; error: - efi_delete_handle(&diskobj->header); - free(diskobj->volume); - free(diskobj); + efi_disk_free_diskobj(diskobj); return ret; } @@ -570,8 +581,7 @@ static int efi_disk_create_raw(struct udevice *dev, efi_handle_t agent_handle) return ret; } if (efi_link_dev(&disk->header, dev)) { - efi_free_pool(disk->dp); - efi_delete_handle(&disk->header); + efi_disk_free_diskobj(disk); return -EINVAL; } @@ -625,8 +635,7 @@ static int efi_disk_create_part(struct udevice *dev, efi_handle_t agent_handle) return -1; } if (efi_link_dev(&disk->header, dev)) { - efi_free_pool(disk->dp); - efi_delete_handle(&disk->header); + efi_disk_free_diskobj(disk); return -1; }
Current error handling of creating raw disk/partition has following issues. - duplicate free for efi handle, efi handle is already freed in efi_delete_handle() - missing free for struct efi_device_path and struct efi_simple_file_system_protocol in some error paths To address those issue, this commit creates the common function to free the struct efi_disk_obj resources and calls it in case of error. Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> --- lib/efi_loader/efi_disk.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)