Skip to content

Commit ec03de7

Browse files
committed
Merge tag 'locking-urgent-2024-09-29' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
Pull locking updates from Ingo Molnar: "lockdep: - Fix potential deadlock between lockdep and RCU (Zhiguo Niu) - Use str_plural() to address Coccinelle warning (Thorsten Blum) - Add debuggability enhancement (Luis Claudio R. Goncalves) static keys & calls: - Fix static_key_slow_dec() yet again (Peter Zijlstra) - Handle module init failure correctly in static_call_del_module() (Thomas Gleixner) - Replace pointless WARN_ON() in static_call_module_notify() (Thomas Gleixner) <linux/cleanup.h>: - Add usage and style documentation (Dan Williams) rwsems: - Move is_rwsem_reader_owned() and rwsem_owner() under CONFIG_DEBUG_RWSEMS (Waiman Long) atomic ops, x86: - Redeclare x86_32 arch_atomic64_{add,sub}() as void (Uros Bizjak) - Introduce the read64_nonatomic macro to x86_32 with cx8 (Uros Bizjak)" Signed-off-by: Ingo Molnar <mingo@kernel.org> * tag 'locking-urgent-2024-09-29' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip: locking/rwsem: Move is_rwsem_reader_owned() and rwsem_owner() under CONFIG_DEBUG_RWSEMS jump_label: Fix static_key_slow_dec() yet again static_call: Replace pointless WARN_ON() in static_call_module_notify() static_call: Handle module init failure correctly in static_call_del_module() locking/lockdep: Simplify character output in seq_line() lockdep: fix deadlock issue between lockdep and rcu lockdep: Use str_plural() to fix Coccinelle warning cleanup: Add usage and style documentation lockdep: suggest the fix for "lockdep bfs error:-1" on print_bfs_bug locking/atomic/x86: Redeclare x86_32 arch_atomic64_{add,sub}() as void locking/atomic/x86: Introduce the read64_nonatomic macro to x86_32 with cx8
2 parents 68e4b0e + ae39e0b commit ec03de7

File tree

10 files changed

+240
-44
lines changed

10 files changed

+240
-44
lines changed

Documentation/core-api/cleanup.rst

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
.. SPDX-License-Identifier: GPL-2.0
2+
3+
===========================
4+
Scope-based Cleanup Helpers
5+
===========================
6+
7+
.. kernel-doc:: include/linux/cleanup.h
8+
:doc: scope-based cleanup helpers

Documentation/core-api/index.rst

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ Library functionality that is used throughout the kernel.
3535

3636
kobject
3737
kref
38+
cleanup
3839
assoc_array
3940
xarray
4041
maple_tree

arch/x86/include/asm/atomic64_32.h

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -163,20 +163,18 @@ static __always_inline s64 arch_atomic64_dec_return(atomic64_t *v)
163163
}
164164
#define arch_atomic64_dec_return arch_atomic64_dec_return
165165

166-
static __always_inline s64 arch_atomic64_add(s64 i, atomic64_t *v)
166+
static __always_inline void arch_atomic64_add(s64 i, atomic64_t *v)
167167
{
168168
__alternative_atomic64(add, add_return,
169169
ASM_OUTPUT2("+A" (i), "+c" (v)),
170170
ASM_NO_INPUT_CLOBBER("memory"));
171-
return i;
172171
}
173172

174-
static __always_inline s64 arch_atomic64_sub(s64 i, atomic64_t *v)
173+
static __always_inline void arch_atomic64_sub(s64 i, atomic64_t *v)
175174
{
176175
__alternative_atomic64(sub, sub_return,
177176
ASM_OUTPUT2("+A" (i), "+c" (v)),
178177
ASM_NO_INPUT_CLOBBER("memory"));
179-
return i;
180178
}
181179

182180
static __always_inline void arch_atomic64_inc(atomic64_t *v)

arch/x86/lib/atomic64_cx8_32.S

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,11 @@
1616
cmpxchg8b (\reg)
1717
.endm
1818

19+
.macro read64_nonatomic reg
20+
movl (\reg), %eax
21+
movl 4(\reg), %edx
22+
.endm
23+
1924
SYM_FUNC_START(atomic64_read_cx8)
2025
read64 %ecx
2126
RET
@@ -51,7 +56,7 @@ SYM_FUNC_START(atomic64_\func\()_return_cx8)
5156
movl %edx, %edi
5257
movl %ecx, %ebp
5358

