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: <6d38ed2a-72cb-3eb6-5af1-caee61d94005@nvidia.com>
Date:   Thu, 3 Feb 2022 00:38:33 -0800
From:   John Hubbard <jhubbard@...dia.com>
To:     Lukas Bulwahn <lukas.bulwahn@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>
Cc:     Jason Gunthorpe <jgg@...dia.com>, Peter Xu <peterx@...hat.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        David Hildenbrand <david@...hat.com>, Jan Kara <jack@...e.cz>,
        Jason Gunthorpe <jgg@...pe.ca>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Subject: Re: Weird code with change "mm/gup: clean up follow_pfn_pte()
 slightly"

On 2/2/22 22:27, Lukas Bulwahn wrote:
> Dear John,
> 
> Your change "mm/gup: clean up follow_pfn_pte() slightly" (see Link),
> visible in linux-next as commit 05fef840b5c6 ("mm/gup: clean up
> follow_pfn_pte() slightly"), is somehow weird.

Well. That sounds like something to be avoided. :)

> 
> In the new branch if (pages), you set page = ERR_PTR(-EFAULT) and goto
> out. However, at the label out, the value of page is not used, but the
> return uses the variables i and ret.

Yes, I think that the complaint is accurate. The intent of this code is
to return either number of pages so far (i) or ret (which should be zero
in this case), because we are just stopping early, rather than calling
this an actual error.

And since we do skip over setting pages[i] = pages, it's pointless to
assign page to anything.

So instead of this:

	if (pages) {
		page = ERR_PTR(-EFAULT);
		goto out;
	}

...I should have written this:

	if (pages)
		goto out;


I'll send an updated series with this correction.

Thank you for the report!


thanks,
-- 
John Hubbard
NVIDIA
> 
> Static analysis tools, such as clang-analyzer, rightfully complain
> about such weird code.
> 
> Maybe you can have another look at what you intended to set in the
> branch of that commit or if you intend to jump to the label out?
> 
> 
> Best regards,
> 
> Lukas
> 
> Link: https://lkml.kernel.org/r/20220201101108.306062-3-jhubbard@nvidia.com

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