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

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

[Lv Zheng]

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

[PATCH v2 2/2] ACPI: Fix memory mapping leaks in current sysfs dumpable ACPI tables support.

[Lv Zheng]

This patch adds balanced acpi_get_table()/acpi_put_table() support for
sysfs table dumping code so that no need to call
acpi_get_validated_table().

Since ACPICA does not use all of the tables, this can help to reduce some
usless memory mappings by utilizing the new table handling APIs.

The original sysfs dumpable ACPI table implementation forces tables to be
mapped after a read operation and never unmaps them again whatever there
are no users in the kernel interested in these tables.  With new balanced
table handling APIs, tables are unmapped after the read operation.

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

diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 1b5ee1e..c3bb6ce 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -333,21 +333,34 @@ 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;
+	len = memory_read_from_buffer(buf, count, &offset,
+				      table_header, table_header->length);
+	acpi_put_table(table_header);
+	return len;
+}
+
+static bool acpi_table_has_multiple_instances(char *signature)
+{
+	acpi_status status;
+	struct acpi_table_header *header;
 
-	return memory_read_from_buffer(buf, count, &offset,
-				       table_header, table_header->length);
+	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];
 
@@ -368,9 +381,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);
@@ -419,11 +432,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)
@@ -435,24 +448,32 @@ static int acpi_tables_sysfs_init(void)
 
 	for (table_index = 0;; table_index++) {
 		status = acpi_get_table_by_index(table_index, &table_header);
-
 		if (status == AE_BAD_PARAMETER)
 			break;
-
 		if (ACPI_FAILURE(status))
-			continue;
+			goto next_table;
 
 		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

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

[Rafael J. Wysocki]

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

Re: [PATCH v2 2/2] ACPI: Fix memory mapping leaks in current sysfs dumpable ACPI tables support.

[Rafael J. Wysocki]

On Thursday, April 27, 2017 04:22:50 PM Lv Zheng wrote:
> This patch adds balanced acpi_get_table()/acpi_put_table() support for
> sysfs table dumping code so that no need to call
> acpi_get_validated_table().
> 
> Since ACPICA does not use all of the tables, this can help to reduce some
> usless memory mappings by utilizing the new table handling APIs.
> 
> The original sysfs dumpable ACPI table implementation forces tables to be
> mapped after a read operation and never unmaps them again whatever there
> are no users in the kernel interested in these tables.  With new balanced
> table handling APIs, tables are unmapped after the read operation.
> 
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> ---
>  drivers/acpi/sysfs.c | 51 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 36 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
> index 1b5ee1e..c3bb6ce 100644
> --- a/drivers/acpi/sysfs.c
> +++ b/drivers/acpi/sysfs.c
> @@ -333,21 +333,34 @@ 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;
> +	len = memory_read_from_buffer(buf, count, &offset,
> +				      table_header, table_header->length);
> +	acpi_put_table(table_header);
> +	return len;
> +}

The above seems to be taken verbatim from the Dan's patch.

If that's the case, please do the below as a separate patch on top of the Dan's
one.

> +
> +static bool acpi_table_has_multiple_instances(char *signature)
> +{
> +	acpi_status status;
> +	struct acpi_table_header *header;
>  
> -	return memory_read_from_buffer(buf, count, &offset,
> -				       table_header, table_header->length);
> +	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];
>  
> @@ -368,9 +381,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);
> @@ -419,11 +432,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)
> @@ -435,24 +448,32 @@ static int acpi_tables_sysfs_init(void)
>  
>  	for (table_index = 0;; table_index++) {
>  		status = acpi_get_table_by_index(table_index, &table_header);
> -
>  		if (status == AE_BAD_PARAMETER)
>  			break;
> -
>  		if (ACPI_FAILURE(status))
> -			continue;
> +			goto next_table;
>  
>  		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);
> 

Thanks,
Rafael

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

[Zheng, Lv]

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

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

[Zheng, Lv]

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

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

[Lv Zheng]

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

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

