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: <744be7c4-8e00-7876-5819-a1d07d3d423f@suse.com>
Date:   Thu, 25 Aug 2022 13:58:59 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.0
Subject: Re: [PATCH v3] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()
Content-Language: en-US
To:     Juergen Gross <jgross@suse.com>
Cc:     Stefano Stabellini <sstabellini@kernel.org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
        stable@vger.kernel.org,
        Rustam Subkhankulov <subkhankulov@ispras.ru>,
        xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org
References: <20220825114004.24843-1-jgross@suse.com>
From:   Jan Beulich <jbeulich@suse.com>
In-Reply-To: <20220825114004.24843-1-jgross@suse.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?REs5MW4wcTRnNEpQUGFsbG5WMmJoWEtqWVFKblVoQzlpNHF5VHFQN2tOWkZZ?=
 =?utf-8?B?bDFpakx0RTlCeGZaRG1RVWRqZTJvb2l1TCt0WE9jMVJMTEgveXYvS2h4eSs2?=
 =?utf-8?B?bGNCK0pkbDhFL2p1d1QvaGErcStSU2JHa0FoTy82L3pGYzVMdnFyaWwrRVA1?=
 =?utf-8?B?R1ljZWhCN0VrdzBpd3V4RE16dmI0dmhmMWwvUU1YZ3d2c1QxODN6eTJ5WmJp?=
 =?utf-8?B?K1BzcTlaTzVEMEpCMFRLTlE1aC9QS1d2R28yek5xS245KzRhT2thbEFQVHFY?=
 =?utf-8?B?QWtKTktCSTcrOVNCbVlBR3BIT21xRWhMWTB6eXVpZXByMkozYjFIMnBQN1VW?=
 =?utf-8?B?WU5CYXlzaWd5eWVqZ2pSSXRNdG9JQ2JhWk5JNWxJbmdKRWlNYmFIaXZRZ2RP?=
 =?utf-8?B?dVo2UXVDY2NxSFNYQ0xJYkYrYkNJVjZ5Q0lGSUp3aVE2YUVPN1dtQ1JsUVh4?=
 =?utf-8?B?UzZhd2JtL2ZtMlAzakF6dk1NY2xRYWhsQXg4aE1Wd0xCUmRpam9rczJhVEZO?=
 =?utf-8?B?S012NGFNMjE3dW5tQllFRUVFeWdFUGRXVjRMRjRwRWttS3krUkpYNDNSTVNO?=
 =?utf-8?B?RmNxU25TYnB3Yko5czA0RzFLc0lTSG1WblBWWHFzeDhRbUJDZkVlNy9MVmVi?=
 =?utf-8?B?R3VFMGlsclhWY21kVnJhWWZjTit3VkhQV2FTU3cxbmRKNjhiRGovRGY1aDQ3?=
 =?utf-8?B?dHVsd1FJZkFOUGN3cVhCMFVYVXlabEFQS1VwT2xka1IzL2I4Tjd5YWNKWitF?=
 =?utf-8?B?UURvVHFaZjczblNWSVA1UHFINVI4bFQ2YnBGeU5yN29STlNyUlI4cnZMaHky?=
 =?utf-8?B?WmxabmozR1VHWDNBVXhmNWJkN2JuMzBaY3JUb1JIRlRWRVppNzVoeFdjcmM2?=
 =?utf-8?B?Q2N2TGcwdlFBb3JEQzFRUlJva3FNTHdNMXlLK2kyUVI3NG5aQ21BT203cytp?=
 =?utf-8?B?NzQ3VGc4RGFRTzJYWXdWaG0xRW5zV2FIMmRLaVh2cWRqTkx1YVpQNzZRekdN?=
 =?utf-8?B?RHdZSG92NnNQYVJTclpsOVN6WE80OE5DN25TSHJzZjU1emNxYUdzMFNYaEMz?=
 =?utf-8?B?K1JhR09PYlJmTWQ0T0RpWEJDbUUvRE42R3lKeS9GWmh5dFkvZFNVeFVuWkpC?=
 =?utf-8?B?ckdpSlZOOXZnYkxjL3ZKajZpT1dHVGxwbUFrcWJnL29QcGdoSGo0SkFzUXh3?=
 =?utf-8?B?cUY0dlZtMW13RVdWUHhGTG5FMGVvRUhzRVBwZ3I5TkJMdGVJV1lIMTZWYnlv?=
 =?utf-8?B?ZGRCcy9RbW8xSE1PSC93NFA3bDR3YlJBMFVlUzlHdXpmTzZEc1plOGlYZCtN?=
 =?utf-8?B?WGx3b05xMFRzMkE1OU5lcmxydllQOUNYaGxlWTRySkp6aHU0Zi9NR1lLY2ZT?=
 =?utf-8?B?dWZHOUY2VUY0b0FTay9Md1JtUGVaTmdPTXJMUzZuNzQ1N1JGbG5raHZMRUE2?=
 =?utf-8?B?b2xobFdIRk9vRGhVMzVjMWFLRytYb21uVm9PV2NpdUFXc2dRQWlwTncwbEhW?=
 =?utf-8?B?d0IrWVM4bytENXdTYTJBRTFyT0YzTGxFcWVJRG1ER0RNWDVRTFFzemhoRHp0?=
 =?utf-8?B?RHpMTVByYkw2dlBWUnAxUkhNZEhBc0w5aGw5bWtsekZkdnFobzJOMTFjUVFX?=
 =?utf-8?B?cmxJekRjMW5kMTQraVhSdEgweHFEMmJTd1NGWi9mWVhYYzJpMmFVLytiN0lX?=
 =?utf-8?B?djJud0F1ZE9EOENlWEQzUDA0RFZIRG9ramRKNW1NY2dOSXNpNGRsS0JnTlpj?=
 =?utf-8?B?RHloQnJNMnNVK0ExbkUrcVl3b3NYUWZxbTZWSVBLNkFqRXJhMk5VMi8rWjh3?=
 =?utf-8?B?RkFVZFBuTUZ2Mmd3aUtDdVdSd3NNMjhZV25sMlJXVTBzcGFBVWc5eDNPUDhq?=
 =?utf-8?B?NU5BdVpLTTIxL0NvTG1YT3FzZDZHeXRNOVFmdmNyL2VRczUxYlp1Szc0d1B1?=
 =?utf-8?B?Y0d2MkIrcUwyaEZWK3U0eldHVEl6YS9Wbml1c1pPdlFFcktYRldRUDJsb05C?=
 =?utf-8?B?WjNxdy9zSGRHc1lxWkVSaWQrWC9UQ29paUViUG5MV1FsZ3BpTjRIR2k0b2No?=
 =?utf-8?B?VkdmUXIwR2hjL1Z0RkNESFMrMndRbzVxdjZhQitIWkpqYlNRRjYraGFHUXcr?=
 =?utf-8?Q?HcqZsymcYqNC3dvhDqY7OZANN?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 14ccf690-428b-4e31-aa92-08da86913362
