linux-kernel - Re: [v2 PATCH 1/2] mm: mempolicy: make the behavior consistent when MPOL_MF_MOVE* and MPOL_MF

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:   Wed, 17 Jul 2019 12:55:59 +0200
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Yang Shi <yang.shi@...ux.alibaba.com>, mhocko@...nel.org,
        mgorman@...hsingularity.net, akpm@...ux-foundation.org
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [v2 PATCH 1/2] mm: mempolicy: make the behavior consistent when
 MPOL_MF_MOVE* and MPOL_MF_STRICT were specified

On 7/16/19 7:18 PM, Yang Shi wrote:
>> I think after your patch, you miss putback_movable_pages() in cases
>> where some were queued, and later the walk returned -EIO. The previous
>> code doesn't miss it, but it's also not obvious due to the multiple if
>> (!err) checks. I would rewrite it some thing like this:
>>
>> if (ret < 0) {
>>      putback_movable_pages(&pagelist);
>>      err = ret;
>>      goto mmap_out; // a new label above up_write()
>> }
> 
> Yes, the old code had putback_movable_pages called when !err. But, I 
> think that is for error handling of mbind_range() if I understand it 
> correctly since if queue_pages_range() returns -EIO (only MPOL_MF_STRICT 
> was specified and there was misplaced page) that page list should be 
> empty . The old code should checked whether that list is empty or not.

Hm I guess you're right, returning with EIO means nothing was queued.
> So, in the new code I just removed that.
> 
>>
>> The rest can have reduced identation now.
> 
> Yes, the goto does eliminate the extra indentation.
> 
>>
>>> +	else {
>>> +		err = mbind_range(mm, start, end, new);
>>>   
>>> -		if (nr_failed && (flags & MPOL_MF_STRICT))
>>> -			err = -EIO;
>>> -	} else
>>> -		putback_movable_pages(&pagelist);
>>> +		if (!err) {
>>> +			int nr_failed = 0;
>>> +
>>> +			if (!list_empty(&pagelist)) {
>>> +				WARN_ON_ONCE(flags & MPOL_MF_LAZY);
>>> +				nr_failed = migrate_pages(&pagelist, new_page,
>>> +					NULL, start, MIGRATE_SYNC,
>>> +					MR_MEMPOLICY_MBIND);
>>> +				if (nr_failed)
>>> +					putback_movable_pages(&pagelist);
>>> +			}
>>> +
>>> +			if ((ret > 0) ||
>>> +			    (nr_failed && (flags & MPOL_MF_STRICT)))
>>> +				err = -EIO;
>>> +		} else
>>> +			putback_movable_pages(&pagelist);
>> While at it, IIRC the kernel style says that when the 'if' part uses
>> '{ }' then the 'else' part should as well, and it shouldn't be mixed.
> 
> Really? The old code doesn't have '{ }' for else, and checkpatch doesn't 
> report any error or warning.

Checkpatch probably doesn't catch it, nor did the reviewers of the older
code. But coding-style.rst says:

Do not unnecessarily use braces where a single statement will do.

...

This does not apply if only one branch of a conditional statement is a
single
statement; in the latter case use braces in both branches:

.. code-block:: c

        if (condition) {
                do_this();
                do_that();
        } else {
                otherwise();
        }


Thanks,
Vlastimil