On Tue, Jun 29, 2021 at 1:20 PM Alexey Gladkov <legion@xxxxxxxxxx> wrote:
>
> Waaaait. task_ucounts() is a different thing. This function only gets a
> field from the task structure without any reference counting. But the
> get_ucounts() is more like get_user_ns() or get_uid(), but does not ignore
> counter overflow.
Alexey, that code cannot be right.
Look here:
rcu_read_lock();
ucounts = task_ucounts(t);
sigpending = inc_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_SIGPENDING, 1);
if (sigpending == 1)
ucounts = get_ucounts(ucounts);
rcu_read_unlock();
so now we've done that inc_rlimit_ucounts() unconditionally on that
task_ucounts() thing.
And then if the allocation fails (or the limit is hit) the code does
if (ucounts && dec_rlimit_ucounts(ucounts, UCOUNT_RLIMIT_SIGPENDING, 1))
put_ucounts(ucounts);
ie now it does the dec_rlimit_ucounts _conditionally_.
See what I'm complaining about? This is not logical, AND IT CANNOT
POSSIBLY BE RIGHT.
My argument is that
(a) the dec_rlimit_ucounts() has to pair up with
inc_rlimit_ucounts(), or you're leaking counts
(b) get_ucounts() has to pair up with put_ucounts().
Note that (a) has to be REGARDLESS of whether get_ucounts() was
successful or not.
> Earlier I tried to use refcount_t which never returns errors [1]. We
> talked and you said that ignoring counter overflow errors is bad
> design for this case.
You can't ignore counter overflow errors, no. But that's exactly what
that code is doing.
If get_ucount() fails due to overflow, you don't return an error. You
just miscount the end result!
So yeah, its' "testing" the overflow condition, but that's not an
argument, when it then DOES EXPLICITLY THE WRONG THING.
At that point, the test is actively harmful and wrong. See?
Linus