54-
read64 %ecx
59+
read64_nonatomic %ecx
5560
1:
5661
movl %eax, %ebx
5762
movl %edx, %ecx
@@ -79,7 +84,7 @@ addsub_return sub sub sbb
7984
SYM_FUNC_START(atomic64_\func\()_return_cx8)
8085
pushl %ebx
8186

82-
read64 %esi
87+
read64_nonatomic %esi
8388
1:
8489
movl %eax, %ebx
8590
movl %edx, %ecx

include/linux/cleanup.h

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,142 @@
44

55
#include <linux/compiler.h>
66

7+
/**
8+
* DOC: scope-based cleanup helpers
9+
*
10+
* The "goto error" pattern is notorious for introducing subtle resource
11+
* leaks. It is tedious and error prone to add new resource acquisition
12+
* constraints into code paths that already have several unwind
13+
* conditions. The "cleanup" helpers enable the compiler to help with
14+
* this tedium and can aid in maintaining LIFO (last in first out)
15+
* unwind ordering to avoid unintentional leaks.
16+
*
17+
* As drivers make up the majority of the kernel code base, here is an
18+
* example of using these helpers to clean up PCI drivers. The target of
19+
* the cleanups are occasions where a goto is used to unwind a device
20+
* reference (pci_dev_put()), or unlock the device (pci_dev_unlock())
21+
* before returning.
22+
*
23+
* The DEFINE_FREE() macro can arrange for PCI device references to be
24+
* dropped when the associated variable goes out of scope::
25+
*
26+
* DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))
27+
* ...
28+
* struct pci_dev *dev __free(pci_dev_put) =
29+
* pci_get_slot(parent, PCI_DEVFN(0, 0));
30+
*
31+
* The above will automatically call pci_dev_put() if @dev is non-NULL
32+
* when @dev goes out of scope (automatic variable scope). If a function
33+
* wants to invoke pci_dev_put() on error, but return @dev (i.e. without
34+
* freeing it) on success, it can do::
35+
*
36+
* return no_free_ptr(dev);
37+
*
38+
* ...or::
39+
*
40+
* return_ptr(dev);
41+
*
42+
* The DEFINE_GUARD() macro can arrange for the PCI device lock to be
43+
* dropped when the scope where guard() is invoked ends::
44+
*
45+
* DEFINE_GUARD(pci_dev, struct pci_dev *, pci_dev_lock(_T), pci_dev_unlock(_T))
46+
* ...
47+
* guard(pci_dev)(dev);
48+
*
49+
* The lifetime of the lock obtained by the guard() helper follows the
50+
* scope of automatic variable declaration. Take the following example::
51+
*
52+
* func(...)
53+
* {
54+
* if (...) {
55+
* ...
56+
* guard(pci_dev)(dev); // pci_dev_lock() invoked here
57+
* ...
58+
* } // <- implied pci_dev_unlock() triggered here
59+
* }
60+
*
61+
* Observe the lock is held for the remainder of the "if ()" block not
62+
* the remainder of "func()".
63+
*
64+
* Now, when a function uses both __free() and guard(), or multiple
65+
* instances of __free(), the LIFO order of variable definition order
66+
* matters. GCC documentation says:
67+
*
68+
* "When multiple variables in the same scope have cleanup attributes,
69+
* at exit from the scope their associated cleanup functions are run in
70+
* reverse order of definition (last defined, first cleanup)."
71+
*
72+
* When the unwind order matters it requires that variables be defined
73+
* mid-function scope rather than at the top of the file. Take the
74+
* following example and notice the bug highlighted by "!!"::
75+
*
76+
* LIST_HEAD(list);
77+
* DEFINE_MUTEX(lock);
78+
*
79+
* struct object {
80+
* struct list_head node;
81+
* };
82+
*
83+
* static struct object *alloc_add(void)
84+
* {
85+
* struct object *obj;
86+
*
87+
* lockdep_assert_held(&lock);
88+
* obj = kzalloc(sizeof(*obj), GFP_KERNEL);
89+
* if (obj) {
90+
* LIST_HEAD_INIT(&obj->node);
91+
* list_add(obj->node, &list):
92+
* }
93+
* return obj;
94+
* }
95+
*
96+
* static void remove_free(struct object *obj)
97+
* {
98+
* lockdep_assert_held(&lock);
99+
* list_del(&obj->node);
100+
* kfree(obj);
101+
* }
102+
*
103+
* DEFINE_FREE(remove_free, struct object *, if (_T) remove_free(_T))
104+
* static int init(void)
105+
* {
106+
* struct object *obj __free(remove_free) = NULL;
107+
* int err;
108+
*
109+
* guard(mutex)(&lock);
110+
* obj = alloc_add();
111+
*
112+
* if (!obj)
113+
* return -ENOMEM;
114+
*
115+
* err = other_init(obj);
116+
* if (err)
117+
* return err; // remove_free() called without the lock!!
118+
*
119+
* no_free_ptr(obj);
120+
* return 0;
121+
* }
122+
*
123+
* That bug is fixed by changing init() to call guard() and define +
124+
* initialize @obj in this order::
125+
*
126+
* guard(mutex)(&lock);
127+
* struct object *obj __free(remove_free) = alloc_add();
128+
*
129+
* Given that the "__free(...) = NULL" pattern for variables defined at
130+
* the top of the function poses this potential interdependency problem
131+
* the recommendation is to always define and assign variables in one
132+
* statement and not group variable definitions at the top of the
133+
* function when __free() is used.
134+
*
135+
* Lastly, given that the benefit of cleanup helpers is removal of
136+
* "goto", and that the "goto" statement can jump between scopes, the
137+
* expectation is that usage of "goto" and cleanup helpers is never
138+
* mixed in the same function. I.e. for a given routine, convert all
139+
* resources that need a "goto" cleanup to scope-based cleanup, or
140+
* convert none of them.
141+
*/
142+
7143
/*
8144
* DEFINE_FREE(name, type, free):
9145
* simple helper macro that defines the required wrapper for a __free()

kernel/jump_label.c

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ bool static_key_slow_inc_cpuslocked(struct static_key *key)
168168
jump_label_update(key);
169169
/*
170170
* Ensure that when static_key_fast_inc_not_disabled() or
171-
* static_key_slow_try_dec() observe the positive value,
171+
* static_key_dec_not_one() observe the positive value,
172172
* they must also observe all the text changes.
173173
*/
174174
atomic_set_release(&key->enabled, 1);
@@ -250,7 +250,7 @@ void static_key_disable(struct static_key *key)
250250
}
251251
EXPORT_SYMBOL_GPL(static_key_disable);
252252

