linux-kernel - Re: [RFC PATCH] cleanup: Introduce "acquire"/"drop" cleanup helpers for interruptible locks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6815790674f6d_1d6a2949d@dwillia2-xfh.jf.intel.com.notmuch>
Date: Fri, 2 May 2025 19:01:42 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>, Dan Williams
	<dan.j.williams@...el.com>
CC: <peterz@...radead.org>, David Lechner <dlechner@...libre.com>, "Fabio M.
 De Francesco" <fabio.maria.de.francesco@...ux.intel.com>, Ingo Molnar
	<mingo@...nel.org>, <linux-kernel@...r.kernel.org>,
	<linux-cxl@...r.kernel.org>
Subject: Re: [RFC PATCH] cleanup: Introduce "acquire"/"drop" cleanup helpers
 for interruptible locks

Linus Torvalds wrote:
[..]
> So I don't think you should take that early off-the-cuff remark of
> mine as some kind of solid advice.
> 
> I don't know what the best solution is, but it's almost certainly not
> this "write out a lot of boiler plate".
[..]
> 
> Your patch may end up fixing some of that failure of cond_guard(), but
> at the same time I note that your patch is horribly broken. Look at
> your change to drivers/cxl/core/mbox.c: you made it use
> 
> +       struct mutex *lock __drop(mutex_unlock) =
> +               mutex_intr_acquire(&mds->poison.lock);
> 
> but then you didn't remove the existing unlocking, so that function still has
> 
>         mutex_unlock(&mds->poison.lock);
> 
> at the end. So I think the patch shows that it's really easy to just
> mess up the conversion, and that this is certainly not a way to "fix"
> things.

So, I didn't hear a hard "no" in the above, which lead me to try again
with the incremental changes below, to remove the boilerplate and to
force any conversions to the new scheme to opt-in to a new type such that
you can not make the same mutex_unlock() mistake.

I do note that the original guard() helpers do not protect against that
mistake.

I will stop here though if this is not moving towards viability.

-- 8< --
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index e3e851813c2a..cec9dfb22567 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1394,8 +1394,7 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
 	int nr_records = 0;
 	int rc;
 
-	struct mutex *lock __drop(mutex_unlock) =
-		mutex_intr_acquire(&mds->poison.lock);
+	CLASS(mutex_intr_acquire, lock)(&mds->poison.lock);
 	if (IS_ERR(lock))
 		return PTR_ERR(lock);
 
@@ -1431,7 +1430,6 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
 		}
 	} while (po->flags & CXL_POISON_FLAG_MORE);
 
-	mutex_unlock(&mds->poison.lock);
 	return rc;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_mem_get_poison, "CXL");
@@ -1467,7 +1465,7 @@ int cxl_poison_state_init(struct cxl_memdev_state *mds)
 		return rc;
 	}
 
-	mutex_init(&mds->poison.lock);
+	mutex_acquire_init(&mds->poison.lock);
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_poison_state_init, "CXL");
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 3ec6b906371b..9b4ab5d1a7c4 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -257,7 +257,7 @@ struct cxl_poison_state {
 	u32 max_errors;
 	DECLARE_BITMAP(enabled_cmds, CXL_POISON_ENABLED_MAX);
 	struct cxl_mbox_poison_out *list_out;
-	struct mutex lock;  /* Protect reads of poison list */
+	struct mutex_acquire lock;  /* Protect reads of poison list */
 };
 
 /*
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 9fef4077604c..9684f0438364 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -64,6 +64,8 @@ do {									\
 	__mutex_init((mutex), #mutex, &__key);				\
 } while (0)
 
+#define mutex_acquire_init(lock) mutex_init(&(lock)->mutex)
+
 /**
  * mutex_init_with_key - initialize a mutex with a given lockdep key
  * @mutex: the mutex to be initialized
@@ -202,15 +204,23 @@ DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T))
 DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))
 DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T) == 0)
 
-DEFINE_DROP(mutex_unlock, struct mutex *, if (!IS_ERR_OR_NULL(_T)) mutex_unlock(_T))
-static inline __must_check struct mutex *mutex_intr_acquire(struct mutex *lock)
-{
-	int ret = mutex_lock_interruptible(lock);
-
-	if (ret)
-		return ERR_PTR(ret);
-	return lock;
-}
+struct mutex_acquire {
+	struct mutex mutex;
+};
+
+DEFINE_CLASS(mutex_intr_acquire, struct mutex_acquire *,
+		if (!IS_ERR_OR_NULL(_T)) mutex_unlock(&_T->mutex),
+		({
+			struct mutex_acquire *__lock;
+			int __ret = mutex_lock_interruptible(&acq->mutex);
+
+			if (__ret)
+				__lock = ERR_PTR(__ret);
+			else
+				__lock = (struct mutex_acquire *) &acq->mutex;
+			__lock;
+		}),
+		struct mutex_acquire *acq)
 
 extern unsigned long mutex_get_owner(struct mutex *lock);