| Message ID | 20191108003602.33526-1-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915/opregion: fix leaking fw on error path | expand |
On Thu, 07 Nov 2019, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > Convert the code to return-early style and fix missing calls > to release_firmware() if vbt is not valid. I don't understand where anything would leak in the current code. Please elaborate. You could make a case for a change in style to avoid so much indentation, but don't claim it fixes stuff if it doesn't. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > drivers/gpu/drm/i915/display/intel_opregion.c | 28 +++++++++++-------- > 1 file changed, 17 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c > index 969ade623691..9738511147b1 100644 > --- a/drivers/gpu/drm/i915/display/intel_opregion.c > +++ b/drivers/gpu/drm/i915/display/intel_opregion.c > @@ -872,23 +872,29 @@ static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv) > return ret; > } > > - if (intel_bios_is_valid_vbt(fw->data, fw->size)) { > - opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL); > - if (opregion->vbt_firmware) { > - DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name); > - opregion->vbt = opregion->vbt_firmware; > - opregion->vbt_size = fw->size; > - ret = 0; > - } else { > - ret = -ENOMEM; > - } > - } else { > + if (!intel_bios_is_valid_vbt(fw->data, fw->size)) { > DRM_DEBUG_KMS("Invalid VBT firmware \"%s\"\n", name); > ret = -EINVAL; > + goto err_release_fw; > + } > + > + opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL); > + if (!opregion->vbt_firmware) { > + ret = -ENOMEM; > + goto err_release_fw; > } > > + opregion->vbt = opregion->vbt_firmware; > + opregion->vbt_size = fw->size; > + > + DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name); > + > release_firmware(fw); > > + return 0; With ret = 0 at the beginning you could just remove the the above three lines and let this run through the below code. > + > +err_release_fw: > + release_firmware(fw); Usually we'd have a blank line before the ret. BR, Jani. > return ret; > }
diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c index 969ade623691..9738511147b1 100644 --- a/drivers/gpu/drm/i915/display/intel_opregion.c +++ b/drivers/gpu/drm/i915/display/intel_opregion.c @@ -872,23 +872,29 @@ static int intel_load_vbt_firmware(struct drm_i915_private *dev_priv) return ret; } - if (intel_bios_is_valid_vbt(fw->data, fw->size)) { - opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL); - if (opregion->vbt_firmware) { - DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name); - opregion->vbt = opregion->vbt_firmware; - opregion->vbt_size = fw->size; - ret = 0; - } else { - ret = -ENOMEM; - } - } else { + if (!intel_bios_is_valid_vbt(fw->data, fw->size)) { DRM_DEBUG_KMS("Invalid VBT firmware \"%s\"\n", name); ret = -EINVAL; + goto err_release_fw; + } + + opregion->vbt_firmware = kmemdup(fw->data, fw->size, GFP_KERNEL); + if (!opregion->vbt_firmware) { + ret = -ENOMEM; + goto err_release_fw; } + opregion->vbt = opregion->vbt_firmware; + opregion->vbt_size = fw->size; + + DRM_DEBUG_KMS("Found valid VBT firmware \"%s\"\n", name); + release_firmware(fw); + return 0; + +err_release_fw: + release_firmware(fw); return ret; }
Convert the code to return-early style and fix missing calls to release_firmware() if vbt is not valid. Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- drivers/gpu/drm/i915/display/intel_opregion.c | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-)