Skip to content

Commit 377563c

Browse files
Tzung-Bi Shihgregkh
Tzung-Bi Shih
authored and
gregkh
committed
revocable: fix SRCU index corruption by requiring caller-provided storage
The struct revocable handle stores the SRCU read-side index (idx) for the duration of a resource access. If multiple threads share the same struct revocable instance, they race on writing to the idx field, corrupting the SRCU state and potentially causing unsafe unlocks. Refactor the API to replace revocable_alloc()/revocable_free() with revocable_init()/revocable_deinit(). This change requires the caller to provide the storage for struct revocable. By moving storage ownership to the caller, the API ensures that concurrent users maintain their own private idx storage, eliminating the race condition. Reported-by: Johan Hovold <johan@kernel.org> Closes: https://lore.kernel.org/all/20260124170535.11756-4-johan@kernel.org/ Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org> Link: https://patch.msgid.link/20260129143733.45618-4-tzungbi@kernel.org Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
1 parent a243f7f commit 377563c

5 files changed

Lines changed: 102 additions & 113 deletions

File tree

‎Documentation/driver-api/driver-model/revocable.rst‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -81,14 +81,14 @@ For Resource Providers
8181

8282
For Resource Consumers
8383
----------------------
84-
.. kernel-doc:: drivers/base/revocable.c
84+
.. kernel-doc:: include/linux/revocable.h
8585
:identifiers: revocable
8686

8787
.. kernel-doc:: drivers/base/revocable.c
88-
:identifiers: revocable_alloc
88+
:identifiers: revocable_init
8989

9090
.. kernel-doc:: drivers/base/revocable.c
91-
:identifiers: revocable_free
91+
:identifiers: revocable_deinit
9292

9393
.. kernel-doc:: drivers/base/revocable.c
9494
:identifiers: revocable_try_access
@@ -104,11 +104,11 @@ Example Usage
104104

