Skip to content

Commit 04acc84

Browse files
implicitfieldimplicitfield
committed
Kernel/FUSEDevice: Improve locking
With this patch, the locking now works both in the way it was intended to, and also in the general sense :P See, the old locking scheme wasn't conceptually sound, as it didn't properly lock both m_instances and m_closing_instances, which could have led to unpredictable results. However, the reality was actually even worse than that, because the locks simply didn't work at all for their intended purpose. This is a consequence of SpinlockProtected using a RecursiveSpinlock under the hood, which isn't appropriate for what we are trying to do here. Proof of this can be seen in the send_request_and_wait_for_a_reply function, which - as written - should have never worked in the first place, because it never released the lock on m_instances, but it still expected to be able to observe a change in whatever instance it was looking at.
1 parent e5f88dc commit 04acc84

File tree

2 files changed

+97
-104
lines changed

2 files changed

+97
-104
lines changed

‎Kernel/Devices/FUSEDevice.cpp‎

Lines changed: 94 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -27,37 +27,33 @@ UNMAP_AFTER_INIT FUSEDevice::~FUSEDevice() = default;
2727

2828
ErrorOr<void> FUSEDevice::initialize_instance(OpenFileDescription const& description)
2929
{
30-
return m_instances.with([&](auto& instances) -> ErrorOr<void> {
31-
VERIFY(!instances.contains(&description));
32-
TRY(instances.try_set(&description, {}));
33-
return {};
34-
});
30+
SpinlockLocker locker(m_lock);
31+
32+
VERIFY(!m_instances.contains(&description));
33+
TRY(m_instances.try_set(&description, {}));
34+
return {};
3535
}
3636

3737
bool FUSEDevice::can_read(OpenFileDescription const& description, u64) const
3838
{
39-
bool drop = m_closing_instances.with([&](auto& closing_instances) {
40-
auto iterator = closing_instances.find(&description);
41-
return iterator != closing_instances.end();
42-
});
39+
SpinlockLocker locker(m_lock);
4340

44-
if (drop)
41+
auto iterator = m_closing_instances.find(&description);
42+
if (iterator != m_closing_instances.end())
4543
return true;
4644

47-
return m_instances.with([&](auto& instances) {
48-
auto instance_iterator = instances.find(&description);
49-
if (instance_iterator == instances.end()) {
50-
VERIFY(instances.is_empty());
51-
return false;
52-
}
53-
54-
auto const& requests_for_instance = (*instance_iterator).value;
55-
for (auto const& request : requests_for_instance.in_reverse()) {
56-
if (request.buffer_ready)
57-
return true;
58-
}
45+
auto instance_iterator = m_instances.find(&description);
46+
if (instance_iterator == m_instances.end()) {
47+
VERIFY(m_instances.is_empty());
5948
return false;
60-
});
49+
}
50+
51+
auto const& requests_for_instance = (*instance_iterator).value;
52+
for (auto const& request : requests_for_instance.in_reverse()) {
53+
if (request.buffer_ready)
54+
return true;
55+
}
56+
return false;
6157
}
6258

6359
bool FUSEDevice::can_write(OpenFileDescription const&, u64) const
@@ -67,120 +63,116 @@ bool FUSEDevice::can_write(OpenFileDescription const&, u64) const
6763

