linux-kernel - Re: [PATCHv3 1/8] powerpc/vdso: unify return paths in setup_additional

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]

Message-ID: <4fac19b7-aeec-f2b7-02fe-85c219151c3b@virtuozzo.com>
Date:   Tue, 8 Nov 2016 15:29:14 +0300
From:   Dmitry Safonov <dsafonov@...tuozzo.com>
To:     Michael Ellerman <mpe@...erman.id.au>,
        <linux-kernel@...r.kernel.org>
CC:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Andy Lutomirski <luto@...capital.net>,
        Oleg Nesterov <oleg@...hat.com>,
        <linuxppc-dev@...ts.ozlabs.org>, <linux-mm@...ck.org>
Subject: Re: [PATCHv3 1/8] powerpc/vdso: unify return paths in
 setup_additional_pages

On 11/08/2016 03:10 AM, Michael Ellerman wrote:
> Hi Dmitry,
>
> Thanks for the patches.
>
> Dmitry Safonov <dsafonov@...tuozzo.com> writes:
>> Impact: cleanup
>
> I'm not a fan of these "Impact" lines, especially when they're not
> correct, ie. this is not a cleanup, a cleanup doesn't change logic.
>
>> Rename `rc' variable which doesn't seems to mean anything into
>> kernel-known `ret'.
>
> 'rc' means "Return Code", it's fairly common. I see at least ~8500
> "int rc" declarations in the kernel.
>
> Please don't rename variables and change logic in one patch.

Ok, right - just didn't saw `rc' that freq as `ret'.
Will leave the name.

>> Combine two function returns into one as it's
>> also easier to read.
>>
>> Cc: Benjamin Herrenschmidt <benh@...nel.crashing.org>
>> Cc: Paul Mackerras <paulus@...ba.org>
>> Cc: Michael Ellerman <mpe@...erman.id.au>
>> Cc: Andy Lutomirski <luto@...capital.net>
>> Cc: Oleg Nesterov <oleg@...hat.com>
>> Cc: linuxppc-dev@...ts.ozlabs.org
>> Cc: linux-mm@...ck.org
>> Signed-off-by: Dmitry Safonov <dsafonov@...tuozzo.com>
>> ---
>>  arch/powerpc/kernel/vdso.c | 19 +++++++------------
>>  1 file changed, 7 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
>> index 4111d30badfa..4ffb82a2d9e9 100644
>> --- a/arch/powerpc/kernel/vdso.c
>> +++ b/arch/powerpc/kernel/vdso.c
>> @@ -154,7 +154,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
>>  	struct page **vdso_pagelist;
>>  	unsigned long vdso_pages;
>>  	unsigned long vdso_base;
>> -	int rc;
>> +	int ret = 0;
>
> Please don't initialise return codes in the declaration, it prevents the
> compiler from warning you if you forget to initialise it in a
> particular path.
>
> AFAICS you never even use the default value either.

Oh, right - I split this patch from converting install_special_mapping()
to special vma version _install_special_mapping(), 6/8 patch in series.
Will move initialization to that patch.

>>  	if (!vdso_ready)
>>  		return 0;
>> @@ -203,8 +203,8 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
>>  				      ((VDSO_ALIGNMENT - 1) & PAGE_MASK),
>>  				      0, 0);
>>  	if (IS_ERR_VALUE(vdso_base)) {
>> -		rc = vdso_base;
>> -		goto fail_mmapsem;
>> +		ret = vdso_base;
>> +		goto out_up_mmap_sem;
>>  	}
>>
>>  	/* Add required alignment. */
>> @@ -227,21 +227,16 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
>>  	 * It's fine to use that for setting breakpoints in the vDSO code
>>  	 * pages though.
>>  	 */
>> -	rc = install_special_mapping(mm, vdso_base, vdso_pages << PAGE_SHIFT,
>> +	ret = install_special_mapping(mm, vdso_base, vdso_pages << PAGE_SHIFT,
>>  				     VM_READ|VM_EXEC|
>>  				     VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
>>  				     vdso_pagelist);
>> -	if (rc) {
>> +	if (ret)
>>  		current->mm->context.vdso_base = 0;
>> -		goto fail_mmapsem;
>> -	}
>> -
>> -	up_write(&mm->mmap_sem);
>> -	return 0;
>>
>> - fail_mmapsem:
>> +out_up_mmap_sem:
>>  	up_write(&mm->mmap_sem);
>> -	return rc;
>> +	return ret;
>>  }
>
>
> If you strip out the variable renames then I think that change would be
> OK.
>
> cheers
>


-- 
              Dmitry