105105
.. code-block:: c
106106
107-
void consumer_use_resource(struct revocable *rev)
107+
void consumer_use_resource(struct revocable_provider *rp)
108108
{
109109
struct foo_resource *res;
110110
111-
REVOCABLE_TRY_ACCESS_WITH(rev, res);
111+
REVOCABLE_TRY_ACCESS_WITH(rp, res);
112112
// Always check if the resource is valid.
113113
if (!res) {
114114
pr_warn("Resource is not available\n");
@@ -129,11 +129,11 @@ Example Usage
129129

130130
.. code-block:: c
131131
132-
void consumer_use_resource(struct revocable *rev)
132+
void consumer_use_resource(struct revocable_provider *rp)
133133
{
134134
struct foo_resource *res;
135135
136-
REVOCABLE_TRY_ACCESS_SCOPED(rev, res) {
136+
REVOCABLE_TRY_ACCESS_SCOPED(rp, res) {
137137
// Always check if the resource is valid.
138138
if (!res) {
139139
pr_warn("Resource is not available\n");

‎drivers/base/revocable.c‎

Lines changed: 12 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -73,16 +73,6 @@ struct revocable_provider {
7373
struct rcu_head rcu;
7474
};
7575

76-
/**
77-
* struct revocable - A handle for resource consumer.
78-
* @rp: The pointer of resource provider.
79-
* @idx: The index for the RCU critical section.
80-
*/
81-
struct revocable {
82-
struct revocable_provider *rp;
83-
int idx;
84-
};
85-
8676
/**
8777
* revocable_provider_alloc() - Allocate struct revocable_provider.
8878
* @res: The pointer of resource.
@@ -145,20 +135,20 @@ void revocable_provider_revoke(struct revocable_provider __rcu **rp_ptr)
145135
EXPORT_SYMBOL_GPL(revocable_provider_revoke);
146136

147137
/**
148-
* revocable_alloc() - Allocate struct revocable.
138+
* revocable_init() - Initialize struct revocable.
149139
* @_rp: The pointer of resource provider.
140+
* @rev: The pointer of resource consumer.
150141
*
151142
* This holds a refcount to the resource provider.
152143
*
153-
* Return: The pointer of struct revocable. NULL on errors.
144+
* Return: 0 on success, -errno otherwise.
154145
*/
155-
struct revocable *revocable_alloc(struct revocable_provider __rcu *_rp)
146+
int revocable_init(struct revocable_provider __rcu *_rp, struct revocable *rev)
156147
{
157148
struct revocable_provider *rp;
158-
struct revocable *rev;
159149

160150
if (!_rp)
161-
return NULL;
151+
return -ENODEV;
162152

163153
/*
164154
* Enter a read-side critical section.
@@ -170,43 +160,36 @@ struct revocable *revocable_alloc(struct revocable_provider __rcu *_rp)
170160
rp = rcu_dereference(_rp);
171161
if (!rp)
172162
/* The revocable provider has been revoked. */
173-
return NULL;
163+
return -ENODEV;
174164

175165
if (!kref_get_unless_zero(&rp->kref))
176166
/*
177167
* The revocable provider is releasing (i.e.,
178168
* revocable_provider_release() has been called).
179169
*/
180-
return NULL;
170+
return -ENODEV;
181171
}
182172
/* At this point, `rp` is safe to access as holding a kref of it */
183173

184-
rev = kzalloc(sizeof(*rev), GFP_KERNEL);
185-
if (!rev) {
186-
kref_put(&rp->kref, revocable_provider_release);
187-
return NULL;
188-
}
189-
190174
rev->rp = rp;
191-
return rev;
175+
return 0;
192176
}
193-
EXPORT_SYMBOL_GPL(revocable_alloc);
177+
EXPORT_SYMBOL_GPL(revocable_init);
194178

195179
/**
196-
* revocable_free() - Free struct revocable.
180+
* revocable_deinit() - Deinitialize struct revocable.
197181
* @rev: The pointer of struct revocable.
198182
*
199183
* This drops a refcount to the resource provider. If it is the final
200184
* reference, revocable_provider_release() will be called to free the struct.
201185
*/
202-
void revocable_free(struct revocable *rev)
186+
void revocable_deinit(struct revocable *rev)
203187
{
204188
struct revocable_provider *rp = rev->rp;
205189

206190
kref_put(&rp->kref, revocable_provider_release);
207-
kfree(rev);
208191
}
209-
EXPORT_SYMBOL_GPL(revocable_free);
192+
EXPORT_SYMBOL_GPL(revocable_deinit);
210193

211194
/**
212195
* revocable_try_access() - Try to access the resource.

‎drivers/base/revocable_test.c‎

Lines changed: 31 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
* - Try Access Macro: Same as "Revocation" but uses the
1616
* REVOCABLE_TRY_ACCESS_WITH() and REVOCABLE_TRY_ACCESS_SCOPED().
1717
*
18-
* - Provider Use-after-free: Verifies revocable_alloc() correctly handles
18+
* - Provider Use-after-free: Verifies revocable_init() correctly handles
1919
* race conditions where the provider is being released.
2020
*/
2121

@@ -26,93 +26,85 @@
2626
static void revocable_test_basic(struct kunit *test)
2727
{
2828
struct revocable_provider __rcu *rp;
29-
struct revocable *rev;
29+
struct revocable rev;
3030
void *real_res = (void *)0x12345678, *res;
31+
int ret;
3132

3233
rp = revocable_provider_alloc(real_res);
3334
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp);
3435

35-
rev = revocable_alloc(rp);
36-
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rev);
36+
ret = revocable_init(rp, &rev);
37+
KUNIT_ASSERT_EQ(test, ret, 0);
3738

38-
res = revocable_try_access(rev);
39+
res = revocable_try_access(&rev);
3940
KUNIT_EXPECT_PTR_EQ(test, res, real_res);
40-
revocable_withdraw_access(rev);
41+
revocable_withdraw_access(&rev);
4142

42-
revocable_free(rev);
43+
revocable_deinit(&rev);
4344
revocable_provider_revoke(&rp);
4445
KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
4546
}
4647

4748
static void revocable_test_revocation(struct kunit *test)
4849
{
4950
struct revocable_provider __rcu *rp;
50-
struct revocable *rev;
51+
struct revocable rev;
5152
void *real_res = (void *)0x12345678, *res;
53+
int ret;
5254

5355
rp = revocable_provider_alloc(real_res);
5456
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp);
5557

56-
rev = revocable_alloc(rp);
57-
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rev);
58+
ret = revocable_init(rp, &rev);
59+
KUNIT_ASSERT_EQ(test, ret, 0);
5860

59-
res = revocable_try_access(rev);
61+
res = revocable_try_access(&rev);
6062
KUNIT_EXPECT_PTR_EQ(test, res, real_res);
61-
revocable_withdraw_access(rev);
63+
revocable_withdraw_access(&rev);
6264

6365
revocable_provider_revoke(&rp);
6466
KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
6567

66-
res = revocable_try_access(rev);
68+
res = revocable_try_access(&rev);
6769
KUNIT_EXPECT_PTR_EQ(test, res, NULL);
68-
revocable_withdraw_access(rev);
70+
revocable_withdraw_access(&rev);
6971

70-
revocable_free(rev);
72+
revocable_deinit(&rev);
7173
}
7274

7375
static void revocable_test_try_access_macro(struct kunit *test)
7476
{
7577
struct revocable_provider __rcu *rp;
76-
struct revocable *rev;
7778
void *real_res = (void *)0x12345678, *res;
7879

7980
rp = revocable_provider_alloc(real_res);
8081
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp);
8182

82-
rev = revocable_alloc(rp);
83-
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rev);
84-
8583
{
86-
REVOCABLE_TRY_ACCESS_WITH(rev, res);
84+
REVOCABLE_TRY_ACCESS_WITH(rp, res);
8785
KUNIT_EXPECT_PTR_EQ(test, res, real_res);
8886
}
8987

