lkml.org 
[lkml]   [2016]   [Nov]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC][PATCH 2/7] kref: Add kref_read()
    On Thu, Nov 17, 2016 at 3:34 AM, Peter Zijlstra <peterz@infradead.org> 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 <peterz@infradead.org> 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.

    \
     
     \ /
      Last update: 2016-11-17 13:46    [W:4.328 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site