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

[Evgeniy Polyakov <johnpol@2ka.mipt.ru>]

On Sat, Oct 28, 2006 at 03:34:52PM +0200, Eric Dumazet (dada1@cosmosbay.com) wrote:
> >>>+	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...
> >
> >This function is always called from appropriate context, where it is
> >guaranteed that it is safe to call list_del:
> >1. when kevent is removed. It is called after check, that given kevent 
> >is in the ready queue.
> >2. when dequeued from ready queue, which means that it can be removed
> >from that queue.
> >
> 
> Could you please check the list_del() function ?
> 
> file include/linux/list.h
> 
> static inline void list_del(struct list_head *entry)
> {
>   __list_del(entry->prev, entry->next);
>   entry->next = LIST_POISON1;
>   entry->prev = LIST_POISON2;
> }
> 
> So, after calling list_del(&k->read_entry);
> next and prev are basically destroyed.
> 
> So when you write later :
> 
> +        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);
> +        }
> 
> 
> then you have a problem, since
> 
> list_entry(k->ready_entry.next, struct kevent, ready_entry);
> 
> will give you garbage.

Ok, I understand you now.
To remove this issue we can delete entry from the list after all checks
with overflow_kevent pointer are completed, i.e. have something like
this:

diff --git a/kernel/kevent/kevent_user.c b/kernel/kevent/kevent_user.c
index 711a8a8..f3fec9b 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;
+
+	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);
+		}
+	}
+	list_del(&k->ready_entry);
+	k->flags &= ~KEVENT_READY;
+	u->ready_num--;
+}
+
+/*
  * 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);
 

Thanks.

> Eric

-- 
	Evgeniy Polyakov