From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Morton Subject: Re: [PATCH] poll/wait/md: allow module to safely support 'poll' on /proc files Date: Tue, 11 Mar 2014 20:03:31 -0700 Message-ID: <20140311200331.2a841ea9.akpm@linux-foundation.org> References: <20140312133638.1fd84632@notabene.brown> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140312133638.1fd84632@notabene.brown> Sender: linux-raid-owner@vger.kernel.org To: NeilBrown Cc: Ingo Molnar , Peter Zijlstra , Alexander Viro , linux-raid@vger.kernel.org, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, majianpeng List-Id: linux-raid.ids On Wed, 12 Mar 2014 13:36:38 +1100 NeilBrown 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. > 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? > +/** > + * 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. > + list_del_init(&wait->task_list); > + spin_unlock_irqrestore(&q->lock, flags); > + } > + rcu_read_unlock(); > +} > +EXPORT_SYMBOL(remove_wait_queue_purgeable);