Re: [PATCH RFC 5/6] net: Generic resolver backend

[Tom Herbert]

On Wed, Sep 14, 2016 at 2:49 AM, Thomas Graf  wrote:
> On 09/09/16 at 04:19pm, Tom Herbert wrote:
>> diff --git a/net/core/resolver.c b/net/core/resolver.c
>> new file mode 100644
>> index 000..61b36c5
>> --- /dev/null
>> +++ b/net/core/resolver.c
>> @@ -0,0 +1,267 @@
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>
> This include list could be stripped down a bit. ila, lwt, fib, ...
>
>> +
>> +static struct net_rslv_ent *net_rslv_new_ent(struct net_rslv *nrslv,
>> +  void *key)
>
> Comment above that net_rslv_get_lock() must be held?
>
>> +{
>> + struct net_rslv_ent *nrent;
>> + int err;
>> +
>> + nrent = kzalloc(sizeof(*nrent) + nrslv->obj_size, GFP_KERNEL);
>
> GFP_ATOMIC since you typically hold net_rslv_get_lock() spinlock?
>
>> + if (!nrent)
>> + return ERR_PTR(-EAGAIN);
>> +
>> + /* Key is always at beginning of object data */
>> + memcpy(nrent->object, key, nrslv->params.key_len);
>> +
>> + /* Initialize user data */
>> + if (nrslv->rslv_init)
>> + nrslv->rslv_init(nrslv, nrent);
>> +
>> + /* Put in hash table */
>> + err = rhashtable_lookup_insert_fast(>rhash_table,
>> + >node, nrslv->params);
>> + if (err)
>> + return ERR_PTR(err);
>> +
>> + if (nrslv->timeout) {
>> + /* Schedule timeout for resolver */
>> + INIT_DELAYED_WORK(>timeout_work, net_rslv_delayed_work);
>
> Should this be done before inserting into rhashtable?
>
Adding to the table and setting delayed work are done under a lock so
I think it should be okay. I'll add a comment to the function that the
lock is held.

>> + schedule_delayed_work(>timeout_work, nrslv->timeout);
>> + }
>> +
>> + nrent->nrslv = nrslv;
>
> Same here.  net_rslv_cancel_all_delayed_work() walking the rhashtable could
> see ->nrslv as NULL.

I'll move it up, but net_rslv_cancel_all_delayed_work is only called
when we're destroying the table so it would be a bug in the higher
layer if it is both destroying the table and adding entries at the
same time.

Thanks,
Tom


>

Re: [PATCH RFC 5/6] net: Generic resolver backend

[Thomas Graf]

On 09/09/16 at 04:19pm, Tom Herbert wrote:
> diff --git a/net/core/resolver.c b/net/core/resolver.c
> new file mode 100644
> index 000..61b36c5
> --- /dev/null
> +++ b/net/core/resolver.c
> @@ -0,0 +1,267 @@
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

This include list could be stripped down a bit. ila, lwt, fib, ...

> +
> +static struct net_rslv_ent *net_rslv_new_ent(struct net_rslv *nrslv,
> +  void *key)

Comment above that net_rslv_get_lock() must be held?

> +{
> + struct net_rslv_ent *nrent;
> + int err;
> +
> + nrent = kzalloc(sizeof(*nrent) + nrslv->obj_size, GFP_KERNEL);

GFP_ATOMIC since you typically hold net_rslv_get_lock() spinlock?

> + if (!nrent)
> + return ERR_PTR(-EAGAIN);
> +
> + /* Key is always at beginning of object data */
> + memcpy(nrent->object, key, nrslv->params.key_len);
> +
> + /* Initialize user data */
> + if (nrslv->rslv_init)
> + nrslv->rslv_init(nrslv, nrent);
> +
> + /* Put in hash table */
> + err = rhashtable_lookup_insert_fast(>rhash_table,
> + >node, nrslv->params);
> + if (err)
> + return ERR_PTR(err);
> +
> + if (nrslv->timeout) {
> + /* Schedule timeout for resolver */
> + INIT_DELAYED_WORK(>timeout_work, net_rslv_delayed_work);

Should this be done before inserting into rhashtable?

> + schedule_delayed_work(>timeout_work, nrslv->timeout);
> + }
> +
> + nrent->nrslv = nrslv;

Same here.  net_rslv_cancel_all_delayed_work() walking the rhashtable could
see ->nrslv as NULL.

[PATCH RFC 5/6] net: Generic resolver backend

[Tom Herbert]

This patch implements the backend of a resolver, specifically it
provides a means to track unresolved addresses and to time them out.

The resolver is mostly a frontend to an rhashtable where the key
of the table is whatever address type or object is tracked. A resolver
instance is created by net_rslv_create. A resolver is destroyed by
net_rslv_destroy.

There are two functions that are used to manipulate entries in the
table: net_rslv_lookup_and_create and net_rslv_resolved.

net_rslv_lookup_and_create is called with an unresolved address as
the argument. It returns a structure of type net_rslv_ent. When
called a lookup is performed to see if an entry for the address
is already in the table, if it is the entry is return and the
false is returned in the new bool pointer argument to indicate that
the entry was preexisting. If an entry is not found, one is create
and true is returned on the new pointer argument. It is expected
that when an entry is new the address resolution protocol is
initiated (for instance a RTM_ADDR_RESOLVE message may be sent to a
userspace daemon as we will do in ILA). If net_rslv_lookup_and_create
returns NULL then presumably the hash table has reached the limit of
number of outstanding unresolved addresses, the caller should take
appropriate actions to avoid spamming the resolution protocol.

net_rslv_resolved is called when resolution is completely (e.g.
ILA locator mapping was instantiated for a locator. The entry is
removed for the hash table.

An argument to net_rslv_create indicates a time for the pending
resolution in milliseconds. If the timer fires before resolution
then the entry is removed from the table. Subsequently, another
attempt to resolve the same address will result in a new entry in
the table.

net_rslv_lookup_and_create allocates an net_rslv_ent struct and
includes allocating related user data. This is the object[] field
in the structure. The key (unresolved address) is always the first
field in the the object. Following that the caller may add it's
own private field data. The key length and size of the user object
(including the key) are specific in net_rslv_create.

There are three callback functions that can be set as arugments in
net_rslv_create:

   - cmp_fn: Compare function for hash table. Arguments are the
   key and an object in the table. If this is NULL then the
   default memcmp of rhashtable is used.

   - init_fn: Initial a new net_rslv_ent structure. This allows
   initialization of the user portion of the structure
   (the object[]).

   - destroy_fn: Called right before a net_rslv_ent is freed.
   This allows cleanup of user data associated with the
   entry.

Note that the resolver backend only tracks unresolved addresses, it
is up to the caller to perform the mechanism of resolution. This
includes the possible of queuing packets awaiting resolution; this
can be accomplished for instance by maintaining an skbuff queue
in the net_rslv_ent user object[] data.

DOS mitigation is done by limiting the number of entries in the
resolver table (the max_size which argument of net_rslv_create)
and setting a timeout. IF the timeout is set then the maximum rate
of new resolution requests is max_table_size / timeout. For
instance, with a maximum size of 1000 entries and a timeout of 100
msecs the maximum rate of resolutions requests is 1/s.

Signed-off-by: Tom Herbert 
---
 include/net/resolver.h |  58 +++
 net/Kconfig|   4 +
 net/core/Makefile  |   1 +
 net/core/resolver.c| 267 +
 4 files changed, 330 insertions(+)
 create mode 100644 include/net/resolver.h
 create mode 100644 net/core/resolver.c

diff --git a/include/net/resolver.h b/include/net/resolver.h
new file mode 100644
index 000..8f73b5c
--- /dev/null
+++ b/include/net/resolver.h
@@ -0,0 +1,58 @@
+#ifndef __NET_RESOLVER_H
+#define __NET_RESOLVER_H
+
+#include 
+
+struct net_rslv;
+struct net_rslv_ent;
+
+typedef int (*net_rslv_cmpfn)(struct net_rslv *nrslv, const void *key,
+ const void *object);
+typedef void (*net_rslv_initfn)(struct net_rslv *nrslv, void *object);
+typedef void (*net_rslv_destroyfn)(struct net_rslv_ent *nrent);
+
+struct net_rslv {
+   struct rhashtable rhash_table;
+   struct rhashtable_params params;
+   net_rslv_cmpfn rslv_cmp;
+   net_rslv_initfn rslv_init;
+   net_rslv_destroyfn rslv_destroy;
+   size_t obj_size;
+   spinlock_t *locks;
+   unsigned int locks_mask;
+   unsigned int hash_rnd;
+   long timeout;
+};
+
+struct net_rslv_ent {
+   struct rcu_head rcu;
+   union {
+   /* Fields set when entry is in hash table */
+   struct {
+   struct rhash_head node;
+   struct delayed_work timeout_work;
+   struct net_rslv *nrslv;
+   };
+
+   /* Fields set when rcu