Skip to contentSkip to conversation

unwind: Fix a few edge cases in unwind_deferred_request()

PATCH New Josh Poimboeuf 1 participant 3 messages Feb 3 Userspace stack unwinding

lore
Share & tools
lore

Josh Poimboeuf

author maintainer
#0 lore raw

cover unwind: Fix a few edge cases in unwind_deferred_request()

Fixes for a couple of issues which were found in code review a few months ago but got overlooked.

Josh Poimboeuf (2):
unwind: Fix negative bit check in unwind_deferred_request()
unwind: Fix return value for already-queued case in
unwind_deferred_request()

 kernel/unwind/deferred.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)
Signature · 1 line
2.52.0
author maintainer
#1 lore raw

PATCH 1/2 unwind: Fix negative bit check in unwind_deferred_request()

The following warning will never trigger because 'bit' is unsigned:

	if (WARN_ON_ONCE(bit < 0))
		return -EINVAL;

That variable is used for two distinct purposes, so split it into two variables accordingly: an 'int bit' and an 'unsigned long bit_mask'. Rename a few other variables for consistency with the "*_mask" scheme.

Fixes 357eda2d7450 ("unwind deferred: Use SRCU unwind_deferred_task_work()")
Reported-by Julia Lawall <[email protected]>
Signed-off-by Josh Poimboeuf <[email protected]>
---
 kernel/unwind/deferred.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)
kernel/unwind/deferred.c +10 -12
index a88fb481c4a3..416af9b98bad 100644
@@ -230,10 +230,9 @@ void unwind_deferred_task_exit(struct task_struct *task)
 int unwind_deferred_request(struct unwind_work *work, u64 *cookie)
 {
 	struct unwind_task_info *info = &current->unwind_info;
+	unsigned long bit_mask, old_mask, new_mask;
 	int twa_mode = TWA_RESUME;
-	unsigned long old, bits;
-	unsigned long bit;
-	int ret;
+	int bit, ret;
 
 	*cookie = 0;
 

@@ -257,17 +256,16 @@ int unwind_deferred_request(struct unwind_work *work, u64 *cookie)
 	if (WARN_ON_ONCE(bit < 0))
 		return -EINVAL;
 
-	/* Only need the mask now */
-	bit = BIT(bit);
+	bit_mask = BIT(bit);
 
 	guard(irqsave)();
 
 	*cookie = get_cookie(info);
 
-	old = atomic_long_read(&info->unwind_mask);
+	old_mask = atomic_long_read(&info->unwind_mask);
 
 	/* Is this already queued or executed */
-	if (old & bit)
+	if (old_mask & bit_mask)
 		return 1;
 
 	/*

@@ -276,15 +274,15 @@ int unwind_deferred_request(struct unwind_work *work, u64 *cookie)
 	 * work's bit or PENDING was already set, then this is already queued
 	 * to have a callback.
 	 */
-	bits = UNWIND_PENDING | bit;
-	old = atomic_long_fetch_or(bits, &info->unwind_mask);
-	if (old & bits) {
+	new_mask = UNWIND_PENDING | bit_mask;
+	old_mask = atomic_long_fetch_or(new_mask, &info->unwind_mask);
+	if (old_mask & new_mask) {
 		/*
 		 * If the work's bit was set, whatever set it had better
 		 * have also set pending and queued a callback.
 		 */
-		WARN_ON_ONCE(!(old & UNWIND_PENDING));
-		return old & bit;
+		WARN_ON_ONCE(!(old_mask & UNWIND_PENDING));
+		return old_mask & bit_mask;
 	}
 
 	/* The work has been claimed, now schedule it. */
Signature · 1 line
2.52.0
author maintainer
#2 lore raw

PATCH 2/2 unwind: Fix return value for already-queued case in unwind_deferred_request()

In reply to Josh Poimboeuf

The unwind_deferred_request() return value is documented in the function header:

* Return: 0 if the callback successfully was queued.
* 1 if the callback is pending or was already executed.
* Negative if there's an error.

However, when the callback is already queued, it may return a non-1 value. Fix it to match the documented behavior.

Fixes be3d526a5b34 ("unwind deferred: Use bitmask to determine which callbacks to call")
Signed-off-by Josh Poimboeuf <[email protected]>
---
 kernel/unwind/deferred.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/unwind/deferred.c +1 -1
index 416af9b98bad..edf39c61130a 100644
@@ -282,7 +282,7 @@ int unwind_deferred_request(struct unwind_work *work, u64 *cookie)
 		 * have also set pending and queued a callback.
 		 */
 		WARN_ON_ONCE(!(old_mask & UNWIND_PENDING));
-		return old_mask & bit_mask;
+		return !!(old_mask & bit_mask);
 	}
 
 	/* The work has been claimed, now schedule it. */
Signature · 1 line
2.52.0