linux-kernel - Re: memory_hotplug: zone_can_shift() returns boolean value

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] [day] [month] [year] [list]

Message-ID: <598c13a6-81d8-e591-0239-ab403306e382@gmail.com>
Date:   Tue, 13 Dec 2016 11:47:04 -0500
From:   Yasuaki Ishimatsu <yasu.isimatu@...il.com>
To:     Reza Arbab <arbab@...ux.vnet.ibm.com>
Cc:     linux-mm@...ck.org, isimatu.yasuaki@...fujitsu.com,
        linux-kernel@...r.kernel.org
Subject: Re: memory_hotplug: zone_can_shift() returns boolean value

Hi Reza,

Thank you for your review.

On 12/13/2016 10:54 AM, Reza Arbab wrote:
> On Mon, Dec 12, 2016 at 03:29:04PM -0500, Yasuaki Ishimatsu wrote:
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -410,15 +410,13 @@ static ssize_t show_valid_zones(struct device *dev,
>>     sprintf(buf, "%s", zone->name);
>>
>>     /* MMOP_ONLINE_KERNEL */
>> -    zone_shift = zone_can_shift(start_pfn, nr_pages, ZONE_NORMAL);
>> -    if (zone_shift) {
>> +    if (zone_can_shift(start_pfn, nr_pages, ZONE_NORMAL, &zone_shift)) {
>>         strcat(buf, " ");
>>         strcat(buf, (zone + zone_shift)->name);
>>     }
>>
>>     /* MMOP_ONLINE_MOVABLE */
>> -    zone_shift = zone_can_shift(start_pfn, nr_pages, ZONE_MOVABLE);
>> -    if (zone_shift) {
>> +    if (zone_can_shift(start_pfn, nr_pages, ZONE_MOVABLE, &zone_shift)) {
>>         strcat(buf, " ");
>>         strcat(buf, (zone + zone_shift)->name);
>>     }
>
> You still need to check zone_shift != 0, otherwise you may get duplicate output:
>
> $ cat /sys/devices/system/node/node1/memory256/valid_zones
> Movable Normal Movable
> $ cat /sys/devices/system/node/node1/memory257/valid_zones
> Movable Movable

I'll update it.

>
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1033,8 +1033,8 @@ static void node_states_set_node(int node, struct memory_notify *arg)
>>     node_set_state(node, N_MEMORY);
>> }
>>
>> -int zone_can_shift(unsigned long pfn, unsigned long nr_pages,
>> -           enum zone_type target)
>> +bool zone_can_shift(unsigned long pfn, unsigned long nr_pages,
>> +           enum zone_type target, int *zone_shift)
>> {
>>     struct zone *zone = page_zone(pfn_to_page(pfn));
>>     enum zone_type idx = zone_idx(zone);
>
> I think you should initialize zone_shift here. It should be 0 if the function returns false.
>
>     *zone_shift = 0;

I'll update it.

Thanks,
Yasuaki Ishimatsu

>> @@ -1043,26 +1043,27 @@ int zone_can_shift(unsigned long pfn, unsigned long nr_pages,
>>     if (idx < target) {
>>         /* pages must be at end of current zone */
>>         if (pfn + nr_pages != zone_end_pfn(zone))
>> -            return 0;
>> +            return false;
>>
>>         /* no zones in use between current zone and target */
>>         for (i = idx + 1; i < target; i++)
>>             if (zone_is_initialized(zone - idx + i))
>> -                return 0;
>> +                return false;
>>     }
>>
>>     if (target < idx) {
>>         /* pages must be at beginning of current zone */
>>         if (pfn != zone->zone_start_pfn)
>> -            return 0;
>> +            return false;
>>
>>         /* no zones in use between current zone and target */
>>         for (i = target + 1; i < idx; i++)
>>             if (zone_is_initialized(zone - idx + i))
>> -                return 0;
>> +                return false;
>>     }
>>
>> -    return target - idx;
>> +    *zone_shift = target - idx;
>> +    return true;
>> }
>>
>> /* Must be protected by mem_hotplug_begin() */
>> @@ -1089,10 +1090,13 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
>>         !can_online_high_movable(zone))
>>         return -EINVAL;
>>
>> -    if (online_type == MMOP_ONLINE_KERNEL)
>> -        zone_shift = zone_can_shift(pfn, nr_pages, ZONE_NORMAL);
>> -    else if (online_type == MMOP_ONLINE_MOVABLE)
>> -        zone_shift = zone_can_shift(pfn, nr_pages, ZONE_MOVABLE);
>> +    if (online_type == MMOP_ONLINE_KERNEL) {
>> +        if (!zone_can_shift(pfn, nr_pages, ZONE_NORMAL, &zone_shift))
>> +            return -EINVAL;
>> +    } else if (online_type == MMOP_ONLINE_MOVABLE) {
>> +        if (!zone_can_shift(pfn, nr_pages, ZONE_MOVABLE, &zone_shift))
>> +            return -EINVAL;
>> +    }
>>
>>     zone = move_pfn_range(zone_shift, pfn, pfn + nr_pages);
>>     if (!zone)
>> --
>> 1.8.3.1
>>
>