Re: [PATCH v6 3/8] securtiy/brute: Detect a brute force attack
Hi, On Sun, Mar 21, 2021 at 11:45:59AM -0700, Kees Cook wrote: > 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: > > 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! My only concern now is the following case: One process crashes with a fatal signal. Then, its stats are updated. Then we get the exec stats (the stats of the task that calls exec). At the same time another CPU frees this same stats. Now, if the first process writes to the exec stats we get a "use after free" bug. If this scenario is possible, we would need to protect all the section inside the task_fatal_signal hook that deals with the exec stats. I think that here a global lock is necessary and also, protect the write of the pointer to stats struct in the task_free hook. Moreover, I can see another scenario: The first CPU gets the exec stats when a task fails with a fatal signal. The second CPU exec()ve after exec()ve over the same task from we get the exec stats with the first CPU. This second CPU resets the stats at the same time that the first CPU updates the same stats. I think we also need lock here. Am I right? Are these paths possible? > > - 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
Re: [PATCH v6 3/8] securtiy/brute: Detect a brute force attack
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(>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(>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(>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(>lock);
> > return last_crash_timestamp;
> >
> > That way readers can (IIUC) safely use READ_ONCE() on jiffies and faults
> > without needing to hold the >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
Re: [PATCH v6 3/8] securtiy/brute: Detect a brute force attack
On Sat, Mar 20, 2021 at 04:34:06PM +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:
> > > @@ -74,7 +84,7 @@ static struct brute_stats *brute_new_stats(void)
> > > {
> > > struct brute_stats *stats;
> > >
> > > - stats = kmalloc(sizeof(struct brute_stats), GFP_KERNEL);
> > > + stats = kmalloc(sizeof(struct brute_stats), GFP_ATOMIC);
> >
> > Why change this here? I'd just start with this in the patch that
> > introduces it.
>
> To be coherent in the previous patch. In the previous patch the kmalloc
> could use GFP_KERNEL due to the call was made out of an atomic context.
> Now, with the new lock it needs GFP_ATOMIC. So the question:
>
> If finally it need to use GFP_ATOMIC, the first patch need to use it even if
> it is not necessary?
It's probably not a big deal, but for me, I'd just do GFP_ATOMIC from
the start, maybe add a comment that says "some LSM hooks are from atomic
context" or something.
> > > if (!stats)
> > > return NULL;
> > >
> > > @@ -99,16 +109,17 @@ static struct brute_stats *brute_new_stats(void)
> > > * It's mandatory to disable interrupts before acquiring the
> > > brute_stats::lock
> > > * since the task_free hook can be called from an IRQ context during the
> > > * execution of the task_alloc hook.
> > > + *
> > > + * Context: Must be called with interrupts disabled and
> > > brute_stats_ptr_lock
> > > + * held.
> > > */
> > > static void brute_share_stats(struct brute_stats *src,
> > > struct brute_stats **dst)
> > > {
> > > - unsigned long flags;
> > > -
> > > - spin_lock_irqsave(>lock, flags);
> > > + spin_lock(>lock);
> > > refcount_inc(>refc);
> > > *dst = src;
> > > - spin_unlock_irqrestore(>lock, flags);
> > > + spin_unlock(>lock);
> > > }
> >
> > I still don't think any locking is needed here; the whole function can
> > go away, IMO.
>
> In this case I think this is possible:
>
> Scenario 1: cpu 1 writes the stats pointer and cpu 2 is navigating the
> processes tree reading the same stats pointer.
>
> Scenario 2: cpu 1 is navigating the processes tree reading the stats
> pointer and in IRQ the same stats pointer is wrote.
>
> So, we need locking. Am I wrong?
But only the refcount is being incremented, yes? That doesn't need a
lock because it's already an atomic.
>
> > > /**
> > > @@ -126,26 +137,36 @@ static void brute_share_stats(struct brute_stats
> > > *src,
> > > * this task and the new one being allocated. Otherwise, share the
> > > statistics
> > > * that the current task already has.
> > > *
> > > + * It's mandatory to disable interrupts before acquiring
> > > brute_stats_ptr_lock
> > > + * and brute_stats::lock since the task_free hook can be called from an
> > > IRQ
> > > + * context during the execution of the task_alloc hook.
> > > + *
> > > * Return: -ENOMEM if the allocation of the new statistics structure
> > > fails. Zero
> > > * otherwise.
> > > */
> > > static int brute_task_alloc(struct task_struct *task, unsigned long
> > > clone_flags)
> > > {
> > > struct brute_stats **stats, **p_stats;
> > > + unsigned long flags;
> > >
> > > stats = brute_stats_ptr(task);
> > > p_stats = brute_stats_ptr(current);
> > > + write_lock_irqsave(_stats_ptr_lock, flags);
> > >
> > > if (likely(*p_stats)) {
> > > brute_share_stats(*p_stats, stats);
> > > + write_unlock_irqrestore(_stats_ptr_lock, flags);
> > > return 0;
> > > }
> > >
> > > *stats = brute_new_stats();
> > > - if (!*stats)
> > > + if (!*stats) {
> > > + write_unlock_irqrestore(_stats_ptr_lock, flags);
> > > return -ENOMEM;
> > > + }
> > >
> > > brute_share_stats(*stats, p_stats);
> > > + write_unlock_irqrestore(_stats_ptr_lock, flags);
> > > return 0;
> > > }
> >
> > I'd much prefer that whatever locking is needed be introduced in the
> > initial patch: this transformation just double the work to review. :)
>
> So, IIUC I need to introduce all the locks in the initial patch even if
> they are not necessary. Am I right?
I would find it easier to follow. Perhaps other reviewers would have a
different opinion.
>
> > >
> > > @@ -167,9 +188,9 @@ static int brute_task_alloc(struct task_struct *task,
> > > unsigned long clone_flags)
> > > * only one task (the task that calls the execve function) points to the
> > > data.
> > > * In this case, the previous allocation is used but the statistics are
> > > reset.
> > > *
> > > - * It's mandatory to disable interrupts before acquiring the
> > > brute_stats::lock
> > > - * since the task_free hook can be called from an IRQ context during the
> > > - * execution of the bprm_committing_creds hook.
> > > + * It's mandatory to disable interrupts before acquiring
> > > brute_stats_ptr_lock
> > > + * and brute_stats::lock since the task_free hook can be called from an
> > > IRQ
> >
Re: [PATCH v6 3/8] securtiy/brute: Detect a brute force attack
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(>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(>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(>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(>lock);
> return last_crash_timestamp;
>
> That way readers can (IIUC) safely use READ_ONCE() on jiffies and faults
> without needing to hold the >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
Re: [PATCH v6 3/8] securtiy/brute: Detect a brute force attack
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:
> > @@ -74,7 +84,7 @@ static struct brute_stats *brute_new_stats(void)
> > {
> > struct brute_stats *stats;
> >
> > - stats = kmalloc(sizeof(struct brute_stats), GFP_KERNEL);
> > + stats = kmalloc(sizeof(struct brute_stats), GFP_ATOMIC);
>
> Why change this here? I'd just start with this in the patch that
> introduces it.
To be coherent in the previous patch. In the previous patch the kmalloc
could use GFP_KERNEL due to the call was made out of an atomic context.
Now, with the new lock it needs GFP_ATOMIC. So the question:
If finally it need to use GFP_ATOMIC, the first patch need to use it even if
it is not necessary?
> > if (!stats)
> > return NULL;
> >
> > @@ -99,16 +109,17 @@ static struct brute_stats *brute_new_stats(void)
> > * It's mandatory to disable interrupts before acquiring the
> > brute_stats::lock
> > * since the task_free hook can be called from an IRQ context during the
> > * execution of the task_alloc hook.
> > + *
> > + * Context: Must be called with interrupts disabled and
> > brute_stats_ptr_lock
> > + * held.
> > */
> > static void brute_share_stats(struct brute_stats *src,
> > struct brute_stats **dst)
> > {
> > - unsigned long flags;
> > -
> > - spin_lock_irqsave(>lock, flags);
> > + spin_lock(>lock);
> > refcount_inc(>refc);
> > *dst = src;
> > - spin_unlock_irqrestore(>lock, flags);
> > + spin_unlock(>lock);
> > }
>
> I still don't think any locking is needed here; the whole function can
> go away, IMO.
In this case I think this is possible:
Scenario 1: cpu 1 writes the stats pointer and cpu 2 is navigating the
processes tree reading the same stats pointer.
Scenario 2: cpu 1 is navigating the processes tree reading the stats
pointer and in IRQ the same stats pointer is wrote.
So, we need locking. Am I wrong?
> > /**
> > @@ -126,26 +137,36 @@ static void brute_share_stats(struct brute_stats *src,
> > * this task and the new one being allocated. Otherwise, share the
> > statistics
> > * that the current task already has.
> > *
> > + * It's mandatory to disable interrupts before acquiring
> > brute_stats_ptr_lock
> > + * and brute_stats::lock since the task_free hook can be called from an IRQ
> > + * context during the execution of the task_alloc hook.
> > + *
> > * Return: -ENOMEM if the allocation of the new statistics structure
> > fails. Zero
> > * otherwise.
> > */
> > static int brute_task_alloc(struct task_struct *task, unsigned long
> > clone_flags)
> > {
> > struct brute_stats **stats, **p_stats;
> > + unsigned long flags;
> >
> > stats = brute_stats_ptr(task);
> > p_stats = brute_stats_ptr(current);
> > + write_lock_irqsave(_stats_ptr_lock, flags);
> >
> > if (likely(*p_stats)) {
> > brute_share_stats(*p_stats, stats);
> > + write_unlock_irqrestore(_stats_ptr_lock, flags);
> > return 0;
> > }
> >
> > *stats = brute_new_stats();
> > - if (!*stats)
> > + if (!*stats) {
> > + write_unlock_irqrestore(_stats_ptr_lock, flags);
> > return -ENOMEM;
> > + }
> >
> > brute_share_stats(*stats, p_stats);
> > + write_unlock_irqrestore(_stats_ptr_lock, flags);
> > return 0;
> > }
>
> I'd much prefer that whatever locking is needed be introduced in the
> initial patch: this transformation just double the work to review. :)
So, IIUC I need to introduce all the locks in the initial patch even if
they are not necessary. Am I right?
> >
> > @@ -167,9 +188,9 @@ static int brute_task_alloc(struct task_struct *task,
> > unsigned long clone_flags)
> > * only one task (the task that calls the execve function) points to the
> > data.
> > * In this case, the previous allocation is used but the statistics are
> > reset.
> > *
> > - * It's mandatory to disable interrupts before acquiring the
> > brute_stats::lock
> > - * since the task_free hook can be called from an IRQ context during the
> > - * execution of the bprm_committing_creds hook.
> > + * It's mandatory to disable interrupts before acquiring
> > brute_stats_ptr_lock
> > + * and brute_stats::lock since the task_free hook can be called from an IRQ
> > + * context during the execution of the bprm_committing_creds hook.
> > */
> > static void brute_task_execve(struct linux_binprm *bprm)
> > {
> > @@ -177,24 +198,33 @@ static void brute_task_execve(struct linux_binprm
> > *bprm)
> > unsigned long flags;
> >
> > stats = brute_stats_ptr(current);
> > - if (WARN(!*stats, "No statistical data\n"))
> > + read_lock_irqsave(_stats_ptr_lock, flags);
> > +
> > + if (WARN(!*stats, "No statistical data\n")) {
> > + read_unlock_irqrestore(_stats_ptr_lock, flags);
> > return;
> > + }
> >
> > - spin_lock_irqsave(&(*stats)->lock, flags);
Re: [PATCH v6 3/8] securtiy/brute: Detect a brute force attack
On Sun, Mar 07, 2021 at 12:30:26PM +0100, John Wood wrote:
> To detect a brute force attack it is necessary that the statistics
> shared by all the fork hierarchy processes be updated in every fatal
> crash and the most important data to update is the application crash
> period. To do so, use the new "task_fatal_signal" LSM hook added in a
> previous step.
>
> The application crash period must be a value that is not prone to change
> due to spurious data and follows the real crash period. So, to compute
> it, the exponential moving average (EMA) is used.
>
> There are two types of brute force attacks that need to be detected. The
> first one is an attack that happens through the fork system call and the
> second one is an attack that happens through the execve system call. The
> first type uses the statistics shared by all the fork hierarchy
> processes, but the second type cannot use this statistical data due to
> these statistics disappear when the involved tasks finished. In this
> last scenario the attack info should be tracked by the statistics of a
> higher fork hierarchy (the hierarchy that contains the process that
> forks before the execve system call).
>
> Moreover, these two attack types have two variants. A slow brute force
> attack that is detected if the maximum number of faults per fork
> hierarchy is reached and a fast brute force attack that is detected if
> the application crash period falls below a certain threshold.
>
> Also, this patch adds locking to protect the statistics pointer hold by
> every process.
>
> Signed-off-by: John Wood
> ---
> security/brute/brute.c | 498 +++--
> 1 file changed, 479 insertions(+), 19 deletions(-)
>
> diff --git a/security/brute/brute.c b/security/brute/brute.c
> index 99d099e45112..870db55332d4 100644
> --- a/security/brute/brute.c
> +++ b/security/brute/brute.c
> @@ -11,9 +11,14 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> +#include
> +#include
> #include
> +#include
> +#include
> #include
> #include
> #include
> @@ -37,6 +42,11 @@ struct brute_stats {
> u64 period;
> };
>
> +/*
> + * brute_stats_ptr_lock - Lock to protect the brute_stats structure pointer.
> + */
> +static DEFINE_RWLOCK(brute_stats_ptr_lock);
Yeow, you've switched from an (unneeded in prior patch) per-stats lock
to a global lock? I think this isn't needed...
> +
> /*
> * brute_blob_sizes - LSM blob sizes.
> *
> @@ -74,7 +84,7 @@ static struct brute_stats *brute_new_stats(void)
> {
> struct brute_stats *stats;
>
> - stats = kmalloc(sizeof(struct brute_stats), GFP_KERNEL);
> + stats = kmalloc(sizeof(struct brute_stats), GFP_ATOMIC);
Why change this here? I'd just start with this in the patch that
introduces it.
> if (!stats)
> return NULL;
>
> @@ -99,16 +109,17 @@ static struct brute_stats *brute_new_stats(void)
> * It's mandatory to disable interrupts before acquiring the
> brute_stats::lock
> * since the task_free hook can be called from an IRQ context during the
> * execution of the task_alloc hook.
> + *
> + * Context: Must be called with interrupts disabled and brute_stats_ptr_lock
> + * held.
> */
> static void brute_share_stats(struct brute_stats *src,
> struct brute_stats **dst)
> {
> - unsigned long flags;
> -
> - spin_lock_irqsave(>lock, flags);
> + spin_lock(>lock);
> refcount_inc(>refc);
> *dst = src;
> - spin_unlock_irqrestore(>lock, flags);
> + spin_unlock(>lock);
> }
I still don't think any locking is needed here; the whole function can
go away, IMO.
>
> /**
> @@ -126,26 +137,36 @@ static void brute_share_stats(struct brute_stats *src,
> * this task and the new one being allocated. Otherwise, share the statistics
> * that the current task already has.
> *
> + * It's mandatory to disable interrupts before acquiring brute_stats_ptr_lock
> + * and brute_stats::lock since the task_free hook can be called from an IRQ
> + * context during the execution of the task_alloc hook.
> + *
> * Return: -ENOMEM if the allocation of the new statistics structure fails.
> Zero
> * otherwise.
> */
> static int brute_task_alloc(struct task_struct *task, unsigned long
> clone_flags)
> {
> struct brute_stats **stats, **p_stats;
> + unsigned long flags;
>
> stats = brute_stats_ptr(task);
> p_stats = brute_stats_ptr(current);
> + write_lock_irqsave(_stats_ptr_lock, flags);
>
> if (likely(*p_stats)) {
> brute_share_stats(*p_stats, stats);
> + write_unlock_irqrestore(_stats_ptr_lock, flags);
> return 0;
> }
>
> *stats = brute_new_stats();
> - if (!*stats)
> + if (!*stats) {
> + write_unlock_irqrestore(_stats_ptr_lock, flags);
> return -ENOMEM;
> + }
>
> brute_share_stats(*stats, p_stats);
> +
