On Thu, Oct 31, 2024 at 08:43:22AM -0700, Andrei Vagin wrote: > On Thu, Oct 31, 2024 at 2:50 AM Alexey Gladkov <legion@xxxxxxxxxx> wrote: > > > > On Thu, Oct 31, 2024 at 04:56:01AM +0000, Andrei Vagin wrote: > > > The inc_rlimit_get_ucounts() increments the specified rlimit counter and > > > then checks its limit. If the value exceeds the limit, the function > > > returns an error without decrementing the counter. > > > > > > Fixes: 15bc01effefe ("ucounts: Fix signal ucount refcounting") > > > Tested-by: Roman Gushchin <roman.gushchin@xxxxxxxxx> > > > Co-debugged-by: Roman Gushchin <roman.gushchin@xxxxxxxxx> > > > Cc: Kees Cook <kees@xxxxxxxxxx> > > > Cc: Andrei Vagin <avagin@xxxxxxxxxx> > > > Cc: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx> > > > Cc: Alexey Gladkov <legion@xxxxxxxxxx> > > > Cc: <stable@xxxxxxxxxxxxxxx> > > > Signed-off-by: Andrei Vagin <avagin@xxxxxxxxxx> > > > --- > > > kernel/ucount.c | 5 ++--- > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > diff --git a/kernel/ucount.c b/kernel/ucount.c > > > index 8c07714ff27d..16c0ea1cb432 100644 > > > --- a/kernel/ucount.c > > > +++ b/kernel/ucount.c > > > @@ -328,13 +328,12 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum rlimit_type type) > > > if (new != 1) > > > continue; > > > if (!get_ucounts(iter)) > > > - goto dec_unwind; > > > + goto unwind; > > > } > > > return ret; > > > -dec_unwind: > > > +unwind: > > > dec = atomic_long_sub_return(1, &iter->rlimit[type]); > > > WARN_ON_ONCE(dec < 0); > > > -unwind: > > > do_dec_rlimit_put_ucounts(ucounts, iter, type); > > > return 0; > > > } > > > > Agree. The do_dec_rlimit_put_ucounts() decreases rlimit up to iter but > > does not include it. > > > > Except for a small NAK because the patch changes goto for get_ucounts() > > and not for rlimit overflow check. > > Do you think it is better to rename the label and use dec_unwind? I don't > think it makes a big difference, but if you think it does, I can send > this version. > > BTW, while investigating this, we found another one. Currently, > sigqueue_alloc enforces a counter limit even when override_rlimit is set > to true. This was introduced by commit f3791f4df569ea ("Fix > UCOUNT_RLIMIT_SIGPENDING counter leak"). This change in behavior has > introduced regressions, causing failures in applications that previously > functioned correctly. > > For example, if the limit is reached and a process receives a SIGSEGV > signal, sigqueue_alloc fails to allocate the necessary resources for the > signal delivery, preventing the signal from being delivered with > siginfo. This prevents the process from correctly identifying the fault > address and handling the error. From the user-space perspective, > applications are unaware that the limit has been reached and that the > siginfo is effectively 'corrupted'. This can lead to unpredictable > behavior and crashes, as we observed with java applications. > > To address this, we think to restore the original logic for > override_rlimit. This will ensure that kernel signals are always > delivered correctly, regardless of the counter limit. Does this > approach seem reasonable? Do you have any concerns? I think override_rlimit argument should be passed into inc_rlimit_get_ucounts() and be handled there properly by ignoring the comparison with max. I gonna prepare a patch later today, if there are no objections for this approach. Thanks!