On Sun, Mar 21, 2021 at 04:01:18PM +0100, John Wood wrote:
On Wed, Mar 17, 2021 at 07:57:10PM -0700, Kees Cook wrote:
On Sun, Mar 07, 2021 at 12:30:26PM +0100, John Wood wrote:
+static u64 brute_update_crash_period(struct brute_stats *stats, u64 now) +{
- u64 current_period;
- u64 last_crash_timestamp;
- spin_lock(&stats->lock);
- current_period = now - stats->jiffies;
- last_crash_timestamp = stats->jiffies;
- stats->jiffies = now;
- stats->period -= brute_mul_by_ema_weight(stats->period);
- stats->period += brute_mul_by_ema_weight(current_period);
- if (stats->faults < BRUTE_MAX_FAULTS)
stats->faults += 1;- spin_unlock(&stats->lock);
- return last_crash_timestamp;
+}
Now *here* locking makes sense, and it only needs to be per-stat, not global, since multiple processes may be operating on the same stat struct. To make this more no-reader-locking-friendly, I'd also update everything at the end, and use WRITE_ONCE():
u64 current_period, period; u64 last_crash_timestamp; u64 faults;
spin_lock(&stats->lock); current_period = now - stats->jiffies; last_crash_timestamp = stats->jiffies;
WRITE_ONCE(stats->period, stats->period - brute_mul_by_ema_weight(stats->period) + brute_mul_by_ema_weight(current_period));
if (stats->faults < BRUTE_MAX_FAULTS) WRITE_ONCE(stats->faults, stats->faults + 1);
WRITE_ONCE(stats->jiffies, now);
spin_unlock(&stats->lock); return last_crash_timestamp;
That way readers can (IIUC) safely use READ_ONCE() on jiffies and faults without needing to hold the &stats->lock (unless they need perfectly matching jiffies, period, and faults).
Sorry, but I try to understand how to use locking properly without luck.
I have read (and tried to understand): tools/memory-model/Documentation/simple.txt tools/memory-model/Documentation/ordering.txt tools/memory-model/Documentation/recipes.txt Documentation/memory-barriers.txt
And I don't find the responses that I need. I'm not saying they aren't there but I don't see them. So my questions:
If in the above function makes sense to use locking, and it is called from the brute_task_fatal_signal hook, then, all the functions that are called from this hook need locking (more than one process can access stats at the same time).
So, as you point, how it is possible and safe to read jiffies and faults (and I think period even though you not mention it) using READ_ONCE() but without holding brute_stats::lock? I'm very confused.
There are, I think, 3 considerations:
- is "stats", itself, a valid allocation in kernel memory? This is the "lifetime" management of the structure: it will only stay allocated as long as there is a task still alive that is attached to it. The use of refcount_t on task creation/death should entirely solve this issue, so that all the other places where you access "stats", the memory will be valid. AFAICT, this one is fine: you're doing all the correct lifetime management.
- changing a task's stats pointer: this is related to lifetime management, but it, I think, entirely solved by the existing refcounting. (And isn't helped by holding stats->lock since this is about stats itself being a valid pointer.) Again, I think this is all correct already in your existing code (due to the implicit locking of "current"). Perhaps I've missed something here, but I guess we'll see!
- are the values in stats getting written by multiple writers, or read during a write, etc?
This last one is the core of what I think could be improved here:
To keep the writes serialized, you (correctly) perform locking in the writers. This is fine.
There is also locking in the readers, which I think is not needed. AFAICT, READ_ONCE() (with WRITE_ONCE() in the writers) is sufficient for the readers here.
IIUC (during the reading of the documentation) READ_ONCE and WRITE_ONCE only guarantees that a variable loaded with WRITE_ONCE can be read safely with READ_ONCE avoiding tearing, etc. So, I see these functions like a form of guarantee atomicity in variables.
Right -- from what I can see about how you're reading the statistics, I don't see a way to have the values get confused (assuming locked writes and READ/WRITE_ONCE()).
Another question. Is it also safe to use WRITE_ONCE without holding the lock? Or this is only appliable to read operations?
No -- you'll still want the writer locked since you update multiple fields in stats during a write, so you could miss increments, or interleave count vs jiffies writes, etc. But the WRITE_ONCE() makes sure that the READ_ONCE() readers will see a stable value (as I understand it), and in the order they were written.
Any light on this will help me to do the best job in the next patches. If somebody can point me to the right direction it would be greatly appreciated.
Is there any documentation for newbies regarding this theme? I'm stuck. I have also read the documentation about spinlocks, semaphores, mutex, etc.. but nothing clears me the concept expose.
Apologies if this question has been answered in the past. But the search in the mailing list has not been lucky.
It's a complex subject! Here are some other docs that might help:
tools/memory-model/Documentation/explanation.txt Documentation/core-api/refcount-vs-atomic.rst
or they may melt your brain further! :) I know mine is always mushy after reading them.
Thanks for your time and patience.
You're welcome; and thank you for your work on this! I've wanted a robust brute force mitigation in the kernel for a long time. :)
-Kees