9088
revocable_provider_revoke(&rp);
9189
KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
9290

9391
{
94-
REVOCABLE_TRY_ACCESS_WITH(rev, res);
92+
REVOCABLE_TRY_ACCESS_WITH(rp, res);
9593
KUNIT_EXPECT_PTR_EQ(test, res, NULL);
9694
}
97-
98-
revocable_free(rev);
9995
}
10096

10197
static void revocable_test_try_access_macro2(struct kunit *test)
10298
{
10399
struct revocable_provider __rcu *rp;
104-
struct revocable *rev;
105100
void *real_res = (void *)0x12345678, *res;
106101
bool accessed;
107102

108103
rp = revocable_provider_alloc(real_res);
109104
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp);
110105

111-
rev = revocable_alloc(rp);
112-
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rev);
113-
114106
accessed = false;
115-
REVOCABLE_TRY_ACCESS_SCOPED(rev, res) {
107+
REVOCABLE_TRY_ACCESS_SCOPED(rp, res) {
116108
KUNIT_EXPECT_PTR_EQ(test, res, real_res);
117109
accessed = true;
118110
}
@@ -122,32 +114,31 @@ static void revocable_test_try_access_macro2(struct kunit *test)
122114
KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
123115

124116
accessed = false;
125-
REVOCABLE_TRY_ACCESS_SCOPED(rev, res) {
117+
REVOCABLE_TRY_ACCESS_SCOPED(rp, res) {
126118
KUNIT_EXPECT_PTR_EQ(test, res, NULL);
127119
accessed = true;
128120
}
129121
KUNIT_EXPECT_TRUE(test, accessed);
130-
131-
revocable_free(rev);
132122
}
133123

134124
static void revocable_test_provider_use_after_free(struct kunit *test)
135125
{
136126
struct revocable_provider __rcu *rp;
137127
struct revocable_provider *old_rp;
128+
struct revocable rev;
138129
void *real_res = (void *)0x12345678;
139-
struct revocable *rev;
130+
int ret;
140131

141132
rp = revocable_provider_alloc(real_res);
142133
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, rp);
143134

144-
rev = revocable_alloc(NULL);
145-
KUNIT_EXPECT_PTR_EQ(test, rev, NULL);
135+
ret = revocable_init(NULL, &rev);
136+
KUNIT_EXPECT_NE(test, ret, 0);
146137

147138
/* Simulate the provider has been freed. */
148139
old_rp = rcu_replace_pointer(rp, NULL, 1);
149-
rev = revocable_alloc(rp);
150-
KUNIT_EXPECT_PTR_EQ(test, rev, NULL);
140+
ret = revocable_init(rp, &rev);
141+
KUNIT_EXPECT_NE(test, ret, 0);
151142
rcu_replace_pointer(rp, old_rp, 1);
152143

153144
struct {
@@ -159,12 +150,14 @@ static void revocable_test_provider_use_after_free(struct kunit *test)
159150

160151
/* Simulate the provider is releasing. */
161152
refcount_set(&rp_internal->kref.refcount, 0);
162-
rev = revocable_alloc(rp);
163-
KUNIT_EXPECT_PTR_EQ(test, rev, NULL);
153+
ret = revocable_init(rp, &rev);
154+
KUNIT_EXPECT_NE(test, ret, 0);
164155
refcount_set(&rp_internal->kref.refcount, 1);
165156

166157
revocable_provider_revoke(&rp);
167158
KUNIT_EXPECT_PTR_EQ(test, unrcu_pointer(rp), NULL);
159+
ret = revocable_init(rp, &rev);
160+
KUNIT_EXPECT_NE(test, ret, 0);
168161
}
169162

170163
static struct kunit_case revocable_test_cases[] = {

0 commit comments

Comments
 (0)