Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <afedd0c6-23c0-7e79-3b14-48fffaed7f99@nvidia.com>
Date:   Thu, 3 Feb 2022 17:22:36 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.1
Subject: Re: Weird code with change "mm/gup: clean up follow_pfn_pte()
 slightly"
Content-Language: en-US
To:     Jason Gunthorpe <jgg@nvidia.com>
Cc:     Lukas Bulwahn <lukas.bulwahn@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux-MM <linux-mm@kvack.org>, Peter Xu <peterx@redhat.com>,
        Alex Williamson <alex.williamson@redhat.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        David Hildenbrand <david@redhat.com>, Jan Kara <jack@suse.cz>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
References: <CAKXUXMxFK9bo8jDoRZbQ0r2j-JwAGg3Xc5cpAcLaHfwHddJ7ew@mail.gmail.com>
 <6d38ed2a-72cb-3eb6-5af1-caee61d94005@nvidia.com>
 <20220203130137.GZ1786498@nvidia.com>
 <222e01e6-bb00-51ee-a94c-089b43cbaf42@nvidia.com>
 <20220204004541.GL1786498@nvidia.com>
 <c0f57bfc-d34b-a34b-4f2d-0d66782e4ae7@nvidia.com>
 <20220204010612.GO1786498@nvidia.com>
From:   John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <20220204010612.GO1786498@nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?V3JLWU1xcEpZQmRobGc0Sm9oOWVrYTBreUpRSU1aTGVlSDFqc2g0NjlFSi9T?=
 =?utf-8?B?MVNBZUhNQllaRTlZMHhxK2kzdHRQempIbHNUU2pDZ1RJMlB0amhHalFOMzFm?=
 =?utf-8?B?b01kaDMvVkc5bWtFS0pldWJ3KzlpMjdDN01TNHI1UldXK0RiQkIyL09Ob2o2?=
 =?utf-8?B?VTZwWGowQ2F5WHBQV0pWNkpiL2hNanZBd243NHZpcVpza0hHbHVNR3NXUlZk?=
 =?utf-8?B?dVN4aXFWZnVhMXhGNmxjRW9rWElYRTJrcUplbnlJUDdjRHU5RXQ5NkJvaDNi?=
 =?utf-8?B?VklZRnk3K3JGVW5zTG1vT1d3OWE4MlZTSmphWnRPcjVvTDBnTnJJRzFsSzBE?=
 =?utf-8?B?c29hYzU2Tm9uMkM2dTczWlRHTjJlUFJsZ1lxa090NGN4V2F6TFJHVnZBaVk0?=
 =?utf-8?B?K0M5Qm1zZnpEQUEwbXhsaDZETTZtMkFScEJLN3hndE11dlMrdUdsTzFzby9n?=
 =?utf-8?B?TFRtYVlaclhQVGFHNWRLdVhRQzFoUVlPc0h3c1VWeU9xa0R3V1JKdURKdmxU?=
 =?utf-8?B?L1I5UDJiT3U4d0d1WGRBSC9ya1F2U3VOeFlGMlVla0JGZWkwbFFmY3U4eE8w?=
 =?utf-8?B?N0JtRG5EWGZBSFhFZmpDL0ZJeFJRdGVDWHpabkFXZ0JGSHFBSjJjVWRWNEJO?=
 =?utf-8?B?SjNrVmxueGxhVE9sL1ZIRzh2TE1iRlNpNnFEbGFqS1h4dDFYcFgvb0xXOVp5?=
 =?utf-8?B?dDZETGFqQVdjc0FLZWMyWFk4S3UzNko3cjl6TXBWUUw3aDJTaUg3Sk1ieHlh?=
 =?utf-8?B?SEp2NlQ0WFFsT2dNd241dm5PcnBxMEtpd2NmOHJCNXhJMVk5dXdxbHdkWU5C?=
 =?utf-8?B?K0RGaExzK1Z4S05pRHhUTHl3MWJpVEh1UWZjU05FdngxWDVRb2xYak1kdDNp?=
 =?utf-8?B?bFJwTms0MENKVEpoQmZFWXpEVVVrNW1keWdkSGY4VnpCa1JFdGdwOEY3aDRs?=
 =?utf-8?B?VVF2TE1JSUgyZTlzUExpUFdGaEJSU0FJTk9ROTBPamtqVTltZktFRitrNjNN?=
 =?utf-8?B?elpyK1YyZjVqaWc5bTlEbnc3eTNaenR1ZERzNU9zL0h4eVh4ZEdRdzZ3ejdT?=
 =?utf-8?B?N1ZQQi9BL1BSR0FYQncxdjBRZnVhOWkvbkxkN3c4VEZpdHRxRi9mZy9hcVd6?=
 =?utf-8?B?aHdqaHZEUUpnQzJXSnZDL2dWa0RjWWd4dHQySTkwcUNNL3RnSDhCRU53Z09n?=
 =?utf-8?B?aTdrdCt3RTFzRVBCcWJpZDBBZ2doSjkzb3laMkt6L0lhbEdWSndDVnRzVzFj?=
 =?utf-8?B?QXVEb3lweDk2cFNJdnFhS1p2eUJPSDkzMjAydjZWOXdMSEhQWEd0bkt2eitr?=
 =?utf-8?B?aG1YdUtMVjZ1RVZZQ1dFdjFXYzgrOG9LcDByZkpoV09VNFRMeUxmUVAyMHdp?=
 =?utf-8?B?Q0VxSVlYeU1QWERtQStWaENJeDAvbXBMS3VGeG45Wk1LR0VYNWhhcXhZUTJS?=
 =?utf-8?B?MXdNTFJod2hyZys0cnRDNy9IZUVBdEdSZlFidkVHTTQvOUNwaFdoQmFTL3dH?=
 =?utf-8?B?ZWJna1l1Q2tiazNqSjlhcStEUWVNejNKeHJRZkRMRFUyY2hnOGxYZkNpVGN0?=
 =?utf-8?B?RWZ3Q1pCMEtDbW9mc2hKbS9zTnllMCszWk4wMzRzSlVxbEFpQnBnYWJ5VXlL?=
 =?utf-8?B?cWk3NmlGQWI0VHluYWdpL0c4WExwTEZiZG1YOFR2MnNtTTY5UGRUSFZucWJS?=
 =?utf-8?B?akwxSmZLc2pBSU14ams0ck9WS3g4R3JLbWVQY2xwdTUvTkpuNFpxSG04VlNW?=
 =?utf-8?B?MWFpdWNva0kzSDlaVUFHMElqUEsranQ5M1ZHTHpPTXhyNEtxejhudXM4aUk0?=
 =?utf-8?B?Qi9MSHhwRnJ6d1FZbDFCNDc3ZzRXU0ZhVkkydWg1MGdZR0cydFBCVGI4UjNG?=
 =?utf-8?B?T1VjYWxWbk5teDQzN0l0bVNLTkVFcjBRNlJucFpReTQ3Zi9BQzB5NmdRNUIv?=
 =?utf-8?B?TjRCMTFZcG5ZaGZkZXM3Wk5IK284RzExVTN2a2c4Z2h0L0FYZzQwRVNoMlQy?=
 =?utf-8?B?MHFRWUxHQXEwMEpGeDdrOU9qeGM5MlZqN0ZadVluYlI4TFViYjZMNmZ5eHlr?=
 =?utf-8?B?eGVoM1VSRko5eVM1TnVvVzBlVTFid1JVengvK1l6MFlhU1VlOUEzYnUwQTI5?=
 =?utf-8?B?SENRYTNNRDU3YVlyWCtFN2V3WG5SS3J5ZUNod1NjWHo1WEh1cGxnYzRZY1Vy?=
 =?utf-8?Q?uBGiMPZbD9BcGm4zg5TW6wU=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6908130f-1da2-46cd-c3d1-08d9e77cd531
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4130.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Feb 2022 01:22:38.9498
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: OEEVCztj19VQNGjrMPzfAMj5UD20v0A8pZuSqAqV4XEPK27HO4AeBx/72yjWXVfvfmL1R03dqSvuuG4OwEJP9A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR1201MB2548
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2/3/22 17:06, Jason Gunthorpe wrote:
> On Thu, Feb 03, 2022 at 04:59:56PM -0800, John Hubbard wrote:
>> On 2/3/22 16:45, Jason Gunthorpe wrote:
>>> On Thu, Feb 03, 2022 at 12:44:57PM -0800, John Hubbard wrote:
>>>> On 2/3/22 05:01, Jason Gunthorpe wrote:
>>>> ...
>>>>>>> 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.
>>>>>
>>>>> IIRC GUP shouldn't return 0, it should return an error code, not zero.
>>>>>
>>>>> Jason
>>>>
>>>> Errors work for single pages, but GUP is a multi-page API call. If it
>>>> returned an error part way through the list of pages, then callers would
>>>> have no way of knowing how many pages to release.
>>>
>>> Yes, but that is returning a positive error code, I said it should not
>>> return zero.
>>>
>>> When it hits an error with pages already loaded it returns that number
>>> and the caller will then do gup once more with the VA pointing at the
>>> problematic page. Then GUP can return the error code because it has 0
>>> pages on the next iteration.
>>>
>>> It should not return 0 here when it got an error.
>>
>> This is perhaps better API design, but it's not what exists now.
> 
> I think it is what exists today, 0 certainly is not implemented as
> 'need retry' anywhere I found.
> 
> So why do we return 0, if it means an error, instead of returning the
> actual errno?