253-
static bool static_key_slow_try_dec(struct static_key *key)
253+
static bool static_key_dec_not_one(struct static_key *key)
254254
{
255255
int v;
256256

@@ -274,6 +274,14 @@ static bool static_key_slow_try_dec(struct static_key *key)
274274
* enabled. This suggests an ordering problem on the user side.
275275
*/
276276
WARN_ON_ONCE(v < 0);
277+
278+
/*
279+
* Warn about underflow, and lie about success in an attempt to
280+
* not make things worse.
281+
*/
282+
if (WARN_ON_ONCE(v == 0))
283+
return true;
284+
277285
if (v <= 1)
278286
return false;
279287
} while (!likely(atomic_try_cmpxchg(&key->enabled, &v, v - 1)));
@@ -284,15 +292,27 @@ static bool static_key_slow_try_dec(struct static_key *key)
284292
static void __static_key_slow_dec_cpuslocked(struct static_key *key)
285293
{
286294
lockdep_assert_cpus_held();
295+
int val;
287296

288-
if (static_key_slow_try_dec(key))
297+
if (static_key_dec_not_one(key))
289298
return;
290299

291300
guard(mutex)(&jump_label_mutex);
292-
if (atomic_cmpxchg(&key->enabled, 1, 0) == 1)
301+
val = atomic_read(&key->enabled);
302+
/*
303+
* It should be impossible to observe -1 with jump_label_mutex held,
304+
* see static_key_slow_inc_cpuslocked().
305+
*/
306+
if (WARN_ON_ONCE(val == -1))
307+
return;
308+
/*
309+
* Cannot already be 0, something went sideways.
310+
*/
311+
if (WARN_ON_ONCE(val == 0))
312+
return;
313+
314+
if (atomic_dec_and_test(&key->enabled))
293315
jump_label_update(key);
294-
else
295-
WARN_ON_ONCE(!static_key_slow_try_dec(key));
296316
}
297317

298318
static void __static_key_slow_dec(struct static_key *key)
@@ -329,7 +349,7 @@ void __static_key_slow_dec_deferred(struct static_key *key,
329349
{
330350
STATIC_KEY_CHECK_USE(key);
331351

332-
if (static_key_slow_try_dec(key))
352+
if (static_key_dec_not_one(key))
333353
return;
334354

335355
schedule_delayed_work(work, timeout);

0 commit comments

Comments
 (0)