Re: [PATCH v4 2/3] ACPI: bus: Allow negotiating OSC capabilities

[Qian Cai <quic_qiancai@quicinc.com>]

On Tue, Mar 01, 2022 at 06:49:07AM -0600, Mario Limonciello wrote:
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 07f604832fd6..302619ad9d31 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -184,6 +184,18 @@ static void acpi_print_osc_error(acpi_handle handle,
>  	pr_debug("\n");
>  }
>  
> +/**
> + * acpi_run_osc - Evaluate the _OSC method for a given ACPI handle
> + * @handle: ACPI handle containing _OSC to evaluate
> + * @context: A structure specifying UUID, revision, and buffer for data
> + *
> + * Used for negotiating with firmware the capabilities that will be used
> + * by the OSPM.  Although the return type is acpi_status, the ACPI_SUCCESS
> + * macro can not be used to determine whether to free the buffer of
> + * returned data.
> + *
> + * The buffer must be freed when this function returns AE_OK or AE_SUPPORT.
> + */
>  acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
>  {
>  	acpi_status status;
> @@ -243,16 +255,19 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
>  			acpi_print_osc_error(handle, context,
>  				"_OSC invalid revision");
>  		if (errors & OSC_CAPABILITIES_MASK_ERROR) {
> +			acpi_print_osc_error(handle, context, "_OSC capabilities masked");
>  			if (((u32 *)context->cap.pointer)[OSC_QUERY_DWORD]
> -			    & OSC_QUERY_ENABLE)
> -				goto out_success;
> -			status = AE_SUPPORT;
> -			goto out_kfree;
> +			    & OSC_QUERY_ENABLE) {
> +				status = AE_SUPPORT;
> +				goto out_masked;
> +			}
>  		}
>  		status = AE_ERROR;
>  		goto out_kfree;
>  	}
> -out_success:
> +
> +	status =  AE_OK;
> +out_masked:
>  	context->ret.length = out_obj->buffer.length;
>  	context->ret.pointer = kmemdup(out_obj->buffer.pointer,
>  				       context->ret.length, GFP_KERNEL);
> @@ -260,7 +275,6 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
>  		status =  AE_NO_MEMORY;
>  		goto out_kfree;
>  	}
> -	status =  AE_OK;
>  
>  out_kfree:
>  	kfree(output.pointer);

Do we forget to free "context.ret.pointer" in acpi_pci_run_osc() as well
when acpi_run_osc() starts to return AE_SUPPORT? I saw memory leaks on the
latest linux-next which include this series.

# cat /sys/kernel/debug/kmemleak
unreferenced object 0xffff07ff968c6e80 (size 128):
  comm "swapper/0", pid 1, jiffies 4294894996 (age 1785.652s)
  hex dump (first 32 bytes):
    11 00 00 00 1f 01 00 00 18 00 00 00 6b 6b 6b 6b  ............kkkk
    6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
  backtrace:
    [<ffffad5bd9d79adc>] __kmalloc_track_caller
    [<ffffad5bd9c439f4>] kmemdup
    [<ffffad5bda97c710>] acpi_run_osc
    acpi_run_osc at drivers/acpi/bus.c:272
    [<ffffad5bdc52f8a8>] acpi_pci_run_osc
    acpi_pci_run_osc at drivers/acpi/pci_root.c:184
    [<ffffad5bdc52fe30>] negotiate_os_control.constprop.0
    acpi_pci_query_osc at drivers/acpi/pci_root.c:205
    (inlined by) acpi_pci_osc_control_set at drivers/acpi/pci_root.c:353
    (inlined by) negotiate_os_control at drivers/acpi/pci_root.c:482
    [<ffffad5bda995748>] acpi_pci_root_add
    [<ffffad5bda982558>] acpi_bus_attach
    [<ffffad5bda9823c8>] acpi_bus_attach
    [<ffffad5bda9823c8>] acpi_bus_attach
    [<ffffad5bda98808c>] acpi_bus_scan
    [<ffffad5bdd9624a0>] acpi_scan_init
    [<ffffad5bdd961e74>] acpi_init
    [<ffffad5bd9595438>] do_one_initcall
    [<ffffad5bdd90201c>] kernel_init_freeable
    [<ffffad5bdc5684e0>] kernel_init
    [<ffffad5bd959841c>] ret_from_fork