Re: epoll: possible bug from wakeup_source activation

[Arve Hjønnevåg]

On Fri, Mar 8, 2013 at 12:49 PM, Eric Wong <normalperson@xxxxxxxx> wrote:
> Arve Hjønnevåg <arve@xxxxxxxxxxx> wrote:
>> On Thu, Mar 7, 2013 at 5:30 PM, Eric Wong <normalperson@xxxxxxxx> wrote:
>> > Eric Wong <normalperson@xxxxxxxx> 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.
>> >
>>
>> The callback function in ep_scan_ready_list can call __pm_relax on it though.
>>
>> > 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.
>>
>> This code is reached when ep_scan_ready_list has set ep->ovflist to
>> NULL before releasing ep->lock. Since the callback function can call
>> __pm_relax on epi->ws without holding ep->lock we call __pm_stay_awake
>> in ep->ws here (the callback does not call __pm_relax on that).
>
> Thanks for the explanation.  I got "deactivate" and "destroy"
> mixed up.  However, I'm still concerned about the "destroy" case:
>
>> >
>> >>               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.
>>
>> ep_scan_ready_list calls __pm_relax on ep->ws when it is done, so this
>> will not work. ep->ws is not a "ep->rdllist not empty wakeup_source is
>> is a "ep_scan_ready_list is running" wakeup_source.
>
> What happens if ep_modify calls ep_destroy_wakeup_source
> while __pm_stay_awake is running on the same epi->ws?

Yes, that looks like a problem. I think calling
ep_destroy_wakeup_source with ep->lock held should fix that. It is not
clear how useful changing EPOLLWAKEUP in ep_modify is, so
alternatively we could remove that feature and instead only allow it
to be set in ep_insert.

-- 
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/