Re: epoll: possible bug from wakeup_source activation

[Eric Wong <normalperson@yhbt.net>]

Eric Wong <normalperson@yhbt.net> wrote:
> Hi Arve, looking at commit 4d7e30d98939a0340022ccd49325a3d70f7e0238
> (epoll: Add a flag, EPOLLWAKEUP, to prevent suspend ...)
> 
> I think the reason for using ep->ws instead of epi->ws in the unlikely
> ovflist case applies to the likely rdllist case, too.  Since epi->ws is
> only protected by ep->mtx, it can also be deactivated while inside
> ep_poll_callback.
> 
> So something like the following patch might be necessary
> (shown here with extra context):
> 
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -968,39 +968,45 @@ static int ep_poll_callback(wait_queue_t *wait, unsigned mode, int sync, void *k
>  	if (unlikely(ep->ovflist != EP_UNACTIVE_PTR)) {
>  		if (epi->next == EP_UNACTIVE_PTR) {
>  			epi->next = ep->ovflist;
>  			ep->ovflist = epi;
>  			if (epi->ws) {
>  				/*
>  				 * Activate ep->ws since epi->ws may get
>  				 * deactivated at any time.
>  				 */
>  				__pm_stay_awake(ep->ws);
>  			}
>  
>  		}

Thinking about this more, it looks like the original ep->ovflist case of
using ep->ws is unnecessary.

ep->ovflist != EP_UNACTIVE_PTR can only happen while ep->mtx is held (in
ep_scan_ready_list); which means ep_modify+friends cannot remove epi->ws.

ep_poll_callback holding ep->lock means ep_poll_callback prevents
ep_scan_ready_list from setting ep->ovflist = EP_UNACTIVE_PTR and
releasing ep->mtx.

>  		goto out_unlock;
>  	}
>  
>  	/* If this file is already in the ready list we exit soon */
>  	if (!ep_is_linked(&epi->rdllink)) {
>  		list_add_tail(&epi->rdllink, &ep->rdllist);
> -		__pm_stay_awake(epi->ws);
> +		if (epi->ws) {
> +			/*
> +			 * Activate ep->ws since epi->ws may get
> +			 * deactivated at any time.
> +			 */
> +			__pm_stay_awake(ep->ws);
> +		}
>  	}

I still think ep->ws needs to be used in the common ep->rdllist case.