Reference counting struct inet_peer

Hi,

I'm working on a patchset that adds overflow protection to kernel
reference counters, as part of the KSPP effort.  We're introducing a
new type, tentatively called refcount_t, that will ultimately replace
atomic_t as the type used for kernel reference counters.  refcount_t
has a constrained interface relative to atomic_t and stores reference
counts as unsigned integers.

While performing an audit of kernel reference counters, we've come
upon a few corner cases that we're unable to cleanly migrate to
refcount_t.  One of these is the reference counting scheme for struct
inet_peer.

struct inet_peer objects get freed when their reference count becomes
-1, not 0 as is the usual case.  Is there a reason why this is so?
The common use case I'm seeing is this:

struct inet_peer *p = inet_getpeer_v[4|6]();
...
inet_putpeer(p);


inet_getpeer_v4() and inet_getpeer_v6() are wrappers around
inet_getpeer().  From inet_getpeer():

struct inet_peer *p;
...
p = lookup_rcu(daddr, base);
...
p = lookup(daddr, stack, base);
if (p != peer_avl_empty) {
    atomic_inc(&p->refcnt);
    write_sequnlock_bh(&base->lock);
    return p;
}
...
p = create ? kmem_cache_alloc(peer_cachep, GFP_ATOMIC) : NULL;
if (p) {
    ...
    atomic_set(&p->refcnt, 1);
    ...
}
return p;

This all looks straightforward: p->refcnt is incremented by one when
it is found in the peer node tree, and it is set to one when it is
newly created.

However, in lookup_rcu(), this same reference count is checked against
-1.  From lookup_rcu():

struct inet_peer *u = rcu_dereference(base->root);
...
/* Before taking a reference, check if this entry was
* deleted (refcnt=-1)
*/
if (!atomic_add_unless(&u->refcnt, 1, -1))
    u = NULL;
return u;

Rather than delve further into net's internal garbage collectors, or
into RCU internals, I figured I'd ask here if there's a reason for the
check against -1 in rcu_lookup().

We're also seeing the same thing (freeing shared objects when their
refcount becomes -1) in ip_vs.h:

http://lxr.free-electrons.com/source/include/net/ip_vs.h#L1424

static inline void ip_vs_dest_put_and_free(struct ip_vs_dest *dest)
{
    if (atomic_dec_return(&dest->refcnt) < 0)
        kfree(dest);
}

Note that this example also appears in a garbage collector internal to net/.

Thanks,
David Windsor

	Hello,

On Mon, 23 Jan 2017, David Windsor wrote:

Hi,

I'm working on a patchset that adds overflow protection to kernel
reference counters, as part of the KSPP effort.  We're introducing a
new type, tentatively called refcount_t, that will ultimately replace
atomic_t as the type used for kernel reference counters.  refcount_t
has a constrained interface relative to atomic_t and stores reference
counts as unsigned integers.

While performing an audit of kernel reference counters, we've come
upon a few corner cases that we're unable to cleanly migrate to
refcount_t.  One of these is the reference counting scheme for struct
inet_peer.

...

We're also seeing the same thing (freeing shared objects when their
refcount becomes -1) in ip_vs.h:

http://lxr.free-electrons.com/source/include/net/ip_vs.h#L1424

static inline void ip_vs_dest_put_and_free(struct ip_vs_dest *dest)
{
    if (atomic_dec_return(&dest->refcnt) < 0)
        kfree(dest);
}

	I think, this is easy to fix. The problem is that
dest_trash currently holds deleted dests (unlinked from RCU lists)
with refcnt=0. If we change the dest_trash to hold dest
with refcnt=1, the above atomic_dec_return can be changed to
atomic_dec_and_test. Change should be small: ip_vs_dest_put
should be removed from __ip_vs_del_dest(), ip_vs_dest_hold()
from ip_vs_trash_get_dest() and refcnt check in
ip_vs_dest_trash_expire() should be updated. Let me know if
this holds your work, I can provide such patch to fix it.

Regards

On Tue, Jan 24, 2017 at 2:38 AM, Julian Anastasov [off-list ref] wrote:

        Hello,

On Mon, 23 Jan 2017, David Windsor wrote:

quoted

Hi,

I'm working on a patchset that adds overflow protection to kernel
reference counters, as part of the KSPP effort.  We're introducing a
new type, tentatively called refcount_t, that will ultimately replace
atomic_t as the type used for kernel reference counters.  refcount_t
has a constrained interface relative to atomic_t and stores reference
counts as unsigned integers.

While performing an audit of kernel reference counters, we've come
upon a few corner cases that we're unable to cleanly migrate to
refcount_t.  One of these is the reference counting scheme for struct
inet_peer.

...

quoted

We're also seeing the same thing (freeing shared objects when their
refcount becomes -1) in ip_vs.h:

http://lxr.free-electrons.com/source/include/net/ip_vs.h#L1424

static inline void ip_vs_dest_put_and_free(struct ip_vs_dest *dest)
{
    if (atomic_dec_return(&dest->refcnt) < 0)
        kfree(dest);
}

        I think, this is easy to fix. The problem is that
dest_trash currently holds deleted dests (unlinked from RCU lists)
with refcnt=0. If we change the dest_trash to hold dest
with refcnt=1, the above atomic_dec_return can be changed to
atomic_dec_and_test. Change should be small: ip_vs_dest_put
should be removed from __ip_vs_del_dest(), ip_vs_dest_hold()
from ip_vs_trash_get_dest() and refcnt check in
ip_vs_dest_trash_expire() should be updated. Let me know if
this holds your work, I can provide such patch to fix it.

Thanks for looking into this.  Your solution does indeed solve the
problem we're looking to fix.  Essentially, we just need the reference
count to never become < 0.  If you have a patch to fix this, that
would be great.

Thanks,
David Windsor

Regards

--
Julian Anastasov [off-list ref]

From: David Windsor <redacted>
Date: Mon, 23 Jan 2017 07:42:51 -0500

struct inet_peer objects get freed when their reference count
becomes -1, not 0 as is the usual case.  Is there a reason why this
is so?

inet peer entries that sit in the tree, but have no other reference
taken, have a reference count of zero.

Therefore, any entry which has a reference count of zero can be
safely garbage collected from the tree.

When the garbage collector purges entries with a zero refcnt, it
atomically sets the refcnt to -1 so that other threads of control
in RCU protected sections that still see this entry in the tree
will not be able to grab it for use.

The -1 marker is used as a synchronization mechanism between the
GC and lookup paths.

Once -1 is atomically set, the GC code knows that no external
reference can be created.