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.
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.
Another question. Is it also safe to use WRITE_ONCE without holding the lock? Or this is only appliable to read operations?
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.
Thanks for your time and patience. John Wood