kernel-hardening - Re: [RFC PATCH v3 1/8] Use refcount

Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <878s8o74iz.fsf@x220.int.ebiederm.org>
Date: Tue, 19 Jan 2021 19:58:44 -0600
From: ebiederm@...ssion.com (Eric W. Biederman)
To: Alexey Gladkov <gladkov.alexey@...il.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,  LKML <linux-kernel@...r.kernel.org>,  io-uring <io-uring@...r.kernel.org>,  Kernel Hardening <kernel-hardening@...ts.openwall.com>,  Linux Containers <containers@...ts.linux-foundation.org>,  Linux-MM <linux-mm@...ck.org>,  Andrew Morton <akpm@...ux-foundation.org>,  Christian Brauner <christian.brauner@...ntu.com>,  Jann Horn <jannh@...gle.com>,  Jens Axboe <axboe@...nel.dk>,  Kees Cook <keescook@...omium.org>,  Oleg Nesterov <oleg@...hat.com>
Subject: Re: [RFC PATCH v3 1/8] Use refcount_t for ucounts reference counting

ebiederm@...ssion.com (Eric W. Biederman) writes:

> Alexey Gladkov <gladkov.alexey@...il.com> writes:
>
>> On Mon, Jan 18, 2021 at 12:34:29PM -0800, Linus Torvalds wrote:
>>> On Mon, Jan 18, 2021 at 11:46 AM Alexey Gladkov
>>> <gladkov.alexey@...il.com> wrote:
>>> >
>>> > Sorry about that. I thought that this code is not needed when switching
>>> > from int to refcount_t. I was wrong.
>>> 
>>> Well, you _may_ be right. I personally didn't check how the return
>>> value is used.
>>> 
>>> I only reacted to "it certainly _may_ be used, and there is absolutely
>>> no comment anywhere about why it wouldn't matter".
>>
>> I have not found examples where checked the overflow after calling
>> refcount_inc/refcount_add.
>>
>> For example in kernel/fork.c:2298 :
>>
>>    current->signal->nr_threads++;                           
>>    atomic_inc(&current->signal->live);                      
>>    refcount_inc(&current->signal->sigcnt);  
>>
>> $ semind search signal_struct.sigcnt
>> def include/linux/sched/signal.h:83  		refcount_t		sigcnt;
>> m-- kernel/fork.c:723 put_signal_struct 		if (refcount_dec_and_test(&sig->sigcnt))
>> m-- kernel/fork.c:1571 copy_signal 		refcount_set(&sig->sigcnt, 1);
>> m-- kernel/fork.c:2298 copy_process 				refcount_inc(&current->signal->sigcnt);
>>
>> It seems to me that the only way is to use __refcount_inc and then compare
>> the old value with REFCOUNT_MAX
>>
>> Since I have not seen examples of such checks, I thought that this is
>> acceptable. Sorry once again. I have not tried to hide these changes.
>
> The current ucount code does check for overflow and fails the increment
> in every case.
>
> So arguably it will be a regression and inferior error handling behavior
> if the code switches to the ``better'' refcount_t data structure.
>
> I originally didn't use refcount_t because silently saturating and not
> bothering to handle the error makes me uncomfortable.
>
> Not having to acquire the ucounts_lock every time seems nice.  Perhaps
> the path forward would be to start with stupid/correct code that always
> takes the ucounts_lock for every increment of ucounts->count, that is
> later replaced with something more optimal.
>
> Not impacting performance in the non-namespace cases and having good
> performance in the other cases is a fundamental requirement of merging
> code like this.

So starting with something easy to comprehend and simple, may make it
easier to figure out how to optimize the code.

Eric
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.