Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752020AbcKQMpj (ORCPT ); Thu, 17 Nov 2016 07:45:39 -0500 Received: from sender163-mail.zoho.com ([74.201.84.163]:21464 "EHLO sender163-mail.zoho.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750794AbcKQMpi (ORCPT ); Thu, 17 Nov 2016 07:45:38 -0500 MIME-Version: 1.0 In-Reply-To: <20161117083458.GZ3142@twins.programming.kicks-ass.net> References: <20161114173946.501528675@infradead.org> <20161114174446.486581399@infradead.org> <20161115073322.GC28248@kroah.com> <20161115080314.GD3142@twins.programming.kicks-ass.net> <20161116100925.GM3142@twins.programming.kicks-ass.net> <20161117083458.GZ3142@twins.programming.kicks-ass.net> From: David Windsor Date: Thu, 17 Nov 2016 07:30:29 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC][PATCH 2/7] kref: Add kref_read() To: Peter Zijlstra Cc: Kees Cook , Greg KH , Will Deacon , "Reshetova, Elena" , Arnd Bergmann , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , LKML , Alexei Starovoitov , Daniel Borkmann Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3203 Lines: 74 On Thu, Nov 17, 2016 at 3:34 AM, Peter Zijlstra wrote: > On Wed, Nov 16, 2016 at 10:58:38AM -0800, Kees Cook wrote: >> On Wed, Nov 16, 2016 at 2:09 AM, Peter Zijlstra wrote: >> > On Tue, Nov 15, 2016 at 12:53:35PM -0800, Kees Cook wrote: >> >> >> >> What should we do about things like this (bpf_prog_put() and callbacks >> >> from kernel/bpf/syscall.c): >> >> >> >> >> >> static void bpf_prog_uncharge_memlock(struct bpf_prog *prog) >> >> { >> >> struct user_struct *user = prog->aux->user; >> >> >> >> atomic_long_sub(prog->pages, &user->locked_vm); >> >> free_uid(user); >> >> } >> >> >> >> static void __bpf_prog_put_rcu(struct rcu_head *rcu) >> >> { >> >> struct bpf_prog_aux *aux = container_of(rcu, struct bpf_prog_aux, rcu); >> >> >> >> free_used_maps(aux); >> >> bpf_prog_uncharge_memlock(aux->prog); >> >> bpf_prog_free(aux->prog); >> >> } >> >> >> >> void bpf_prog_put(struct bpf_prog *prog) >> >> { >> >> if (atomic_dec_and_test(&prog->aux->refcnt)) >> >> call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu); >> >> } >> >> >> >> >> >> Not only do we want to protect prog->aux->refcnt, but I think we want >> >> to protect user->locked_vm too ... I don't think it's sane for >> >> user->locked_vm to be a stats_t ? >> > >> > Why would you want to mess with locked_vm? You seem of the opinion that >> > everything atomic_t is broken, this isn't the case. >> >> What I mean to say is that while the refcnt here should clearly be >> converted to kref or refcount_t, it looks like locked_vm should become >> a new stats_t. However, it seems weird for locked_vm to ever wrap >> either... > > No, its not a statistic. Also, I'm far from convinced stats_t is an > actually useful thing to have. > Regarding this, has there been any thought given as to how stats_t will meaningfully differ from atomic_t? If refcount_t is semantically "atomic_t with reference counter overflow protection," what services/guarantees does stats_t provide? I cannot think of any that don't require implementing overflow detection of some sort, which incurs a performance hit. One conceivable service/guarantee would be to give stats_t the ability to detect/report when an overflow has occurred, but not ultimately with the offending process getting killed. On x86, this could be done by having stats_t overflows generate a different exception number and corresponding handler than refcount_t-generated overflows. It would still contain the mechanisms for detecting and responding to overflows, but the response to stats_t overflows would differ from that of refcount_t overflows. Semantically, this version of stats_t would be "refcount_t minus 'kill the offending process'." I'm not sure if this abstraction is in fact useful, or indeed worth the requisite performance hit; I'm just suggesting a possible semantic difference between atomic_t and stats_t. > refcount_t brought special semantics that clearly are different from > regular atomic_t, stats_t would not, so why would it need to exist. > > Not to mention that you seem over eager to apply it, which doesn't > inspire confidence.