lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 28 Dec 2017 11:04:57 +0800
From:   Chao Fan <fanc.fnst@...fujitsu.com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     <linux-kernel@...r.kernel.org>, <bhe@...hat.com>,
        <tokunaga.keiich@...fujitsu.com>, <douly.fnst@...fujitsu.com>
Subject: Re: [RFC PATCH] memory-hotplug: add sysfs immovable_mem attribute

On Wed, Dec 27, 2017 at 01:47:26PM +0100, Greg KH wrote:
>On Wed, Dec 27, 2017 at 08:30:12PM +0800, Chao Fan wrote:
>> In sometimes users specify the memory region in immovable node in
>> some kernel commandline, such as "kernel_core" or the "immovable_mem="
>> in the patchset that I have send. But users don't know the memory
>> region. So add this interface to print it.
>> 
>> It will show like this: "nn@ss,nn@ss,...". "nn" means the size of memory
>> region, "ss" means the start position of this region.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@...fujitsu.com>
>> ---
>>  drivers/base/memory.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 50 insertions(+)

Hi Greg,

Thanks for your review.

>
>Why did you not also create the needed Documentation/ABI/ file update?

Sorry, I didn't notice the document issues.

>
>That's required for sysfs attributes.
>
>> 
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index 1d60b58a8c19..9cadf1a9dccb 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -25,6 +25,7 @@
>>  
>>  #include <linux/atomic.h>
>>  #include <linux/uaccess.h>
>> +#include <linux/acpi.h>
>>  
>>  static DEFINE_MUTEX(mem_sysfs_mutex);
>>  
>> @@ -389,6 +390,52 @@ static ssize_t show_phys_device(struct device *dev,
>>  }
>>  
>>  #ifdef CONFIG_MEMORY_HOTREMOVE
>> +/*
>> + * Immovable memory region
>> + */
>> +
>> +static ssize_t
>> +show_immovable_mem(struct device *dev, struct device_attribute *attr,
>> +		   char *buf)
>> +{
>> +	struct acpi_table_header *table_header = NULL;
>> +	struct acpi_srat_mem_affinity *ma;
>> +	struct acpi_subtable_header *th;
>> +	unsigned long long table_size;
>> +	unsigned long long table_end;
>> +	char pbuf[35], *p = buf;
>> +	int len;
>> +
>> +	acpi_get_table(ACPI_SIG_SRAT, 0, &table_header);
>> +
>> +	table_size = sizeof(struct acpi_table_srat);
>> +	table_end = (unsigned long)table_header + table_header->length;
>> +	th = (struct acpi_subtable_header *)((unsigned long)
>> +	      table_header + table_size);
>> +
>> +	while (((unsigned long)th) +
>> +	       sizeof(struct acpi_subtable_header) < table_end) {
>> +		if (th->type == 1) {
>> +			ma = (struct acpi_srat_mem_affinity *)th;
>> +			if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)
>> +				continue;
>> +			len = sprintf(pbuf, "%llx@...x",
>> +			       ma->length, ma->base_address);
>
>sysfs is "one value per file", and if you ever have to care if you are
>overrunning the length of the buffer, that's a huge hint you are doing

Yes, you are right, the length of "buf" should be limited here.
I saw in this file, one sysfs only has one value here. But the memory
has more than one region and may be discontinuous, so I stitch the string.
So should I send the new version, or you think this feature should not
be placed here?

Thanks,
Chao Fan

>something wrong here.
>
>sorry,
>
>greg k-h
>
>


Powered by blists - more mailing lists