Re: [PATCH] ACPI: NFIT: limit string attribute write
On 05/07/2023 19:34, Dave Jiang wrote:
On 7/4/23 01:17, Ben Dooks wrote:
If we're writing what could be an arbitrary sized string into an
attribute
we should probably use snprintf() just to be safe. Most of the other
attriubtes are some sort of integer so unlikely to be an issue so not
altered at this time.
Signed-off-by: Ben Dooks
---
drivers/acpi/nfit/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 9213b426b125..d7e9d9cd16d2 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1579,7 +1579,7 @@ static ssize_t id_show(struct device *dev,
{
struct nfit_mem *nfit_mem = to_nfit_mem(dev);
- return sprintf(buf, "%s\n", nfit_mem->id);
+ return snprintf(buf, PAGE_SIZE, "%s\n", nfit_mem->id);
Why not just convert it to sysfs_emit()?
I'll look into that.
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
https://www.codethink.co.uk/privacy.html
[PATCH] ACPI: NFIT: limit string attribute write
If we're writing what could be an arbitrary sized string into an attribute
we should probably use sysfs_emit() just to be safe. Most of the other
attriubtes are some sort of integer so unlikely to be an issue so not
altered at this time.
Signed-off-by: Ben Dooks
---
v2:
- use sysfs_emit() instead of snprintf.
---
drivers/acpi/nfit/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 9213b426b125..d7e9d9cd16d2 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1579,7 +1579,7 @@ static ssize_t id_show(struct device *dev,
{
struct nfit_mem *nfit_mem = to_nfit_mem(dev);
- return sprintf(buf, "%s\n", nfit_mem->id);
+ return snprintf(buf, PAGE_SIZE, "%s\n", nfit_mem->id);
}
static DEVICE_ATTR_RO(id);
--
2.40.1
[PATCH v2] ACPI: NFIT: limit string attribute write
If we're writing what could be an arbitrary sized string into an attribute
we should probably use sysfs_emit() just to be safe. Most of the other
attriubtes are some sort of integer so unlikely to be an issue so not
altered at this time.
Signed-off-by: Ben Dooks
---
v2:
- use sysfs_emit() instead of snprintf.
---
drivers/acpi/nfit/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 9213b426b125..d7e9d9cd16d2 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1579,7 +1579,7 @@ static ssize_t id_show(struct device *dev,
{
struct nfit_mem *nfit_mem = to_nfit_mem(dev);
- return sprintf(buf, "%s\n", nfit_mem->id);
+ return snprintf(buf, PAGE_SIZE, "%s\n", nfit_mem->id);
}
static DEVICE_ATTR_RO(id);
--
2.40.1
Re: [PATCH] ACPI: NFIT: limit string attribute write
On 11/07/2023 10:22, Ben Dooks wrote:
If we're writing what could be an arbitrary sized string into an attribute
we should probably use sysfs_emit() just to be safe. Most of the other
attriubtes are some sort of integer so unlikely to be an issue so not
altered at this time.
Signed-off-by: Ben Dooks
---
v2:
- use sysfs_emit() instead of snprintf.
---
drivers/acpi/nfit/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 9213b426b125..d7e9d9cd16d2 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1579,7 +1579,7 @@ static ssize_t id_show(struct device *dev,
{
struct nfit_mem *nfit_mem = to_nfit_mem(dev);
- return sprintf(buf, "%s\n", nfit_mem->id);
+ return snprintf(buf, PAGE_SIZE, "%s\n", nfit_mem->id);
}
static DEVICE_ATTR_RO(id);
whoops, forgot to add before hitting ammend...
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
https://www.codethink.co.uk/privacy.html
Re: [PATCH 3/3] dax/kmem: Always enroll hotplugged memory for memmap_on_memory
David Hildenbrand writes:
> On 16.06.23 00:00, Vishal Verma wrote:
>> With DAX memory regions originating from CXL memory expanders or
>> NVDIMMs, the kmem driver may be hot-adding huge amounts of system memory
>> on a system without enough 'regular' main memory to support the memmap
>> for it. To avoid this, ensure that all kmem managed hotplugged memory is
>> added with the MHP_MEMMAP_ON_MEMORY flag to place the memmap on the
>> new memory region being hot added.
>>
>> To do this, call add_memory() in chunks of memory_block_size_bytes() as
>> that is a requirement for memmap_on_memory. Additionally, Use the
>> mhp_flag to force the memmap_on_memory checks regardless of the
>> respective module parameter setting.
>>
>> Cc: "Rafael J. Wysocki"
>> Cc: Len Brown
>> Cc: Andrew Morton
>> Cc: David Hildenbrand
>> Cc: Oscar Salvador
>> Cc: Dan Williams
>> Cc: Dave Jiang
>> Cc: Dave Hansen
>> Cc: Huang Ying
>> Signed-off-by: Vishal Verma
>> ---
>> drivers/dax/kmem.c | 49 -
>> 1 file changed, 36 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
>> index 7b36db6f1cbd..0751346193ef 100644
>> --- a/drivers/dax/kmem.c
>> +++ b/drivers/dax/kmem.c
>> @@ -12,6 +12,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include "dax-private.h"
>> #include "bus.h"
>>
>> @@ -105,6 +106,7 @@ static int dev_dax_kmem_probe(struct dev_dax *dev_dax)
>> data->mgid = rc;
>>
>> for (i = 0; i < dev_dax->nr_range; i++) {
>> +u64 cur_start, cur_len, remaining;
>> struct resource *res;
>> struct range range;
>>
>> @@ -137,21 +139,42 @@ static int dev_dax_kmem_probe(struct dev_dax *dev_dax)
>> res->flags = IORESOURCE_SYSTEM_RAM;
>>
>> /*
>> - * Ensure that future kexec'd kernels will not treat
>> - * this as RAM automatically.
>> + * Add memory in chunks of memory_block_size_bytes() so that
>> + * it is considered for MHP_MEMMAP_ON_MEMORY
>> + * @range has already been aligned to memory_block_size_bytes(),
>> + * so the following loop will always break it down cleanly.
>> */
>> -rc = add_memory_driver_managed(data->mgid, range.start,
>> -range_len(&range), kmem_name, MHP_NID_IS_MGID);
>> +cur_start = range.start;
>> +cur_len = memory_block_size_bytes();
>> +remaining = range_len(&range);
>> +while (remaining) {
>> +mhp_t mhp_flags = MHP_NID_IS_MGID;
>>
>> -if (rc) {
>> -dev_warn(dev, "mapping%d: %#llx-%#llx memory add
>> failed\n",
>> -i, range.start, range.end);
>> -remove_resource(res);
>> -kfree(res);
>> -data->res[i] = NULL;
>> -if (mapped)
>> -continue;
>> -goto err_request_mem;
>> +if (mhp_supports_memmap_on_memory(cur_len,
>> + MHP_MEMMAP_ON_MEMORY))
>> +mhp_flags |= MHP_MEMMAP_ON_MEMORY;
>> +/*
>> + * Ensure that future kexec'd kernels will not treat
>> + * this as RAM automatically.
>> + */
>> +rc = add_memory_driver_managed(data->mgid, cur_start,
>> + cur_len, kmem_name,
>> + mhp_flags);
>> +
>> +if (rc) {
>> +dev_warn(dev,
>> + "mapping%d: %#llx-%#llx memory add
>> failed\n",
>> + i, cur_start, cur_start + cur_len - 1);
>> +remove_resource(res);
>> +kfree(res);
>> +data->res[i] = NULL;
>> +if (mapped)
>> +continue;
>> +goto err_request_mem;
>> +}
>> +
>> +cur_start += cur_len;
>> +remaining -= cur_len;
>> }
>> mapped++;
>> }
>>
>
> Maybe the better alternative is teach
> add_memory_resource()/try_remove_memory() to do that internally.
>
> In the add_memory_resource() case, it might be a loop around that
> memmap_on_memory + arch_add_memory code path (well, and the error path
> also needs adjustment):
>
> /*
>* Self hosted memmap array
>*/
> if (mhp_flags & MHP_MEMMAP_ON_MEMORY) {
> if (!mhp_supports_memmap_on_memory(size)) {
> ret = -EINVAL;
> goto error;
> }
> mhp
Re: [PATCH 3/3] dax/kmem: Always enroll hotplugged memory for memmap_on_memory
On 11.07.23 16:30, Aneesh Kumar K.V wrote:
David Hildenbrand writes:
On 16.06.23 00:00, Vishal Verma wrote:
With DAX memory regions originating from CXL memory expanders or
NVDIMMs, the kmem driver may be hot-adding huge amounts of system memory
on a system without enough 'regular' main memory to support the memmap
for it. To avoid this, ensure that all kmem managed hotplugged memory is
added with the MHP_MEMMAP_ON_MEMORY flag to place the memmap on the
new memory region being hot added.
To do this, call add_memory() in chunks of memory_block_size_bytes() as
that is a requirement for memmap_on_memory. Additionally, Use the
mhp_flag to force the memmap_on_memory checks regardless of the
respective module parameter setting.
Cc: "Rafael J. Wysocki"
Cc: Len Brown
Cc: Andrew Morton
Cc: David Hildenbrand
Cc: Oscar Salvador
Cc: Dan Williams
Cc: Dave Jiang
Cc: Dave Hansen
Cc: Huang Ying
Signed-off-by: Vishal Verma
---
drivers/dax/kmem.c | 49 -
1 file changed, 36 insertions(+), 13 deletions(-)
diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c
index 7b36db6f1cbd..0751346193ef 100644
--- a/drivers/dax/kmem.c
+++ b/drivers/dax/kmem.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include "dax-private.h"
#include "bus.h"
@@ -105,6 +106,7 @@ static int dev_dax_kmem_probe(struct dev_dax *dev_dax)
data->mgid = rc;
for (i = 0; i < dev_dax->nr_range; i++) {
+ u64 cur_start, cur_len, remaining;
struct resource *res;
struct range range;
@@ -137,21 +139,42 @@ static int dev_dax_kmem_probe(struct dev_dax *dev_dax)
res->flags = IORESOURCE_SYSTEM_RAM;
/*
-* Ensure that future kexec'd kernels will not treat
-* this as RAM automatically.
+* Add memory in chunks of memory_block_size_bytes() so that
+* it is considered for MHP_MEMMAP_ON_MEMORY
+* @range has already been aligned to memory_block_size_bytes(),
+* so the following loop will always break it down cleanly.
*/
- rc = add_memory_driver_managed(data->mgid, range.start,
- range_len(&range), kmem_name, MHP_NID_IS_MGID);
+ cur_start = range.start;
+ cur_len = memory_block_size_bytes();
+ remaining = range_len(&range);
+ while (remaining) {
+ mhp_t mhp_flags = MHP_NID_IS_MGID;
- if (rc) {
- dev_warn(dev, "mapping%d: %#llx-%#llx memory add
failed\n",
- i, range.start, range.end);
- remove_resource(res);
- kfree(res);
- data->res[i] = NULL;
- if (mapped)
- continue;
- goto err_request_mem;
+ if (mhp_supports_memmap_on_memory(cur_len,
+ MHP_MEMMAP_ON_MEMORY))
+ mhp_flags |= MHP_MEMMAP_ON_MEMORY;
+ /*
+* Ensure that future kexec'd kernels will not treat
+* this as RAM automatically.
+*/
+ rc = add_memory_driver_managed(data->mgid, cur_start,
+ cur_len, kmem_name,
+ mhp_flags);
+
+ if (rc) {
+ dev_warn(dev,
+"mapping%d: %#llx-%#llx memory add
failed\n",
+i, cur_start, cur_start + cur_len - 1);
+ remove_resource(res);
+ kfree(res);
+ data->res[i] = NULL;
+ if (mapped)
+ continue;
+ goto err_request_mem;
+ }
+
+ cur_start += cur_len;
+ remaining -= cur_len;
}
mapped++;
}
Maybe the better alternative is teach
add_memory_resource()/try_remove_memory() to do that internally.
In the add_memory_resource() case, it might be a loop around that
memmap_on_memory + arch_add_memory code path (well, and the error path
also needs adjustment):
/*
* Self hosted memmap array
*/
if (mhp_flags & MHP_MEMMAP_ON_MEMORY) {
if (!mhp_supports_memmap_on_memory(size)) {
ret = -EINVAL;
goto error;
}
mhp_altmap.free = PHYS_PFN(size);
mhp_altmap.base_pfn = PHYS_PFN(start);
Re: [PATCH v2] ACPI: NFIT: limit string attribute write
On 7/11/23 02:37, Ben Dooks wrote:
If we're writing what could be an arbitrary sized string into an attribute
we should probably use sysfs_emit() just to be safe. Most of the other
attriubtes are some sort of integer so unlikely to be an issue so not
altered at this time.
Signed-off-by: Ben Dooks
---
v2:
- use sysfs_emit() instead of snprintf.
---
drivers/acpi/nfit/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 9213b426b125..d7e9d9cd16d2 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1579,7 +1579,7 @@ static ssize_t id_show(struct device *dev,
{
struct nfit_mem *nfit_mem = to_nfit_mem(dev);
- return sprintf(buf, "%s\n", nfit_mem->id);
+ return snprintf(buf, PAGE_SIZE, "%s\n", nfit_mem->id);
Doesn't look like you updated your patch with your new changes
}
static DEVICE_ATTR_RO(id);
