Re: [PATCH] poll/wait/md: allow module to safely support 'poll' on /proc files

[NeilBrown]

On Tue, 11 Mar 2014 20:03:31 -0700 Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
wrote:

> On Wed, 12 Mar 2014 13:36:38 +1100 NeilBrown <neilb@xxxxxxx> wrote:
> 
> > 
> > The md driver currently supports 'poll' on /proc/mdstat.
> > This is unsafe as if the md-mod module is removed while a 'poll'
> > or 'select' is outstanding on /proc/mdstat, an oops occurs
> > when the syscall completes.
> > poll_freewait() will call remove_wait_queue() on a wait_queue_head_t
> > which was local to the module which no-longer exists.
> > 
> > This problem is particular to /proc.  Most filesystems do not
> > allow the module to be unloaded while any files are open on it.
> > /proc only blocks module unloading while a file_operations
> > call is currently active into the module, not while the file is open.
> > kernfs has this property too but kernfs allocates a wait_queue_head_t
> > in its internal data structures so the module doesn't need to provide
> > one.
> > (A previous patch to add a similar allocation to procfs was not
> > accepted).
> 
> By who, me?  I was hoping we could somehow keep the implementation
> contained within md.  I don't think I actually looked at it to any
> significant extent!
> 
> Was hoping that viro would pipe up.

Was not accepted by anybody.  Everybody is guilty :-)
You were at least nice enough to comment (thanks).

I think I like this version better so it might not be a problem that the
previous version was not accepted.  Depends on what the scheduler guys think
though....

> 
> > This patch takes a different approach and allows a module to
> > disconnect the wait_queue_head_t that was passed to poll_wait()
> > from all the clients which are waiting on it.  Thus after calling
> >  proc_remove_entry("mdstat", NULL);
> > we simply call
> >  wait_queue_purge(&md_event_waiters);
> > 
> > and then know that it is safe to remove the module.
> > 
> > rcu infrastructure is used to avoid races.
> > poll_freewait() checks if the purge has happened under rcu_read_lock()
> > to ensure that it never touches any freed memory.  wait_queue_purge()
> > uses synchronize_rcu() to ensure no poll_freewait() could still be
> > looking at the wait_queue_head_t.
> > 
> > ...
> >
> > +/**
> > + * wait_queue_purge - remove all waiter from a wait_queue
> > + * @q: The queue to be purged
> > + *
> > + * Unlink all pending waiters from the queue.
> > + * This can be used prior to freeing a queue providing all waiters are
> > + * prepared for queue purging.
> > + * Waiters must call remove_wait_queue_puregeable() rather than
> > + * remove_wait_queue().
> > + *
> > + */
> > +void wait_queue_purge(wait_queue_head_t *q)
> > +{
> > +	spin_lock(&q->lock);
> > +	while (!list_empty(&q->task_list))
> > +		list_del_init(q->task_list.next);
> > +	spin_unlock(&q->lock);
> > +	synchronize_rcu();
> > +}
> > +EXPORT_SYMBOL(wait_queue_purge);
> 
> I don't get this.  If a task is waiting on that wait_queue_head_t, how
> does it get woken?

This is only expected to be used by tasks which have some other means of
being woken up.  Possibly a timeout passed to 'select' or 'poll', possibly a
signal.

> 
> > +/**
> > + * remove_wait_queue_puregeable - remove_wait_queue if wait_queue_purge might be used.
> > + * @q: the queue, which may already be purged, to remove from
> > + * @wait: the waiter to remove
> > + *
> > + * Remove a waiter from a queue if it hasn't already been purged.
> > + * If the queue has already been purged then task_list will be empty.
> > + * If it isn't then it is still safe to lock the queue and remove
> > + * the task.
> > + */
> > +void remove_wait_queue_purgeable(wait_queue_head_t *q, wait_queue_t *wait)
> > +{
> > +	unsigned long flags;
> > +
> > +	rcu_read_lock();
> > +	if (!list_empty(&wait->task_list)) {
> > +		spin_lock_irqsave(&q->lock, flags);
> 
> Mixture of spin_lock_irqsave() here and spin_lock() in
> wait_queue_purge() looks odd.

True - I was copying remove_wait_queue().  Maybe I should have just called
remove_wait_queue() directly (we don't actually need that _init on the
list_del).

> 
> > +		list_del_init(&wait->task_list);
> > +		spin_unlock_irqrestore(&q->lock, flags);
> > +	}
> > +	rcu_read_unlock();
> > +}
> > +EXPORT_SYMBOL(remove_wait_queue_purgeable);

Thanks,
NeilBrown

Attachment: signature.asc
Description: PGP signature