poll/wait/md: allow module to safely support 'poll' on /proc files

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).

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.

...

kernel/sched/wait.c +10 -0

+/**
+ * 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().
+ *

kernel/sched/wait.c +7 -0

+/**
+ * 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.

kernel/sched/wait.c +6 -0

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

Quoted 8 lines · On Wed, 12 Mar 2014 13:36:38 +1100 NeilBrown <[email protected]> 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.

Quoted 9 lines

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.

Quoted 33 lines

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.

...

kernel/sched/wait.c +15 -0

+/**
+ * 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))

kernel/sched/wait.c void remove_wait_queue(wait_queue_head_t *q, wait_queue_t *wait) · line 74

Quoted 5 lines

kernel/sched/wait.c +5 -0

+		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?

Quoted 13 lines

kernel/sched/wait.c +12 -0

+/**
+ * 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)
+{

kernel/sched/wait.c void remove_wait_queue(wait_queue_head_t *q, wait_queue_t *wait) · line 92

Quoted 5 lines

kernel/sched/wait.c +5 -0

+	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.

kernel/sched/wait.c void remove_wait_queue(wait_queue_head_t *q, wait_queue_t *wait) · line 98

Quoted 7 lines

kernel/sched/wait.c +6 -0

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

Quoted 24 lines · On Tue, 11 Mar 2014 20:03:31 -0700 Andrew Morton <[email protected]>…

Quoted 8 lines · On Wed, 12 Mar 2014 13:36:38 +1100 NeilBrown <[email protected]> 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.

Quoted 9 lines

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!

Quoted 1 line

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....

Quoted 17 lines

Quoted 17 lines

...

kernel/sched/wait.c +15 -0

+/**
+ * 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))

Quoted 8 lines

kernel/sched/wait.c void remove_wait_queue(wait_queue_head_t *q, wait_queue_t *wait) · line 74

Quoted 5 lines

kernel/sched/wait.c +5 -0

+		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.

Quoted 13 lines

Quoted 13 lines

kernel/sched/wait.c +12 -0

+/**
+ * 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)
+{

Quoted 8 lines

kernel/sched/wait.c void remove_wait_queue(wait_queue_head_t *q, wait_queue_t *wait) · line 92

Quoted 5 lines

kernel/sched/wait.c +5 -0

+	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).

Quoted 27 lines · On Wed, 12 Mar 2014 14:10:25 +1100 NeilBrown <[email protected]> wrote:

Quoted 24 lines · On Tue, 11 Mar 2014 20:03:31 -0700 Andrew Morton <[email protected]>…

Quoted 8 lines · On Wed, 12 Mar 2014 13:36:38 +1100 NeilBrown <[email protected]> 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.

Quoted 9 lines

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!

Quoted 8 lines

Quoted 1 line

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....

OK..

Quoted 26 lines

Quoted 25 lines

Quoted 12 lines

...

kernel/sched/wait.c +10 -0

+/**
+ * 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().
+ *

kernel/sched/wait.c void remove_wait_queue(wait_queue_head_t *q, wait_queue_t *wait) · line 74

Quoted 10 lines

kernel/sched/wait.c +10 -0

+ */
+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?

Quoted 3 lines

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.

Oh. So the caller is supposed to take the tasks off the queue via wait_queue_purge(), then to wake them up (which implies the caller has a second way of looking the tasks up).

And the tasks themselves ... do they need to know what happened? If they run remove_wait_queue() then will still take wait_queue_head_t.lock, so the calling task need to somehow keep the wait_queue_head_t alive until everyone has woken and cleared off.

Complex! Could we please get that all fleshed out in the changelog and kerneldoc?

Quoted 23 lines

Quoted 14 lines

Quoted 14 lines

kernel/sched/wait.c +12 -0

+/**
+ * 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)
+{

Quoted 8 lines

kernel/sched/wait.c void remove_wait_queue(wait_queue_head_t *q, wait_queue_t *wait) · line 92

Quoted 5 lines

kernel/sched/wait.c +5 -0

+	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.

Quoted 3 lines

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).

lockdep should have complained about the spin_lock().