* Re: [PATCH RFC] ipv6: use stronger hash for reassembly queue hash table
From: Jesper Dangaard Brouer @ 2013-03-14 9:18 UTC (permalink / raw)
To: Hannes Frederic Sowa; +Cc: Eric Dumazet, netdev, yoshfuji
In-Reply-To: <20130314072839.GD4129@order.stressinduktion.org>
On Thu, 2013-03-14 at 08:28 +0100, Hannes Frederic Sowa wrote:
> On Thu, Mar 14, 2013 at 08:23:41AM +0100, Hannes Frederic Sowa wrote:
> > On Thu, Mar 14, 2013 at 08:10:40AM +0100, Jesper Dangaard Brouer wrote:
> > > On Thu, 2013-03-14 at 02:37 +0100, Hannes Frederic Sowa wrote:
> > >
> > > > [PATCH net] inet: limit length of fragment queue hash table bucket lists
> > > >
> > > > This patch introduces a constant limit of the fragment queue hash
> > > > table bucket list lengths. Currently the limit 128 is choosen somewhat
> > > > arbitrary and just ensures that we can fill up the fragment cache with
> > > > empty packets up to the default ip_frag_high_thresh limits. It should
> > > > just protect from list iteration eating considerable amounts of cpu.
> > > >
> > > > If we reach the maximum length in one hash bucket a warning is printed.
> > > > This is implemented on the caller side of inet_frag_find to distinguish
> > > > between the different users of inet_fragment.c.
> > >
> > > I like the idea of having a safe guard on the fragment queue hash table
> > > bucket list lengths. But I'm considering another cleanup/evictor
> > > strategy, where we drop the LRU list, and do frag eviction on a hash
> > > bucket level (which will be more cache optimal). This strategy would
> > > also involve a list length limit.
> >
> > I would try to get a simple guard into v3.9. In 3.9 the hashing of the key
> > of ipv6 fragments changed in such a way that an attacker could generate
> > fragments which would end up in just one hash chain, thus eating a lot
> > of cpu time because of list traversal. Later on, when you post your
> > patches we could simply revert/update this safeguard to your version.
>
> I just wanted to mention that if you plan to target v3.9 with some patches we
> could simply drop this patch.
I will start working on this as soon as Netfilter Workshop is over and I
have recovered from organizing this event. DaveM told me I needed to
finish my frag patches first, before I could implement all the other
cool ideas we have come up with during the workshop ;-)
But I don't know if my frag changes can make v3.9, maybe v3.10 is more
realistic? In which case we should use your patch to close this problem
on v3.9 IMHO.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: RFC crap-patch [PATCH] net: Per CPU separate frag mem accounting
From: Jesper Dangaard Brouer @ 2013-03-14 8:59 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Hannes Frederic Sowa, netdev, yoshfuji
In-Reply-To: <1363245955.14913.21.camel@localhost>
On Thu, 2013-03-14 at 08:25 +0100, Jesper Dangaard Brouer wrote:
> This is NOT the patch I just mentioned in the other thread, of removing
> the LRU list. This patch does real per cpu mem acct, and LRU per CPU.
>
> I get really good performance number with this patch, but I still think
> this might not be the correct solution.
The reason is this depend on fragments entering the same HW queue, some
NICs might not put the first fragment (which have the full header
tuples) and the remaining fragments on the same queue. In which case
this patch will loose its performance gain.
> My current best results, which got applied recently, compared to this
> patch:
> - Test-type: Test-20G64K Test-20G3F 20G64K+DoS 20G3F+DoS
> - Patch-06: 18486.7 Mbit/s 10723.20 3657.85 4560.64 Mbit/s
> - curr-best: 19041.0 Mbit/s 12105.20 10160.40 11179.30 Mbit/s
I noticed that this also included some other patches in my stack
- New patchset-B:
- PatchB-07: Fix LRU list head multi CPU race
- PatchB-07: 18731.5 Mbit/s 10721.9 4079.22 5208.73 Mbit/s
- PatchB-08: Per hash bucket locking
- PatchB-08: 15959.5 Mbit/s 10968.9 4294.63 6365.16 Mbit/s
As you can see I'm looking into why "PatchB-08" which implement per hash
bucket locking is reducing throughput in Test-20G64K.
> Thus, I have almost solved DoS effect Test-20G3F 12GBit/s -> 11Gbit/s
> under DoS. The 64K+DoS case is not perfect yet, 19Gbit/s -> 11 Gbit/s.
>
> --Jesper
^ permalink raw reply
* Re: [PATCH RFC] ipv6: use stronger hash for reassembly queue hash table
From: Hannes Frederic Sowa @ 2013-03-14 7:28 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Eric Dumazet, netdev, yoshfuji
In-Reply-To: <20130314072341.GC4129@order.stressinduktion.org>
On Thu, Mar 14, 2013 at 08:23:41AM +0100, Hannes Frederic Sowa wrote:
> On Thu, Mar 14, 2013 at 08:10:40AM +0100, Jesper Dangaard Brouer wrote:
> > On Thu, 2013-03-14 at 02:37 +0100, Hannes Frederic Sowa wrote:
> >
> > > [PATCH net] inet: limit length of fragment queue hash table bucket lists
> > >
> > > This patch introduces a constant limit of the fragment queue hash
> > > table bucket list lengths. Currently the limit 128 is choosen somewhat
> > > arbitrary and just ensures that we can fill up the fragment cache with
> > > empty packets up to the default ip_frag_high_thresh limits. It should
> > > just protect from list iteration eating considerable amounts of cpu.
> > >
> > > If we reach the maximum length in one hash bucket a warning is printed.
> > > This is implemented on the caller side of inet_frag_find to distinguish
> > > between the different users of inet_fragment.c.
> >
> > I like the idea of having a safe guard on the fragment queue hash table
> > bucket list lengths. But I'm considering another cleanup/evictor
> > strategy, where we drop the LRU list, and do frag eviction on a hash
> > bucket level (which will be more cache optimal). This strategy would
> > also involve a list length limit.
>
> I would try to get a simple guard into v3.9. In 3.9 the hashing of the key
> of ipv6 fragments changed in such a way that an attacker could generate
> fragments which would end up in just one hash chain, thus eating a lot
> of cpu time because of list traversal. Later on, when you post your
> patches we could simply revert/update this safeguard to your version.
I just wanted to mention that if you plan to target v3.9 with some patches we
could simply drop this patch.
^ permalink raw reply
* RFC crap-patch [PATCH] net: Per CPU separate frag mem accounting
From: Jesper Dangaard Brouer @ 2013-03-14 7:25 UTC (permalink / raw)
To: Eric Dumazet, Hannes Frederic Sowa; +Cc: netdev, yoshfuji
In-Reply-To: <20130308221744.5312.14924.stgit@dragon>
His is NOT the patch I just mentioned in the other thread, of removing
the LRU list. This patch does real per cpu mem acct, and LRU per CPU.
I get really good performance number with this patch, but I still think
this might not be the correct solution.
My current best results, which got applied recently, compared to this
patch:
- Test-type: Test-20G64K Test-20G3F 20G64K+DoS 20G3F+DoS
- Patch-06: 18486.7 Mbit/s 10723.20 3657.85 4560.64 Mbit/s
- curr-best: 19041.0 Mbit/s 12105.20 10160.40 11179.30 Mbit/s
Thus, I have almost solved DoS effect Test-20G3F 12GBit/s -> 11Gbit/s
under DoS. The 64K+DoS case is not perfect yet, 19Gbit/s -> 11 Gbit/s.
--Jesper
On Fri, 2013-03-08 at 23:17 +0100, Jesper Dangaard Brouer wrote:
> ... testing if the percpu_counter does not scale in DoS situations
> ---
>
> include/net/inet_frag.h | 99 ++++++++++++++++++-------------
> net/ipv4/inet_fragment.c | 61 +++++++++++++++----
> net/ipv4/ip_fragment.c | 3 +
> net/ipv6/netfilter/nf_conntrack_reasm.c | 2 -
> net/ipv6/reassembly.c | 2 -
> 5 files changed, 110 insertions(+), 57 deletions(-)
>
> diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
> index f2b46a5..974434a 100644
> --- a/include/net/inet_frag.h
> +++ b/include/net/inet_frag.h
> @@ -1,22 +1,31 @@
> #ifndef __NET_FRAG_H__
> #define __NET_FRAG_H__
>
> -#include <linux/percpu_counter.h>
> +//#include <linux/percpu_counter.h>
> +#include <linux/spinlock.h>
> +#include <linux/atomic.h>
> +#include <linux/percpu.h>
>
> -struct netns_frags {
> - int nqueues;
> - struct list_head lru_list;
> - spinlock_t lru_lock;
> +/* Need to maintain these resource limits per CPU, else we will kill
> + * performance due to cache-line bouncing
> + */
> +struct frag_cpu_limit {
> + atomic_t mem;
> + struct list_head lru_list;
> + spinlock_t lru_lock;
> +} ____cacheline_aligned_in_smp;
>
> - /* The percpu_counter "mem" need to be cacheline aligned.
> - * mem.count must not share cacheline with other writers
> - */
> - struct percpu_counter mem ____cacheline_aligned_in_smp;
> +struct netns_frags {
>
> /* sysctls */
> int timeout;
> int high_thresh;
> int low_thresh;
> +
> + struct frag_cpu_limit __percpu *percpu;
> +
> + // TODO move "nqueues" elsewere...
> + int nqueues ____cacheline_aligned_in_smp;
> };
>
> struct inet_frag_queue {
> @@ -25,6 +34,7 @@ struct inet_frag_queue {
> struct list_head lru_list; /* lru list member */
> struct hlist_node list;
> atomic_t refcnt;
> + u32 cpu_alloc; /* used for mem limit accounting */
> struct sk_buff *fragments; /* list of received fragments */
> struct sk_buff *fragments_tail;
> ktime_t stamp;
> @@ -80,7 +90,8 @@ void inet_frags_exit_net(struct netns_frags *nf, struct inet_frags *f);
> void inet_frag_kill(struct inet_frag_queue *q, struct inet_frags *f);
> void inet_frag_destroy(struct inet_frag_queue *q,
> struct inet_frags *f, int *work);
> -int inet_frag_evictor(struct netns_frags *nf, struct inet_frags *f, bool force);
> +int inet_frag_evictor(struct netns_frags *nf, struct inet_frags *f,
> + bool force, int on_cpu);
> struct inet_frag_queue *inet_frag_find(struct netns_frags *nf,
> struct inet_frags *f, void *key, unsigned int hash)
> __releases(&f->lock);
> @@ -93,59 +104,65 @@ static inline void inet_frag_put(struct inet_frag_queue *q, struct inet_frags *f
>
> /* Memory Tracking Functions. */
>
> -/* The default percpu_counter batch size is not big enough to scale to
> - * fragmentation mem acct sizes.
> - * The mem size of a 64K fragment is approx:
> - * (44 fragments * 2944 truesize) + frag_queue struct(200) = 129736 bytes
> - */
> -static unsigned int frag_percpu_counter_batch = 130000;
> -
> -static inline int frag_mem_limit(struct netns_frags *nf)
> -{
> - return percpu_counter_read(&nf->mem);
> -}
> -
> static inline void sub_frag_mem_limit(struct inet_frag_queue *q, int i)
> {
> - __percpu_counter_add(&q->net->mem, -i, frag_percpu_counter_batch);
> + int cpu = q->cpu_alloc;
> + struct frag_cpu_limit *percpu = per_cpu_ptr(q->net->percpu, cpu);
> + atomic_sub(i, &percpu->mem);
> }
>
> static inline void add_frag_mem_limit(struct inet_frag_queue *q, int i)
> {
> - __percpu_counter_add(&q->net->mem, i, frag_percpu_counter_batch);
> -}
> -
> -static inline void init_frag_mem_limit(struct netns_frags *nf)
> -{
> - percpu_counter_init(&nf->mem, 0);
> + int cpu = q->cpu_alloc;
> + struct frag_cpu_limit *percpu = per_cpu_ptr(q->net->percpu, cpu);
> + atomic_add(i, &percpu->mem);
> }
>
> static inline int sum_frag_mem_limit(struct netns_frags *nf)
> {
> - return percpu_counter_sum_positive(&nf->mem);
> + unsigned int sum = 0;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + struct frag_cpu_limit *percpu = per_cpu_ptr(nf->percpu, cpu);
> +
> + sum += atomic_read(&percpu->mem);
> + }
> + return sum;
> }
>
> +/* LRU (Least Recently Used) resource functions */
> +
> static inline void inet_frag_lru_move(struct inet_frag_queue *q)
> {
> - spin_lock(&q->net->lru_lock);
> - list_move_tail(&q->lru_list, &q->net->lru_list);
> - spin_unlock(&q->net->lru_lock);
> + int cpu = q->cpu_alloc;
> + struct frag_cpu_limit *percpu = per_cpu_ptr(q->net->percpu, cpu);
> +
> + spin_lock(&percpu->lru_lock);
> + list_move_tail(&q->lru_list, &percpu->lru_list);
> + spin_unlock(&percpu->lru_lock);
> }
>
> static inline void inet_frag_lru_del(struct inet_frag_queue *q)
> {
> - spin_lock(&q->net->lru_lock);
> - list_del(&q->lru_list);
> - q->net->nqueues--;
> - spin_unlock(&q->net->lru_lock);
> + int cpu = q->cpu_alloc;
> + struct frag_cpu_limit *percpu = per_cpu_ptr(q->net->percpu, cpu);
> +
> + spin_lock(&percpu->lru_lock);
> + list_del(&q->lru_list);
> + q->net->nqueues--; //FIXME
> + spin_unlock(&percpu->lru_lock);
> }
>
> static inline void inet_frag_lru_add(struct netns_frags *nf,
> struct inet_frag_queue *q)
> {
> - spin_lock(&nf->lru_lock);
> - list_add_tail(&q->lru_list, &nf->lru_list);
> - q->net->nqueues++;
> - spin_unlock(&nf->lru_lock);
> + int cpu = q->cpu_alloc;
> + struct frag_cpu_limit *percpu = per_cpu_ptr(nf->percpu, cpu);
> +
> + spin_lock(&percpu->lru_lock);
> + list_add_tail(&q->lru_list, &percpu->lru_list);
> + q->net->nqueues++; //FIXME
> + spin_unlock(&percpu->lru_lock);
> }
> #endif
> diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
> index e5c426f..f09fa7e 100644
> --- a/net/ipv4/inet_fragment.c
> +++ b/net/ipv4/inet_fragment.c
> @@ -23,6 +23,18 @@
>
> #include <net/inet_frag.h>
>
> +static inline int frag_mem_limit_on_cpu(struct netns_frags *nf, int on_cpu)
> +{
> + struct frag_cpu_limit *percpu = per_cpu_ptr(nf->percpu, on_cpu);
> + return atomic_read(&percpu->mem);
> +}
> +
> +static inline int frag_mem_limit(struct netns_frags *nf)
> +{
> + int cpu = smp_processor_id();
> + return frag_mem_limit_on_cpu(nf, cpu);
> +}
> +
> static void inet_frag_secret_rebuild(unsigned long dummy)
> {
> struct inet_frags *f = (struct inet_frags *)dummy;
> @@ -81,12 +93,28 @@ void inet_frags_init(struct inet_frags *f)
> }
> EXPORT_SYMBOL(inet_frags_init);
>
> +static int inet_frags_init_percpu_limit(struct netns_frags *nf)
> +{
> + int cpu;
> +
> + nf->percpu = alloc_percpu(struct frag_cpu_limit);
> + if (!nf->percpu)
> + return -ENOMEM;
> +
> + for_each_possible_cpu(cpu) {
> + struct frag_cpu_limit *percpu = per_cpu_ptr(nf->percpu, cpu);
> +
> + INIT_LIST_HEAD(&percpu->lru_list);
> + spin_lock_init(&percpu->lru_lock);
> + atomic_set(&percpu->mem, 0);
> + }
> + return 1;
> +}
> +
> void inet_frags_init_net(struct netns_frags *nf)
> {
> nf->nqueues = 0; //remove?
> - init_frag_mem_limit(nf);
> - INIT_LIST_HEAD(&nf->lru_list);
> - spin_lock_init(&nf->lru_lock);
> + inet_frags_init_percpu_limit(nf);
> }
> EXPORT_SYMBOL(inet_frags_init_net);
>
> @@ -98,13 +126,16 @@ EXPORT_SYMBOL(inet_frags_fini);
>
> void inet_frags_exit_net(struct netns_frags *nf, struct inet_frags *f)
> {
> + int cpu;
> +
> nf->low_thresh = 0;
>
> local_bh_disable();
> - inet_frag_evictor(nf, f, true);
> + for_each_possible_cpu(cpu)
> + inet_frag_evictor(nf, f, true, cpu);
> local_bh_enable();
>
> - percpu_counter_destroy(&nf->mem);
> + free_percpu(nf->percpu);
> }
> EXPORT_SYMBOL(inet_frags_exit_net);
>
> @@ -179,33 +210,36 @@ void inet_frag_destroy(struct inet_frag_queue *q, struct inet_frags *f,
> }
> EXPORT_SYMBOL(inet_frag_destroy);
>
> -int inet_frag_evictor(struct netns_frags *nf, struct inet_frags *f, bool force)
> +int inet_frag_evictor(struct netns_frags *nf, struct inet_frags *f,
> + bool force, int on_cpu)
> {
> struct inet_frag_queue *q;
> int work, evicted = 0;
> + int cpu = (likely(on_cpu < 0)) ? smp_processor_id() : on_cpu;
> + struct frag_cpu_limit *percpu = per_cpu_ptr(nf->percpu, cpu);
>
> if (!force) {
> - if (frag_mem_limit(nf) <= nf->high_thresh)
> + if (frag_mem_limit_on_cpu(nf, cpu) <= nf->high_thresh)
> return 0;
> }
>
> - work = frag_mem_limit(nf) - nf->low_thresh;
> + work = frag_mem_limit_on_cpu(nf, cpu) - nf->low_thresh;
> while (work > 0) {
> - spin_lock(&nf->lru_lock);
> + spin_lock(&percpu->lru_lock);
>
> - if (list_empty(&nf->lru_list)) {
> - spin_unlock(&nf->lru_lock);
> + if (list_empty(&percpu->lru_list)) {
> + spin_unlock(&percpu->lru_lock);
> break;
> }
>
> - q = list_first_entry(&nf->lru_list,
> + q = list_first_entry(&percpu->lru_list,
> struct inet_frag_queue, lru_list);
> atomic_inc(&q->refcnt);
>
> // TEST: remove q from list to avoid more CPUs grabbing it
> list_del_init(&q->lru_list);
>
> - spin_unlock(&nf->lru_lock);
> + spin_unlock(&percpu->lru_lock);
>
> spin_lock(&q->lock);
> if (!(q->last_in & INET_FRAG_COMPLETE))
> @@ -283,6 +317,7 @@ static struct inet_frag_queue *inet_frag_alloc(struct netns_frags *nf,
> return NULL;
>
> q->net = nf;
> + q->cpu_alloc = (u32) smp_processor_id();
> f->constructor(q, arg);
> add_frag_mem_limit(q, f->qsize);
>
> diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
> index 1211613..4241417 100644
> --- a/net/ipv4/ip_fragment.c
> +++ b/net/ipv4/ip_fragment.c
> @@ -18,6 +18,7 @@
> * John McDonald : 0 length frag bug.
> * Alexey Kuznetsov: SMP races, threading, cleanup.
> * Patrick McHardy : LRU queue of frag heads for evictor.
> + * Jesper Brouer : SMP/NUMA scalability
> */
>
> #define pr_fmt(fmt) "IPv4: " fmt
> @@ -212,7 +213,7 @@ static void ip_evictor(struct net *net)
> {
> int evicted;
>
> - evicted = inet_frag_evictor(&net->ipv4.frags, &ip4_frags, false);
> + evicted = inet_frag_evictor(&net->ipv4.frags, &ip4_frags, false, -1);
> if (evicted)
> IP_ADD_STATS_BH(net, IPSTATS_MIB_REASMFAILS, evicted);
> }
> diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
> index c674f15..37291f9 100644
> --- a/net/ipv6/netfilter/nf_conntrack_reasm.c
> +++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
> @@ -569,7 +569,7 @@ struct sk_buff *nf_ct_frag6_gather(struct sk_buff *skb, u32 user)
> fhdr = (struct frag_hdr *)skb_transport_header(clone);
>
> local_bh_disable();
> - inet_frag_evictor(&net->nf_frag.frags, &nf_frags, false);
> + inet_frag_evictor(&net->nf_frag.frags, &nf_frags, false, -1);
> local_bh_enable();
>
> fq = fq_find(net, fhdr->identification, user, &hdr->saddr, &hdr->daddr);
> diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
> index bab2c27..d1e70dd 100644
> --- a/net/ipv6/reassembly.c
> +++ b/net/ipv6/reassembly.c
> @@ -529,7 +529,7 @@ static int ipv6_frag_rcv(struct sk_buff *skb)
> return 1;
> }
>
> - evicted = inet_frag_evictor(&net->ipv6.frags, &ip6_frags, false);
> + evicted = inet_frag_evictor(&net->ipv6.frags, &ip6_frags, false, -1);
> if (evicted)
> IP6_ADD_STATS_BH(net, ip6_dst_idev(skb_dst(skb)),
> IPSTATS_MIB_REASMFAILS, evicted);
>
^ permalink raw reply
* Re: [PATCH RFC] ipv6: use stronger hash for reassembly queue hash table
From: Hannes Frederic Sowa @ 2013-03-14 7:23 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: Eric Dumazet, netdev, yoshfuji
In-Reply-To: <1363245040.14913.11.camel@localhost>
On Thu, Mar 14, 2013 at 08:10:40AM +0100, Jesper Dangaard Brouer wrote:
> On Thu, 2013-03-14 at 02:37 +0100, Hannes Frederic Sowa wrote:
>
> > [PATCH net] inet: limit length of fragment queue hash table bucket lists
> >
> > This patch introduces a constant limit of the fragment queue hash
> > table bucket list lengths. Currently the limit 128 is choosen somewhat
> > arbitrary and just ensures that we can fill up the fragment cache with
> > empty packets up to the default ip_frag_high_thresh limits. It should
> > just protect from list iteration eating considerable amounts of cpu.
> >
> > If we reach the maximum length in one hash bucket a warning is printed.
> > This is implemented on the caller side of inet_frag_find to distinguish
> > between the different users of inet_fragment.c.
>
> I like the idea of having a safe guard on the fragment queue hash table
> bucket list lengths. But I'm considering another cleanup/evictor
> strategy, where we drop the LRU list, and do frag eviction on a hash
> bucket level (which will be more cache optimal). This strategy would
> also involve a list length limit.
I would try to get a simple guard into v3.9. In 3.9 the hashing of the key
of ipv6 fragments changed in such a way that an attacker could generate
fragments which would end up in just one hash chain, thus eating a lot
of cpu time because of list traversal. Later on, when you post your
patches we could simply revert/update this safeguard to your version.
Thanks,
Hannes
^ permalink raw reply
* Re: [PATCH RFC] ipv6: use stronger hash for reassembly queue hash table
From: Hannes Frederic Sowa @ 2013-03-14 7:14 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Eric Dumazet, netdev, yoshfuji, brouer
In-Reply-To: <CAOaVG148gUzcb2qmg0B7kdWNzNzerMdXZzv2qamXq8ToUFsUyw@mail.gmail.com>
On Wed, Mar 13, 2013 at 09:36:52PM -0700, Stephen Hemminger wrote:
> > +#define INET_FRAG_FIND_CHECK(val) \
> > + ({ \
> > + static const char ___mem[] = \
> > + KERN_ERR pr_fmt( \
> > + "inet_frag_find: No memory left." \
> > + " Dropping fragment.\n"); \
> > + static const char ___limit[] = \
> > + KERN_WARNING pr_fmt( \
> > + "inet_frag_find: Fragment hash bucket" \
> > + " list length grew above limit " \
> > + __stringify(INETFRAGS_MAXDEPTH) \
> > + ". Dropping fragment.\n"); \
> > + bool ___b = true; \
> > + if (IS_ERR_OR_NULL(val)) { \
> > + ___b = false; \
> > + if (PTR_ERR(val) == -ENOBUFS) \
> > + LIMIT_NETDEBUG(___limit); \
> > + else \
> > + LIMIT_NETDEBUG(___mem); \
> > + } \
> > + ___b; \
> > + })
> > +
>
> Big macros suck, write it as an inline function or better yet a real function.
I switched to the macro to have string expansion with pr_fmt. So it is visible
from the dmesg if IPv4, IPv6 or IPv6-nf did generate the message. This could
be done with a function, too, but would require a bit more string handling.
^ permalink raw reply
* Re: [PATCH RFC] ipv6: use stronger hash for reassembly queue hash table
From: Jesper Dangaard Brouer @ 2013-03-14 7:10 UTC (permalink / raw)
To: Hannes Frederic Sowa; +Cc: Eric Dumazet, netdev, yoshfuji
In-Reply-To: <20130314013702.GA4129@order.stressinduktion.org>
On Thu, 2013-03-14 at 02:37 +0100, Hannes Frederic Sowa wrote:
> [PATCH net] inet: limit length of fragment queue hash table bucket lists
>
> This patch introduces a constant limit of the fragment queue hash
> table bucket list lengths. Currently the limit 128 is choosen somewhat
> arbitrary and just ensures that we can fill up the fragment cache with
> empty packets up to the default ip_frag_high_thresh limits. It should
> just protect from list iteration eating considerable amounts of cpu.
>
> If we reach the maximum length in one hash bucket a warning is printed.
> This is implemented on the caller side of inet_frag_find to distinguish
> between the different users of inet_fragment.c.
I like the idea of having a safe guard on the fragment queue hash table
bucket list lengths. But I'm considering another cleanup/evictor
strategy, where we drop the LRU list, and do frag eviction on a hash
bucket level (which will be more cache optimal). This strategy would
also involve a list length limit.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net-next] bond: add support to read speed and duplex via ethtool
From: Glen Turner @ 2013-03-14 6:57 UTC (permalink / raw)
To: Andy Gospodarek; +Cc: netdev
In-Reply-To: <20130308153505.GB7606@gospo.rdu.redhat.com>
On 09/03/2013, at 2:05 AM, Andy Gospodarek <andy@greyhouse.net> wrote:
>
> That is a great point. Are you saying that quagga or other
> outside-the-kernel routing daemons actually query ethtool settings to
> determine link speed and weight?
Wasn't sure. I was hinting if it did happen it would be serious, as bonding is commonly used in network designs to reduce L3 flapping.
I downloaded Quagga for a look.
Quagga uses the NetLink protocol to talk with the kernel, and it doesn't seem to be possible to retrieve an interface's bandwidth using that. That's possibly because bandwidth is a hairy concept (eg, you can argue that it's more a attribute of links than interfaces). Netlink does return a metric for links, but not a bandwidth.
So Quagga can retrieve each link's metric, that metric is then interpreted by the various routing daemons. But in Linux metrics are usually 1, not based on link speeds like they are with routers.
So Quagga in practice relies on manual setting of bandwidth in zebra.conf:
interface eth0
! 1000Base-T
bandwidth 1000000
If the interface bandwidth is nonzero then Quagga uses that (adjusted by "auto-cost reference-bandwidth") to set the link metric used by the routing protocol. Otherwise it seems to adjust the Linux metric (I didn't check how or why) and use that.
So, at least in the Quagga instance, ethtool's reported bandwidth isn't used for routing.
The more I think about the look of the graphs the more I think that they are not going to work as you hope. The graphed value shouldn't suddenly halve just because a link has come back online.
I really think you'll be happier reporting the addition of the maximum bandwidth of all the interfaces in the bond, whatever their carrier state or negotiated value. Maybe you could change the bonding driver so that if this results is poor scaling then you can set a bond's speed with "ethtool -s bond0 400" and "ethtool bond0" will report that. After all it's not like setting a bond's speed could have any physical meaning.
Best wishes, glen
--
Glen Turner <http://www.gdt.id.au/~gdt/>
^ permalink raw reply
* Re: [PATCH 1/1] Fix dst_neigh_lookup/dst_neigh_lookup_skb return value handling bug
From: Zhouyi Zhou @ 2013-03-14 5:48 UTC (permalink / raw)
To: netdev
Cong Wang <xiyou.wangcong@gmail.com> wrote
>> neigh = dst_neigh_lookup(ep->dst,
>> &ep->com.cm_id->remote_addr.sin_addr.s_addr);
>> + if (!neigh) {
>> + pr_err("%s - cannot alloc neigh.\n", __func__);
>> + err = -ENOMEM;
>> + goto fail4;
>You don't need to print error messages for OOM, the mm subsystem will
>do.
Thanks for reviewing, I clone these error messages print from the code
that follows:
1612 if (!ep->l2t) {
1613 pr_err("%s - cannot alloc l2e.\n", __func__);
1614 err = -ENOMEM;
1615 goto fail4;
1616 }
^ permalink raw reply
* Re: [PATCH 1/1] Fix dst_neigh_lookup/dst_neigh_lookup_skb return value handling bug
From: Cong Wang @ 2013-03-14 5:26 UTC (permalink / raw)
To: netdev; +Cc: linux-rdma
In-Reply-To: <1363232492-4562-1-git-send-email-zhouzhouyi@gmail.com>
On Thu, 14 Mar 2013 at 03:41 GMT, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
> Tested on my x86_64 machine
>
> Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
> ---
> drivers/infiniband/hw/cxgb4/cm.c | 12 ++++++++++++
> include/net/dst.h | 6 ++++--
> 2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
> index 565bfb1..6b95851 100644
> --- a/drivers/infiniband/hw/cxgb4/cm.c
> +++ b/drivers/infiniband/hw/cxgb4/cm.c
> @@ -1575,6 +1575,12 @@ static int c4iw_reconnect(struct c4iw_ep *ep)
>
> neigh = dst_neigh_lookup(ep->dst,
> &ep->com.cm_id->remote_addr.sin_addr.s_addr);
> + if (!neigh) {
> + pr_err("%s - cannot alloc neigh.\n", __func__);
> + err = -ENOMEM;
> + goto fail4;
You don't need to print error messages for OOM, the mm subsystem will
do.
^ permalink raw reply
* Re: [PATCH RFC] ipv6: use stronger hash for reassembly queue hash table
From: Stephen Hemminger @ 2013-03-14 4:36 UTC (permalink / raw)
To: Eric Dumazet, netdev, yoshfuji, brouer
In-Reply-To: <20130314013702.GA4129@order.stressinduktion.org>
> +#define INET_FRAG_FIND_CHECK(val) \
> + ({ \
> + static const char ___mem[] = \
> + KERN_ERR pr_fmt( \
> + "inet_frag_find: No memory left." \
> + " Dropping fragment.\n"); \
> + static const char ___limit[] = \
> + KERN_WARNING pr_fmt( \
> + "inet_frag_find: Fragment hash bucket" \
> + " list length grew above limit " \
> + __stringify(INETFRAGS_MAXDEPTH) \
> + ". Dropping fragment.\n"); \
> + bool ___b = true; \
> + if (IS_ERR_OR_NULL(val)) { \
> + ___b = false; \
> + if (PTR_ERR(val) == -ENOBUFS) \
> + LIMIT_NETDEBUG(___limit); \
> + else \
> + LIMIT_NETDEBUG(___mem); \
> + } \
> + ___b; \
> + })
> +
Big macros suck, write it as an inline function or better yet a real function.
^ permalink raw reply
* [PATCH iproute2] Add missing space
From: roopa @ 2013-03-14 4:14 UTC (permalink / raw)
To: shemminger, roopa; +Cc: netdev
From: roopa <roopa@cumulusnetworks.com>
Reported-by: Andreas Henriksson <andreas@fatal.se>
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
ip/ipaddress.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index fdd2a74..0712164 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -470,7 +470,7 @@ int print_linkinfo(const struct sockaddr_nl *who,
if (do_link && tb[IFLA_IFALIAS]) {
fprintf(fp, "%s", _SL_);
- fprintf(fp," alias %s",
+ fprintf(fp, " alias %s",
rta_getattr_str(tb[IFLA_IFALIAS]));
}
--
1.7.2.5
^ permalink raw reply related
* Re: [PATCH] Rate should be u64 to avoid integer overflow at high speeds (>= ~35Gbit)
From: Bill Fink @ 2013-03-14 4:08 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Eric Dumazet, Thomas Graf, Chris Friesen, Vimal, netdev,
shemminger
In-Reply-To: <20130313083400.2329e982@nehalam.linuxnetplumber.net>
On Wed, 13 Mar 2013, Stephen Hemminger wrote:
> On Wed, 13 Mar 2013 11:29:50 -0400
> Bill Fink <billfink@mindspring.com> wrote:
>
> > On Wed, 13 Mar 2013, Eric Dumazet wrote:
> >
> > > On Wed, 2013-03-13 at 02:01 -0400, Bill Fink wrote:
> > >
> > > > The last time this was discussed appears to be (on 2011-03-28):
> > > >
> > > > http://marc.info/?l=linux-netdev&m=130128741907282&w=2
> > > >
> > > > where Maciej Żenczykowski argued that creating a new 64-bit
> > > > Netlink attribute for this would be much more complex than for
> > > > the IFLA_STATS64 support. There was no reply.
> > > >
> > > > Providing a new multiplier/shift parameter would be a simple
> > > > way to extend support for higher rates, and would not break
> > > > existing user space that doesn't require the higher rates.
> > > > I imagine the user would not explicitly specify the multiplier/
> > > > shift parameter, but would just normally specify the desired
> > > > rate, and a newer tc would figure out what multiplier/shift
> > > > to use if a high enough rate demanded it. To maintain user
> > > > space compatibility, the kernel should report back the same
> > > > rate and multiplier/shift it was given, and the newer tc would
> > > > convert it back to the user's originally specified rate. Older
> > > > user space that was fine with the ~34 Gbps rate limitation would
> > > > always have the default multiplier of 1 or shift of 0 bits, and
> > > > would see the exact same unmultiplied/unshifted rate it always
> > > > did.
> > >
> > > We already said no to such a hack. Maybe its not clear enough ?
> > >
> > > netlink allows us to a proper way, and Thomas Graf explained how we
> > > expect the thing to be done.
> > >
> > > Yes, this is not a one liner patch, its a bit more of work, and its how
> > > it will be done when someone does the job.
> >
> > I've no problem with that since it is a cleaner solution, but
> > one that requires significantly more work. I was only arguing
> > that the multiplier/shift approach was also a workable solution
> > and should be simpler to implement. But since there appears to
> > be developer consensus that it's not a desired method, I'm fine
> > with going along with that expert opinion.
> >
> > -Bill
>
> As others have said the multiplier shift approach is a not a workable
> solution because it is likely to cause too many compatibility surprises.
> Older kernels would ignore the multiplier and therefore not give the users
> the effective rate they wanted.
Hopefully they would get an error saying that the rate was not
supported by the running kernel, from a failure of trying to
set the multiplier/shift. You can't get new features from an
old kernel. But anything working today should still work since
if your rate is less than or equal to ~34 Gbps, your multiplier
would be 1 (or shift of 0 bits), and thus the effective rate is
unmodified from what the user specified (and thus no need to even
use the new interface).
Today if you specify a rate greater than ~34 Gbps, you don't get
what you expected, since from what I understand the value just
gets silently truncated so 40 Gbps results in 5.64 Gbps. See:
http://marc.info/?l=linux-netdev&m=130103727012841&w=2
So I don't think anything would get broken that isn't already
broken (or just currently impossible to do).
But I've already said that I'm fine with a more proper solution.
I just hope someone will implement it before another 2 years
passes, as I suspect 100G NICs will become available in that
timeframe.
-Bill
^ permalink raw reply
* net: clean leftover of COMPAT_NET_DEV_OPS removal
From: Fernando Luis Vázquez Cao @ 2013-03-14 2:57 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev
COMPAT_NET_DEV_OPS was removed a while back and with it the definition of
netdev_resync_ops() went away. Let's finish the clean-up.
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---
diff -urNp linux-3.9-rc2-orig/include/linux/netdevice.h linux-3.9-rc2/include/linux/netdevice.h
--- linux-3.9-rc2-orig/include/linux/netdevice.h 2013-03-11 08:54:19.000000000 +0900
+++ linux-3.9-rc2/include/linux/netdevice.h 2013-03-14 11:45:58.686715874 +0900
@@ -1684,7 +1684,6 @@ extern int netdev_refcnt_read(const st
extern void free_netdev(struct net_device *dev);
extern void synchronize_net(void);
extern int init_dummy_netdev(struct net_device *dev);
-extern void netdev_resync_ops(struct net_device *dev);
extern struct net_device *dev_get_by_index(struct net *net, int ifindex);
extern struct net_device *__dev_get_by_index(struct net *net, int ifindex);
^ permalink raw reply
* [PATCH 1/1] Fix dst_neigh_lookup/dst_neigh_lookup_skb return value handling bug
From: Zhouyi Zhou @ 2013-03-14 3:41 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA,
yizhouzhou-6xnC4QlcT6v/PtFMR13I2A, swise-ut6Up61K2wZBDgjK7y7TUQ,
roland-DgEjT+Ai2ygdnm+yROfE0A, sean.hefty-ral2JQCrhuEAvxtiuMwx3w,
hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w, Zhouyi Zhou
Tested on my x86_64 machine
Signed-off-by: Zhouyi Zhou <zhouzhouyi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/infiniband/hw/cxgb4/cm.c | 12 ++++++++++++
include/net/dst.h | 6 ++++--
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
index 565bfb1..6b95851 100644
--- a/drivers/infiniband/hw/cxgb4/cm.c
+++ b/drivers/infiniband/hw/cxgb4/cm.c
@@ -1575,6 +1575,12 @@ static int c4iw_reconnect(struct c4iw_ep *ep)
neigh = dst_neigh_lookup(ep->dst,
&ep->com.cm_id->remote_addr.sin_addr.s_addr);
+ if (!neigh) {
+ pr_err("%s - cannot alloc neigh.\n", __func__);
+ err = -ENOMEM;
+ goto fail4;
+ }
+
/* get a l2t entry */
if (neigh->dev->flags & IFF_LOOPBACK) {
PDBG("%s LOOPBACK\n", __func__);
@@ -3053,6 +3059,12 @@ static int rx_pkt(struct c4iw_dev *dev, struct sk_buff *skb)
dst = &rt->dst;
neigh = dst_neigh_lookup_skb(dst, skb);
+ if (!neigh) {
+ pr_err("%s - failed to allocate neigh!\n",
+ __func__);
+ goto free_dst;
+ }
+
if (neigh->dev->flags & IFF_LOOPBACK) {
pdev = ip_dev_find(&init_net, iph->daddr);
e = cxgb4_l2t_get(dev->rdev.lldi.l2t, neigh,
diff --git a/include/net/dst.h b/include/net/dst.h
index 853cda1..c083b5a 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -413,13 +413,15 @@ static inline int dst_neigh_output(struct dst_entry *dst, struct neighbour *n,
static inline struct neighbour *dst_neigh_lookup(const struct dst_entry *dst, const void *daddr)
{
- return dst->ops->neigh_lookup(dst, NULL, daddr);
+ struct neighbour *n = dst->ops->neigh_lookup(dst, NULL, daddr);
+ return IS_ERR(n) ? NULL : n;
}
static inline struct neighbour *dst_neigh_lookup_skb(const struct dst_entry *dst,
struct sk_buff *skb)
{
- return dst->ops->neigh_lookup(dst, skb, NULL);
+ struct neighbour *n = dst->ops->neigh_lookup(dst, skb, NULL);
+ return IS_ERR(n) ? NULL : n;
}
static inline void dst_link_failure(struct sk_buff *skb)
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* net: clean leftover of COMPAT_NET_DEV_OPS removal
From: Fernando Luis Vázquez Cao @ 2013-03-14 2:55 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev
COMPAT_NET_DEV_OPS was removed a while back and with it the definition of
netdev_resync_ops() went away. Let's finish the clean-up.
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---
diff -urNp linux-3.9-rc2-orig/include/linux/netdevice.h linux-3.9-rc2/include/linux/netdevice.h
--- linux-3.9-rc2-orig/include/linux/netdevice.h 2013-03-11 08:54:19.000000000 +0900
+++ linux-3.9-rc2/include/linux/netdevice.h 2013-03-14 11:45:58.686715874 +0900
@@ -1684,7 +1684,6 @@ extern int netdev_refcnt_read(const st
extern void free_netdev(struct net_device *dev);
extern void synchronize_net(void);
extern int init_dummy_netdev(struct net_device *dev);
-extern void netdev_resync_ops(struct net_device *dev);
extern struct net_device *dev_get_by_index(struct net *net, int ifindex);
extern struct net_device *__dev_get_by_index(struct net *net, int ifindex);
^ permalink raw reply
* Re: [PATCH] Revert "ip_gre: make ipgre_tunnel_xmit() not parse network header as IP unconditionally"
From: Isaku Yamahata @ 2013-03-14 2:53 UTC (permalink / raw)
To: Timo Teras; +Cc: netdev, Eric Dumazet, David S. Miller
In-Reply-To: <20130313175643.16218a15@vostro>
On Wed, Mar 13, 2013 at 05:56:43PM +0200, Timo Teras wrote:
> On Thu, 14 Mar 2013 00:05:10 +0900
> Isaku Yamahata <yamahata@valinux.co.jp> wrote:
>
> > Hi.
> >
> > On Wed, Mar 13, 2013 at 02:37:49PM +0200, Timo Teräs wrote:
> > > This reverts commit 412ed94744d16806fbec3bd250fd94e71cde5a1f.
> > >
> > > The commit is wrong as tiph points to the outer IPv4 header which is
> > > installed at ipgre_header() and not the inner one which is protocol
> > > dependant.
> > >
> > > This commit broke succesfully opennhrp which use PF_PACKET socket
> > > with ETH_P_NHRP protocol. Additionally ssl_addr is set to the
> > > link-layer IPv4 address. This address is written by ipgre_header()
> > > to the skb earlier, and this is the IPv4 header tiph should point
> > > to - regardless of the inner protocol payload.
> >
> > Is this the case only for ETH_P_HNRP?
> > I wrote the patch having MPLS over GRE in mind.
> > Should it be something like this?
> > if (protocol == htons(ETH_P_IP) || protocol == htons(ETH_P_NHRP))
> > ....
>
> No, the original code was correct for all protocols.
>
> tiph refers to the IPv4 headers pushed by ip_gre driver in
> ipgre_header() function. This is _always_ present regardless of the
> inner protocol. Thus no checking for protocol should be done.
>
> And for reference: NHRP does not have iphdr, it is completely different
> payload.
>
> What happens is on xmit path is:
>
> 1. ipgre_header() is always called first with NBMA destination
> address and outer IPv4 + GRE headers are pushed here with
> per-packet destination set
>
> 2. ipgre_tunnel_xmit() is called, tiph points to the tunnel's
> IPv4 header, gre_hlen is set to zero so that the header is not
> pushed twice, but the existing header is modified later
>
> Thus, it does not matter if the payload is MPLS, we will not refer to
> the mpls data - the skb has already tunnel's outer IPv4 and GRE header
> pushed, and tiph will refer to the tunnel's IPv4 header.
You're right. Thanks for explanation.
Originally I tried to copy TTL in MPLS label into outer IP header.
But dropped MPLS bits.
--
yamahata
^ permalink raw reply
* Re: [PATCH RFC] ipv6: use stronger hash for reassembly queue hash table
From: Hannes Frederic Sowa @ 2013-03-14 1:37 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, yoshfuji, brouer
In-Reply-To: <1363152568.13690.35.camel@edumazet-glaptop>
On Wed, Mar 13, 2013 at 06:29:28AM +0100, Eric Dumazet wrote:
> I would issue a one one time warning in syslog when depth exceeds the
> limit.
I addressed your suggestion to simplify this patch.
I decided against a once message but used net_ratelimit() (as it was
already used by the warning about no memory available). I don't have
a strong opinion on that, just thought it could be a recurring event
which would be worth reporting again because it should only happen on
strange/malicous traffic patterns where admins should act.
I based this patch on the net tree.
Thanks!
[PATCH net] inet: limit length of fragment queue hash table bucket lists
This patch introduces a constant limit of the fragment queue hash
table bucket list lengths. Currently the limit 128 is choosen somewhat
arbitrary and just ensures that we can fill up the fragment cache with
empty packets up to the default ip_frag_high_thresh limits. It should
just protect from list iteration eating considerable amounts of cpu.
If we reach the maximum length in one hash bucket a warning is printed.
This is implemented on the caller side of inet_frag_find to distinguish
between the different users of inet_fragment.c.
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Jesper Dangaard Brouer <jbrouer@redhat.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
include/net/inet_frag.h | 30 ++++++++++++++++++++++++++++++
net/ipv4/inet_fragment.c | 7 ++++++-
net/ipv4/ip_fragment.c | 9 ++-------
net/ipv6/netfilter/nf_conntrack_reasm.c | 10 ++++------
net/ipv6/reassembly.c | 6 ++++--
5 files changed, 46 insertions(+), 16 deletions(-)
diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
index 76c3fe5..0350468 100644
--- a/include/net/inet_frag.h
+++ b/include/net/inet_frag.h
@@ -43,6 +43,13 @@ struct inet_frag_queue {
#define INETFRAGS_HASHSZ 64
+/* averaged:
+ * max_depth = default ipfrag_high_thresh / INETFRAGS_HASHSZ /
+ * rounded up (SKB_TRUELEN(0) + sizeof(struct ipq or
+ * struct frag_queue))
+ */
+#define INETFRAGS_MAXDEPTH 128
+
struct inet_frags {
struct hlist_head hash[INETFRAGS_HASHSZ];
/* This rwlock is a global lock (seperate per IPv4, IPv6 and
@@ -77,6 +84,29 @@ struct inet_frag_queue *inet_frag_find(struct netns_frags *nf,
struct inet_frags *f, void *key, unsigned int hash)
__releases(&f->lock);
+#define INET_FRAG_FIND_CHECK(val) \
+ ({ \
+ static const char ___mem[] = \
+ KERN_ERR pr_fmt( \
+ "inet_frag_find: No memory left." \
+ " Dropping fragment.\n"); \
+ static const char ___limit[] = \
+ KERN_WARNING pr_fmt( \
+ "inet_frag_find: Fragment hash bucket" \
+ " list length grew above limit " \
+ __stringify(INETFRAGS_MAXDEPTH) \
+ ". Dropping fragment.\n"); \
+ bool ___b = true; \
+ if (IS_ERR_OR_NULL(val)) { \
+ ___b = false; \
+ if (PTR_ERR(val) == -ENOBUFS) \
+ LIMIT_NETDEBUG(___limit); \
+ else \
+ LIMIT_NETDEBUG(___mem); \
+ } \
+ ___b; \
+ })
+
static inline void inet_frag_put(struct inet_frag_queue *q, struct inet_frags *f)
{
if (atomic_dec_and_test(&q->refcnt))
diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 245ae07..0022a3e 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -277,6 +277,7 @@ struct inet_frag_queue *inet_frag_find(struct netns_frags *nf,
__releases(&f->lock)
{
struct inet_frag_queue *q;
+ int depth = 0;
hlist_for_each_entry(q, &f->hash[hash], list) {
if (q->net == nf && f->match(q, key)) {
@@ -284,9 +285,13 @@ struct inet_frag_queue *inet_frag_find(struct netns_frags *nf,
read_unlock(&f->lock);
return q;
}
+ depth++;
}
read_unlock(&f->lock);
- return inet_frag_create(nf, f, key);
+ if (depth <= INETFRAGS_MAXDEPTH)
+ return inet_frag_create(nf, f, key);
+ else
+ return ERR_PTR(-ENOBUFS);
}
EXPORT_SYMBOL(inet_frag_find);
diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index b6d30ac..8533316 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -292,14 +292,9 @@ static inline struct ipq *ip_find(struct net *net, struct iphdr *iph, u32 user)
hash = ipqhashfn(iph->id, iph->saddr, iph->daddr, iph->protocol);
q = inet_frag_find(&net->ipv4.frags, &ip4_frags, &arg, hash);
- if (q == NULL)
- goto out_nomem;
-
+ if (!INET_FRAG_FIND_CHECK(q))
+ return NULL;
return container_of(q, struct ipq, q);
-
-out_nomem:
- LIMIT_NETDEBUG(KERN_ERR pr_fmt("ip_frag_create: no memory left !\n"));
- return NULL;
}
/* Is the fragment too far ahead to be part of ipq? */
diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index 54087e9..f56468b 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -14,6 +14,8 @@
* 2 of the License, or (at your option) any later version.
*/
+#define pr_fmt(fmt) "IPv6-nf: " fmt
+
#include <linux/errno.h>
#include <linux/types.h>
#include <linux/string.h>
@@ -180,13 +182,9 @@ static inline struct frag_queue *fq_find(struct net *net, __be32 id,
q = inet_frag_find(&net->nf_frag.frags, &nf_frags, &arg, hash);
local_bh_enable();
- if (q == NULL)
- goto oom;
-
+ if (!INET_FRAG_FIND_CHECK(q))
+ return NULL;
return container_of(q, struct frag_queue, q);
-
-oom:
- return NULL;
}
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index 3c6a772..7dd0841 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -26,6 +26,9 @@
* YOSHIFUJI,H. @USAGI Always remove fragment header to
* calculate ICV correctly.
*/
+
+#define pr_fmt(fmt) "IPv6: " fmt
+
#include <linux/errno.h>
#include <linux/types.h>
#include <linux/string.h>
@@ -185,9 +188,8 @@ fq_find(struct net *net, __be32 id, const struct in6_addr *src, const struct in6
hash = inet6_hash_frag(id, src, dst, ip6_frags.rnd);
q = inet_frag_find(&net->ipv6.frags, &ip6_frags, &arg, hash);
- if (q == NULL)
+ if (!INET_FRAG_FIND_CHECK(q))
return NULL;
-
return container_of(q, struct frag_queue, q);
}
--
1.8.1.4
^ permalink raw reply related
* Re: [RFC PATCH] udp: don't rereference dst_entry dev pointer on rcv
From: Eric Dumazet @ 2013-03-14 1:18 UTC (permalink / raw)
To: Tom Parkin; +Cc: David Miller, netdev
In-Reply-To: <20130313232743.GA3686@raven>
On Wed, 2013-03-13 at 23:27 +0000, Tom Parkin wrote:
> I've been working to this end, and while I don't have a root cause as
> yet, I do have some more information.
>
> I think what's happening is that the dst_entry refcounting is getting
> screwed up either by the ip defrag code, or by something before that
> in the rcv path. What I see from ftrace debugging is that an skb
> fragment ends up queued on the reassembly queue while pointing to a
> dst_entry with a refcount of 0. If the dst_entry should be deleted before
> the final fragment in the frame arrives, then we end up accessing
> free'd memory.
>
> So far as I can make out, the l2tp code isn't doing anything untoward
> which is causing this bug. My stress test simply makes it easier to
> reproduce because I'm setting up and tearing down routes and devices
> a lot while passing data. I'm lucky in that my dev branch seems to
> reproduce this more easily than net-next master does, although the
> same oops occurs on master if you're prepared to wait around for long
> enough.
>
> This ftrace debug log snippet shows the sort of behaviour I'm seeing.
> The numbers in brackets after some dst pointer values represent the
> refcount for that dst:
>
> # The dst_entry is created with a refcount of 1
> <idle>-0 [000] ..s2 112.770192: dst_alloc: dst ffff880012bbb0c0, refcnt 1
> # First fragment is queued
> <idle>-0 [000] ..s2 112.770193: ip_local_deliver: skb ffff880012864600, dst ffff880012bbb0c0(1) : is fragment
> <idle>-0 [000] ..s2 112.770206: ip_local_deliver: skb ffff880012864600, dst ffff880012bbb0c0 : fragment queued
> # Second and final fragment arrives, reassemble
> ip-10970 [000] ..s1 112.770678: ip_local_deliver: skb ffff880010937e00, dst ffff880012bbb0c0(1) : is fragment
> # skb_morph bumps refcount to 2, skb_consume drops it back down to 1
> ip-10970 [000] ..s2 112.770682: ip_defrag: >>> clone skb ffff880010937e00 with dst ffff880012bbb0c0
> ip-10970 [000] ..s2 112.770691: __copy_skb_header: don't dst_clone ffff880012bbb0c0
> ip-10970 [000] ..s2 112.770691: ip_defrag: >>> morph skb ffff880010937e00 from ffff880012864600
> ip-10970 [000] ..s2 112.770692: skb_release_head_state: drop skb ffff880010937e00 dst ref
> ip-10970 [000] ..s2 112.770692: __copy_skb_header: cloning dst ffff880012bbb0c0 (skb ffff880012864600 -> skb ffff880010937e00)
> ip-10970 [000] ..s2 112.770692: ip_defrag: >>> consume skb ffff880012864600
> ip-10970 [000] ..s2 112.770693: skb_release_head_state: drop skb ffff880012864600 dst ref
> ip-10970 [000] ..s2 112.770693: dst_release: dst ffff880012bbb0c0 newrefcnt 1
> ip-10970 [000] ..s2 112.770698: ip_defrag: >>> coalesce loop
> ip-10970 [000] ..s2 112.770698: ip_defrag: kfree_skb_partial(ffff880010937500, false)
> ip-10970 [000] ..s2 112.770699: skb_release_head_state: drop skb ffff880010937500 dst ref
> # skb is reassembled and delivered, dst has refcount of 1 now
> ip-10970 [000] ..s1 112.770705: ip_local_deliver: skb ffff880010937e00, dst ffff880012bbb0c0(1) : queue defragmented
> # l2tp_eth uses dev_forward_skb, which calls skb_dst_drop
> ip-10970 [000] ..s1 112.770707: skb_release_head_state: drop skb ffff880010937e00 dst ref
> ip-10970 [000] ..s1 112.770708: dst_release: dst ffff880012bbb0c0 newrefcnt 0
> # Another skb arrives; dst refcount remains at 0
> <idle>-0 [000] ..s2 112.771481: ip_local_deliver: skb ffff880012864500, dst ffff880012bbb0c0(0) : is fragment
> <idle>-0 [000] ..s2 112.771494: ip_local_deliver: skb ffff880012864500, dst ffff880012bbb0c0 : fragment queued
>
> The strange thing is that once the dst refcount reaches zero, another
> skb hitting ip_input doesn't bump the refcount back up. This is
> partially why I'm not sure whether the error is caused by the defrag
> code, or by something prior to that in the rcv path.
Ah thanks for this, as this definitely makes more sense ;)
Could you try the following fix ?
diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index b6d30ac..87f4ecb 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -529,6 +529,7 @@ found:
qp->q.meat == qp->q.len)
return ip_frag_reasm(qp, prev, dev);
+ skb_dst_force(skb);
inet_frag_lru_move(&qp->q);
return -EINPROGRESS;
^ permalink raw reply related
* Re: [PATCH net-next 2/4] bridge: Allow an ability to designate an uplink port
From: Vlad Yasevich @ 2013-03-13 23:43 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, bridge
In-Reply-To: <20130313203333.GC9821@redhat.com>
On 03/13/2013 04:33 PM, Michael S. Tsirkin wrote:
> On Tue, Mar 12, 2013 at 09:45:24PM -0400, Vlad Yasevich wrote:
>> Allow a ports to be designated as uplink. Multiple ports
>> may be designated as uplinks and they will be kept in a
>> list.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>
> It looks like if you make two links uplink, bridging
> between them won't work at all?
only if they are non-promisc, but yes it does look like that...
hm... I need to think about this...
-vlad
>
>> ---
>> include/uapi/linux/if_link.h | 1 +
>> net/bridge/br_netlink.c | 2 ++
>> net/bridge/br_private.h | 1 +
>> net/bridge/br_sysfs_if.c | 2 ++
>> 4 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>> index c4edfe1..b9444e9 100644
>> --- a/include/uapi/linux/if_link.h
>> +++ b/include/uapi/linux/if_link.h
>> @@ -220,6 +220,7 @@ enum {
>> IFLA_BRPORT_GUARD, /* bpdu guard */
>> IFLA_BRPORT_PROTECT, /* root port protection */
>> IFLA_BRPORT_FAST_LEAVE, /* multicast fast leave */
>> + IFLA_BRPORT_UPLINK, /* uplink port */
>> __IFLA_BRPORT_MAX
>> };
>> #define IFLA_BRPORT_MAX (__IFLA_BRPORT_MAX - 1)
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index 27aa3ee..e08a50e 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -283,6 +283,7 @@ static const struct nla_policy ifla_brport_policy[IFLA_BRPORT_MAX + 1] = {
>> [IFLA_BRPORT_MODE] = { .type = NLA_U8 },
>> [IFLA_BRPORT_GUARD] = { .type = NLA_U8 },
>> [IFLA_BRPORT_PROTECT] = { .type = NLA_U8 },
>> + [IFLA_BRPORT_UPLINK] = { .type = NLA_U8 },
>> };
>>
>> /* Change the state of the port and notify spanning tree */
>> @@ -329,6 +330,7 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[])
>> br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE);
>> br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD);
>> br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE, BR_MULTICAST_FAST_LEAVE);
>> + br_set_port_flag(p, tb, IFLA_BRPORT_UPLINK, BR_UPLINK);
>>
>> if (tb[IFLA_BRPORT_COST]) {
>> err = br_stp_set_path_cost(p, nla_get_u32(tb[IFLA_BRPORT_COST]));
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 4a0fa29..44ae584 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -156,6 +156,7 @@ struct net_bridge_port
>> #define BR_BPDU_GUARD 0x00000002
>> #define BR_ROOT_BLOCK 0x00000004
>> #define BR_MULTICAST_FAST_LEAVE 0x00000008
>> +#define BR_UPLINK 0x00000010
>>
>> #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>> u32 multicast_startup_queries_sent;
>> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
>> index a1ef1b6..1f28cd4 100644
>> --- a/net/bridge/br_sysfs_if.c
>> +++ b/net/bridge/br_sysfs_if.c
>> @@ -158,6 +158,7 @@ static BRPORT_ATTR(flush, S_IWUSR, NULL, store_flush);
>> BRPORT_ATTR_FLAG(hairpin_mode, BR_HAIRPIN_MODE);
>> BRPORT_ATTR_FLAG(bpdu_guard, BR_BPDU_GUARD);
>> BRPORT_ATTR_FLAG(root_block, BR_ROOT_BLOCK);
>> +BRPORT_ATTR_FLAG(uplink, BR_UPLINK);
>>
>> #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>> static ssize_t show_multicast_router(struct net_bridge_port *p, char *buf)
>> @@ -195,6 +196,7 @@ static const struct brport_attribute *brport_attrs[] = {
>> &brport_attr_hairpin_mode,
>> &brport_attr_bpdu_guard,
>> &brport_attr_root_block,
>> + &brport_attr_uplink,
>> #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>> &brport_attr_multicast_router,
>> &brport_attr_multicast_fast_leave,
>> --
>> 1.7.7.6
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC PATCH] udp: don't rereference dst_entry dev pointer on rcv
From: Tom Parkin @ 2013-03-13 23:27 UTC (permalink / raw)
To: David Miller; +Cc: eric.dumazet, netdev
In-Reply-To: <20130307.181527.390191009324148471.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 5477 bytes --]
On Thu, Mar 07, 2013 at 06:15:27PM -0500, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Thu, 07 Mar 2013 14:47:24 -0800
>
> > On Thu, 2013-03-07 at 22:36 +0000, Tom Parkin wrote:
> >> When a fragmented IP packet is queued during device teardown, it is possible
> >> for the reassembled packet to hit the UDP rcv path with a NULL dst_entry dev
> >> pointer. Drop such packets to prevent an oops.
> >> ---
> >> net/ipv4/udp.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> >> index 0a073a2..c38a4b1 100644
> >> --- a/net/ipv4/udp.c
> >> +++ b/net/ipv4/udp.c
> >> @@ -1700,6 +1700,9 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
> >> return __udp4_lib_mcast_deliver(net, skb, uh,
> >> saddr, daddr, udptable);
> >>
> >> + if (skb_dst(skb)->dev == NULL)
> >> + goto drop;
> >> +
> >> sk = __udp4_lib_lookup_skb(skb, uh->source, uh->dest, udptable);
> >>
> >> if (sk != NULL) {
> >
> >
> > Hmm... couldnt it be tested in reassembly layer instead ?
> >
> > Why is it specific to UDP ?
>
> Furthermore, when devices are unregistered we set the route's device
> pointer to point to the loopback device, not NULL, exactly to avoid
> this kind of problem.
>
> I don't see anything in our generic DST handler nor the ipv4 specific
> route handling, that would set dst->dev to NULL.
>
> You really have to show us how this can actually happen.
I've been working to this end, and while I don't have a root cause as
yet, I do have some more information.
I think what's happening is that the dst_entry refcounting is getting
screwed up either by the ip defrag code, or by something before that
in the rcv path. What I see from ftrace debugging is that an skb
fragment ends up queued on the reassembly queue while pointing to a
dst_entry with a refcount of 0. If the dst_entry should be deleted before
the final fragment in the frame arrives, then we end up accessing
free'd memory.
So far as I can make out, the l2tp code isn't doing anything untoward
which is causing this bug. My stress test simply makes it easier to
reproduce because I'm setting up and tearing down routes and devices
a lot while passing data. I'm lucky in that my dev branch seems to
reproduce this more easily than net-next master does, although the
same oops occurs on master if you're prepared to wait around for long
enough.
This ftrace debug log snippet shows the sort of behaviour I'm seeing.
The numbers in brackets after some dst pointer values represent the
refcount for that dst:
# The dst_entry is created with a refcount of 1
<idle>-0 [000] ..s2 112.770192: dst_alloc: dst ffff880012bbb0c0, refcnt 1
# First fragment is queued
<idle>-0 [000] ..s2 112.770193: ip_local_deliver: skb ffff880012864600, dst ffff880012bbb0c0(1) : is fragment
<idle>-0 [000] ..s2 112.770206: ip_local_deliver: skb ffff880012864600, dst ffff880012bbb0c0 : fragment queued
# Second and final fragment arrives, reassemble
ip-10970 [000] ..s1 112.770678: ip_local_deliver: skb ffff880010937e00, dst ffff880012bbb0c0(1) : is fragment
# skb_morph bumps refcount to 2, skb_consume drops it back down to 1
ip-10970 [000] ..s2 112.770682: ip_defrag: >>> clone skb ffff880010937e00 with dst ffff880012bbb0c0
ip-10970 [000] ..s2 112.770691: __copy_skb_header: don't dst_clone ffff880012bbb0c0
ip-10970 [000] ..s2 112.770691: ip_defrag: >>> morph skb ffff880010937e00 from ffff880012864600
ip-10970 [000] ..s2 112.770692: skb_release_head_state: drop skb ffff880010937e00 dst ref
ip-10970 [000] ..s2 112.770692: __copy_skb_header: cloning dst ffff880012bbb0c0 (skb ffff880012864600 -> skb ffff880010937e00)
ip-10970 [000] ..s2 112.770692: ip_defrag: >>> consume skb ffff880012864600
ip-10970 [000] ..s2 112.770693: skb_release_head_state: drop skb ffff880012864600 dst ref
ip-10970 [000] ..s2 112.770693: dst_release: dst ffff880012bbb0c0 newrefcnt 1
ip-10970 [000] ..s2 112.770698: ip_defrag: >>> coalesce loop
ip-10970 [000] ..s2 112.770698: ip_defrag: kfree_skb_partial(ffff880010937500, false)
ip-10970 [000] ..s2 112.770699: skb_release_head_state: drop skb ffff880010937500 dst ref
# skb is reassembled and delivered, dst has refcount of 1 now
ip-10970 [000] ..s1 112.770705: ip_local_deliver: skb ffff880010937e00, dst ffff880012bbb0c0(1) : queue defragmented
# l2tp_eth uses dev_forward_skb, which calls skb_dst_drop
ip-10970 [000] ..s1 112.770707: skb_release_head_state: drop skb ffff880010937e00 dst ref
ip-10970 [000] ..s1 112.770708: dst_release: dst ffff880012bbb0c0 newrefcnt 0
# Another skb arrives; dst refcount remains at 0
<idle>-0 [000] ..s2 112.771481: ip_local_deliver: skb ffff880012864500, dst ffff880012bbb0c0(0) : is fragment
<idle>-0 [000] ..s2 112.771494: ip_local_deliver: skb ffff880012864500, dst ffff880012bbb0c0 : fragment queued
The strange thing is that once the dst refcount reaches zero, another
skb hitting ip_input doesn't bump the refcount back up. This is
partially why I'm not sure whether the error is caused by the defrag
code, or by something prior to that in the rcv path.
--
Tom Parkin
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 1/6] GRE: Use RTNL for atomic protocol registration.
From: Michał Mirosław @ 2013-03-13 23:22 UTC (permalink / raw)
To: Pravin B Shelar; +Cc: davem, stephen, netdev, jesse
In-Reply-To: <1363214907-3257-1-git-send-email-pshelar@nicira.com>
2013/3/13 Pravin B Shelar <pshelar@nicira.com>:
> Currently gre demultiplexer uses its own spin-lock. We should
> rather RTNL lock.
[...]
> + rtnl_lock();
> err = gre_add_protocol(&gre_pptp_protocol, GREPROTO_PPTP);
> + rtnl_unlock();
Why not keep lock/unlock inside gre_add/del_protocol()?
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: [PATCH net-next 5/6] VXLAN: Use IP Tunnels tunnel ENC encap API
From: Stephen Hemminger @ 2013-03-13 22:59 UTC (permalink / raw)
To: Pravin B Shelar; +Cc: davem, netdev, jesse
In-Reply-To: <1363214926-3385-1-git-send-email-pshelar@nicira.com>
On Wed, 13 Mar 2013 15:48:46 -0700
Pravin B Shelar <pshelar@nicira.com> wrote:
> Use common ecn_encap functions from ip_tunnel module.
>
> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
Thanks for doing all this.
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
^ permalink raw reply
* Re: [PATCH net-next 4/6] VXLAN: Fix vxlan stats handling.
From: Stephen Hemminger @ 2013-03-13 22:58 UTC (permalink / raw)
To: Pravin B Shelar; +Cc: davem, xiyou.wangcong, netdev, jesse
In-Reply-To: <1363214923-3353-1-git-send-email-pshelar@nicira.com>
On Wed, 13 Mar 2013 15:48:43 -0700
Pravin B Shelar <pshelar@nicira.com> wrote:
> Fixes bug in VXLAN code where is iptunnel_xmit() called with NULL
> dev->tstats.
> This bug was introduced in commit 6aed0c8bf7d2f389b (tunnel: use
> iptunnel_xmit() again).
>
> Following patch fixes bug by setting dev->tstats. It uses ip_tunnel
> module code to share stats function.
>
> CC: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
^ permalink raw reply
* [PATCH 9/9] batman-adv: network coding - receive coded packets and decode them
From: Antonio Quartulli @ 2013-03-13 22:56 UTC (permalink / raw)
To: davem
Cc: netdev, b.a.t.m.a.n, Martin Hundebøll, Marek Lindner,
Antonio Quartulli
In-Reply-To: <1363215399-18875-1-git-send-email-ordex@autistici.org>
From: Martin Hundebøll <martin@hundeboll.net>
When receiving a network coded packet, the decoding buffer is searched
for a packet to use for decoding. The source, destination, and crc32 from
the coded packet is used to identify the wanted packet. The decoded
packet is passed to the usual unicast receiver function, as had it never
been network coded.
Signed-off-by: Martin Hundebøll <martin@hundeboll.net>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
net/batman-adv/network-coding.c | 232 ++++++++++++++++++++++++++++++++++++++++
net/batman-adv/soft-interface.c | 4 +
net/batman-adv/types.h | 10 ++
3 files changed, 246 insertions(+)
diff --git a/net/batman-adv/network-coding.c b/net/batman-adv/network-coding.c
index 3d2ed2f..5728079 100644
--- a/net/batman-adv/network-coding.c
+++ b/net/batman-adv/network-coding.c
@@ -25,11 +25,14 @@
#include "send.h"
#include "originator.h"
#include "hard-interface.h"
+#include "routing.h"
static struct lock_class_key batadv_nc_coding_hash_lock_class_key;
static struct lock_class_key batadv_nc_decoding_hash_lock_class_key;
static void batadv_nc_worker(struct work_struct *work);
+static int batadv_nc_recv_coded_packet(struct sk_buff *skb,
+ struct batadv_hard_iface *recv_if);
/**
* batadv_nc_start_timer - initialise the nc periodic worker
@@ -67,6 +70,11 @@ int batadv_nc_init(struct batadv_priv *bat_priv)
batadv_hash_set_lock_class(bat_priv->nc.coding_hash,
&batadv_nc_decoding_hash_lock_class_key);
+ /* Register our packet type */
+ if (batadv_recv_handler_register(BATADV_CODED,
+ batadv_nc_recv_coded_packet) < 0)
+ goto err;
+
INIT_DELAYED_WORK(&bat_priv->nc.work, batadv_nc_worker);
batadv_nc_start_timer(bat_priv);
@@ -1486,11 +1494,235 @@ void batadv_nc_skb_store_sniffed_unicast(struct batadv_priv *bat_priv,
}
/**
+ * batadv_nc_skb_decode_packet - decode given skb using the decode data stored
+ * in nc_packet
+ * @skb: unicast skb to decode
+ * @nc_packet: decode data needed to decode the skb
+ *
+ * Returns pointer to decoded unicast packet if the packet was decoded or NULL
+ * in case of an error.
+ */
+static struct batadv_unicast_packet *
+batadv_nc_skb_decode_packet(struct sk_buff *skb,
+ struct batadv_nc_packet *nc_packet)
+{
+ const int h_size = sizeof(struct batadv_unicast_packet);
+ const int h_diff = sizeof(struct batadv_coded_packet) - h_size;
+ struct batadv_unicast_packet *unicast_packet;
+ struct batadv_coded_packet coded_packet_tmp;
+ struct ethhdr *ethhdr, ethhdr_tmp;
+ uint8_t *orig_dest, ttl, ttvn;
+ unsigned int coding_len;
+
+ /* Save headers temporarily */
+ memcpy(&coded_packet_tmp, skb->data, sizeof(coded_packet_tmp));
+ memcpy(ðhdr_tmp, skb_mac_header(skb), sizeof(ethhdr_tmp));
+
+ if (skb_cow(skb, 0) < 0)
+ return NULL;
+
+ if (unlikely(!skb_pull_rcsum(skb, h_diff)))
+ return NULL;
+
+ /* Data points to batman header, so set mac header 14 bytes before
+ * and network to data
+ */
+ skb_set_mac_header(skb, -ETH_HLEN);
+ skb_reset_network_header(skb);
+
+ /* Reconstruct original mac header */
+ ethhdr = (struct ethhdr *)skb_mac_header(skb);
+ memcpy(ethhdr, ðhdr_tmp, sizeof(*ethhdr));
+
+ /* Select the correct unicast header information based on the location
+ * of our mac address in the coded_packet header
+ */
+ if (batadv_is_my_mac(coded_packet_tmp.second_dest)) {
+ /* If we are the second destination the packet was overheard,
+ * so the Ethernet address must be copied to h_dest and
+ * pkt_type changed from PACKET_OTHERHOST to PACKET_HOST
+ */
+ memcpy(ethhdr->h_dest, coded_packet_tmp.second_dest, ETH_ALEN);
+ skb->pkt_type = PACKET_HOST;
+
+ orig_dest = coded_packet_tmp.second_orig_dest;
+ ttl = coded_packet_tmp.second_ttl;
+ ttvn = coded_packet_tmp.second_ttvn;
+ } else {
+ orig_dest = coded_packet_tmp.first_orig_dest;
+ ttl = coded_packet_tmp.header.ttl;
+ ttvn = coded_packet_tmp.first_ttvn;
+ }
+
+ coding_len = ntohs(coded_packet_tmp.coded_len);
+
+ if (coding_len > skb->len)
+ return NULL;
+
+ /* Here the magic is reversed:
+ * extract the missing packet from the received coded packet
+ */
+ batadv_nc_memxor(skb->data + h_size,
+ nc_packet->skb->data + h_size,
+ coding_len);
+
+ /* Resize decoded skb if decoded with larger packet */
+ if (nc_packet->skb->len > coding_len + h_size)
+ pskb_trim_rcsum(skb, coding_len + h_size);
+
+ /* Create decoded unicast packet */
+ unicast_packet = (struct batadv_unicast_packet *)skb->data;
+ unicast_packet->header.packet_type = BATADV_UNICAST;
+ unicast_packet->header.version = BATADV_COMPAT_VERSION;
+ unicast_packet->header.ttl = ttl;
+ memcpy(unicast_packet->dest, orig_dest, ETH_ALEN);
+ unicast_packet->ttvn = ttvn;
+
+ batadv_nc_packet_free(nc_packet);
+ return unicast_packet;
+}
+
+/**
+ * batadv_nc_find_decoding_packet - search through buffered decoding data to
+ * find the data needed to decode the coded packet
+ * @bat_priv: the bat priv with all the soft interface information
+ * @ethhdr: pointer to the ethernet header inside the coded packet
+ * @coded: coded packet we try to find decode data for
+ *
+ * Returns pointer to nc packet if the needed data was found or NULL otherwise.
+ */
+static struct batadv_nc_packet *
+batadv_nc_find_decoding_packet(struct batadv_priv *bat_priv,
+ struct ethhdr *ethhdr,
+ struct batadv_coded_packet *coded)
+{
+ struct batadv_hashtable *hash = bat_priv->nc.decoding_hash;
+ struct batadv_nc_packet *tmp_nc_packet, *nc_packet = NULL;
+ struct batadv_nc_path *nc_path, nc_path_key;
+ uint8_t *dest, *source;
+ __be32 packet_id;
+ int index;
+
+ if (!hash)
+ return NULL;
+
+ /* Select the correct packet id based on the location of our mac-addr */
+ dest = ethhdr->h_source;
+ if (!batadv_is_my_mac(coded->second_dest)) {
+ source = coded->second_source;
+ packet_id = coded->second_crc;
+ } else {
+ source = coded->first_source;
+ packet_id = coded->first_crc;
+ }
+
+ batadv_nc_hash_key_gen(&nc_path_key, source, dest);
+ index = batadv_nc_hash_choose(&nc_path_key, hash->size);
+
+ /* Search for matching coding path */
+ rcu_read_lock();
+ hlist_for_each_entry_rcu(nc_path, &hash->table[index], hash_entry) {
+ /* Find matching nc_packet */
+ spin_lock_bh(&nc_path->packet_list_lock);
+ list_for_each_entry(tmp_nc_packet,
+ &nc_path->packet_list, list) {
+ if (packet_id == tmp_nc_packet->packet_id) {
+ list_del(&tmp_nc_packet->list);
+
+ nc_packet = tmp_nc_packet;
+ break;
+ }
+ }
+ spin_unlock_bh(&nc_path->packet_list_lock);
+
+ if (nc_packet)
+ break;
+ }
+ rcu_read_unlock();
+
+ if (!nc_packet)
+ batadv_dbg(BATADV_DBG_NC, bat_priv,
+ "No decoding packet found for %u\n", packet_id);
+
+ return nc_packet;
+}
+
+/**
+ * batadv_nc_recv_coded_packet - try to decode coded packet and enqueue the
+ * resulting unicast packet
+ * @skb: incoming coded packet
+ * @recv_if: pointer to interface this packet was received on
+ */
+static int batadv_nc_recv_coded_packet(struct sk_buff *skb,
+ struct batadv_hard_iface *recv_if)
+{
+ struct batadv_priv *bat_priv = netdev_priv(recv_if->soft_iface);
+ struct batadv_unicast_packet *unicast_packet;
+ struct batadv_coded_packet *coded_packet;
+ struct batadv_nc_packet *nc_packet;
+ struct ethhdr *ethhdr;
+ int hdr_size = sizeof(*coded_packet);
+
+ /* Check if network coding is enabled */
+ if (!atomic_read(&bat_priv->network_coding))
+ return NET_RX_DROP;
+
+ /* Make sure we can access (and remove) header */
+ if (unlikely(!pskb_may_pull(skb, hdr_size)))
+ return NET_RX_DROP;
+
+ coded_packet = (struct batadv_coded_packet *)skb->data;
+ ethhdr = (struct ethhdr *)skb_mac_header(skb);
+
+ /* Verify frame is destined for us */
+ if (!batadv_is_my_mac(ethhdr->h_dest) &&
+ !batadv_is_my_mac(coded_packet->second_dest))
+ return NET_RX_DROP;
+
+ /* Update stat counter */
+ if (batadv_is_my_mac(coded_packet->second_dest))
+ batadv_inc_counter(bat_priv, BATADV_CNT_NC_SNIFFED);
+
+ nc_packet = batadv_nc_find_decoding_packet(bat_priv, ethhdr,
+ coded_packet);
+ if (!nc_packet) {
+ batadv_inc_counter(bat_priv, BATADV_CNT_NC_DECODE_FAILED);
+ return NET_RX_DROP;
+ }
+
+ /* Make skb's linear, because decoding accesses the entire buffer */
+ if (skb_linearize(skb) < 0)
+ goto free_nc_packet;
+
+ if (skb_linearize(nc_packet->skb) < 0)
+ goto free_nc_packet;
+
+ /* Decode the packet */
+ unicast_packet = batadv_nc_skb_decode_packet(skb, nc_packet);
+ if (!unicast_packet) {
+ batadv_inc_counter(bat_priv, BATADV_CNT_NC_DECODE_FAILED);
+ goto free_nc_packet;
+ }
+
+ /* Mark packet as decoded to do correct recoding when forwarding */
+ BATADV_SKB_CB(skb)->decoded = true;
+ batadv_inc_counter(bat_priv, BATADV_CNT_NC_DECODE);
+ batadv_add_counter(bat_priv, BATADV_CNT_NC_DECODE_BYTES,
+ skb->len + ETH_HLEN);
+ return batadv_recv_unicast_packet(skb, recv_if);
+
+free_nc_packet:
+ batadv_nc_packet_free(nc_packet);
+ return NET_RX_DROP;
+}
+
+/**
* batadv_nc_free - clean up network coding memory
* @bat_priv: the bat priv with all the soft interface information
*/
void batadv_nc_free(struct batadv_priv *bat_priv)
{
+ batadv_recv_handler_unregister(BATADV_CODED);
cancel_delayed_work_sync(&bat_priv->nc.work);
batadv_nc_purge_paths(bat_priv, bat_priv->nc.coding_hash, NULL);
diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c
index 75204ec..f93ae42 100644
--- a/net/batman-adv/soft-interface.c
+++ b/net/batman-adv/soft-interface.c
@@ -671,6 +671,10 @@ static const struct {
{ "nc_recode" },
{ "nc_recode_bytes" },
{ "nc_buffer" },
+ { "nc_decode" },
+ { "nc_decode_bytes" },
+ { "nc_decode_failed" },
+ { "nc_sniffed" },
#endif
};
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
index 5f3640d..aba8364 100644
--- a/net/batman-adv/types.h
+++ b/net/batman-adv/types.h
@@ -280,6 +280,12 @@ struct batadv_bcast_duplist_entry {
* @BATADV_CNT_NC_RECODE: transmitted nc-recombined traffic packet counter
* @BATADV_CNT_NC_RECODE_BYTES: transmitted nc-recombined traffic bytes counter
* @BATADV_CNT_NC_BUFFER: counter for packets buffered for later nc decoding
+ * @BATADV_CNT_NC_DECODE: received and nc-decoded traffic packet counter
+ * @BATADV_CNT_NC_DECODE_BYTES: received and nc-decoded traffic bytes counter
+ * @BATADV_CNT_NC_DECODE_FAILED: received and decode-failed traffic packet
+ * counter
+ * @BATADV_CNT_NC_SNIFFED: counter for nc-decoded packets received in promisc
+ * mode.
* @BATADV_CNT_NUM: number of traffic counters
*/
enum batadv_counters {
@@ -313,6 +319,10 @@ enum batadv_counters {
BATADV_CNT_NC_RECODE,
BATADV_CNT_NC_RECODE_BYTES,
BATADV_CNT_NC_BUFFER,
+ BATADV_CNT_NC_DECODE,
+ BATADV_CNT_NC_DECODE_BYTES,
+ BATADV_CNT_NC_DECODE_FAILED,
+ BATADV_CNT_NC_SNIFFED,
#endif
BATADV_CNT_NUM,
};
--
1.8.1.5
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox