Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1631956 > unrolled thread

[PATCH v2 1/2] ACPICA: Tables: Fix regression introduced by a too early mechanism enabling

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 1/2] ACPICA: Tables: Fix regression introduced by a too early mechanism enabling Lv Zheng <lv.zheng@intel.com> - 2017-04-27 10:30 +0200
    Re: [PATCH v2 1/2] ACPICA: Tables: Fix regression introduced by a too early mechanism enabling "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-04-28 00:40 +0200
      RE: [PATCH v2 1/2] ACPICA: Tables: Fix regression introduced by a  too early mechanism enabling "Zheng, Lv" <lv.zheng@intel.com> - 2017-04-28 03:30 +0200
      RE: [PATCH v2 1/2] ACPICA: Tables: Fix regression introduced by a  too early mechanism enabling "Zheng, Lv" <lv.zheng@intel.com> - 2017-04-28 06:00 +0200
    [PATCH v2 1/4] ACPICA: Tables: Fix regression introduced by a too early mechanism enabling Lv Zheng <lv.zheng@intel.com> - 2017-04-28 07:30 +0200
    [PATCH v3 1/4] ACPICA: Tables: Fix regression introduced by a too early mechanism enabling Lv Zheng <lv.zheng@intel.com> - 2017-04-28 07:40 +0200
    [PATCH v3 4/4] ACPI: Fix memory mapping leaks in current sysfs dumpable ACPI tables support Lv Zheng <lv.zheng@intel.com> - 2017-04-28 07:40 +0200
    [PATCH v3 3/4] ACPI: sysfs: Fix acpi_get_table() leak Lv Zheng <lv.zheng@intel.com> - 2017-04-28 07:40 +0200
    [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently Lv Zheng <lv.zheng@intel.com> - 2017-04-28 07:40 +0200
      Re: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-04-28 23:10 +0200
        RE: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to  balance late stage acpi_get_table() independently "Zheng, Lv" <lv.zheng@intel.com> - 2017-05-04 09:20 +0200
          Re: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance  late stage acpi_get_table() independently Dan Williams <dan.j.williams@intel.com> - 2017-05-04 17:50 +0200
            RE: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to  balance late stage acpi_get_table() independently "Zheng, Lv" <lv.zheng@intel.com> - 2017-05-05 03:00 +0200
          Re: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-05-05 22:50 +0200
            RE: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to  balance late stage acpi_get_table() independently "Zheng, Lv" <lv.zheng@intel.com> - 2017-05-09 04:00 +0200
    [PATCH v4 3/4] ACPI: sysfs: Fix acpi_get_table() leak Lv Zheng <lv.zheng@intel.com> - 2017-05-09 08:00 +0200
    [PATCH v4 4/4] ACPI: Fix memory mapping leaks in current sysfs dumpable ACPI tables support Lv Zheng <lv.zheng@intel.com> - 2017-05-09 08:00 +0200
    [PATCH v4 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently Lv Zheng <lv.zheng@intel.com> - 2017-05-09 08:00 +0200
    [PATCH v4 1/4] ACPICA: Tables: Fix regression introduced by a too early mechanism enabling Lv Zheng <lv.zheng@intel.com> - 2017-05-09 08:00 +0200

#1631956 — [PATCH v2 1/2] ACPICA: Tables: Fix regression introduced by a too early mechanism enabling

In the Linux kernel side, acpi_get_table() clones haven't been fully
balanced by acpi_put_table() invocations. In ACPICA side, due to the design
change, there are also unbalanced acpi_get_table_by_index() invocations
requiring special care to be cleaned up.

So it is not a good timing to report acpi_get_table() counting errors for
this period. The strict balanced validation count check should only be
enabled after confirming that all invocations are safe and compliant to
their designed purposes.

Thus this patch removes the fatal error along with lthe error report to
fix this issue. Reported by Dan Williams, fixed by Lv Zheng.

Fixes: 174cc7187e6f ("ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel")
Reported-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/acpica/tbutils.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
index 5a968a7..8175c70 100644
--- a/drivers/acpi/acpica/tbutils.c
+++ b/drivers/acpi/acpica/tbutils.c
@@ -418,11 +418,13 @@ acpi_tb_get_table(struct acpi_table_desc *table_desc,
 
 	table_desc->validation_count++;
 	if (table_desc->validation_count == 0) {
+		table_desc->validation_count--;
+#if 0
 		ACPI_ERROR((AE_INFO,
 			    "Table %p, Validation count is zero after increment\n",
 			    table_desc));
-		table_desc->validation_count--;
 		return_ACPI_STATUS(AE_LIMIT);
+#endif
 	}
 
 	*out_table = table_desc->pointer;
-- 
2.7.4

[toc] | [next] | [standalone]

#1632420

On Thursday, April 27, 2017 04:22:44 PM Lv Zheng wrote:
> In the Linux kernel side, acpi_get_table() clones haven't been fully
> balanced by acpi_put_table() invocations. In ACPICA side, due to the design
> change, there are also unbalanced acpi_get_table_by_index() invocations
> requiring special care to be cleaned up.
> 
> So it is not a good timing to report acpi_get_table() counting errors for
> this period. The strict balanced validation count check should only be
> enabled after confirming that all invocations are safe and compliant to
> their designed purposes.
> 
> Thus this patch removes the fatal error along with lthe error report to
> fix this issue. Reported by Dan Williams, fixed by Lv Zheng.
> 
> Fixes: 174cc7187e6f ("ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel")
> Reported-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>

Please do not add #if 0 anywhere to the kernel code base.

If you need to drop some piece of code, just drop it.

And in this particular case validation_count should be dropped from the data
type definition too.

> ---
>  drivers/acpi/acpica/tbutils.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
> index 5a968a7..8175c70 100644
> --- a/drivers/acpi/acpica/tbutils.c
> +++ b/drivers/acpi/acpica/tbutils.c
> @@ -418,11 +418,13 @@ acpi_tb_get_table(struct acpi_table_desc *table_desc,
>  
>  	table_desc->validation_count++;
>  	if (table_desc->validation_count == 0) {
> +		table_desc->validation_count--;
> +#if 0
>  		ACPI_ERROR((AE_INFO,
>  			    "Table %p, Validation count is zero after increment\n",
>  			    table_desc));
> -		table_desc->validation_count--;
>  		return_ACPI_STATUS(AE_LIMIT);
> +#endif
>  	}
>  
>  	*out_table = table_desc->pointer;
> 

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1632474 — RE: [PATCH v2 1/2] ACPICA: Tables: Fix regression introduced by a too early mechanism enabling

Hi, Rafael

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> Wysocki
> Subject: Re: [PATCH v2 1/2] ACPICA: Tables: Fix regression introduced by a too early mechanism
> enabling
> 
> On Thursday, April 27, 2017 04:22:44 PM Lv Zheng wrote:
> > In the Linux kernel side, acpi_get_table() clones haven't been fully
> > balanced by acpi_put_table() invocations. In ACPICA side, due to the design
> > change, there are also unbalanced acpi_get_table_by_index() invocations
> > requiring special care to be cleaned up.
> >
> > So it is not a good timing to report acpi_get_table() counting errors for
> > this period. The strict balanced validation count check should only be
> > enabled after confirming that all invocations are safe and compliant to
> > their designed purposes.
> >
> > Thus this patch removes the fatal error along with lthe error report to
> > fix this issue. Reported by Dan Williams, fixed by Lv Zheng.
> >
> > Fixes: 174cc7187e6f ("ACPICA: Tables: Back port acpi_get_table_with_size() and
> early_acpi_os_unmap_memory() from Linux kernel")
> > Reported-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> 
> Please do not add #if 0 anywhere to the kernel code base.
> 
> If you need to drop some piece of code, just drop it.
> 
> And in this particular case validation_count should be dropped from the data
> type definition too.

Then I would prefer to fix all acpi_get_table() clones in ACPICA.
That could avoid removing the useful code.

Without fixing sysfs/Load/table_handler ones, there will always cases frequently increasing/decreasing the count.
Others seem to be not urgent.

I'll post the fix as an urgent fix here with the ACPICA upstream process bypassed and back port it to ACPICA later.
If there are changes during the back port, I'll take care of making the differences eliminated in the corresponding ACPICA release cycle.

Thanks
Lv


> 
> > ---
> >  drivers/acpi/acpica/tbutils.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
> > index 5a968a7..8175c70 100644
> > --- a/drivers/acpi/acpica/tbutils.c
> > +++ b/drivers/acpi/acpica/tbutils.c
> > @@ -418,11 +418,13 @@ acpi_tb_get_table(struct acpi_table_desc *table_desc,
> >
> >  	table_desc->validation_count++;
> >  	if (table_desc->validation_count == 0) {
> > +		table_desc->validation_count--;
> > +#if 0
> >  		ACPI_ERROR((AE_INFO,
> >  			    "Table %p, Validation count is zero after increment\n",
> >  			    table_desc));
> > -		table_desc->validation_count--;
> >  		return_ACPI_STATUS(AE_LIMIT);
> > +#endif
> >  	}
> >
> >  	*out_table = table_desc->pointer;
> >
> 
> Thanks,
> Rafael
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1632514 — RE: [PATCH v2 1/2] ACPICA: Tables: Fix regression introduced by a too early mechanism enabling

Hi, Rafael

I reconsidered your comments.
Seems there are several problems you might not be aware of.
Let me reply again.

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> Wysocki
> Subject: Re: [PATCH v2 1/2] ACPICA: Tables: Fix regression introduced by a too early mechanism
> enabling
> 
> On Thursday, April 27, 2017 04:22:44 PM Lv Zheng wrote:
> > In the Linux kernel side, acpi_get_table() clones haven't been fully
> > balanced by acpi_put_table() invocations. In ACPICA side, due to the design
> > change, there are also unbalanced acpi_get_table_by_index() invocations
> > requiring special care to be cleaned up.
> >
> > So it is not a good timing to report acpi_get_table() counting errors for
> > this period. The strict balanced validation count check should only be
> > enabled after confirming that all invocations are safe and compliant to
> > their designed purposes.
> >
> > Thus this patch removes the fatal error along with lthe error report to
> > fix this issue. Reported by Dan Williams, fixed by Lv Zheng.
> >
> > Fixes: 174cc7187e6f ("ACPICA: Tables: Back port acpi_get_table_with_size() and
> early_acpi_os_unmap_memory() from Linux kernel")
> > Reported-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> 
> Please do not add #if 0 anywhere to the kernel code base.
> 
> If you need to drop some piece of code, just drop it.

OK. This comment is addressable.

> 
> And in this particular case validation_count should be dropped from the data
> type definition too.

I don't think this comment is addressable.

===========================================================================
The validation_count is not only used for the late stage, it's also used
for the early boot stage. And during the early boot stage, I'm sure the
validation count is used by us now to correctly unmap early maps. If it is
dropped, the early maps will be leaked to the late stage, leading to some
kernel crashes.

The use case has already been validated to be working in previous Linux
release cycles, also with you, Dan and the other driver maintainers' help.
All crashing early cases are identified and the validation counts are
ensured to be balanced during the early stage.
===========================================================================
Then I thought again if you did have some special concerns for using this
mechanism in the late stage without making the validation count balanced.
The only case we need to worry about is when acpi_get_table() is invoked
more than 65535 times, then more than 65535 acpi_put_table() could
potentially release the table mapping while there are still users using
the mapped table, this could cause kernel crashes.

What does this problem mean to us? It mean: For all frequently invoked
acpi_get_table(), without fixing them altogether, we SHOULD NOT add
acpi_put_table() for them.

As far as I can see, the followings are frequent acpi_get_table() invoking
cases, they need to be fixed altogether:
1. sysfs table access
2. load/unload tables, could potentially be invoked in a
   per-cpu-hotplugging frequent style.
3. get/put in some loadable kernel modules (this actually is not such
   frequent)
4. tables used across suspend/resume (I checked, FACS used in this case is
   safe)

I began to doubt the correctness of fixing only the sysfs case without
fixing all the other "frequent acpi_get_table() cases". It seems fixing it
only adds unwanted acpi_put_table() during the special period, and this
actually breaks the planned way, and can potentially break the end users.
===========================================================================
Then I checked how we could make Load/Unload invocations balanced inside of
ACPICA. It looks we have the following things that must be done to achieve
the balanced validation count inside of ACPICA:
1. There are many "table handler" invocations, we need a single place to
   invoke get/put before/after invoking the handler.
2. There is an acpi_tb_set_table_loaded_flag() API, we need to invoke
   get/put inside of it when the flag is set/unset.
3. For all other acpi_get_table() clone invocations, we need to add
   balanced put(s) to the get(s) except those pinned as global maps.

Then we'll soon figure out that we need to do the following prerequisites:
1. Collect table handlers into 1 function;
2. Should stop invoking acpi_ns_load_table() from other places, but only
   invoke it from acpi_tb_load_table();
3. After changing due to 1 and 2, we need to make sure the table mutex is
   still correctly used;
4. After changing due to 2 and 3, we actually extends some sanity checks
   to the processes originally not covered, and we need to make sure the
   changed sanity checks is still working.
So you'll see the entire work must be based on what Hans De Geode is
asking for: "https://github.com/acpica/acpica/pull/121". This pull request
is still pending for further Windows behavior validation for how signature
is checked by Windows. And on top of that, several changes are still
required for achieving the balanced validation count in ACPICA.

So I really don't think upstreaming all the required changes is a short
period.
===========================================================================

My conclusion is:
We should go back to our planned way, and the only safe regression fix for
the reported issue is to remove the error logs and the failure returning
code from acpi_tb_get_table(). While the error in acpi_tb_put_table()
should be kept, it alerts us that there are too many unexpected
acpi_tb_put_table() invocations.
If we don't stick to the planned way, we possibly should add code in
acpi_tb_put_table() that when the get side is about to expre, stop do any
put side decrement to prevent unwanted unmaps.

Thanks and best regards
Lv

> 
> > ---
> >  drivers/acpi/acpica/tbutils.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
> > index 5a968a7..8175c70 100644
> > --- a/drivers/acpi/acpica/tbutils.c
> > +++ b/drivers/acpi/acpica/tbutils.c
> > @@ -418,11 +418,13 @@ acpi_tb_get_table(struct acpi_table_desc *table_desc,
> >
> >  	table_desc->validation_count++;
> >  	if (table_desc->validation_count == 0) {
> > +		table_desc->validation_count--;
> > +#if 0
> >  		ACPI_ERROR((AE_INFO,
> >  			    "Table %p, Validation count is zero after increment\n",
> >  			    table_desc));
> > -		table_desc->validation_count--;
> >  		return_ACPI_STATUS(AE_LIMIT);
> > +#endif
> >  	}
> >
> >  	*out_table = table_desc->pointer;
> >
> 
> Thanks,
> Rafael
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1632534 — [PATCH v2 1/4] ACPICA: Tables: Fix regression introduced by a too early mechanism enabling

In the Linux kernel side, acpi_get_table() clones haven't been fully
balanced by acpi_put_table() invocations. In ACPICA side, due to the design
change, there are also unbalanced acpi_get_table_by_index() invocations
requiring special care.

So it is not a good timing to report acpi_get_table() counting errors. The
strict balanced validation count check should only be enabled after
confirming that all invocations are safe and compliant to their designed
purposes.

Thus this patch removes the fatal error along with the error report to
fix this issue. Reported by Dan Williams, fixed by Lv Zheng.

Fixes: 174cc7187e6f ("ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel")
Cc: Dan Williams <dan.j.williams@intel.com>
Reported-by: Anush Seetharaman <anush.seetharaman@intel.com>
Reported-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/acpica/tbutils.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
index 5a968a7..7abe665 100644
--- a/drivers/acpi/acpica/tbutils.c
+++ b/drivers/acpi/acpica/tbutils.c
@@ -418,11 +418,7 @@ acpi_tb_get_table(struct acpi_table_desc *table_desc,
 
 	table_desc->validation_count++;
 	if (table_desc->validation_count == 0) {
-		ACPI_ERROR((AE_INFO,
-			    "Table %p, Validation count is zero after increment\n",
-			    table_desc));
 		table_desc->validation_count--;
-		return_ACPI_STATUS(AE_LIMIT);
 	}
 
 	*out_table = table_desc->pointer;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1632537 — [PATCH v3 1/4] ACPICA: Tables: Fix regression introduced by a too early mechanism enabling

In the Linux kernel side, acpi_get_table() clones haven't been fully
balanced by acpi_put_table() invocations. In ACPICA side, due to the design
change, there are also unbalanced acpi_get_table_by_index() invocations
requiring special care.

So it is not a good timing to report acpi_get_table() counting errors. The
strict balanced validation count check should only be enabled after
confirming that all invocations are safe and compliant to their designed
purposes.

Thus this patch removes the fatal error along with the error report to
fix this issue. Reported by Dan Williams, fixed by Lv Zheng.

Fixes: 174cc7187e6f ("ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel")
Cc: Anush Seetharaman <anush.seetharaman@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Reported-by: Anush Seetharaman <anush.seetharaman@intel.com>
Reported-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/acpica/tbutils.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
index 5a968a7..7abe665 100644
--- a/drivers/acpi/acpica/tbutils.c
+++ b/drivers/acpi/acpica/tbutils.c
@@ -418,11 +418,7 @@ acpi_tb_get_table(struct acpi_table_desc *table_desc,
 
 	table_desc->validation_count++;
 	if (table_desc->validation_count == 0) {
-		ACPI_ERROR((AE_INFO,
-			    "Table %p, Validation count is zero after increment\n",
-			    table_desc));
 		table_desc->validation_count--;
-		return_ACPI_STATUS(AE_LIMIT);
 	}
 
 	*out_table = table_desc->pointer;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1632538 — [PATCH v3 4/4] ACPI: Fix memory mapping leaks in current sysfs dumpable ACPI tables support

This patch adds acpi_put_table() to make all acpi_get_table() clone
invocations balanced for sysfs ACPI table dump code.

Since Linux does not use all of the tables, this can help to reduce some
usless memory mappings.

While originally, all tables will be remained to be mapped after a
userspace acpidump execution, potentially causing problem on server
platforms. With the new APIs, it is possible to release such useless table
mappings.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/sysfs.c | 41 +++++++++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 2bbf722..14425dc 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -346,11 +346,22 @@ static ssize_t acpi_table_show(struct file *filp, struct kobject *kobj,
 	return len;
 }
 
+static bool acpi_table_has_multiple_instances(char *signature)
+{
+	acpi_status status;
+	struct acpi_table_header *header;
+
+	status = acpi_get_table(signature, 2, &header);
+	if (ACPI_FAILURE(status))
+		return false;
+	acpi_put_table(header);
+	return true;
+}
+
 static int acpi_table_attr_init(struct kobject *tables_obj,
 				struct acpi_table_attr *table_attr,
 				struct acpi_table_header *table_header)
 {
-	struct acpi_table_header *header = NULL;
 	struct acpi_table_attr *attr = NULL;
 	char instance_str[ACPI_INST_SIZE];
 
@@ -371,9 +382,9 @@ static int acpi_table_attr_init(struct kobject *tables_obj,
 
 	ACPI_MOVE_NAME(table_attr->filename, table_header->signature);
 	table_attr->filename[ACPI_NAME_SIZE] = '\0';
-	if (table_attr->instance > 1 || (table_attr->instance == 1 &&
-					 !acpi_get_table
-					 (table_header->signature, 2, &header))) {
+	if (table_attr->instance > 1 ||
+	    (table_attr->instance == 1 &&
+	     acpi_table_has_multiple_instances(table_header->signature))) {
 		snprintf(instance_str, sizeof(instance_str), "%u",
 			 table_attr->instance);
 		strcat(table_attr->filename, instance_str);
@@ -422,11 +433,11 @@ acpi_status acpi_sysfs_table_handler(u32 event, void *table, void *context)
 
 static int acpi_tables_sysfs_init(void)
 {
-	struct acpi_table_attr *table_attr;
+	struct acpi_table_attr *table_attr = NULL;
 	struct acpi_table_header *table_header = NULL;
 	int table_index;
 	acpi_status status;
-	int ret;
+	int ret = 0;
 
 	tables_kobj = kobject_create_and_add("tables", acpi_kobj);
 	if (!tables_kobj)
@@ -446,16 +457,26 @@ static int acpi_tables_sysfs_init(void)
 			continue;
 
 		table_attr = kzalloc(sizeof(*table_attr), GFP_KERNEL);
-		if (!table_attr)
-			return -ENOMEM;
+		if (!table_attr) {
+			ret = -ENOMEM;
+			goto next_table;
+		}
 
 		ret = acpi_table_attr_init(tables_kobj,
 					   table_attr, table_header);
+		if (ret)
+			goto next_table;
+		list_add_tail(&table_attr->node, &acpi_table_attr_list);
+
+next_table:
+		acpi_put_table(table_header);
 		if (ret) {
-			kfree(table_attr);
+			if (table_attr) {
+				kfree(table_attr);
+				table_attr = NULL;
+			}
 			return ret;
 		}
-		list_add_tail(&table_attr->node, &acpi_table_attr_list);
 	}
 
 	kobject_uevent(tables_kobj, KOBJ_ADD);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1632539 — [PATCH v3 3/4] ACPI: sysfs: Fix acpi_get_table() leak

From: Dan Williams <dan.j.williams@intel.com>

Reading an ACPI table through the /sys/firmware/acpi/tables interface
more than 65,536 times leads to the following log message:

 ACPI Error: Table ffff88033595eaa8, Validation count is zero after
increment
  (20170119/tbutils-423)

Add the missing acpi_put_table() so the table ->validation_count is
decremented after each read.

Reported-by: Anush Seetharaman <anush.seetharaman@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/sysfs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 1b5ee1e..2bbf722 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -333,14 +333,17 @@ static ssize_t acpi_table_show(struct file *filp, struct kobject *kobj,
 	    container_of(bin_attr, struct acpi_table_attr, attr);
 	struct acpi_table_header *table_header = NULL;
 	acpi_status status;
+	ssize_t len;
 
 	status = acpi_get_table(table_attr->name, table_attr->instance,
 				&table_header);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
-	return memory_read_from_buffer(buf, count, &offset,
-				       table_header, table_header->length);
+	len = memory_read_from_buffer(buf, count, &offset,
+				      table_header, table_header->length);
+	acpi_put_table(table_header);
+	return len;
 }
 
 static int acpi_table_attr_init(struct kobject *tables_obj,
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1632540 — [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently

For all frequent late stage acpi_get_table() clone invocations, we should
only fix them altogether, otherwise, excessive acpi_put_table() could
unexpectedly unmap the table used by the other users. Thus the current plan
is to fix all acpi_get_table() clones together or to fix none of them. This
prevents kernel developers from improving the late stage code quality
without waiting for the ACPICA upstream to improve first.

This patch adds a mechanism to stop decrementing validation count to
prevent the table unmapping operations so that acpi_put_table() balance
fixes can be done independently to each others.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/acpica/tbutils.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
index 7abe665..b517bd0 100644
--- a/drivers/acpi/acpica/tbutils.c
+++ b/drivers/acpi/acpica/tbutils.c
@@ -445,12 +445,18 @@ void acpi_tb_put_table(struct acpi_table_desc *table_desc)
 
 	ACPI_FUNCTION_TRACE(acpi_tb_put_table);
 
-	if (table_desc->validation_count == 0) {
+	if ((table_desc->validation_count + 1) == 0) {
 		ACPI_WARNING((AE_INFO,
-			      "Table %p, Validation count is zero before decrement\n",
+			      "Table %p, Validation count is about to expire, decrement is unsafe\n",
 			      table_desc));
 		return_VOID;
 	}
+	if (table_desc->validation_count == 0) {
+		ACPI_ERROR((AE_INFO,
+			   "Table %p, Validation count is zero before decrement\n",
+			   table_desc));
+		return_VOID;
+	}
 	table_desc->validation_count--;
 
 	if (table_desc->validation_count == 0) {
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1633161 — Re: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently

On Friday, April 28, 2017 01:30:20 PM Lv Zheng wrote:
> For all frequent late stage acpi_get_table() clone invocations, we should
> only fix them altogether, otherwise, excessive acpi_put_table() could
> unexpectedly unmap the table used by the other users. Thus the current plan
> is to fix all acpi_get_table() clones together or to fix none of them.

I honestly don't think that fixing none of them is a valid option here.

> This prevents kernel developers from improving the late stage code quality
> without waiting for the ACPICA upstream to improve first.
> 
> This patch adds a mechanism to stop decrementing validation count to
> prevent the table unmapping operations so that acpi_put_table() balance
> fixes can be done independently to each others.
> 
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> ---
>  drivers/acpi/acpica/tbutils.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
> index 7abe665..b517bd0 100644
> --- a/drivers/acpi/acpica/tbutils.c
> +++ b/drivers/acpi/acpica/tbutils.c
> @@ -445,12 +445,18 @@ void acpi_tb_put_table(struct acpi_table_desc *table_desc)
>  
>  	ACPI_FUNCTION_TRACE(acpi_tb_put_table);
>  
> -	if (table_desc->validation_count == 0) {
> +	if ((table_desc->validation_count + 1) == 0) {

This means that validation_count has reached the maximum value, right?

>  		ACPI_WARNING((AE_INFO,
> -			      "Table %p, Validation count is zero before decrement\n",
> +			      "Table %p, Validation count is about to expire, decrement is unsafe\n",
>  			      table_desc));

So why is it unsafe to decrement it?

>  		return_VOID;
>  	}
> +	if (table_desc->validation_count == 0) {
> +		ACPI_ERROR((AE_INFO,
> +			   "Table %p, Validation count is zero before decrement\n",
> +			   table_desc));
> +		return_VOID;
> +	}
>  	table_desc->validation_count--;
>  
>  	if (table_desc->validation_count == 0) {
> 

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1635445 — RE: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently

Hi, Rafael

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> Wysocki
> Subject: Re: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage
> acpi_get_table() independently
> 
> On Friday, April 28, 2017 01:30:20 PM Lv Zheng wrote:
> > For all frequent late stage acpi_get_table() clone invocations, we should
> > only fix them altogether, otherwise, excessive acpi_put_table() could
> > unexpectedly unmap the table used by the other users. Thus the current plan
> > is to fix all acpi_get_table() clones together or to fix none of them.
> 
> I honestly don't think that fixing none of them is a valid option here.

That's just exactly the old behavior, maybe shouldn't be called as "fix".
Should say "change to use the new behavior together" all stay unchanged.

I actually want to make the change from ACPICA side.
But it's costly to persuade ACPICA upstream to take both the "acpi_get_table_with_size()/early_acpi_os_unmap_memory() divergence reduction" change and the "table map on-demand" change.

So we just made 2 things separated, and did 1 thing once.

> 
> > This prevents kernel developers from improving the late stage code quality
> > without waiting for the ACPICA upstream to improve first.
> >
> > This patch adds a mechanism to stop decrementing validation count to
> > prevent the table unmapping operations so that acpi_put_table() balance
> > fixes can be done independently to each others.
> >
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > ---
> >  drivers/acpi/acpica/tbutils.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
> > index 7abe665..b517bd0 100644
> > --- a/drivers/acpi/acpica/tbutils.c
> > +++ b/drivers/acpi/acpica/tbutils.c
> > @@ -445,12 +445,18 @@ void acpi_tb_put_table(struct acpi_table_desc *table_desc)
> >
> >  	ACPI_FUNCTION_TRACE(acpi_tb_put_table);
> >
> > -	if (table_desc->validation_count == 0) {
> > +	if ((table_desc->validation_count + 1) == 0) {
> 
> This means that validation_count has reached the maximum value, right?
> 
> >  		ACPI_WARNING((AE_INFO,
> > -			      "Table %p, Validation count is zero before decrement\n",
> > +			      "Table %p, Validation count is about to expire, decrement is unsafe\n",
> >  			      table_desc));
> 
> So why is it unsafe to decrement it?

Considering this case:
A program opens a sysfs table file 65535 times: validation_count = 65535.
Load opcode is invoked by the AML interpreter, but it cannot increase the validation count, see acpi_tb_get_table(): validation_count = 65535.
Now the program closes the sysfs table file: validation_count = 0, which triggers table unmap.
But it is likely that the AML code is still accessing the namespace objects provided by this table.
A kernel crash then can be seen.

So after applying this patch, 65535 now is the threshold.
When it is reached, validation_count will remain 65535 from then on (see both acpi_tb_get_table()/acpi_tb_put_table()).
When it is reached, the 65535 validation count ensures "the old behavior" - for late stage;
When it is not reached, the 65535 validation count ensures "the new behavior" - for early stage.

Then you can see, if there's no acpi_put_table() invoked for such old behavior dependent users, the validation count can also remain 65535.
That's why I said PATCH 3 is actually breaking things.

IMO, if we really want the acpi_put_table() balance work proceeded without waiting for the ACPICA upstream to change.
We need this commit.

I actually generated this commit once.
But hesitated to send it to ACPICA upstream as it didn't look like a good idea to increase communication cost to upstream a commit that hadn't been determined to be used by ACPICA.

However if other driver maintainers want to make their acpi_get_table() invocations balanced like what Dan did here.
This commit is required.

Thanks and best regards
Lv

> 
> >  		return_VOID;
> >  	}
> > +	if (table_desc->validation_count == 0) {
> > +		ACPI_ERROR((AE_INFO,
> > +			   "Table %p, Validation count is zero before decrement\n",
> > +			   table_desc));
> > +		return_VOID;
> > +	}
> >  	table_desc->validation_count--;
> >
> >  	if (table_desc->validation_count == 0) {
> >
> 
> Thanks,
> Rafael
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1635854 — Re: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently

On Thu, May 4, 2017 at 12:18 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi, Rafael
>
>> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
>> Wysocki
>> Subject: Re: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage
>> acpi_get_table() independently
>>
>> On Friday, April 28, 2017 01:30:20 PM Lv Zheng wrote:
>> > For all frequent late stage acpi_get_table() clone invocations, we should
>> > only fix them altogether, otherwise, excessive acpi_put_table() could
>> > unexpectedly unmap the table used by the other users. Thus the current plan
>> > is to fix all acpi_get_table() clones together or to fix none of them.
>>
>> I honestly don't think that fixing none of them is a valid option here.
>
> That's just exactly the old behavior, maybe shouldn't be called as "fix".
> Should say "change to use the new behavior together" all stay unchanged.
>
> I actually want to make the change from ACPICA side.
> But it's costly to persuade ACPICA upstream to take both the "acpi_get_table_with_size()/early_acpi_os_unmap_memory() divergence reduction" change and the "table map on-demand" change.
>
> So we just made 2 things separated, and did 1 thing once.
>
>>
>> > This prevents kernel developers from improving the late stage code quality
>> > without waiting for the ACPICA upstream to improve first.
>> >
>> > This patch adds a mechanism to stop decrementing validation count to
>> > prevent the table unmapping operations so that acpi_put_table() balance
>> > fixes can be done independently to each others.
>> >
>> > Cc: Dan Williams <dan.j.williams@intel.com>
>> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
>> > ---
>> >  drivers/acpi/acpica/tbutils.c | 10 ++++++++--
>> >  1 file changed, 8 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
>> > index 7abe665..b517bd0 100644
>> > --- a/drivers/acpi/acpica/tbutils.c
>> > +++ b/drivers/acpi/acpica/tbutils.c
>> > @@ -445,12 +445,18 @@ void acpi_tb_put_table(struct acpi_table_desc *table_desc)
>> >
>> >     ACPI_FUNCTION_TRACE(acpi_tb_put_table);
>> >
>> > -   if (table_desc->validation_count == 0) {
>> > +   if ((table_desc->validation_count + 1) == 0) {
>>
>> This means that validation_count has reached the maximum value, right?
>>
>> >             ACPI_WARNING((AE_INFO,
>> > -                         "Table %p, Validation count is zero before decrement\n",
>> > +                         "Table %p, Validation count is about to expire, decrement is unsafe\n",
>> >                           table_desc));
>>
>> So why is it unsafe to decrement it?
>
> Considering this case:
> A program opens a sysfs table file 65535 times: validation_count = 65535.
> Load opcode is invoked by the AML interpreter, but it cannot increase the validation count, see acpi_tb_get_table(): validation_count = 65535.
> Now the program closes the sysfs table file: validation_count = 0, which triggers table unmap.
> But it is likely that the AML code is still accessing the namespace objects provided by this table.
> A kernel crash then can be seen.
>
> So after applying this patch, 65535 now is the threshold.
> When it is reached, validation_count will remain 65535 from then on (see both acpi_tb_get_table()/acpi_tb_put_table()).
> When it is reached, the 65535 validation count ensures "the old behavior" - for late stage;
> When it is not reached, the 65535 validation count ensures "the new behavior" - for early stage.
>
> Then you can see, if there's no acpi_put_table() invoked for such old behavior dependent users, the validation count can also remain 65535.
> That's why I said PATCH 3 is actually breaking things.
>
> IMO, if we really want the acpi_put_table() balance work proceeded without waiting for the ACPICA upstream to change.
> We need this commit.
>
> I actually generated this commit once.
> But hesitated to send it to ACPICA upstream as it didn't look like a good idea to increase communication cost to upstream a commit that hadn't been determined to be used by ACPICA.
>
> However if other driver maintainers want to make their acpi_get_table() invocations balanced like what Dan did here.
> This commit is required.
>

Why do we need validation_count at all? I would think you would only
need that if tables can be hot-removed and you need to wait for all
active users to drain. Most tables don't have that behavior, right?
Should we instead be reference counting the few tables that might be
removed and leave the rest statically allocated?

[toc] | [prev] | [next] | [standalone]

#1636109 — RE: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently

Hi, Dan

> From: Dan Williams [mailto:dan.j.williams@intel.com]
> Sent: Thursday, May 4, 2017 11:45 PM
> Subject: Re: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage
> acpi_get_table() independently
> 
> On Thu, May 4, 2017 at 12:18 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> > Hi, Rafael
> >
> >> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of
> Rafael J.
> >> Wysocki
> >> Subject: Re: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage
> >> acpi_get_table() independently
> >>
> >> On Friday, April 28, 2017 01:30:20 PM Lv Zheng wrote:
> >> > For all frequent late stage acpi_get_table() clone invocations, we should
> >> > only fix them altogether, otherwise, excessive acpi_put_table() could
> >> > unexpectedly unmap the table used by the other users. Thus the current plan
> >> > is to fix all acpi_get_table() clones together or to fix none of them.
> >>
> >> I honestly don't think that fixing none of them is a valid option here.
> >
> > That's just exactly the old behavior, maybe shouldn't be called as "fix".
> > Should say "change to use the new behavior together" all stay unchanged.
> >
> > I actually want to make the change from ACPICA side.
> > But it's costly to persuade ACPICA upstream to take both the
> "acpi_get_table_with_size()/early_acpi_os_unmap_memory() divergence reduction" change and the "table
> map on-demand" change.
> >
> > So we just made 2 things separated, and did 1 thing once.
> >
> >>
> >> > This prevents kernel developers from improving the late stage code quality
> >> > without waiting for the ACPICA upstream to improve first.
> >> >
> >> > This patch adds a mechanism to stop decrementing validation count to
> >> > prevent the table unmapping operations so that acpi_put_table() balance
> >> > fixes can be done independently to each others.
> >> >
> >> > Cc: Dan Williams <dan.j.williams@intel.com>
> >> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> >> > ---
> >> >  drivers/acpi/acpica/tbutils.c | 10 ++++++++--
> >> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
> >> > index 7abe665..b517bd0 100644
> >> > --- a/drivers/acpi/acpica/tbutils.c
> >> > +++ b/drivers/acpi/acpica/tbutils.c
> >> > @@ -445,12 +445,18 @@ void acpi_tb_put_table(struct acpi_table_desc *table_desc)
> >> >
> >> >     ACPI_FUNCTION_TRACE(acpi_tb_put_table);
> >> >
> >> > -   if (table_desc->validation_count == 0) {
> >> > +   if ((table_desc->validation_count + 1) == 0) {
> >>
> >> This means that validation_count has reached the maximum value, right?
> >>
> >> >             ACPI_WARNING((AE_INFO,
> >> > -                         "Table %p, Validation count is zero before decrement\n",
> >> > +                         "Table %p, Validation count is about to expire, decrement is unsafe\n",
> >> >                           table_desc));
> >>
> >> So why is it unsafe to decrement it?
> >
> > Considering this case:
> > A program opens a sysfs table file 65535 times: validation_count = 65535.
> > Load opcode is invoked by the AML interpreter, but it cannot increase the validation count, see
> acpi_tb_get_table(): validation_count = 65535.
> > Now the program closes the sysfs table file: validation_count = 0, which triggers table unmap.
> > But it is likely that the AML code is still accessing the namespace objects provided by this table.
> > A kernel crash then can be seen.
> >
> > So after applying this patch, 65535 now is the threshold.
> > When it is reached, validation_count will remain 65535 from then on (see both
> acpi_tb_get_table()/acpi_tb_put_table()).
> > When it is reached, the 65535 validation count ensures "the old behavior" - for late stage;
> > When it is not reached, the 65535 validation count ensures "the new behavior" - for early stage.
> >
> > Then you can see, if there's no acpi_put_table() invoked for such old behavior dependent users, the
> validation count can also remain 65535.
> > That's why I said PATCH 3 is actually breaking things.
> >
> > IMO, if we really want the acpi_put_table() balance work proceeded without waiting for the ACPICA
> upstream to change.
> > We need this commit.
> >
> > I actually generated this commit once.
> > But hesitated to send it to ACPICA upstream as it didn't look like a good idea to increase
> communication cost to upstream a commit that hadn't been determined to be used by ACPICA.
> >
> > However if other driver maintainers want to make their acpi_get_table() invocations balanced like
> what Dan did here.
> > This commit is required.
> >
> 
> Why do we need validation_count at all? I would think you would only
> need that if tables can be hot-removed and you need to wait for all
> active users to drain. Most tables don't have that behavior, right?
> Should we instead be reference counting the few tables that might be
> removed and leave the rest statically allocated?

For now, we need that for hot-removal in early boot stage, not late stage.
Otherwise you'll see warnings reported from check_early_ioremap_leak().
acpi_get_table()/acpi_put_table() is now just a replacement of acpi_get_table_with_size()/early_acpi_os_unmap_memory().
They are required to be paired for early stage.
But not yet enabled for late stage.

Please check https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=174cc7187
You can see a long story in this patch description.

Thanks and best regards
Lv

[toc] | [prev] | [next] | [standalone]

#1636726 — Re: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently

On Thursday, May 04, 2017 07:18:28 AM Zheng, Lv wrote:
> Hi, Rafael
> 
> > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> > Wysocki
> > Subject: Re: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage
> > acpi_get_table() independently
> > 
> > On Friday, April 28, 2017 01:30:20 PM Lv Zheng wrote:
> > > For all frequent late stage acpi_get_table() clone invocations, we should
> > > only fix them altogether, otherwise, excessive acpi_put_table() could
> > > unexpectedly unmap the table used by the other users. Thus the current plan
> > > is to fix all acpi_get_table() clones together or to fix none of them.
> > 
> > I honestly don't think that fixing none of them is a valid option here.
> 
> That's just exactly the old behavior, maybe shouldn't be called as "fix".
> Should say "change to use the new behavior together" all stay unchanged.
> 
> I actually want to make the change from ACPICA side.
> But it's costly to persuade ACPICA upstream to take both the "acpi_get_table_with_size()/early_acpi_os_unmap_memory() divergence reduction" change and the "table map on-demand" change.
> 
> So we just made 2 things separated, and did 1 thing once.
> 
> > 
> > > This prevents kernel developers from improving the late stage code quality
> > > without waiting for the ACPICA upstream to improve first.
> > >
> > > This patch adds a mechanism to stop decrementing validation count to
> > > prevent the table unmapping operations so that acpi_put_table() balance
> > > fixes can be done independently to each others.
> > >
> > > Cc: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > > ---
> > >  drivers/acpi/acpica/tbutils.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
> > > index 7abe665..b517bd0 100644
> > > --- a/drivers/acpi/acpica/tbutils.c
> > > +++ b/drivers/acpi/acpica/tbutils.c
> > > @@ -445,12 +445,18 @@ void acpi_tb_put_table(struct acpi_table_desc *table_desc)
> > >
> > >  	ACPI_FUNCTION_TRACE(acpi_tb_put_table);
> > >
> > > -	if (table_desc->validation_count == 0) {
> > > +	if ((table_desc->validation_count + 1) == 0) {
> > 
> > This means that validation_count has reached the maximum value, right?
> > 
> > >  		ACPI_WARNING((AE_INFO,
> > > -			      "Table %p, Validation count is zero before decrement\n",
> > > +			      "Table %p, Validation count is about to expire, decrement is unsafe\n",
> > >  			      table_desc));
> > 
> > So why is it unsafe to decrement it?
> 
> Considering this case:
> A program opens a sysfs table file 65535 times: validation_count = 65535.
> Load opcode is invoked by the AML interpreter, but it cannot increase the validation count, see acpi_tb_get_table(): validation_count = 65535.
> Now the program closes the sysfs table file: validation_count = 0, which triggers table unmap.
> But it is likely that the AML code is still accessing the namespace objects provided by this table.
> A kernel crash then can be seen.
> 
> So after applying this patch, 65535 now is the threshold.

OK, so this is overflow detection in disguise. :-)

It is quite confusing, IMO.  It would be better to define a limit symbol like
ACPI_TABLE_VCOUNT_MAX below the natural maximum of the data type
(say, make it equal to 65534 if the data type is unsigned short int) and then
make *both* acpi_tb_get_table() and acpi_tb_put_table() refuse to update
validation_count *and* print a "validation count overflow" message once it
has become greater than ACPI_TABLE_VCOUNT_MAX (in which case it will
natrually stay at ACPI_TABLE_VCOUNT_MAX+1).

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1637822 — RE: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently

Hi,

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> Wysocki
> Subject: Re: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage
> acpi_get_table() independently
> 
> On Thursday, May 04, 2017 07:18:28 AM Zheng, Lv wrote:
> > Hi, Rafael
> >
> > > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of
> Rafael J.
> > > Wysocki
> > > Subject: Re: [PATCH v3 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage
> > > acpi_get_table() independently
> > >
> > > On Friday, April 28, 2017 01:30:20 PM Lv Zheng wrote:
> > > > For all frequent late stage acpi_get_table() clone invocations, we should
> > > > only fix them altogether, otherwise, excessive acpi_put_table() could
> > > > unexpectedly unmap the table used by the other users. Thus the current plan
> > > > is to fix all acpi_get_table() clones together or to fix none of them.
> > >
> > > I honestly don't think that fixing none of them is a valid option here.
> >
> > That's just exactly the old behavior, maybe shouldn't be called as "fix".
> > Should say "change to use the new behavior together" all stay unchanged.
> >
> > I actually want to make the change from ACPICA side.
> > But it's costly to persuade ACPICA upstream to take both the
> "acpi_get_table_with_size()/early_acpi_os_unmap_memory() divergence reduction" change and the "table
> map on-demand" change.
> >
> > So we just made 2 things separated, and did 1 thing once.
> >
> > >
> > > > This prevents kernel developers from improving the late stage code quality
> > > > without waiting for the ACPICA upstream to improve first.
> > > >
> > > > This patch adds a mechanism to stop decrementing validation count to
> > > > prevent the table unmapping operations so that acpi_put_table() balance
> > > > fixes can be done independently to each others.
> > > >
> > > > Cc: Dan Williams <dan.j.williams@intel.com>
> > > > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > > > ---
> > > >  drivers/acpi/acpica/tbutils.c | 10 ++++++++--
> > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
> > > > index 7abe665..b517bd0 100644
> > > > --- a/drivers/acpi/acpica/tbutils.c
> > > > +++ b/drivers/acpi/acpica/tbutils.c
> > > > @@ -445,12 +445,18 @@ void acpi_tb_put_table(struct acpi_table_desc *table_desc)
> > > >
> > > >  	ACPI_FUNCTION_TRACE(acpi_tb_put_table);
> > > >
> > > > -	if (table_desc->validation_count == 0) {
> > > > +	if ((table_desc->validation_count + 1) == 0) {
> > >
> > > This means that validation_count has reached the maximum value, right?
> > >
> > > >  		ACPI_WARNING((AE_INFO,
> > > > -			      "Table %p, Validation count is zero before decrement\n",
> > > > +			      "Table %p, Validation count is about to expire, decrement is unsafe\n",
> > > >  			      table_desc));
> > >
> > > So why is it unsafe to decrement it?
> >
> > Considering this case:
> > A program opens a sysfs table file 65535 times: validation_count = 65535.
> > Load opcode is invoked by the AML interpreter, but it cannot increase the validation count, see
> acpi_tb_get_table(): validation_count = 65535.
> > Now the program closes the sysfs table file: validation_count = 0, which triggers table unmap.
> > But it is likely that the AML code is still accessing the namespace objects provided by this table.
> > A kernel crash then can be seen.
> >
> > So after applying this patch, 65535 now is the threshold.
> 
> OK, so this is overflow detection in disguise. :-)
> 
> It is quite confusing, IMO.  It would be better to define a limit symbol like
> ACPI_TABLE_VCOUNT_MAX below the natural maximum of the data type
> (say, make it equal to 65534 if the data type is unsigned short int) and then
> make *both* acpi_tb_get_table() and acpi_tb_put_table() refuse to update
> validation_count *and* print a "validation count overflow" message once it
> has become greater than ACPI_TABLE_VCOUNT_MAX (in which case it will
> natrually stay at ACPI_TABLE_VCOUNT_MAX+1).

OK, I'll also make the error message less verbose in the next revision.

Thanks,
Lv

[toc] | [prev] | [next] | [standalone]

#1637879 — [PATCH v4 3/4] ACPI: sysfs: Fix acpi_get_table() leak

From: Dan Williams <dan.j.williams@intel.com>

Reading an ACPI table through the /sys/firmware/acpi/tables interface
more than 65,536 times leads to the following log message:

 ACPI Error: Table ffff88033595eaa8, Validation count is zero after
increment
  (20170119/tbutils-423)

Add the missing acpi_put_table() so the table ->validation_count is
decremented after each read.

Reported-by: Anush Seetharaman <anush.seetharaman@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/sysfs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 1b5ee1e..2bbf722 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -333,14 +333,17 @@ static ssize_t acpi_table_show(struct file *filp, struct kobject *kobj,
 	    container_of(bin_attr, struct acpi_table_attr, attr);
 	struct acpi_table_header *table_header = NULL;
 	acpi_status status;
+	ssize_t len;
 
 	status = acpi_get_table(table_attr->name, table_attr->instance,
 				&table_header);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
-	return memory_read_from_buffer(buf, count, &offset,
-				       table_header, table_header->length);
+	len = memory_read_from_buffer(buf, count, &offset,
+				      table_header, table_header->length);
+	acpi_put_table(table_header);
+	return len;
 }
 
 static int acpi_table_attr_init(struct kobject *tables_obj,
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1637881 — [PATCH v4 4/4] ACPI: Fix memory mapping leaks in current sysfs dumpable ACPI tables support

This patch adds acpi_put_table() to make all acpi_get_table() clone
invocations balanced for sysfs ACPI table dump code.

Since Linux does not use all of the tables, this can help to reduce some
usless memory mappings.

While originally, all tables will be remained to be mapped after a
userspace acpidump execution, potentially causing problem on server
platforms. With the new APIs, it is possible to release such useless table
mappings.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/sysfs.c | 41 +++++++++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 2bbf722..14425dc 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -346,11 +346,22 @@ static ssize_t acpi_table_show(struct file *filp, struct kobject *kobj,
 	return len;
 }
 
+static bool acpi_table_has_multiple_instances(char *signature)
+{
+	acpi_status status;
+	struct acpi_table_header *header;
+
+	status = acpi_get_table(signature, 2, &header);
+	if (ACPI_FAILURE(status))
+		return false;
+	acpi_put_table(header);
+	return true;
+}
+
 static int acpi_table_attr_init(struct kobject *tables_obj,
 				struct acpi_table_attr *table_attr,
 				struct acpi_table_header *table_header)
 {
-	struct acpi_table_header *header = NULL;
 	struct acpi_table_attr *attr = NULL;
 	char instance_str[ACPI_INST_SIZE];
 
@@ -371,9 +382,9 @@ static int acpi_table_attr_init(struct kobject *tables_obj,
 
 	ACPI_MOVE_NAME(table_attr->filename, table_header->signature);
 	table_attr->filename[ACPI_NAME_SIZE] = '\0';
-	if (table_attr->instance > 1 || (table_attr->instance == 1 &&
-					 !acpi_get_table
-					 (table_header->signature, 2, &header))) {
+	if (table_attr->instance > 1 ||
+	    (table_attr->instance == 1 &&
+	     acpi_table_has_multiple_instances(table_header->signature))) {
 		snprintf(instance_str, sizeof(instance_str), "%u",
 			 table_attr->instance);
 		strcat(table_attr->filename, instance_str);
@@ -422,11 +433,11 @@ acpi_status acpi_sysfs_table_handler(u32 event, void *table, void *context)
 
 static int acpi_tables_sysfs_init(void)
 {
-	struct acpi_table_attr *table_attr;
+	struct acpi_table_attr *table_attr = NULL;
 	struct acpi_table_header *table_header = NULL;
 	int table_index;
 	acpi_status status;
-	int ret;
+	int ret = 0;
 
 	tables_kobj = kobject_create_and_add("tables", acpi_kobj);
 	if (!tables_kobj)
@@ -446,16 +457,26 @@ static int acpi_tables_sysfs_init(void)
 			continue;
 
 		table_attr = kzalloc(sizeof(*table_attr), GFP_KERNEL);
-		if (!table_attr)
-			return -ENOMEM;
+		if (!table_attr) {
+			ret = -ENOMEM;
+			goto next_table;
+		}
 
 		ret = acpi_table_attr_init(tables_kobj,
 					   table_attr, table_header);
+		if (ret)
+			goto next_table;
+		list_add_tail(&table_attr->node, &acpi_table_attr_list);
+
+next_table:
+		acpi_put_table(table_header);
 		if (ret) {
-			kfree(table_attr);
+			if (table_attr) {
+				kfree(table_attr);
+				table_attr = NULL;
+			}
 			return ret;
 		}
-		list_add_tail(&table_attr->node, &acpi_table_attr_list);
 	}
 
 	kobject_uevent(tables_kobj, KOBJ_ADD);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1637886 — [PATCH v4 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently

For all frequent late stage acpi_get_table() clone invocations, we should
only change them altogether, otherwise, excessive acpi_put_table() could
unexpectedly unmap the table used by the other users. Thus the current plan
is to change all acpi_get_table() clones together or to change none of
them. However in practical, this is not convenient as this can prevent
kernel developers' efforts of improving the late stage code quality before
waiting for the ACPICA upstream to improve first.

This patch adds a validation count threashold, when it is reached, the
validation count can no longer be incremented/decremented to invalidate the
table descriptor (means preventing table unmappings) so that acpi_put_table()
balance changes can be done independently to each others. Lv Zheng.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/acpica/tbutils.c | 24 +++++++++++++++---------
 include/acpi/actbl.h          |  9 +++++++++
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
index 7abe665..04beafc 100644
--- a/drivers/acpi/acpica/tbutils.c
+++ b/drivers/acpi/acpica/tbutils.c
@@ -416,9 +416,13 @@ acpi_tb_get_table(struct acpi_table_desc *table_desc,
 		}
 	}
 
-	table_desc->validation_count++;
-	if (table_desc->validation_count == 0) {
-		table_desc->validation_count--;
+	if (table_desc->validation_count < ACPI_MAX_TABLE_VALIDATIONS) {
+		table_desc->validation_count++;
+		if (table_desc->validation_count >= ACPI_MAX_TABLE_VALIDATIONS) {
+			ACPI_WARNING((AE_INFO,
+				      "Table %p, Validation count overflows\n",
+				      table_desc));
+		}
 	}
 
 	*out_table = table_desc->pointer;
@@ -445,13 +449,15 @@ void acpi_tb_put_table(struct acpi_table_desc *table_desc)
 
 	ACPI_FUNCTION_TRACE(acpi_tb_put_table);
 
-	if (table_desc->validation_count == 0) {
-		ACPI_WARNING((AE_INFO,
-			      "Table %p, Validation count is zero before decrement\n",
-			      table_desc));
-		return_VOID;
+	if (table_desc->validation_count < ACPI_MAX_TABLE_VALIDATIONS) {
+		table_desc->validation_count--;
+		if (table_desc->validation_count >= ACPI_MAX_TABLE_VALIDATIONS) {
+			ACPI_WARNING((AE_INFO,
+				      "Table %p, Validation count underflows\n",
+				      table_desc));
+			return_VOID;
+		}
 	}
-	table_desc->validation_count--;
 
 	if (table_desc->validation_count == 0) {
 
diff --git a/include/acpi/actbl.h b/include/acpi/actbl.h
index d92543f..8e1bff8 100644
--- a/include/acpi/actbl.h
+++ b/include/acpi/actbl.h
@@ -374,6 +374,15 @@ struct acpi_table_desc {
 	u16 validation_count;
 };
 
+/*
+ * Maximum validation count, when it is reached, validation count can no
+ * longer be changed. Which means, the table can no longer be invalidated.
+ * This mechanism is implemented for backward compatibility, where in OS
+ * late stage, old drivers are not facilitated with paired validations and
+ * invalidations.
+ */
+#define ACPI_MAX_TABLE_VALIDATIONS          5
+
 /* Masks for Flags field above */
 
 #define ACPI_TABLE_ORIGIN_EXTERNAL_VIRTUAL  (0)	/* Virtual address, external maintained */
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1637888 — [PATCH v4 1/4] ACPICA: Tables: Fix regression introduced by a too early mechanism enabling

In the Linux kernel side, acpi_get_table() clones haven't been fully
balanced by acpi_put_table() invocations. In ACPICA side, due to the design
change, there are also unbalanced acpi_get_table_by_index() invocations
requiring special care.

So it is not a good timing to report acpi_get_table() counting errors. The
strict balanced validation count check should only be enabled after
confirming that all invocations are safe and compliant to their designed
purposes.

Thus this patch removes the fatal error along with the error report to
fix this issue. Reported by Dan Williams, fixed by Lv Zheng.

Fixes: 174cc7187e6f ("ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel")
Cc: <stable@vger.kernel.org>
Reported-by: Anush Seetharaman <anush.seetharaman@intel.com>
Reported-by: Dan Williams <dan.j.williams@intel.com>
Cc: Anush Seetharaman <anush.seetharaman@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/acpica/tbutils.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
index 5a968a7..7abe665 100644
--- a/drivers/acpi/acpica/tbutils.c
+++ b/drivers/acpi/acpica/tbutils.c
@@ -418,11 +418,7 @@ acpi_tb_get_table(struct acpi_table_desc *table_desc,
 
 	table_desc->validation_count++;
 	if (table_desc->validation_count == 0) {
-		ACPI_ERROR((AE_INFO,
-			    "Table %p, Validation count is zero after increment\n",
-			    table_desc));
 		table_desc->validation_count--;
-		return_ACPI_STATUS(AE_LIMIT);
 	}
 
 	*out_table = table_desc->pointer;
-- 
2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web