All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jiri Slaby <jslaby@suse.cz>
To: stable@vger.kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>,
	Rusty Russell <rusty@rustcorp.com.au>,
	Jiri Slaby <jslaby@suse.cz>
Subject: [patch added to the 3.12 stable tree] module: Fix locking in symbol_put_addr()
Date: Mon,  9 Nov 2015 12:56:31 +0100	[thread overview]
Message-ID: <1447070198-11171-10-git-send-email-jslaby@suse.cz> (raw)
In-Reply-To: <1447070198-11171-1-git-send-email-jslaby@suse.cz>

From: Peter Zijlstra <peterz@infradead.org>

This patch has been added to the 3.12 stable tree. If you have any
objections, please let us know.

===============

commit 275d7d44d802ef271a42dc87ac091a495ba72fc5 upstream.

Poma (on the way to another bug) reported an assertion triggering:

  [<ffffffff81150529>] module_assert_mutex_or_preempt+0x49/0x90
  [<ffffffff81150822>] __module_address+0x32/0x150
  [<ffffffff81150956>] __module_text_address+0x16/0x70
  [<ffffffff81150f19>] symbol_put_addr+0x29/0x40
  [<ffffffffa04b77ad>] dvb_frontend_detach+0x7d/0x90 [dvb_core]

Laura Abbott <labbott@redhat.com> produced a patch which lead us to
inspect symbol_put_addr(). This function has a comment claiming it
doesn't need to disable preemption around the module lookup
because it holds a reference to the module it wants to find, which
therefore cannot go away.

This is wrong (and a false optimization too, preempt_disable() is really
rather cheap, and I doubt any of this is on uber critical paths,
otherwise it would've retained a pointer to the actual module anyway and
avoided the second lookup).

While its true that the module cannot go away while we hold a reference
on it, the data structure we do the lookup in very much _CAN_ change
while we do the lookup. Therefore fix the comment and add the
required preempt_disable().

Reported-by: poma <pomidorabelisima@gmail.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fixes: a6e6abd575fc ("module: remove module_text_address()")
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 kernel/module.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index a97785308f25..3e3f90d82ecc 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -950,11 +950,15 @@ void symbol_put_addr(void *addr)
 	if (core_kernel_text(a))
 		return;
 
-	/* module_text_address is safe here: we're supposed to have reference
-	 * to module from symbol_get, so it can't go away. */
+	/*
+	 * Even though we hold a reference on the module; we still need to
+	 * disable preemption in order to safely traverse the data structure.
+	 */
+	preempt_disable();
 	modaddr = __module_text_address(a);
 	BUG_ON(!modaddr);
 	module_put(modaddr);
+	preempt_enable();
 }
 EXPORT_SYMBOL_GPL(symbol_put_addr);
 
-- 
2.6.3


  parent reply	other threads:[~2015-11-09 11:56 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-11-09 11:56 [patch added to the 3.12 stable tree] spi: fix pointer-integer size mismatch warning Jiri Slaby
2015-11-09 11:56 ` [patch added to the 3.12 stable tree] drm/nouveau/gem: return only valid domain when there's only one Jiri Slaby
2015-11-09 11:56 ` [patch added to the 3.12 stable tree] drm/i915: Restore lost DPLL register write on gen2-4 Jiri Slaby
2015-11-11 22:25   ` Luis Henriques
2015-11-12 13:10     ` Jiri Slaby
2015-11-09 11:56 ` [patch added to the 3.12 stable tree] rbd: require stable pages if message data CRCs are enabled Jiri Slaby
2015-11-09 11:56 ` [patch added to the 3.12 stable tree] Revert "ARM64: unwind: Fix PC calculation" Jiri Slaby
2015-11-09 11:56 ` [patch added to the 3.12 stable tree] dm btree: fix leak of bufio-backed block in btree_split_beneath error path Jiri Slaby
2015-11-09 11:56 ` [patch added to the 3.12 stable tree] xhci: handle no ping response error properly Jiri Slaby
2015-11-09 11:56 ` [patch added to the 3.12 stable tree] xhci: Add spurious wakeup quirk for LynxPoint-LP controllers Jiri Slaby
2015-11-09 11:56 ` [patch added to the 3.12 stable tree] xen-blkfront: check for null drvdata in blkback_changed (XenbusStateClosing) Jiri Slaby
2015-11-09 11:56 ` Jiri Slaby [this message]
2015-11-09 11:56 ` [patch added to the 3.12 stable tree] crypto: api - Only abort operations on fatal signal Jiri Slaby
2015-11-09 11:56 ` [patch added to the 3.12 stable tree] md/raid1: submit_bio_wait() returns 0 on success Jiri Slaby
2015-11-09 11:56 ` [patch added to the 3.12 stable tree] md/raid10: " Jiri Slaby
2015-11-09 11:56 ` [patch added to the 3.12 stable tree] mvsas: Fix NULL pointer dereference in mvs_slot_task_free Jiri Slaby
2015-11-09 11:56 ` [patch added to the 3.12 stable tree] IB/cm: Fix rb-tree duplicate free and use-after-free Jiri Slaby
2015-11-09 11:56 ` [patch added to the 3.12 stable tree] serial: 8250_pci: Add support for 16 port Exar boards Jiri Slaby
2015-11-09 11:56 ` [patch added to the 3.12 stable tree] serial: 8250_pci: Add support for 12 " Jiri Slaby

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1447070198-11171-10-git-send-email-jslaby@suse.cz \
    --to=jslaby@suse.cz \
    --cc=peterz@infradead.org \
    --cc=rusty@rustcorp.com.au \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.