On Tue, 11 Mar 2014 20:03:31 -0700 Andrew Morton <a...@linux-foundation.org> wrote:
> On Wed, 12 Mar 2014 13:36:38 +1100 NeilBrown <ne...@suse.de> 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
signature.asc
Description: PGP signature