6864
ErrorOr<size_t> FUSEDevice::read(OpenFileDescription& description, u64, UserOrKernelBuffer& buffer, size_t size)
6965
{
70-
TRY(m_closing_instances.with([&](auto& closing_instances) -> ErrorOr<void> {
71-
bool removed = closing_instances.remove_first_matching([&](auto const* closing_description) { return closing_description == &description; });
72-
if (removed)
73-
return Error::from_errno(ENODEV);
66+
SpinlockLocker locker(m_lock);
7467

75-
return {};
76-
}));
68+
bool removed = m_closing_instances.remove_first_matching([&](auto const* closing_description) { return closing_description == &description; });
69+
if (removed)
70+
return Error::from_errno(ENODEV);
7771

7872
if (size < 0x21000)
7973
return Error::from_errno(EIO);
8074

81-
return m_instances.with([&](auto& instances) -> ErrorOr<size_t> {
82-
auto instance_iterator = instances.find(&description);
83-
if (instance_iterator == instances.end())
84-
return Error::from_errno(ENODEV);
75+
auto instance_iterator = m_instances.find(&description);
76+
if (instance_iterator == m_instances.end())
77+
return Error::from_errno(ENODEV);
8578

86-
auto& requests_for_instance = (*instance_iterator).value;
79+
auto& requests_for_instance = (*instance_iterator).value;
8780

88-
for (auto& request : requests_for_instance.in_reverse()) {
89-
if (!request.buffer_ready)
90-
continue;
81+
for (auto& request : requests_for_instance.in_reverse()) {
82+
if (!request.buffer_ready)
83+
continue;
9184

92-
TRY(buffer.write(request.pending_request->bytes()));
93-
request.buffer_ready = false;
94-
return request.pending_request->size();
95-
}
85+
TRY(buffer.write(request.pending_request->bytes()));
86+
request.buffer_ready = false;
87+
return request.pending_request->size();
88+
}
9689

97-
return Error::from_errno(ENOENT);
98-
});
90+
return Error::from_errno(ENOENT);
9991
}
10092

10193
ErrorOr<size_t> FUSEDevice::write(OpenFileDescription& description, u64, UserOrKernelBuffer const& buffer, size_t size)
10294
{
103-
return m_instances.with([&](auto& instances) -> ErrorOr<size_t> {
104-
auto instance_iterator = instances.find(&description);
95+
SpinlockLocker locker(m_lock);
10596

106-
if (instance_iterator == instances.end())
107-
return Error::from_errno(ENODEV);
97+
auto instance_iterator = m_instances.find(&description);
98+
if (instance_iterator == m_instances.end())
99+
return Error::from_errno(ENODEV);
108100

109-
auto& requests_for_instance = (*instance_iterator).value;
110-
auto& instance = requests_for_instance.last();
101+
auto& requests_for_instance = (*instance_iterator).value;
102+
auto& instance = requests_for_instance.last();
111103

112-
if (instance.expecting_header) {
113-
memset(instance.response->data(), 0, instance.response->size());
104+
if (instance.expecting_header) {
105+
memset(instance.response->data(), 0, instance.response->size());
114106

115-
fuse_out_header header;
116-
TRY(buffer.read(&header, 0, sizeof(fuse_out_header)));
107+
fuse_out_header header;
108+
TRY(buffer.read(&header, 0, sizeof(fuse_out_header)));
117109

118-
dbgln_if(FUSE_DEBUG, "header: length: {}, error: {}, unique: {}", header.len, header.error, header.unique);
119-
memcpy(instance.response->data(), &header, sizeof(fuse_out_header));
110+
dbgln_if(FUSE_DEBUG, "header: length: {}, error: {}, unique: {}", header.len, header.error, header.unique);
111+
memcpy(instance.response->data(), &header, sizeof(fuse_out_header));
120112

121-
if (header.len > sizeof(fuse_out_header))
122-
instance.expecting_header = false;
123-
else
124-
instance.response_ready = true;
125-
} else {
126-
fuse_out_header* existing_header = bit_cast<fuse_out_header*>(instance.response->data());
113+
if (header.len > sizeof(fuse_out_header))
114+
instance.expecting_header = false;
115+
else
116+
instance.response_ready = true;
117+
} else {
118+
fuse_out_header* existing_header = bit_cast<fuse_out_header*>(instance.response->data());
127119

128-
instance.expecting_header = true;
129-
if (existing_header->len > instance.response->size())
130-
return Error::from_errno(EINVAL);
120+
instance.expecting_header = true;
121+
if (existing_header->len > instance.response->size())
122+
return Error::from_errno(EINVAL);
131123

132-
instance.response_ready = true;
133-
u64 length = existing_header->len - sizeof(fuse_out_header);
134-
dbgln_if(FUSE_DEBUG, "request: response length: {}", length);
135-
TRY(buffer.read(instance.response->data() + sizeof(fuse_out_header), 0, length));
136-
}
124+
instance.response_ready = true;
125+
u64 length = existing_header->len - sizeof(fuse_out_header);
126+
dbgln_if(FUSE_DEBUG, "request: response length: {}", length);
127+
TRY(buffer.read(instance.response->data() + sizeof(fuse_out_header), 0, length));
128+
}
137129

138-
return size;
139-
});
130+
return size;
140131
}
141132

