Re: [take21 1/4] kevent: Core files.

Eric Dumazet
2006-10-28T13:23:43Z

Evgeniy Polyakov a e'crit :
> On Sat, Oct 28, 2006 at 02:36:31PM +0200, Eric Dumazet (dada1@cosmosbay.com) wrote:
>> Evgeniy Polyakov a e'crit :
>>> On Sat, Oct 28, 2006 at 12:28:12PM +0200, Eric Dumazet
>>> (dada1@cosmosbay.com) wrote:
>>>> I really dont understand how you manage to queue multiple kevents in the
>>>> 'overflow list'. You just queue one kevent at most. What am I missing ?
>>> There is no overflow list - it is a pointer to the first kevent in the
>>> ready queue, which was not put into ring buffer. It is an optimisation,
>>> which allows to not search for that position each time new event should
>>> be placed into the buffer, when it starts to have an empty slot.
>> This overflow list (you may call it differently, but still it IS a list),
>> is not complete. I feel you add it just to make me happy, but I am not (yet
>> :) )
>
> There is no overflow list.
> There is ready queue, part of which (first several entries) is copied
> into the ring buffer, overflow_kevent is a pointer to the first kevent which
> was not copied.
>
>> For example, you make no test at kevent_finish_user_complete() time.
>>
>> Obviously, you can have a dangling pointer, and crash your box in certain
>> conditions.
>
> You are right, I did not put overflow_kevent check into all places which
> can remove kevent.
>
> Here is a patch I am about to commit into the kevent tree:
>
> diff --git a/kernel/kevent/kevent_user.c b/kernel/kevent/kevent_user.c
> index 711a8a8..ecee668 100644
> --- a/kernel/kevent/kevent_user.c
> +++ b/kernel/kevent/kevent_user.c
> @@ -235,6 +235,36 @@ static void kevent_free_rcu(struct rcu_h
> }
>
> /*
> + * Must be called under u->ready_lock.
> + * This function removes kevent from ready queue and
> + * tries to add new kevent into ring buffer.
> + */
> +static void kevent_remove_ready(struct kevent *k)
> +{
> + struct kevent_user *u = k->user;
> +
> + list_del(&k->ready_entry);

Arg... no

You cannot call list_del() , then check overflow_kevent.

I you call list_del on what happens to be the kevent pointed by
overflow_kevent, you loose...

> + k->flags &= ~KEVENT_READY;
> + u->ready_num--;
> + if (++u->pring[0]->uidx == KEVENT_MAX_EVENTS)
> + u->pring[0]->uidx = 0;
> +
> + if (u->overflow_kevent) {
> + int err;
> +
> + err = kevent_user_ring_add_event(u->overflow_kevent);
> + if (!err || u->overflow_kevent == k) {
> + if (u->overflow_kevent->ready_entry.next == &u->ready_list)
> + u->overflow_kevent = NULL;
> + else
> + u->overflow_kevent =
> + list_entry(u->overflow_kevent->ready_entry.next,
> + struct kevent, ready_entry);
> + }
> + }
> +}
> +
> +/*
> * Complete kevent removing - it dequeues kevent from storage list
> * if it is requested, removes kevent from ready list, drops userspace
> * control block reference counter and schedules kevent freeing through RCU.
> @@ -248,11 +278,8 @@ static void kevent_finish_user_complete(
> kevent_dequeue(k);
>
> spin_lock_irqsave(&u->ready_lock, flags);
> - if (k->flags & KEVENT_READY) {
> - list_del(&k->ready_entry);
> - k->flags &= ~KEVENT_READY;
> - u->ready_num--;
> - }
> + if (k->flags & KEVENT_READY)
> + kevent_remove_ready(k);
> spin_unlock_irqrestore(&u->ready_lock, flags);
>
> kevent_user_put(u);
> @@ -303,25 +330,7 @@ static struct kevent *kqueue_dequeue_rea
> spin_lock_irqsave(&u->ready_lock, flags);
> if (u->ready_num && !list_empty(&u->ready_list)) {
> k = list_entry(u->ready_list.next, struct kevent, ready_entry);
> - list_del(&k->ready_entry);
> - k->flags &= ~KEVENT_READY;
> - u->ready_num--;
> - if (++u->pring[0]->uidx == KEVENT_MAX_EVENTS)
> - u->pring[0]->uidx = 0;
> -
> - if (u->overflow_kevent) {
> - int err;
> -
> - err = kevent_user_ring_add_event(u->overflow_kevent);
> - if (!err) {
> - if (u->overflow_kevent->ready_entry.next == &u->ready_list)
> - u->overflow_kevent = NULL;
> - else
> - u->overflow_kevent =
> - list_entry(u->overflow_kevent->ready_entry.next,
> - struct kevent, ready_entry);
> - }
> - }
> + kevent_remove_ready(k);
> }
> spin_unlock_irqrestore(&u->ready_lock, flags);
>