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: <460fdc5f-7613-5164-0247-254939cedc71@intel.com>
Date:   Thu, 30 Mar 2023 15:32:20 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.9.0
Subject: Re: [PATCH V2 2/8] vfio/pci: Remove negative check on unsigned vector
Content-Language: en-US
To:     Alex Williamson <alex.williamson@redhat.com>
CC:     <jgg@nvidia.com>, <yishaih@nvidia.com>,
        <shameerali.kolothum.thodi@huawei.com>, <kevin.tian@intel.com>,
        <tglx@linutronix.de>, <darwi@linutronix.de>, <kvm@vger.kernel.org>,
        <dave.jiang@intel.com>, <jing2.liu@intel.com>,
        <ashok.raj@intel.com>, <fenghua.yu@intel.com>,
        <tom.zanussi@linux.intel.com>, <linux-kernel@vger.kernel.org>
References: <cover.1680038771.git.reinette.chatre@intel.com>
 <0dc2a0c8e25b13a3a41db75ab192f387a1548c80.1680038771.git.reinette.chatre@intel.com>
 <20230330142657.3930c68b.alex.williamson@redhat.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230330142657.3930c68b.alex.williamson@redhat.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?U3dMZjFCa1FmSFlHSFlGRHFzRUVyRVJqbmZ0cGRQWVVhQVNvbDhJYVdabldo?=
 =?utf-8?B?ZlExQ05Fd0RBdzBqOGxkS21vdkkrTksva1J3MmNZeVRSQ204Y0VjNDVKS2VC?=
 =?utf-8?B?NDVWU1hLdVZiOHY3Qlc0OEVncmx3djRYWGdDNVoxUVg0OG52TGM1UUh1OWJq?=
 =?utf-8?B?SmJNcFo5dk1mcGorVG9rVDl5ekprVmVIQU5YRFFsUkc3b2Z4NUhUdlk5ZTBM?=
 =?utf-8?B?QWg2TzAxUTZlV1lVYWpYY3NmdFVjRnhpclRmZWRncVJOMFhjTnI5WlNGWWlh?=
 =?utf-8?B?T1JvcDA4SnA5Ylg2MkVKRHFrNlJ4Qk1LQVJMcXdJUlhMQ0VSYkZpRzhQR3A5?=
 =?utf-8?B?UE1FSlBPcFB5cXdPeFpSQ1VRWE1GMzBtMExsL0dXREY2a1VEbm5BWXlCc0hs?=
 =?utf-8?B?Z1ljOGJ5YUZzbVN0ZzFDMW1ua3psWXBMQjhyU0I2RVQ4K3I4ZjRSMXdPczd2?=
 =?utf-8?B?aGFpaG9aaktrTk9LbnY1cTE0UVVpREY0eFg5Tm92dUN5N1dMTkxJenB2MkhI?=
 =?utf-8?B?dlZsem9jc2VPTVFqSDBiM2wvYnEvb0REZGVYWHlrdGNhQzZ6czNzT0t1Mitx?=
 =?utf-8?B?YW9rdXdNWGtZci9LUjJjci9hbXM1WGxsVStGc2xsTmpiL1kyRExGU2pYRG9B?=
 =?utf-8?B?ZnJkMHduUlhUcEdja3IxVWlMTUpHcWpMaldjMTNTWnNZVytZNFJFZzVCUXVn?=
 =?utf-8?B?UTdkbXdQcE05blhRUjFYSTdPaDBDb2lpY2Z0Mzh3V0RLSWFXZDZ3cEZKR0g2?=
 =?utf-8?B?QWdaRVRkLzZ3YTBlRHdScFZESytpNmd4YjRlZXRNaE93WkV6NnltV1pFTS9h?=
 =?utf-8?B?UEc5UlIyRW1hdUNRRGdxSmJkRUhMV0liTVlad2F5NkVqSENLeFp1Yi9kRTMx?=
 =?utf-8?B?U3BTUnhSV0RNNU51RXM5TGxXRXJiNG1ER2tHVC9WU0tDU0hidEI3aTZhdEo3?=
 =?utf-8?B?U0RrdjV3aVZiTU9EOWo5aW4wbWxSMGVWczN6N3RabDUrOUVDWW83RlEwMWlo?=
 =?utf-8?B?emFwNUVPMEMweDJ2MjZTVTVkalQ5OWlYQlc5aXMxbDgrRmFLUm5lMUJndzE5?=
 =?utf-8?B?ak5URUFCTnFFL3JoZGZTb0xNekpFREMvRG80RzJrclZEMFNkRjhFQU4zRDUy?=
 =?utf-8?B?Z0R1OGtJNE9vUlBOQWZDNmIrQWxDWWhUUENUNW5wL1cwckxaVVplQUp3SjVp?=
 =?utf-8?B?S1ZRM1ZwNkwweUo2ZlZKQk1LNk8vWnJyUDcxTFo5bzBwSWU0NWZoSkRmWi9Q?=
 =?utf-8?B?c1crZklBbzdtN3pjcHc2UVFnVXhuZ0pRS2I4WnpSb2ovR1ZZZjRWWDkzTU5n?=
 =?utf-8?B?ZW9YWVNwazMwd3pCUkhYZitzeHRVQTh3ZHp6N3pBem43aTl4Y0hCaXhMN0hp?=
 =?utf-8?B?Nk5jZ2VmZUVvaDd2Y0xVKzk2Vy9xTjZ4bnlUa3dPQllMZXl6ZEdwS213QTZ1?=
 =?utf-8?B?bDZHZ2UyTW9QYnZ6NEdUSVo3KyswL0RESkNoZ2ZXcEo1OEVGWGZ2dmJXOFNO?=
 =?utf-8?B?alFOa1IrZ05TMDJpSi9POWUzdXZRYWZESW5SVTdpOThmZGxrQ0RQbFlpR1RZ?=
 =?utf-8?B?S2tTYVRIRWRaWnNxZlQ2K2lQUUZtNzN4WEh4NFRveFZzRDBIMjFuRERuZXNO?=
 =?utf-8?B?NlZyczJLOXJ1VWpkaU01aGZpY2FTRklXMmJzcFA5Zm9iQTZzYTFBMThCaEdV?=
 =?utf-8?B?Y2NTbjJwZUVDZFBMQlFZVmxUR09SOFVmdnR3WHE4R2toR1k1c1dJdXB5OHRI?=
 =?utf-8?B?ZkxYTXl2U25HcGNFK3ZsMVlJMUdURTM1VHhEcVlLZTNHdlp3aGxWcXBFVmlm?=
 =?utf-8?B?VlEzK1g1TVBHTVF3VkxiZjliUE9xdW43Ulk1Vm8wNTdmYk9yV0FzaEQyV09T?=
 =?utf-8?B?eCtRVmVwamV6bUlWYUhkbng3WDR2Q0tIaFF4VkZwOG05Y0I0WWgvRDdiOEFm?=
 =?utf-8?B?T3ZsV2RxZjlreGg0L1NkQ29HbFNiaHI0L2FtOGpDR1Fjc1BlNDdiTi9CVGtn?=
 =?utf-8?B?T0JZbVh1T2Q2ZUVxcHdUMTl0V0g3T0pwVU5uZFdsRXI5OHZGb0ZCQk9KazhX?=
 =?utf-8?B?TzdDQkRLWldnb3Q5OFdUOVltNjlyOTlGeXV5VFQ5KytwazVUZjlkYjI0RUtK?=
 =?utf-8?B?YTJVYXBKZjhkWFdjbjNQc2dERnd3Yk1NNWIydC9vRUY2ODdFbE5weGVTaUhK?=
 =?utf-8?B?SHc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8ca94087-516f-4f4c-c98f-08db316ea1f1
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Mar 2023 22:32:23.8540
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: O0ycH1GBH0QtoNlcWxAcQj9iH9N1KLish1LHYsO4/2oqJXsRBeX04LNe0b4nZKS9OYGsqDcqbP6lHT0KUwC4n3o+hXPO210oP9ax30wOxc8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB7155
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=unavailable 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

