Re: [PATCH v6 2/2] Drivers: hv: Introduce mshv_vtl driver

[Naman Jain <namjain@linux.microsoft.com>]

On 7/25/2025 8:05 PM, Michael Kelley wrote:
> From: Naman Jain <namjain@linux.microsoft.com> Sent: Thursday, July 24, 2025 10:54 PM
>>
>> On 7/25/2025 8:52 AM, Michael Kelley wrote:
>>> From: Naman Jain <namjain@linux.microsoft.com> Sent: Thursday, July 24, 2025 1:26 AM
>>>>
> 
> [snip]
> 
>>>> +
>>>> +static int mshv_vtl_sint_ioctl_set_eventfd(struct mshv_vtl_set_eventfd __user *arg)
>>>> +{
>>>> +	struct mshv_vtl_set_eventfd set_eventfd;
>>>> +	struct eventfd_ctx *eventfd, *old_eventfd;
>>>> +
>>>> +	if (copy_from_user(&set_eventfd, arg, sizeof(set_eventfd)))
>>>> +		return -EFAULT;
>>>> +	if (set_eventfd.flag >= HV_EVENT_FLAGS_COUNT)
>>>> +		return -EINVAL;
>>>> +
>>>> +	eventfd = NULL;
>>>> +	if (set_eventfd.fd >= 0) {
>>>> +		eventfd = eventfd_ctx_fdget(set_eventfd.fd);
>>>> +		if (IS_ERR(eventfd))
>>>> +			return PTR_ERR(eventfd);
>>>> +	}
>>>> +
>>>> +	guard(mutex)(&flag_lock);
>>>> +	old_eventfd = READ_ONCE(flag_eventfds[set_eventfd.flag]);
>>>> +	WRITE_ONCE(flag_eventfds[set_eventfd.flag], eventfd);
>>>> +
>>>> +	if (old_eventfd) {
>>>> +		synchronize_rcu();
>>>> +		eventfd_ctx_put(old_eventfd);
>>>
>>> Again, I wonder if is OK to do eventfd_ctx_put() while holding
>>> flag_lock, since the use of guard() changes the scope of the lock
>>> compared with the previous version of this patch.
>>>
>>
>> I didn't find eventfd_ctx_put() to be a blocking operation, so I thought
>> of keeping guard() here. Although, synchronize_rcu() is a blocking
>> operation. Please advise, I am Ok with removing the guard, as the lock
>> is just being used here, and automatic cleanup should not be an issue
>> here.
> 
> Yes, I think you are right. I saw the kref_put() and was unsure what
> would be called if the object was freed. But the "free" function is
> right there staring at me. :-) All it does is ida_free() and kfree(),
> both of which would be safe.
> 
> You should be good keeping the guard().
> 
> Michael

Acked.

> 
>>
>>
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +