linux-kernel - Re: [RFC PATCH v8 06/10] ACPICA: Add __free() based cleanup function for acpi_put

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240419190642.00005ee7@huawei.com>
Date: Fri, 19 Apr 2024 19:06:42 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: <shiju.jose@...wei.com>
CC: <linux-cxl@...r.kernel.org>, <linux-acpi@...r.kernel.org>,
	<linux-mm@...ck.org>, <dan.j.williams@...el.com>, <dave@...olabs.net>,
	<dave.jiang@...el.com>, <alison.schofield@...el.com>,
	<vishal.l.verma@...el.com>, <ira.weiny@...el.com>,
	<linux-edac@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<david@...hat.com>, <Vilas.Sridharan@....com>, <leo.duran@....com>,
	<Yazen.Ghannam@....com>, <rientjes@...gle.com>, <jiaqiyan@...gle.com>,
	<tony.luck@...el.com>, <Jon.Grimm@....com>, <dave.hansen@...ux.intel.com>,
	<rafael@...nel.org>, <lenb@...nel.org>, <naoya.horiguchi@....com>,
	<james.morse@....com>, <jthoughton@...gle.com>, <somasundaram.a@....com>,
	<erdemaktas@...gle.com>, <pgonda@...gle.com>, <duenwen@...gle.com>,
	<mike.malvestuto@...el.com>, <gthelen@...gle.com>,
	<wschwartz@...erecomputing.com>, <dferguson@...erecomputing.com>,
	<wbs@...amperecomputing.com>, <nifan.cxl@...il.com>, <tanxiaofei@...wei.com>,
	<prime.zeng@...ilicon.com>, <kangkang.shen@...urewei.com>,
	<wanghuiqiang@...wei.com>, <linuxarm@...wei.com>
Subject: Re: [RFC PATCH v8 06/10] ACPICA: Add __free() based cleanup
 function for acpi_put_table

On Sat, 20 Apr 2024 00:47:15 +0800
<shiju.jose@...wei.com> wrote:

> From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> 
> Add __free() based cleanup function for acpi_put_table.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Signed-off-by: Shiju Jose <shiju.jose@...wei.com>
> ---

Reviewing (and rejecting) my own patch time ;(

I was thinking this would be useful more widely but hadn't looked
as closely as I should have done.  Sorry Shiju for sending you
down a bad path.

>  include/acpi/acpixf.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> index 3d90716f9522..fc64d903a703 100644
> --- a/include/acpi/acpixf.h
> +++ b/include/acpi/acpixf.h
> @@ -492,6 +492,8 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status
>  					    **out_table))
>  ACPI_EXTERNAL_RETURN_VOID(void acpi_put_table(struct acpi_table_header *table))
>  
> +DEFINE_FREE(acpi_put_table, struct acpi_table_header *, if (!IS_ERR_OR_NULL(_T)) acpi_put_table(_T))

This is reliant on acpi_get_table2() in patch 8 / below being used as acpi_get_table()
doesn't return the table.

Maybe we are better off treating acpi_get_table() / acpi_put_table() as if it were a
conditional lock? Or change the 93 instances of acpi_get_table to deal with it returning
a copy of the table handle pointer

That would bring it inline with many other get functions in the kernel + make our life
easier using tooling like this.

+static struct acpi_table_header *acpi_get_table2(acpi_string signature,
+						  u32 instance)
+{
+	struct acpi_table_header *header = NULL;
+	acpi_status status = acpi_get_table(signature, instance, &header);
+
+	if (ACPI_FAILURE(status))
+		return ERR_PTR(-EINVAL);
+
+	return header;
+}
So that we could do things like:
+	struct acpi_table_header *pAcpiTable __free(acpi_put_table) =
+						acpi_get_table2("RAS2", 0);

and avoid having to call acpi_put_table() in error paths etc.

The snag is that acpi_get_table() is from acpica (via this wrapper) so any
modification would be a little messy. Also a number of cases use the status
value via 
const char *msg = acpi_format_exception(status);

Which we'd need to return via some path (a parameter probably). We 'could'
do that but the advantages of this are getting eroded.

Upshot, this is messier than I thought, so we probably shouldn't do it.

The code in ras2 can be done reasonably neatly an outer wrapper function
that gets the table and an inner one that deals with the actual processing
of the entries.

Pity as there are some messy bits of code this would tidy up. In most of
those a helper function also works.

Jonathan

p.s. Whilst looking at this I noticed that acpi_has_watchdog() if it
succeeds doesn't put the wdat table which seems suspicious as a side
effect.

> +
>  ACPI_EXTERNAL_RETURN_STATUS(acpi_status
>  			    acpi_get_table_by_index(u32 table_index,
>  						    struct acpi_table_header