Hi Alex,

On 3/30/2023 1:26 PM, Alex Williamson wrote:
> On Tue, 28 Mar 2023 14:53:29 -0700
> Reinette Chatre <reinette.chatre@intel.com> wrote:
...

>> @@ -399,7 +399,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
>>  static int vfio_msi_set_block(struct vfio_pci_core_device *vdev, unsigned start,
>>  			      unsigned count, int32_t *fds, bool msix)
>>  {
>> -	int i, j, ret = 0;
>> +	int i, ret = 0;
>> +	unsigned int j;
>>  
>>  	if (start >= vdev->num_ctx || start + count > vdev->num_ctx)
>>  		return -EINVAL;
> 
> Unfortunately this turns the unwind portion of the function into an
> infinite loop in the common case when @start is zero:
> 
>                 for (--j; j >= (int)start; j--)
>                         vfio_msi_set_vector_signal(vdev, j, -1, msix);
> 
> 

Thank you very much for catching this. It is not clear to me how you
would prefer to resolve this. Would you prefer that the vector parameter
in vfio_msi_set_vector_signal() continue to be an int and this patch be
dropped and the "if (vector < 0)" check remains (option A)? Or, alternatively,
I see two other possible solutions where the vector parameter in
vfio_msi_set_vector_signal() becomes an unsigned int and the above snippet
could be one of:

option B:
vfio_msi_set_block()
{
	int i, j, ret = 0;

	...
		for (--j; j >= (int)start; j--)
			vfio_msi_set_vector_signal(vdev, (unsigned int)j, -1, msix);
}

option C:
vfio_msi_set_block()
{
	int i, ret = 0;
	unsigned int j;

	...
		for (--j; j >= start && j < start + count; j--)
			vfio_msi_set_vector_signal(vdev, j, -1, msix);
}

What would you prefer?

Reinette