linux-kernel - Re: [PATCH] fanotify: use killable wait for waiting response for permission events

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: <a7b4501c-ce1d-6590-3a48-a951740cf681@yandex-team.ru>
Date:   Tue, 21 Aug 2018 16:42:26 +0300
From:   Konstantin Khlebnikov <khlebnikov@...dex-team.ru>
To:     Jan Kara <jack@...e.cz>
Cc:     linux-fsdevel@...r.kernel.org, Amir Goldstein <amir73il@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fanotify: use killable wait for waiting response for
 permission events

On 20.08.2018 13:53, Jan Kara wrote:
> Hi!
> 
> On Mon 20-08-18 10:09:42, Konstantin Khlebnikov wrote:
>> Waiting in uninterruptible state for response from userspace
>> easily produces deadlocks and hordes of unkillable tasks.
>>
>> This patch makes this wait killable.
>>
>> At receiving fatal signal task will remove queued event and die.
>> If event is already handled then response will be received as usual.
>>
>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@...dex-team.ru>
> 
> Thanks for the patch. I like the idea. Some comments inline.
> 
>> ---
>>   fs/notify/fanotify/fanotify.c |   22 +++++++++++++++++++++-
>>   1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
>> index eb4e75175cfb..7a0c37790c89 100644
>> --- a/fs/notify/fanotify/fanotify.c
>> +++ b/fs/notify/fanotify/fanotify.c
>> @@ -64,7 +64,27 @@ static int fanotify_get_response(struct fsnotify_group *group,
>>   
>>   	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
>>   
>> -	wait_event(group->fanotify_data.access_waitq, event->response);
>> +	ret = wait_event_killable(group->fanotify_data.access_waitq,
>> +				  event->response);
>> +	if (ret) {
>> +		/* Try to remove pending event from the queue */
>> +		spin_lock(&group->notification_lock);
>> +		if (!list_empty(&event->fae.fse.list))
>> +			list_del_init(&event->fae.fse.list);
> 
> Here you forget to decrement group->q_len like
> fsnotify_remove_first_event() does.
> 

Yep

>> +		else
>> +			ret = 0;
>> +		spin_unlock(&group->notification_lock);
> 
> So the above check for list_empty can hit either when response is just
> being processed (and then we'll be woken up very soon) or when the event is
> just in the process of being copied from event queue to userspace (in which
> case we are in the same situation as in the old code). So it would be
> weird that in rare cases wait would not be really killable. I think we
> could detect this situation in fanotify_read() before adding event to
> access_list and just wakeup waiter in fanotify_get_response() again and
> avoid reporting the event to userspace. Hmm?

I've missed that move from list to list in fanotify_read().

So, fanotify_read needs event alive for a long time - copy_to_user might block forever.

We have to transfer ownership and destroy event in fanotify_read.
I'll try this approach.

> 
> 								Honza
> 
>> +
>> +		if (ret)
>> +			return ret;
>> +
>> +		/*
>> +		 * We cannot return, this will destroy event while
>> +		 * process_access_response() fills response.
>> +		 * Just wait for wakeup and continue normal flow.
>> +		 */
>> +		wait_event(group->fanotify_data.access_waitq, event->response);
>> +	}
>>   
>>   	/* userspace responded, convert to something usable */
>>   	switch (event->response & ~FAN_AUDIT) {
>>