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

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: <454359DC.4020905@cosmosbay.com>
Date:	Sat, 28 Oct 2006 15:23:40 +0200
From:	Eric Dumazet <dada1@...mosbay.com>
To:	Evgeniy Polyakov <johnpol@....mipt.ru>
Cc:	David Miller <davem@...emloft.net>,
	Ulrich Drepper <drepper@...hat.com>,
	Andrew Morton <akpm@...l.org>, netdev <netdev@...r.kernel.org>,
	Zach Brown <zach.brown@...cle.com>,
	Christoph Hellwig <hch@...radead.org>,
	Chase Venters <chase.venters@...entec.com>,
	Johann Borck <johann.borck@...sedata.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [take21 1/4] kevent: Core files.

Evgeniy Polyakov a e'crit :
> On Sat, Oct 28, 2006 at 02:36:31PM +0200, Eric Dumazet (dada1@...mosbay.com) wrote:
>> Evgeniy Polyakov a e'crit :
>>> On Sat, Oct 28, 2006 at 12:28:12PM +0200, Eric Dumazet 
>>> (dada1@...mosbay.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);
>  


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