Well, now returning 0 sounds all wrong, when you put it like that. :)

So, simply this approach? :

@@ -1205,8 +1201,15 @@ static long __get_user_pages(struct mm_struct *mm,
  		} else if (PTR_ERR(page) == -EEXIST) {
  			/*
  			 * Proper page table entry exists, but no corresponding
-			 * struct page.
+			 * struct page. If the caller expects **pages to be
+			 * filled in, bail out now, because that can't be done
+			 * for this page.
  			 */
+			if (pages) {
+				ret = PTR_ERR(page);
+				goto out;
+			}
+
  			goto next_page;
  		} else if (IS_ERR(page)) {
  			ret = PTR_ERR(page);

> 
>> The call sites today handle 0 pages ret value correctly,
> 
> This isn't correct though:
> 
>   	if (get_user_pages(vaddr, 1, write ? FOLL_WRITE : 0, &page, NULL) <= 0)
>   		return -EFAULT;
> 
> If GUP wanted the caller to permanently fail with -EFAULT, it should
> have directly returned EFAULT.
> 
> 0 means 'to be retried', whatever that means, and there is no retry
> in the above.
> 
> IOW, the above does not handle a 0 return correctly, according to the
> comment.
> 

I recall seeing several sites that do a quick attempt at one page and
force a -errno failure if anything other than ret==1 occurs. I guess the
good news is that changing GUP to return -errno instead of 0 won't affect
them.


thanks,
-- 
John Hubbard
NVIDIA