X-MS-Exchange-CrossTenant-AuthSource: VE1PR04MB6560.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Aug 2022 11:59:01.8393
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: pEB7r9IoTbm9jjMhgTGqSUrd/gi8QRX5XwJL331qlFBpxSmE7I4b7VAa40Bwxfq67EobPomMjVqCMlTQY2oRxA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0402MB3370
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 25.08.2022 13:40, Juergen Gross wrote:
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -581,7 +581,7 @@ static int lock_pages(
>  	struct privcmd_dm_op_buf kbufs[], unsigned int num,
>  	struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
>  {
> -	unsigned int i;
> +	unsigned int i, off = 0;
>  
>  	for (i = 0; i < num; i++) {
>  		unsigned int requested;
> @@ -589,19 +589,23 @@ static int lock_pages(
>  
>  		requested = DIV_ROUND_UP(
>  			offset_in_page(kbufs[i].uptr) + kbufs[i].size,
> -			PAGE_SIZE);
> +			PAGE_SIZE) - off;
>  		if (requested > nr_pages)
>  			return -ENOSPC;
>  
>  		page_count = pin_user_pages_fast(
> -			(unsigned long) kbufs[i].uptr,
> +			(unsigned long)kbufs[i].uptr + off * PAGE_SIZE,
>  			requested, FOLL_WRITE, pages);
> -		if (page_count < 0)
> -			return page_count;
> +		if (page_count <= 0)
> +			return page_count ? : -EFAULT;
>  
>  		*pinned += page_count;
>  		nr_pages -= page_count;
>  		pages += page_count;
> +
> +		off = requested - page_count;
> +		if (off)
> +			i--;
>  	}

Initially I thought this would go wrong only on the 3rd iteration, but
meanwhile I think it's wrong already on the 2nd. What I think you need
is

		if (page_count < requested)
			i--;
		off += page_count;

or with the i++ from the loop header absorbed here

		if (page_count == requested)
			i++;
		off += page_count;

Plus of course off needs resetting to zero whenever i advances. I.e.

		if (page_count == requested) {
			i++;
			off = 0;
		} else {
			off += page_count;
		}

Jan