[Lv Zheng]

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

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

[Lv Zheng]

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

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

[Lv Zheng]

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

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

[Lv Zheng]

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

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

[Rafael J. Wysocki]

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

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

[Zheng, Lv]

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

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

[Dan Williams]

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?

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

[Zheng, Lv]

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

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

[Rafael J. Wysocki]

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

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

[Zheng, Lv]

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

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

[Lv Zheng]

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

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

[Lv Zheng]

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

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

[Lv Zheng]

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

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

[Lv Zheng]

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

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

[Rafael J. Wysocki]

On Tuesday, May 09, 2017 01:57:41 PM Lv Zheng wrote:
> 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) {

Is this going to ever trigger?

We've already verified that validation_count is not 0 and that it is less than
ACPI_MAX_TABLE_VALIDATIONS and we have decremented it, so how can it be
greater than or equal to ACPI_MAX_TABLE_VALIDATIONS here?

> +			ACPI_WARNING((AE_INFO,
> +				      "Table %p, Validation count underflows\n",
> +				      table_desc));
> +			return_VOID;
> +		}
>  	}
> -	table_desc->validation_count--;
>  
>  	if (table_desc->validation_count == 0) {
>

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Friday, May 12, 2017 11:03:52 PM Rafael J. Wysocki wrote:
> On Tuesday, May 09, 2017 01:57:41 PM Lv Zheng wrote:
> > 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) {
> 
> Is this going to ever trigger?
> 
> We've already verified that validation_count is not 0 and that it is less than
> ACPI_MAX_TABLE_VALIDATIONS and we have decremented it, so how can it be
> greater than or equal to ACPI_MAX_TABLE_VALIDATIONS here?

Wrong question, sorry.

I think that the check is in case validation_count was 0 before the decrementation,
right?

So then, I'd still check if validation_count == 0 and if so, set it to
ACPI_MAX_TABLE_VALIDATIONS.

Next, if validation_count => ACPI_MAX_TABLE_VALIDATIONS, I'd print the warning
message and return.

Then, the decrementation would not underflow, so it would be safe to do it.

Wouldn't that be somewhat easier to follow?

> > +			ACPI_WARNING((AE_INFO,
> > +				      "Table %p, Validation count underflows\n",
> > +				      table_desc));
> > +			return_VOID;
> > +		}
> >  	}
> > -	table_desc->validation_count--;
> >  
> >  	if (table_desc->validation_count == 0) {
> >

Thanks,
Rafael

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

[Zheng, Lv]

Hi, Rafael

> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Subject: Re: [PATCH v4 2/4] ACPICA: Tables: Add mechanism to allow to balance late stage
> acpi_get_table() independently
> 
> On Tuesday, May 09, 2017 01:57:41 PM Lv Zheng wrote:
> > 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) {
> 
> Is this going to ever trigger?
> 
> We've already verified that validation_count is not 0 and that it is less than
> ACPI_MAX_TABLE_VALIDATIONS and we have decremented it, so how can it be
> greater than or equal to ACPI_MAX_TABLE_VALIDATIONS here?

This is just a no-op change equivalent to
 "
  if (validation_count == 0) { warn and return }
  decrement
 "
It expands "decrement" to "validation_count == 0" case so that it can implement warn_once for the warning message.

See:
A. validation_count == 0:
   A.1. "if (validation_count < ACPI_MAX_TABLE_VALIDATIONS)" matches, and
        After decrementing validation_count, it will be "0xFFFF";
        Then "if (validation_count >= ACPI_MAX_TABLE_VALIDATIONS)" matches as "validation_count == 0xFFFF(ACPI_MAX_TABLE_VALIDATIONS)" now;
        The warning message is printed;
   A.2. "if (validation_count == ACPI_MAX_TABLE_VALIDATIONS)" doesn't match as "validation_count == 0xFFFF(ACPI_MAX_TABLE_VALIDATIONS)" now
        the rest of this function will be skipped just like return_VOID.
B. validation_count == 0xFFFF:
   A.1. Both acpi_tb_get_table() and acpi_tb_put_table() won't be able to change validation_count as
        validation_count increment/decrement code fragments are only executed "if (validation_count < ACPI_MAX_TABLE_VALIDATIONS)"
        Thus validation_count is kept as 0xFFFF (in this case, overflowed/underflowed values are same).
   A.2. "if (validation_count == ACPI_MAX_TABLE_VALIDATIONS)" doesn't match as "validation_count == 0xFFFF" now
        the rest of this function will be skipped just like return_VOID.
C. otherwise, validation_count will be decremented like old code

Benefits of using the new algorithm are:
1. ACPI_MAX_TABLE_VALIDATIONS can be something other than 0xFFFF.
   It can be anything now, and the expected behavior can always be ensured.
   IOW, the new algorithm actually supports cases where overflowed/underflowed values are not same.
   You can check this by defining ACPI_MAX_TABLE_VALIDATIONS to 8.
   And you'll see that the 2 functions are still working.
   1.1. If something is broken in acpi_tb_get_table(), validation_count will be kept as 0x0008.
   1.2. If something is broken in acpi_tb_put_table(), validation_count will be kept as 0xFFFF.
        Both 0x0008 and 0xFFFF cannot make
         "if (validation_count < ACPI_MAX_TABLE_VALIDATIONS)" and
         "if (validation_count == 0)" to return true, and
        Thus validation_count is kept unchanged after overflow/underflow.
2. The key benefit of this change is to make the old warning in acpi_tb_put_table() as warn_once.
   For example:
     acpi_get_table();
     for (i = 0; i < 100; i++)
       acpi_put_table()
   Using the old algorithm, the acpi_tb_put_table() warning message will be seen 99 times.
   Using the new algorithm, the acpi_tb_put_table() warning message will be seen only once.
3. logics in acpi_tb_put_table() will be exactly the reversal of the logics in acpi_tb_get_table().
   It'll be easier to maintain both of them with the new overflow/underflow algorithm.
Hope you'll like such a change.

Thanks and best regards
Lv

> 
> > +			ACPI_WARNING((AE_INFO,
> > +				      "Table %p, Validation count underflows\n",
> > +				      table_desc));
> > +			return_VOID;
> > +		}
> >  	}
> > -	table_desc->validation_count--;
> >
> >  	if (table_desc->validation_count == 0) {
> >
> 
> Thanks,
> Rafael

[PATCH v5] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently

[Lv Zheng]

Considering this case:
1. A program opens a sysfs table file 65535 times, it can increase
   validation_count and first increment cause the table to be mapped:
    validation_count = 65535
2. AML execution causes "Load" to be executed on the same table, this time
   it cannot increase validation_count, so validation_count remains:
    validation_count = 65535
3. The program closes sysfs table file 65535 times, it can decrease
   validation_count and the last decrement cause the table to be unmapped:
    validation_count = 0
4. AML code still accessing the loaded table, kernel crash can be observed.

This is because orginally ACPICA doesn't support unmapping tables during
OS late stage. So the current code only allows unmapping tables during OS
early stage, and for late stage, no acpi_put_table() clones should be
invoked, especially cases that can trigger frequent invocations of
acpi_get_table()/acpi_put_table() are forbidden:
1. sysfs table accesses
2. dynamic Load/Unload opcode executions
3. acpi_load_table()
4. etc.
Such frequent acpi_put_table() balance changes have to be done altogether.

This philosophy is not convenient for Linux driver writers. Since the API
is just there, developers will start to use acpi_put_table() during late
stage. So we need to consider a better mechanism to allow them to safely
invoke acpi_put_table().

This patch provides such a mechanism by adding 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.

Note: code added in acpi_tb_put_table() is actually a no-op but changes the
warning message into a warning once message. Lv Zheng.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/acpica/tbutils.c | 36 +++++++++++++++++++++++++++---------
 include/acpi/actbl.h          | 13 +++++++++++++
 2 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
index cd96026..4048523 100644
--- a/drivers/acpi/acpica/tbutils.c
+++ b/drivers/acpi/acpica/tbutils.c
@@ -416,9 +416,19 @@ 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++;
+
+		/*
+		 * Ensure the warning message can only be displayed once. The
+		 * warning message occurs when the "get" operations are performed
+		 * more than "put" operations, causing count overflow.
+		 */
+		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 +455,21 @@ 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--;
+
+		/*
+		 * Ensure the warning message can only be displayed once. The
+		 * warning message occurs when the "put" operations are performed
+		 * more than "get" operations, causing count underflow.
+		 */
+		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..f42e6d5 100644
--- a/include/acpi/actbl.h
+++ b/include/acpi/actbl.h
@@ -374,6 +374,19 @@ 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.
+ * The maximum validation count can be defined to any value, but should be
+ * greater than the maximum number of OS early stage mapping slots as it
+ * must be ensured that no early stage mappings can be leaked to the late
+ * stage.
+ */
+#define ACPI_MAX_TABLE_VALIDATIONS          ACPI_UINT16_MAX
+
 /* Masks for Flags field above */
 
 #define ACPI_TABLE_ORIGIN_EXTERNAL_VIRTUAL  (0)	/* Virtual address, external maintained */
-- 
2.7.4

Re: [PATCH v5] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently

[Dan Williams]

On Tue, Jun 6, 2017 at 9:54 PM, Lv Zheng <lv.zheng@intel.com> wrote:
> Considering this case:
> 1. A program opens a sysfs table file 65535 times, it can increase
>    validation_count and first increment cause the table to be mapped:
>     validation_count = 65535
> 2. AML execution causes "Load" to be executed on the same table, this time
>    it cannot increase validation_count, so validation_count remains:
>     validation_count = 65535
> 3. The program closes sysfs table file 65535 times, it can decrease
>    validation_count and the last decrement cause the table to be unmapped:
>     validation_count = 0
> 4. AML code still accessing the loaded table, kernel crash can be observed.
>
> This is because orginally ACPICA doesn't support unmapping tables during
> OS late stage. So the current code only allows unmapping tables during OS
> early stage, and for late stage, no acpi_put_table() clones should be
> invoked, especially cases that can trigger frequent invocations of
> acpi_get_table()/acpi_put_table() are forbidden:
> 1. sysfs table accesses
> 2. dynamic Load/Unload opcode executions
> 3. acpi_load_table()
> 4. etc.
> Such frequent acpi_put_table() balance changes have to be done altogether.
>
> This philosophy is not convenient for Linux driver writers. Since the API
> is just there, developers will start to use acpi_put_table() during late
> stage. So we need to consider a better mechanism to allow them to safely
> invoke acpi_put_table().
>
> This patch provides such a mechanism by adding 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.
>
> Note: code added in acpi_tb_put_table() is actually a no-op but changes the
> warning message into a warning once message. Lv Zheng.
>

This still seems to be unnecessary gymnastics to keep the validation
count around and make it work for random drivers. Which ACPI tables
might be hot removed? If it's only a small handful of tables why not
teach the code that handles those exceptional cases to manage a
dedicated reference count mechanism? That way the other cases can be
left alone and not worry about balancing their references.

Re: [PATCH v5] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently

[Rafael J. Wysocki]

On Wed, Jun 7, 2017 at 8:41 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Tue, Jun 6, 2017 at 9:54 PM, Lv Zheng <lv.zheng@intel.com> wrote:
>> Considering this case:
>> 1. A program opens a sysfs table file 65535 times, it can increase
>>    validation_count and first increment cause the table to be mapped:
>>     validation_count = 65535
>> 2. AML execution causes "Load" to be executed on the same table, this time
>>    it cannot increase validation_count, so validation_count remains:
>>     validation_count = 65535
>> 3. The program closes sysfs table file 65535 times, it can decrease
>>    validation_count and the last decrement cause the table to be unmapped:
>>     validation_count = 0
>> 4. AML code still accessing the loaded table, kernel crash can be observed.
>>
>> This is because orginally ACPICA doesn't support unmapping tables during
>> OS late stage. So the current code only allows unmapping tables during OS
>> early stage, and for late stage, no acpi_put_table() clones should be
>> invoked, especially cases that can trigger frequent invocations of
>> acpi_get_table()/acpi_put_table() are forbidden:
>> 1. sysfs table accesses
>> 2. dynamic Load/Unload opcode executions
>> 3. acpi_load_table()
>> 4. etc.
>> Such frequent acpi_put_table() balance changes have to be done altogether.
>>
>> This philosophy is not convenient for Linux driver writers. Since the API
>> is just there, developers will start to use acpi_put_table() during late
>> stage. So we need to consider a better mechanism to allow them to safely
>> invoke acpi_put_table().
>>
>> This patch provides such a mechanism by adding 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.
>>
>> Note: code added in acpi_tb_put_table() is actually a no-op but changes the
>> warning message into a warning once message. Lv Zheng.
>>
>
> This still seems to be unnecessary gymnastics to keep the validation
> count around and make it work for random drivers.

Well, I'm not sure I agree here.

If we can make it work at one point, it should not be too hard to
maintain that status.

Thanks,
Rafael

Re: [PATCH v5] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently

[Dan Williams]

On Wed, Jun 7, 2017 at 2:14 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Jun 7, 2017 at 8:41 AM, Dan Williams <dan.j.williams@intel.com> wrote:
>> On Tue, Jun 6, 2017 at 9:54 PM, Lv Zheng <lv.zheng@intel.com> wrote:
>>> Considering this case:
>>> 1. A program opens a sysfs table file 65535 times, it can increase
>>>    validation_count and first increment cause the table to be mapped:
>>>     validation_count = 65535
>>> 2. AML execution causes "Load" to be executed on the same table, this time
>>>    it cannot increase validation_count, so validation_count remains:
>>>     validation_count = 65535
>>> 3. The program closes sysfs table file 65535 times, it can decrease
>>>    validation_count and the last decrement cause the table to be unmapped:
>>>     validation_count = 0
>>> 4. AML code still accessing the loaded table, kernel crash can be observed.
>>>
>>> This is because orginally ACPICA doesn't support unmapping tables during
>>> OS late stage. So the current code only allows unmapping tables during OS
>>> early stage, and for late stage, no acpi_put_table() clones should be
>>> invoked, especially cases that can trigger frequent invocations of
>>> acpi_get_table()/acpi_put_table() are forbidden:
>>> 1. sysfs table accesses
>>> 2. dynamic Load/Unload opcode executions
>>> 3. acpi_load_table()
>>> 4. etc.
>>> Such frequent acpi_put_table() balance changes have to be done altogether.
>>>
>>> This philosophy is not convenient for Linux driver writers. Since the API
>>> is just there, developers will start to use acpi_put_table() during late
>>> stage. So we need to consider a better mechanism to allow them to safely
>>> invoke acpi_put_table().
>>>
>>> This patch provides such a mechanism by adding 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.
>>>
>>> Note: code added in acpi_tb_put_table() is actually a no-op but changes the
>>> warning message into a warning once message. Lv Zheng.
>>>
>>
>> This still seems to be unnecessary gymnastics to keep the validation
>> count around and make it work for random drivers.
>
> Well, I'm not sure I agree here.
>
> If we can make it work at one point, it should not be too hard to
> maintain that status.
>

I agree with that, my concern was with driver writers needing to be
worried about when it is safe to call acpi_put_table(). This reference
count behaves differently than other reference counts like kobjects.
The difference is not necessarily bad, but hopefully it can be
contained within the acpi core.

RE: [PATCH v5] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table() independently

[Zheng, Lv]

Hi, Dan

> From: Dan Williams [mailto:dan.j.williams@intel.com]
> Subject: Re: [PATCH v5] ACPICA: Tables: Add mechanism to allow to balance late stage acpi_get_table()
> independently
> 
> On Wed, Jun 7, 2017 at 2:14 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Jun 7, 2017 at 8:41 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> >> On Tue, Jun 6, 2017 at 9:54 PM, Lv Zheng <lv.zheng@intel.com> wrote:
> >>> Considering this case:
> >>> 1. A program opens a sysfs table file 65535 times, it can increase
> >>>    validation_count and first increment cause the table to be mapped:
> >>>     validation_count = 65535
> >>> 2. AML execution causes "Load" to be executed on the same table, this time
> >>>    it cannot increase validation_count, so validation_count remains:
> >>>     validation_count = 65535
> >>> 3. The program closes sysfs table file 65535 times, it can decrease
> >>>    validation_count and the last decrement cause the table to be unmapped:
> >>>     validation_count = 0
> >>> 4. AML code still accessing the loaded table, kernel crash can be observed.
> >>>
> >>> This is because orginally ACPICA doesn't support unmapping tables during
> >>> OS late stage. So the current code only allows unmapping tables during OS
> >>> early stage, and for late stage, no acpi_put_table() clones should be
> >>> invoked, especially cases that can trigger frequent invocations of
> >>> acpi_get_table()/acpi_put_table() are forbidden:
> >>> 1. sysfs table accesses
> >>> 2. dynamic Load/Unload opcode executions
> >>> 3. acpi_load_table()
> >>> 4. etc.
> >>> Such frequent acpi_put_table() balance changes have to be done altogether.
> >>>
> >>> This philosophy is not convenient for Linux driver writers. Since the API
> >>> is just there, developers will start to use acpi_put_table() during late
> >>> stage. So we need to consider a better mechanism to allow them to safely
> >>> invoke acpi_put_table().
> >>>
> >>> This patch provides such a mechanism by adding 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.
> >>>
> >>> Note: code added in acpi_tb_put_table() is actually a no-op but changes the
> >>> warning message into a warning once message. Lv Zheng.
> >>>
> >>
> >> This still seems to be unnecessary gymnastics to keep the validation
> >> count around and make it work for random drivers.
> >
> > Well, I'm not sure I agree here.
> >
> > If we can make it work at one point, it should not be too hard to
> > maintain that status.
> >
> 
> I agree with that, my concern was with driver writers needing to be
> worried about when it is safe to call acpi_put_table(). This reference
> count behaves differently than other reference counts like kobjects.

I don't think they behave differently.

"kref" needn't consider unbalanced "get/put".
Because when the drivers(users) are deploying "kref",
they are responsible for ensuring balanced "get/put".
"kref" needn't take too much care about "overflow/underflow"
as if all users ensure balanced "get/put",
"overflow/underflow" is not possible.
Occurrence of "overflow/underflow" means bugs.
And can be further captured as "panic".

If "kref" considers to "warn_once" overflow/underflow users,
the logic in this commit can also be introduced to kref.
However it's useless as all users have ensured balanced "get/put".
Putting useless check than panic on hot path could be a waste.

> The difference is not necessarily bad, but hopefully it can be
> contained within the acpi core.

The old warning logic for table desc is just derived from utdelete.c.
Which reduces communication cost when the mechanism is upstreamed.

ACPICA table "validation_count" is deployed on top of old design.
Where "table unmap" is forbidden for late stage.
Thus there is no users ensuring balanced "get/put".
Under this circumstances, when we start to deploy balanced "get/put",
we need to consider all users as a whole.
You cannot say current unbalanced "get/put" users have bugs.
They are there just because of historical reasons.

Fortunately after applying this patch,
drivers should be able to have a better environment to use the new APIs.

Cheers,
Lv

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

[Rafael J. Wysocki]

On Tuesday, May 09, 2017 01:57:54 PM Lv Zheng wrote:
> 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;
> +}

To be honest, I'm not convinced this is the best way to do that.

AFAICS there's no guarantee that the second instance would not go away after it
had been found and before this returned.

Thanks,
Rafael