142133
ErrorOr<NonnullOwnPtr<KBuffer>> FUSEDevice::send_request_and_wait_for_a_reply(OpenFileDescription const& description, Bytes bytes)
143134
{
144-
return m_instances.with([&](auto& instances) -> ErrorOr<NonnullOwnPtr<KBuffer>> {
145-
auto instance_iterator = instances.find(&description);
146-
VERIFY(instance_iterator != instances.end());
147-
auto& requests_for_instance = (*instance_iterator).value;
148-
149-
TRY(requests_for_instance.try_append({
150-
&description,
151-
TRY(KBuffer::try_create_with_size("FUSE: Pending request buffer"sv, 0x21000)),
152-
TRY(KBuffer::try_create_with_size("FUSE: Response buffer"sv, 0x21000)),
153-
}));
154-
155-
size_t instance_index = requests_for_instance.size() - 1;
156-
auto& instance = requests_for_instance.last();
157-
VERIFY(bytes.size() <= 0x21000);
158-
159-
memset(instance.pending_request->data(), 0, instance.pending_request->size());
160-
memcpy(instance.pending_request->data(), bytes.data(), bytes.size());
161-
instance.buffer_ready = true;
162-
evaluate_block_conditions();
135+
SpinlockLocker locker(m_lock);
136+
137+
auto instance_iterator = m_instances.find(&description);
138+
VERIFY(instance_iterator != m_instances.end());
139+
auto& requests_for_instance = (*instance_iterator).value;
163140

164-
while (!instance.response_ready)
165-
(void)Thread::current()->sleep(Duration::from_microseconds(100));
141+
TRY(requests_for_instance.try_append({
142+
&description,
143+
TRY(KBuffer::try_create_with_size("FUSE: Pending request buffer"sv, 0x21000)),
144+
TRY(KBuffer::try_create_with_size("FUSE: Response buffer"sv, 0x21000)),
145+
}));
146+
147+
size_t instance_index = requests_for_instance.size() - 1;
148+
auto& instance = requests_for_instance.last();
149+
VERIFY(bytes.size() <= 0x21000);
150+
151+
memset(instance.pending_request->data(), 0, instance.pending_request->size());
152+
memcpy(instance.pending_request->data(), bytes.data(), bytes.size());
153+
instance.buffer_ready = true;
154+
155+
while (!instance.response_ready) {
156+
locker.unlock();
157+
evaluate_block_conditions();
158+
Scheduler::yield();
159+
locker.lock();
160+
}
166161

167-
auto result = KBuffer::try_create_with_bytes("FUSEDevice: Response"sv, instance.response->bytes());
168-
requests_for_instance.remove(instance_index);
162+
auto result = KBuffer::try_create_with_bytes("FUSEDevice: Response"sv, instance.response->bytes());
163+
requests_for_instance.remove(instance_index);
169164

170-
return result;
171-
});
165+
return result;
172166
}
173167

174168
void FUSEDevice::shutdown_for_description(OpenFileDescription const& description)
175169
{
176-
m_instances.with([&](auto& instances) {
177-
VERIFY(instances.remove(&description));
178-
});
170+
SpinlockLocker locker(m_lock);
179171

180-
m_closing_instances.with([&](auto& closing_instances) {
181-
closing_instances.append(&description);
182-
});
172+
VERIFY(m_instances.remove(&description));
173+
m_closing_instances.append(&description);
183174

175+
locker.unlock();
184176
evaluate_block_conditions();
185177
}
186178

‎Kernel/Devices/FUSEDevice.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,9 @@ class FUSEDevice final : public CharacterDevice {
4646
virtual bool can_write(OpenFileDescription const&, u64) const override;
4747
virtual StringView class_name() const override { return "FUSEDevice"sv; }
4848

49-
SpinlockProtected<HashMap<OpenFileDescription const*, Vector<FUSEInstance>>, LockRank::None> m_instances;
50-
SpinlockProtected<Vector<OpenFileDescription const*>, LockRank::None> m_closing_instances;
49+
mutable Spinlock<LockRank::None> m_lock {};
50+
HashMap<OpenFileDescription const*, Vector<FUSEInstance>> m_instances;
51+
Vector<OpenFileDescription const*> m_closing_instances;
5152
};
5253

5354
}

0 commit comments

Comments
 (0)