diff mbox series

printk/nbcon: Add assert that CPU migration is disabled when calling nbcon_context_try_acquire()

Message ID 20230920125136.15504-1-pmladek@suse.com
State New
Headers
Series printk/nbcon: Add assert that CPU migration is disabled when calling nbcon_context_try_acquire() |

Commit Message

Petr Mladek Sept. 20, 2023, 12:51 p.m. UTC 
  The nbcon console is locked when the 'prio' and 'cpu' fields in
console->nbcon_state stay the same. The locking algorithm would
break when the locked context got migrated to another CPU.

Add assert into nbcon_context_try_acquire(). It would warn when
the function is called in a context where the CPU migration
is possible.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
I propose this patch on top of the patchset adding basic support 
for nbcon consoles, see
https://lore.kernel.org/r/20230916192007.608398-1-john.ogness@linutronix.de

Or another way, it is on top of rework/nbcon-base branch in
printk/linux.git tree.
---
kernel/printk/nbcon.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

John Ogness Sept. 20, 2023, 1 p.m. UTC | #1 
On 2023-09-20, Petr Mladek <pmladek@suse.com> wrote:
> The nbcon console is locked when the 'prio' and 'cpu' fields in
> console->nbcon_state stay the same. The locking algorithm would
> break when the locked context got migrated to another CPU.
>
> Add assert into nbcon_context_try_acquire(). It would warn when
> the function is called in a context where the CPU migration
> is possible.

static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
{
        unsigned int cpu = smp_processor_id();

Doesn't smp_processor_id() already provide the necessary checking and
warning?

John
kernel test robot Sept. 20, 2023, 6:24 p.m. UTC | #2 
Hi Petr,

kernel test robot noticed the following build errors:

[auto build test ERROR on next-20230920]
[cannot apply to linus/master v6.6-rc2 v6.6-rc1 v6.5 v6.6-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Petr-Mladek/printk-nbcon-Add-assert-that-CPU-migration-is-disabled-when-calling-nbcon_context_try_acquire/20230920-205406
base:   next-20230920
patch link:    https://lore.kernel.org/r/20230920125136.15504-1-pmladek%40suse.com
patch subject: [PATCH] printk/nbcon: Add assert that CPU migration is disabled when calling nbcon_context_try_acquire()
config: um-allnoconfig (https://download.01.org/0day-ci/archive/20230921/202309210239.GwxqY2v3-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230921/202309210239.GwxqY2v3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309210239.GwxqY2v3-lkp@intel.com/

All errors (new ones prefixed by >>):

>> kernel/printk/nbcon.c:595:2: error: call to undeclared function 'nbcon_assert_cpu_migration_disabled'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     595 |         nbcon_assert_cpu_migration_disabled();
         |         ^
   1 error generated.


vim +/nbcon_assert_cpu_migration_disabled +595 kernel/printk/nbcon.c

   575	
   576	/**
   577	 * nbcon_context_try_acquire - Try to acquire nbcon console
   578	 * @ctxt:	The context of the caller
   579	 *
   580	 * Return:	True if the console was acquired. False otherwise.
   581	 *
   582	 * If the caller allowed an unsafe hostile takeover, on success the
   583	 * caller should check the current console state to see if it is
   584	 * in an unsafe state. Otherwise, on success the caller may assume
   585	 * the console is not in an unsafe state.
   586	 */
   587	__maybe_unused
   588	static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
   589	{
   590		unsigned int cpu = smp_processor_id();
   591		struct console *con = ctxt->console;
   592		struct nbcon_state cur;
   593		int err;
   594	
 > 595		nbcon_assert_cpu_migration_disabled();
   596	
   597		nbcon_state_read(con, &cur);
   598	try_again:
   599		err = nbcon_context_try_acquire_direct(ctxt, &cur);
   600		if (err != -EBUSY)
   601			goto out;
   602	
   603		err = nbcon_context_try_acquire_handover(ctxt, &cur);
   604		if (err == -EAGAIN)
   605			goto try_again;
   606		if (err != -EBUSY)
   607			goto out;
   608	
   609		err = nbcon_context_try_acquire_hostile(ctxt, &cur);
   610	out:
   611		if (err)
   612			return false;
   613	
   614		/* Acquire succeeded. */
   615	
   616		/* Assign the appropriate buffer for this context. */
   617		if (atomic_read(&panic_cpu) == cpu)
   618			ctxt->pbufs = &panic_nbcon_pbufs;
   619		else
   620			ctxt->pbufs = con->pbufs;
   621	
   622		/* Set the record sequence for this context to print. */
   623		ctxt->seq = nbcon_seq_read(ctxt->console);
   624	
   625		return true;
   626	}
   627
diff mbox series

Patch

diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index b96077152f49..2c2e98d61660 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -234,6 +234,19 @@  static void nbcon_seq_try_update(struct nbcon_context *ctxt, u64 new_seq)
 	}
 }
 
+#ifdef CONFIG_PROVE_LOCKING
+static inline void nbcon_assert_cpu_migration_disabled(void)
+{
+	WARN_ON_ONCE(IS_ENABLED(CONFIG_SMP) &&
+		     __lockdep_enabled &&
+		     this_cpu_read(hardirqs_enabled) &&
+		     preempt_count() == 0 &&
+		     !current->migration_disabled);
+}
+#else
+#define nbcon_assert_cpu_migration(void) {}
+#endif
+
 /**
  * nbcon_context_try_acquire_direct - Try to acquire directly
  * @ctxt:	The context of the caller
@@ -579,6 +592,8 @@  static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
 	struct nbcon_state cur;
 	int err;
 
+	nbcon_assert_cpu_migration_disabled();
+
 	nbcon_state_read(con, &cur);
 try_again:
 	err = nbcon_context_try_acquire_direct(ctxt, &cur);