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]
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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