Re: [patch] Minor PAGG attach/detach semantic change for 2.6.11

Hi,

Erik Jacobson wrote:
> I fixed this in the RCU version of pnotify I'm working per the lse-tech 
> community discussion - thanks for the reminder the other day (in a non-list 
> email).
> 
> If the RCU version crashes and burns for some reason and we go back to
> the non-CUu one, I'll need to make the fix there too.  The function now
> looks like this.  I hope this is what you had in mind (untested as of
> this moment).

In my understanding, any _write_ operations can not be implemented
without locking, even if we can use RCU.
(In addition, RCU conscious writing/update style is required.)

For example, pnotify permits to attach a new pnotify_subscriber object
to another task. If someone calls pnotify_subscribe() for other task
which is doing fork(), there is a possibility to break the
pnotify_subscriber_list of victim task.

Therefore, procedures with updates such __pnotify_fork() should be
serialized by somethig locking. RCU is so effective for seldom-write/
frequentrly-read pass, such as SELinux's Access Vector Cache(AVC).
But it's not omnipotence, and it restricts write methodology.

In the past, I made a proposition of applying RCU for PAGG. But it might
be inappropriate for pnotify/PAGG as a general framework.


I have attention to another respect. The current pnotify implementation
requires to hold pnotify_event_list_sem before calling pnotify_get_events().
Threfore, we must repeat read_lock/unlock(&tasklist_lock) on
do_each_thread()/while_each_thread() loop as follows:

----------------------------
read_lock(&tasklist_lock);
do_each_thread(g, p) {
	get_task_struct(p);
	read_unlock(&tasklist_lock);

	down_read(&p->pnotify_subscriber_list_sem);
	subscriber = pnotify_get_subscriber(p, events->name);
		:
	up_read(&p->pnotify_subscriber_list_sem);
	read_lock(&tasklist_lock);
	<< checking, p is dead or not ? >>
} while_each_thread(g, p);
read_unlock(&tasklist_lock);
----------------------------

I'm happy, if pnotify_subscriber_list would be protected by rwlock.

If rwlock is used, we can not implement pnotify_subscribe() with current
spec.  But is it impossible to prepare pnotify_subscribe_atomic() or
pnotify_subscribe_bind() which associates task_struct with pre-allocated
pnotify_events object ?

---- in rwlock world :-) ---
read_lock(&tasklist_lock);
do_each_thread(g, p) {
	read_lock(&p->pnotify_subscriber_list_rwlock);
	subscriber = pnotify_get_subscriber(p, events->name);
		:
	read_unlock(&p->pnotify_subscriber_list_rwlock);
} while_each_thread(g, p);
read_unlock(&tasklist_lock);
----------------------------

Thanks,


> /**
>  * __pnotify_fork - Add kernel module subscriber to same subscribers as parent
>  * @to_task: The child task that will inherit the parent's subscribers
>  * @from_task: The parent task
>  *
>  * Used to attach a new task to the same subscribers the parent has in its
>  * subscriber list.
>  *
>  * The "from" argument is the parent task.  The "to" argument is the child
>  * task.
>  *
>  * See Documentation/pnotify.txt for details on
>  * how to handle return codes from the attach function pointer.
>  *
>  * Locking: The to_task is currently in-construction, so we don't
>  * need to worry about write-locks.  We do need to be sure the parent's
>  * subscriber list, which we copy here, doesn't go away on us.  This is
>  * done via RCU.
>  *
>  */
> int
> __pnotify_fork(struct task_struct *to_task, struct task_struct *from_task)
> {
> 	struct pnotify_subscriber *from_subscriber;
> 	int ret;
> 
> 	/* We need to be sure the parent's list we copy from doesn't disappear */
> 	rcu_read_lock();
> 
> 	list_for_each_entry_rcu(from_subscriber, &from_task->pnotify_subscriber_list, entry) {
> 		struct pnotify_subscriber *to_subscriber = NULL;
> 
> 		to_subscriber = pnotify_subscribe(to_task, from_subscriber->events);
> 		if (!to_subscriber) {
> 			ret=-ENOMEM;
> 			__pnotify_exit(to_task);
> 			rcu_read_unlock();
> 			return ret;
> 		}
> 		ret = to_subscriber->events->fork(to_task, to_subscriber,
> 		  from_subscriber->data);
> 	
> 		rcu_read_unlock(); /* no more to do with the parent's data */

rcu_read_unlovk(); should be deployed outside of the list_for_each_entry_rcu(){...}.

> 
> 		if (ret < 0) {
> 			/* Propagates to copy_process as a fork failure */
> 			/* No __pnotify_exit because there is one in the failure path
> 			 * for copy_process in fork.c */
> 			return ret; /* Fork failure */
> 		}
> 		else if (ret > 0) {
> 			/* Success, but fork function pointer in the pnotify_events structure
> 			 * doesn't want the kenrel module subscribed */
> 			/* Again, this is the in-construction-child so no write lock */
> 			pnotify_unsubscribe(to_subscriber);
> 		}
> 	}
> 
> 	return 0;	/* success */
> }

-- 
Linux Promotion Center, NEC
KaiGai Kohei <[email protected]>