Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH net] ipv6: don't stop backtracking in fib6_lookup_1 if subtree does not match
From: Hannes Frederic Sowa @ 2013-08-07 20:06 UTC (permalink / raw)
  To: Teco Boot; +Cc: netdev, yoshfuji, equinox, boutier
In-Reply-To: <EF96A1CB-549E-474A-AF58-10F2D2F2BF4A@inf-net.nl>

On Wed, Aug 07, 2013 at 09:58:31PM +0200, Teco Boot wrote:
> Seems to work!!
> 
> babe has one entry less than cafe, this is correct.
> Tomorrow I'll test with real traffic.

Thanks for testing! I already used real traffic but it is better to
double check.

Greetings,

  Hannes

^ permalink raw reply

* Re: [PATCH v2 net-next] net: Add low-latency/polling support for UDP multicast
From: Eric Dumazet @ 2013-08-07 20:22 UTC (permalink / raw)
  To: Shawn Bohrer; +Cc: davem, eliezer.tamir, netdev, Amir Vadai, tomk
In-Reply-To: <1375818663-12318-1-git-send-email-sbohrer@rgmadvisors.com>

On Tue, 2013-08-06 at 14:51 -0500, Shawn Bohrer wrote:
> Set the napi id for each socket in the multicast path to enable
> low-latency/polling support.
> 
> Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
> ---
> v2 include ipv6 support

This might help your workload, but I doubt it is generic enough.

One UDP socket is supposed to receive traffic from many endpoints,
so we have no guarantee all the received traffic will end on a single RX
queue on the NIC.

That's the same logic than RFS here.

sk_mark_napi_id() in UDP are wrong IMHO.

It should be guarded by the following test in  
__udp_queue_rcv_skb()

if (inet_sk(sk)->inet_daddr) {
    sock_rps_save_rxhash(sk, skb);
    sk_mark_napi_id(sk, skb);
}

(To occur only for connected UDP sockets, where we are 100% sure all
packets will use this same rxhash/rx queue)

^ permalink raw reply

* Re: low latency/busy poll feedback and bugs
From: Eric Dumazet @ 2013-08-07 20:23 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Eliezer Tamir, Shawn Bohrer, Amir Vadai, netdev
In-Reply-To: <1375905901.27403.22.camel@deadeye.wl.decadent.org.uk>

On Wed, 2013-08-07 at 22:05 +0200, Ben Hutchings wrote:

> It seems like sk_mark_napi_id() should only be called on connected
> sockets for now.

I concur.

^ permalink raw reply

* Re: [PATCH 3/5] netfilter: add SYNPROXY core/target
From: Jesper Dangaard Brouer @ 2013-08-07 20:26 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: pablo, netfilter-devel, netdev, mph, as
In-Reply-To: <1375897371-18430-4-git-send-email-kaber@trash.net>

On Wed,  7 Aug 2013 19:42:49 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Add a SYNPROXY for netfilter. The code is split into two parts, the
> synproxy core with common functions and an address family specific
> target.
> 
> The SYNPROXY receives the connection request from the client,
> responds with a SYN/ACK containing a SYN cookie and announcing a zero
> window and checks whether the final ACK from the client contains a
> valid cookie.
> 
> It then establishes a connection to the original destination and, if
> successful, sends a window update to the client with the window size
> announced by the server.
> 
> Support for timestamps, SACK, window scaling and MSS options can be
> statically configured as target parameters if the features of the
> server are known. If timestamps are used, the timestamp value sent
> back to the client in the SYN/ACK will be different from the real
> timestamp of the server. In order to now break PAWS, the timestamps
> are translated in the direction server->client.
> 
> Signed-off-by: Patrick McHardy <kaber@trash.net>
> ---

[...]

> +static void
> +synproxy_send_server_syn(const struct synproxy_net *snet,
> +			 const struct sk_buff *skb, const struct
> tcphdr *th,
> +			 const struct synproxy_options *opts)
> +{
> +	struct sk_buff *nskb;
> +	struct iphdr *iph, *niph;
> +	struct tcphdr *nth;
> +	unsigned int tcp_hdr_size;
> +
> +	iph = ip_hdr(skb);
> +
> +	tcp_hdr_size = sizeof(*nth) + synproxy_options_size(opts);
> +	nskb = alloc_skb(sizeof(*niph) + tcp_hdr_size + LL_MAX_HEADER,
> +			 GFP_ATOMIC);
> +	if (nskb == NULL)
> +		return;
> +	skb_reserve(nskb, LL_MAX_HEADER);
> +
> +	niph = synproxy_build_ip(nskb, iph->saddr, iph->daddr);
> +
> +	skb_reset_transport_header(nskb);
> +	nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
> +	nth->source	= th->source;
> +	nth->dest	= th->dest;
> +	nth->seq	= htonl(ntohl(th->seq) - 1);
> +	nth->ack_seq	= htonl(ntohl(th->ack_seq) - 1);;

Strange double ";;"

Besides shouldn't nth->ack_seq be zero, in a SYN packet? This is the
SYN "replayed" towards the server right?

I also pointed to this in an earlier patch Martin showed me, but he
reported that changing this resulted in bad behavior.  So, I would
request Martin to re-test this part.


> +	tcp_flag_word(nth) = TCP_FLAG_SYN;
> +	if (opts->options & XT_SYNPROXY_OPT_ECN)
> +		tcp_flag_word(nth) |= TCP_FLAG_ECE | TCP_FLAG_CWR;
> +	nth->doff	= tcp_hdr_size / 4;
> +	nth->window	= th->window;
> +	nth->check	= 0;
> +	nth->urg_ptr	= 0;
> +
> +	synproxy_build_options(nth, opts);
> +
> +	synproxy_send_tcp(skb, nskb, &snet->tmpl->ct_general, IP_CT_NEW,
> +			  niph, nth, tcp_hdr_size);
> +}

[...]

> +static unsigned int
> +synproxy_tg4(struct sk_buff *skb, const struct xt_action_param *par)
> +{
> +	const struct xt_synproxy_info *info = par->targinfo;
> +	struct synproxy_net *snet = synproxy_pernet(dev_net(par->in));
> +	struct synproxy_options opts = {};
> +	struct tcphdr *th, _th;
> +
> +	if (nf_ip_checksum(skb, par->hooknum, par->thoff, IPPROTO_TCP))
> +		return NF_DROP;
> +
> +	th = skb_header_pointer(skb, par->thoff, sizeof(_th), &_th);
> +	if (th == NULL)
> +		return NF_DROP;
> +
> +	synproxy_parse_options(skb, par->thoff, th, &opts);
> +
> +	if (th->syn) {
> +		/* Initial SYN from client */
> +		this_cpu_inc(snet->stats->syn_received);
> +
> +		if (th->ece && th->cwr)
> +			opts.options |= XT_SYNPROXY_OPT_ECN;
> +
> +		opts.options &= info->options;
> +		if (opts.options & XT_SYNPROXY_OPT_TIMESTAMP)
> +			synproxy_init_timestamp_cookie(info, &opts);
> +		else
> +			opts.options &= ~(XT_SYNPROXY_OPT_WSCALE |
> +					  XT_SYNPROXY_OPT_SACK_PERM |
> +					  XT_SYNPROXY_OPT_ECN);
> +
> +		synproxy_send_client_synack(skb, th, &opts);
> +	} else if (th->ack && !(th->fin || th->rst)) {

This could also match SYN+ACK... we are only interested in the ACK
(from the 3WHS) here, right?

> +		/* ACK from client */
> +		int mss = __cookie_v4_check(ip_hdr(skb), th,
> +					    ntohl(th->ack_seq) - 1);
> +		if (mss == 0) {
> +			this_cpu_inc(snet->stats->cookie_invalid);
> +			return NF_DROP;
> +		}
> +
> +		this_cpu_inc(snet->stats->cookie_valid);
> +		opts.mss = mss;
> +
> +		if (opts.options & XT_SYNPROXY_OPT_TIMESTAMP)
> +			synproxy_check_timestamp_cookie(&opts);
> +
> +		synproxy_send_server_syn(snet, skb, th, &opts);
> +	}
> +
> +	return NF_DROP;
> +}

[...]

> diff --git a/net/netfilter/nf_synproxy_core.c
> b/net/netfilter/nf_synproxy_core.c new file mode 100644
> index 0000000..d887a84
> --- /dev/null
> +++ b/net/netfilter/nf_synproxy_core.c
> @@ -0,0 +1,428 @@
> +/*
> + * Copyright (c) 2013 Patrick McHardy <kaber@trash.net>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
[...]
> +
> +void
> +synproxy_parse_options(const struct sk_buff *skb, unsigned int doff,
> +		       const struct tcphdr *th, struct synproxy_options *opts)
> +{
> +	int length = (th->doff * 4) - sizeof(*th);
> +	u8 buf[40], *ptr;
> +
> +	ptr = skb_header_pointer(skb, doff + sizeof(*th), length, buf);
> +	BUG_ON(ptr == NULL);
> +
> +	opts->options = 0;
> +	while (length > 0) {
> +		int opcode = *ptr++;
> +		int opsize;
> +
> +		switch (opcode) {
> +		case TCPOPT_EOL:
> +			return;
> +		case TCPOPT_NOP:
> +			length--;
> +			continue;
> +		default:
> +			opsize = *ptr++;
> +			if (opsize < 2)
> +				return;
> +			if (opsize > length)
> +				return;
> +
> +			switch (opcode) {
> +			case TCPOPT_MSS:
> +				if (opsize == TCPOLEN_MSS) {
> +					 opts->mss = get_unaligned_be16(ptr);

Strange indention, extra space before "opts->mss".

> +					 opts->options |= XT_SYNPROXY_OPT_MSS;

Strange indention, extra space before "opts->options".


> +				}
> +				break;
> +			case TCPOPT_WINDOW:
> +				if (opsize == TCPOLEN_WINDOW) {
> +					opts->wscale = *ptr;
> +					if (opts->wscale > 14)
> +						opts->wscale = 14;
> +					opts->options |= XT_SYNPROXY_OPT_WSCALE;
> +				}
> +				break;
> +			case TCPOPT_TIMESTAMP:
> +				if (opsize == TCPOLEN_TIMESTAMP) {
> +					opts->tsval = get_unaligned_be32(ptr);
> +					opts->tsecr = get_unaligned_be32(ptr + 4);
> +					opts->options |= XT_SYNPROXY_OPT_TIMESTAMP;
> +				}
> +				break;
> +			case TCPOPT_SACK_PERM:
> +				if (opsize == TCPOLEN_SACK_PERM)
> +					opts->options |= XT_SYNPROXY_OPT_SACK_PERM;
> +				break;
> +			}
> +
> +			ptr += opsize - 2;
> +			length -= opsize;
> +		}
> +	}
> +}
> +EXPORT_SYMBOL_GPL(synproxy_parse_options);

[...]

> +static int synproxy_cpu_seq_show(struct seq_file *seq, void *v)
> +{
> +	struct synproxy_stats *stats = v;
> +
> +	if (v == SEQ_START_TOKEN) {
> +		seq_printf(seq, "syn_received\tcookie_invalid\tcookie_valid\n");
> +		return 0;
> +	}
> +
> +	seq_printf(seq, "%08u\t%08x\t%08x\n",

Shouldn't all numbers be printed in hex? (%08u -> %08x)

Besides when using net->proc_net_stat, then the first entry is usually
"entries" which is not percpu, this will likely confusing the tool:
  lnstat -f synproxy -c 42


> +		   stats->syn_received,
> +		   stats->cookie_invalid,
> +		   stats->cookie_valid);
> +
> +	return 0;
> +}

[...]


-- 
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 4/5] net: syncookies: export cookie_v6_init_sequence/cookie_v6_check
From: Jesper Dangaard Brouer @ 2013-08-07 20:27 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: pablo, netfilter-devel, netdev, mph, jesper.brouer, as
In-Reply-To: <1375897371-18430-5-git-send-email-kaber@trash.net>

On Wed,  7 Aug 2013 19:42:50 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Extract the local TCP stack independant parts of
> tcp_v6_init_sequence() and cookie_v6_check() and export them for use
> by the upcoming IPv6 SYNPROXY target.
> 
> Signed-off-by: Patrick McHardy <kaber@trash.net>

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

-- 
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 5/5] netfilter: add IPv6 SYNPROXY target
From: Jesper Dangaard Brouer @ 2013-08-07 20:34 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: pablo, netfilter-devel, netdev, mph, as
In-Reply-To: <1375897371-18430-6-git-send-email-kaber@trash.net>


On Wed,  7 Aug 2013 19:42:51 +0200 Patrick McHardy <kaber@trash.net> wrote:

> Add an IPv6 version of the SYNPROXY target. The main differences to
> the IPv4 version is routing and IP header construction.
> 
> Signed-off-by: Patrick McHardy <kaber@trash.net>

[...]

> diff --git a/net/ipv6/netfilter/ip6t_SYNPROXY.c
> b/net/ipv6/netfilter/ip6t_SYNPROXY.c new file mode 100644
> index 0000000..ee773da
> --- /dev/null
> +++ b/net/ipv6/netfilter/ip6t_SYNPROXY.c
[...]

> +static void
> +synproxy_send_server_syn(const struct synproxy_net *snet,
> +			 const struct sk_buff *skb, const struct tcphdr *th,
> +			 const struct synproxy_options *opts)
> +{
> +	struct sk_buff *nskb;
> +	struct ipv6hdr *iph, *niph;
> +	struct tcphdr *nth;
> +	unsigned int tcp_hdr_size;
> +
> +	iph = ipv6_hdr(skb);
> +
> +	tcp_hdr_size = sizeof(*nth) + synproxy_options_size(opts);
> +	nskb = alloc_skb(sizeof(*niph) + tcp_hdr_size + LL_MAX_HEADER,
> +			 GFP_ATOMIC);
> +	if (nskb == NULL)
> +		return;
> +	skb_reserve(nskb, LL_MAX_HEADER);
> +
> +	niph = synproxy_build_ip(nskb, &iph->saddr, &iph->daddr);
> +
> +	skb_reset_transport_header(nskb);
> +	nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
> +	nth->source	= th->source;
> +	nth->dest	= th->dest;
> +	nth->seq	= htonl(ntohl(th->seq) - 1);
> +	nth->ack_seq	= htonl(ntohl(th->ack_seq) - 1);;

Strange double ";;".

And as IPv4, shouldn't this be zero? I might be wrong...


> +	tcp_flag_word(nth) = TCP_FLAG_SYN;
> +	if (opts->options & XT_SYNPROXY_OPT_ECN)
> +		tcp_flag_word(nth) |= TCP_FLAG_ECE | TCP_FLAG_CWR;
> +	nth->doff	= tcp_hdr_size / 4;
> +	nth->window	= th->window;
> +	nth->check	= 0;
> +	nth->urg_ptr	= 0;
> +
> +	synproxy_build_options(nth, opts);
> +
> +	synproxy_send_tcp(skb, nskb, &snet->tmpl->ct_general, IP_CT_NEW,
> +			  niph, nth, tcp_hdr_size);
> +}
> +
> +static void
> +synproxy_send_server_ack(const struct synproxy_net *snet,
> +			 const struct ip_ct_tcp *state,
> +			 const struct sk_buff *skb, const struct tcphdr *th,
> +			 const struct synproxy_options *opts)
> +{
> +	struct sk_buff *nskb;
> +	struct ipv6hdr *iph, *niph;
> +	struct tcphdr *nth;
> +	unsigned int tcp_hdr_size;
> +
> +	iph = ipv6_hdr(skb);
> +
> +	tcp_hdr_size = sizeof(*nth) + synproxy_options_size(opts);
> +	nskb = alloc_skb(sizeof(*niph) + tcp_hdr_size + LL_MAX_HEADER,
> +			 GFP_ATOMIC);
> +	if (nskb == NULL)
> +		return;
> +	skb_reserve(nskb, LL_MAX_HEADER);
> +
> +	niph = synproxy_build_ip(nskb, &iph->daddr, &iph->saddr);
> +
> +	skb_reset_transport_header(nskb);
> +	nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
> +	nth->source	= th->dest;
> +	nth->dest	= th->source;
> +	nth->seq	= htonl(ntohl(th->ack_seq));
> +	nth->ack_seq	= htonl(ntohl(th->seq) + 1);;

Strange double ";;"


> +	tcp_flag_word(nth) = TCP_FLAG_ACK;
> +	nth->doff	= tcp_hdr_size / 4;
> +	nth->window	=
> htons(state->seen[IP_CT_DIR_ORIGINAL].td_maxwin);
> +	nth->check	= 0;
> +	nth->urg_ptr	= 0;
> +
> +	synproxy_build_options(nth, opts);
> +
> +	synproxy_send_tcp(skb, nskb, skb->nfct, IP_CT_ESTABLISHED,
> +			  niph, nth, tcp_hdr_size);
> +}
> +


> +static int synproxy_tg6_check(const struct xt_tgchk_param *par)
> +{
> +	/// XXX PROTO match TCP

Ups, this looks like an comment to your self ;-)

> +	return nf_ct_l3proto_try_module_get(par->family);
> +}



-- 
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 3/5] netfilter: add SYNPROXY core/target
From: Patrick McHardy @ 2013-08-07 20:56 UTC (permalink / raw)
  To: Jesper Dangaard Brouer; +Cc: pablo, netfilter-devel, netdev, mph, as
In-Reply-To: <20130807222600.51eeca09@redhat.com>

On Wed, Aug 07, 2013 at 10:26:00PM +0200, Jesper Dangaard Brouer wrote:
> On Wed,  7 Aug 2013 19:42:49 +0200
> Patrick McHardy <kaber@trash.net> wrote:
> 
> > +	nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
> > +	nth->source	= th->source;
> > +	nth->dest	= th->dest;
> > +	nth->seq	= htonl(ntohl(th->seq) - 1);
> > +	nth->ack_seq	= htonl(ntohl(th->ack_seq) - 1);;
> 
> Strange double ";;"

Thanks, fixed.

> Besides shouldn't nth->ack_seq be zero, in a SYN packet? This is the
> SYN "replayed" towards the server right?
> 
> I also pointed to this in an earlier patch Martin showed me, but he
> reported that changing this resulted in bad behavior.  So, I would
> request Martin to re-test this part.

Right, it should be zero, but it doesn't matter since the ACK flag isn't
set. This is used to propagate the sequence number to the hook function
to initialize the sequence adjustment data. While in the target function,
we don't have any connection tracking state to store this in. We could
set it to zero after that, but it shouldn't matter.

> > +static unsigned int
> > +synproxy_tg4(struct sk_buff *skb, const struct xt_action_param *par)
> > +{
> > +	const struct xt_synproxy_info *info = par->targinfo;
> > +	struct synproxy_net *snet = synproxy_pernet(dev_net(par->in));
> > +	struct synproxy_options opts = {};
> > +	struct tcphdr *th, _th;
> > +
> > +	if (nf_ip_checksum(skb, par->hooknum, par->thoff, IPPROTO_TCP))
> > +		return NF_DROP;
> > +
> > +	th = skb_header_pointer(skb, par->thoff, sizeof(_th), &_th);
> > +	if (th == NULL)
> > +		return NF_DROP;
> > +
> > +	synproxy_parse_options(skb, par->thoff, th, &opts);
> > +
> > +	if (th->syn) {
> > +		/* Initial SYN from client */
> > +		this_cpu_inc(snet->stats->syn_received);
> > +
> > +		if (th->ece && th->cwr)
> > +			opts.options |= XT_SYNPROXY_OPT_ECN;
> > +
> > +		opts.options &= info->options;
> > +		if (opts.options & XT_SYNPROXY_OPT_TIMESTAMP)
> > +			synproxy_init_timestamp_cookie(info, &opts);
> > +		else
> > +			opts.options &= ~(XT_SYNPROXY_OPT_WSCALE |
> > +					  XT_SYNPROXY_OPT_SACK_PERM |
> > +					  XT_SYNPROXY_OPT_ECN);
> > +
> > +		synproxy_send_client_synack(skb, th, &opts);
> > +	} else if (th->ack && !(th->fin || th->rst)) {
> 
> This could also match SYN+ACK... we are only interested in the ACK
> (from the 3WHS) here, right?

Right, we shouldn't see a SYN/ACK here, but I'll add an explicit check.

> > +			switch (opcode) {
> > +			case TCPOPT_MSS:
> > +				if (opsize == TCPOLEN_MSS) {
> > +					 opts->mss = get_unaligned_be16(ptr);
> 
> Strange indention, extra space before "opts->mss".
> 
> > +					 opts->options |= XT_SYNPROXY_OPT_MSS;
> 
> Strange indention, extra space before "opts->options".

Thanks, fixed.

> > +static int synproxy_cpu_seq_show(struct seq_file *seq, void *v)
> > +{
> > +	struct synproxy_stats *stats = v;
> > +
> > +	if (v == SEQ_START_TOKEN) {
> > +		seq_printf(seq, "syn_received\tcookie_invalid\tcookie_valid\n");
> > +		return 0;
> > +	}
> > +
> > +	seq_printf(seq, "%08u\t%08x\t%08x\n",
> 
> Shouldn't all numbers be printed in hex? (%08u -> %08x)

Right, fixed.

> Besides when using net->proc_net_stat, then the first entry is usually
> "entries" which is not percpu, this will likely confusing the tool:
>   lnstat -f synproxy -c 42

I'll look into that.

Thanks Jesper.

^ permalink raw reply

* Re: [PATCH 5/5] netfilter: add IPv6 SYNPROXY target
From: Patrick McHardy @ 2013-08-07 20:57 UTC (permalink / raw)
  To: Jesper Dangaard Brouer; +Cc: pablo, netfilter-devel, netdev, mph, as
In-Reply-To: <20130807223440.0a40a7c1@redhat.com>

On Wed, Aug 07, 2013 at 10:34:40PM +0200, Jesper Dangaard Brouer wrote:
> 
> On Wed,  7 Aug 2013 19:42:51 +0200 Patrick McHardy <kaber@trash.net> wrote:
> 
> > Add an IPv6 version of the SYNPROXY target. The main differences to
> > the IPv4 version is routing and IP header construction.
> > 
> > Signed-off-by: Patrick McHardy <kaber@trash.net>
> 
> > +static int synproxy_tg6_check(const struct xt_tgchk_param *par)
> > +{
> > +	/// XXX PROTO match TCP
> 
> Ups, this looks like an comment to your self ;-)
> 
> > +	return nf_ct_l3proto_try_module_get(par->family);
> > +}

Oops right, I intended to add a check for proto TCP match in the rule
to make sure thoff is initialized by ip6tables.

I'll wait for more comments before sending an updated series.

^ permalink raw reply

* Re: [PATCH RFC 0/5] netfilter: implement netfilter SYN proxy
From: Patrick McHardy @ 2013-08-07 20:59 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: pablo, netfilter-devel, netdev, mph, jesper.brouer, as
In-Reply-To: <1375898766.4004.37.camel@edumazet-glaptop>

On Wed, Aug 07, 2013 at 11:06:06AM -0700, Eric Dumazet wrote:
> On Wed, 2013-08-07 at 19:42 +0200, Patrick McHardy wrote:
> 
> > 
> > The SYNPROXY operates by marking the initial SYN from the client as UNTRACKED
> > and directing it to the SYNPROXY target. The target responds with a SYN/ACK
> > containing a cookie and encodes options such as window scaling factor, SACK
> > perm etc. into the timestamp, if timestamps are used (similar to TCP). The
> > window size is set to zero. The response is also sent as untracked packet.
> 
> TCP timestamps are not really used, for various reasons ...
> 
> Have you taken a look at 
> 
> <http://lists.freebsd.org/pipermail/freebsd-net/2013-July/035999.html>

No, not yet, will have a look. Not sure what you mean by "TCP timestamps
are not really used" though. I might be biased by usually only looking at
Linux traffic, but I was under that impression that everyone is using
TCP timestamps nowadays?

^ permalink raw reply

* Re: [PATCH RFC 0/5] netfilter: implement netfilter SYN proxy
From: Hannes Frederic Sowa @ 2013-08-07 21:05 UTC (permalink / raw)
  To: Patrick McHardy
  Cc: Eric Dumazet, pablo, netfilter-devel, netdev, mph, jesper.brouer,
	as
In-Reply-To: <20130807205959.GC21463@macbook.localnet>

On Wed, Aug 07, 2013 at 10:59:59PM +0200, Patrick McHardy wrote:
> On Wed, Aug 07, 2013 at 11:06:06AM -0700, Eric Dumazet wrote:
> > TCP timestamps are not really used, for various reasons ...
> > 
> > Have you taken a look at 
> > 
> > <http://lists.freebsd.org/pipermail/freebsd-net/2013-July/035999.html>
> 
> No, not yet, will have a look. Not sure what you mean by "TCP timestamps
> are not really used" though. I might be biased by usually only looking at
> Linux traffic, but I was under that impression that everyone is using
> TCP timestamps nowadays?

We had a thread here on netdev:
<http://thread.gmane.org/gmane.linux.network/275681/>

It seems, Windows stopped using tcp timestamps at least in windows 8 by
default.

^ permalink raw reply

* Re: [PATCH RFC 0/5] netfilter: implement netfilter SYN proxy
From: Patrick McHardy @ 2013-08-07 21:24 UTC (permalink / raw)
  To: Eric Dumazet, pablo, netfilter-devel, netdev, mph, jesper.brouer,
	as
In-Reply-To: <20130807210540.GE32257@order.stressinduktion.org>

On Wed, Aug 07, 2013 at 11:05:40PM +0200, Hannes Frederic Sowa wrote:
> On Wed, Aug 07, 2013 at 10:59:59PM +0200, Patrick McHardy wrote:
> > On Wed, Aug 07, 2013 at 11:06:06AM -0700, Eric Dumazet wrote:
> > > TCP timestamps are not really used, for various reasons ...
> > > 
> > > Have you taken a look at 
> > > 
> > > <http://lists.freebsd.org/pipermail/freebsd-net/2013-July/035999.html>
> > 
> > No, not yet, will have a look. Not sure what you mean by "TCP timestamps
> > are not really used" though. I might be biased by usually only looking at
> > Linux traffic, but I was under that impression that everyone is using
> > TCP timestamps nowadays?
> 
> We had a thread here on netdev:
> <http://thread.gmane.org/gmane.linux.network/275681/>
> 
> It seems, Windows stopped using tcp timestamps at least in windows 8 by
> default.

I see. Well, that seems to be a general problem with SYN cookies, I guess
in that case the encoding Linux uses should be changed. I'll have a closer
look at the changes proposed in that thread tommorrow.

^ permalink raw reply

* Re: [PATCH RFC 0/5] netfilter: implement netfilter SYN proxy
From: Eric Dumazet @ 2013-08-07 21:39 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: pablo, netfilter-devel, netdev, mph, jesper.brouer, as
In-Reply-To: <20130807212410.GA22932@macbook.localnet>

On Wed, 2013-08-07 at 23:24 +0200, Patrick McHardy wrote:

> I see. Well, that seems to be a general problem with SYN cookies, I guess
> in that case the encoding Linux uses should be changed. I'll have a closer
> look at the changes proposed in that thread tommorrow.

I did a quick check on a host, and it turns out that 111664 SYN had TS
option (total of 146123 SYN messages received)

So maybe its not a big issue. We probably need a poll ;)



^ permalink raw reply

* Re: af_packet: when sending ethernet frames, parse header for skb->protocol
From: Andrew Vagin @ 2013-08-07 22:03 UTC (permalink / raw)
  To: Phil Sutter; +Cc: David S. Miller, netdev

Hi,

I compiled the akpm branch of linux-next and found that my virtual
machine didn't get an IP address by dhcp.

git bisect said that the problem is in "af_packet: when sending ethernet
frames, parse header for skb->protocol"

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=0f75b09c798ed00c30d7d5551b896be883bc2aeb

I have double checked, the VM gets an IP address w/o this patch and it
does not get an address with this patch.

Let me know if you need any information to investigate this issue.

Thanks.

[avagin@localhost linux-2.6]$ git bisect log
# bad: [4a806113d2dfac9e936089859fdd073b5ff31d2e] debug
# good: [c095ba7224d8edc71dcef0d655911399a8bd4a3f] Linux 3.11-rc4
git bisect start 'akpm' 'HEAD'
# bad: [54578244e06fd945bdab70c3d37b6c311b2b1bc2] Merge remote-tracking branch 'spi/for-next'
git bisect bad 54578244e06fd945bdab70c3d37b6c311b2b1bc2
# bad: [79581039eba18f63be7bc1ea2c43da030708b9a0] Merge remote-tracking branch 'wireless-next/master'
git bisect bad 79581039eba18f63be7bc1ea2c43da030708b9a0
# good: [37101b9e3432536727f699e916b3b430cd8ce9e4] Merge remote-tracking branch 'libata/for-next'
git bisect good 37101b9e3432536727f699e916b3b430cd8ce9e4
# bad: [0d0cffdcc6fc4f57080b9a443401e8dad5ca77f8] gianfar: Cleanup TxFCB insertion on xmit
git bisect bad 0d0cffdcc6fc4f57080b9a443401e8dad5ca77f8
# good: [66cae9ed6bc46b8cc57a9693f99f69926f3cc7ef] rtnl: export physical port id via RT netlink
git bisect good 66cae9ed6bc46b8cc57a9693f99f69926f3cc7ef
# good: [9ab5ec59c8a9cc0e4b94252b48200b6023c716aa] tile: support PTP using the tilegx mPIPE (IEEE 1588)
git bisect good 9ab5ec59c8a9cc0e4b94252b48200b6023c716aa
# bad: [c483e02614551e44ced3fe6eedda8e36d3277ccc] af_packet: simplify VLAN frame check in packet_snd
git bisect bad c483e02614551e44ced3fe6eedda8e36d3277ccc
# good: [b9c119844c42a46a6c6006d158ee33af81fe76ae] qlcnic: Enhance diagnostic loopback error codes.
git bisect good b9c119844c42a46a6c6006d158ee33af81fe76ae
# good: [e216975ad97cfcfc436789aa66d59a0e93f337f7] uapi: Convert some uses of 6 to ETH_ALEN
git bisect good e216975ad97cfcfc436789aa66d59a0e93f337f7
# good: [ba5082c71476891623757956ebfc36040ac317e2] Merge branch 'eth_alen'
git bisect good ba5082c71476891623757956ebfc36040ac317e2
# bad: [0f75b09c798ed00c30d7d5551b896be883bc2aeb] af_packet: when sending ethernet frames, parse header for skb->protocol
git bisect bad 0f75b09c798ed00c30d7d5551b896be883bc2aeb
# good: [d27fc78208b53ccdfd6a57d4ac44a459ca66806f] sctp: Don't lookup dst if transport dst is still valid
git bisect good d27fc78208b53ccdfd6a57d4ac44a459ca66806f
# first bad commit: [0f75b09c798ed00c30d7d5551b896be883bc2aeb] af_packet: when sending ethernet frames, parse header for skb->protocol

^ permalink raw reply

* Re: [PATCH 3/5] netfilter: add SYNPROXY core/target
From: Eric Dumazet @ 2013-08-07 22:11 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: pablo, netfilter-devel, netdev, mph, jesper.brouer, as
In-Reply-To: <1375897371-18430-4-git-send-email-kaber@trash.net>

On Wed, 2013-08-07 at 19:42 +0200, Patrick McHardy wrote:
> Add a SYNPROXY for netfilter. The code is split into two parts, the synproxy
> core with common functions and an address family specific target.
> 
> The SYNPROXY receives the connection request from the client, responds with
> a SYN/ACK containing a SYN cookie and announcing a zero window and checks
> whether the final ACK from the client contains a valid cookie.
> 
> It then establishes a connection to the original destination and, if
> successful, sends a window update to the client with the window size
> announced by the server.
> 
> Support for timestamps, SACK, window scaling and MSS options can be
> statically configured as target parameters if the features of the server
> are known. If timestamps are used, the timestamp value sent back to
> the client in the SYN/ACK will be different from the real timestamp of
> the server. In order to now break PAWS, the timestamps are translated in
> the direction server->client.
> 
> Signed-off-by: Patrick McHardy <kaber@trash.net>


> +static struct iphdr *
> +synproxy_build_ip(struct sk_buff *skb, u32 saddr, u32 daddr)
> +{
> +	struct iphdr *iph;
> +
> +	skb_reset_network_header(skb);
> +	iph = (struct iphdr *)skb_put(skb, sizeof(*iph));
> +	iph->version	= 4;
> +	iph->ihl	= sizeof(*iph) / 4;
> +	iph->tos	= 0;
> +	iph->id		= 0;
> +	iph->frag_off	= htons(IP_DF);
> +	iph->ttl	= 64;

sysctl_ip_default_ttl ?

> +	iph->protocol	= IPPROTO_TCP;
> +	iph->check	= 0;
> +	iph->saddr	= saddr;
> +	iph->daddr	= daddr;
> +
> +	return iph;
> +}
> +


> +static void
> +synproxy_send_client_synack(const struct sk_buff *skb, const struct tcphdr *th,
> +			    const struct synproxy_options *opts)
> +{
> +	struct sk_buff *nskb;
> +	struct iphdr *iph, *niph;
> +	struct tcphdr *nth;
> +	unsigned int tcp_hdr_size;
> +	u16 mss = opts->mss;
> +
> +	iph = ip_hdr(skb);
> +
> +	tcp_hdr_size = sizeof(*nth) + synproxy_options_size(opts);
> +	nskb = alloc_skb(sizeof(*niph) + tcp_hdr_size + LL_MAX_HEADER,
> +			 GFP_ATOMIC);
> +	if (nskb == NULL)
> +		return;
> +	skb_reserve(nskb, LL_MAX_HEADER);

s/LL_MAX_HEADER/MAX_TCP_HEADER ? 

> +
> +	niph = synproxy_build_ip(nskb, iph->daddr, iph->saddr);
> +
> +	skb_reset_transport_header(nskb);
> +	nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
> +	nth->source	= th->dest;
> +	nth->dest	= th->source;
> +	nth->seq	= htonl(__cookie_v4_init_sequence(iph, th, &mss));
> +	nth->ack_seq	= htonl(ntohl(th->seq) + 1);
> +	tcp_flag_word(nth) = TCP_FLAG_SYN | TCP_FLAG_ACK;
> +	if (opts->options & XT_SYNPROXY_OPT_ECN)
> +		tcp_flag_word(nth) |= TCP_FLAG_ECE;
> +	nth->doff	= tcp_hdr_size / 4;
> +	nth->window	= 0;
> +	nth->check	= 0;
> +	nth->urg_ptr	= 0;
> +
> +	synproxy_build_options(nth, opts);
> +
> +	synproxy_send_tcp(skb, nskb, skb->nfct, IP_CT_ESTABLISHED_REPLY,
> +			  niph, nth, tcp_hdr_size);
> +}

Also please check your uses of kfree_skb() .

Some of them would better be consume_skb() (for example in
ipv4_synproxy_hook())

I wonder if this code could be generic for IPv4/IPv6, instead of
duplicating in IPv6




^ permalink raw reply

* Re: linux-next: Tree for Aug 7
From: David Miller @ 2013-08-07 23:27 UTC (permalink / raw)
  To: phil
  Cc: eric.dumazet, johannes, sedat.dilek, sfr, linux-next,
	linux-kernel, netdev, hannes, linux-wireless, linville
In-Reply-To: <20130807183758.GB16263@orbit.nwl.cc>

From: Phil Sutter <phil@nwl.cc>
Date: Wed, 7 Aug 2013 20:37:58 +0200

> One could simply call skb_push(skb, ETH_HLEN) right after calling
> eth_type_trans(skb, dev) in order to undo the 'data' and 'len'
> adjustment. Not sure if this kind of hack is the right way to go here,
> or if the whole af_packet parses ethernet header discussion should be
> opened again instead.

That's completely pointless work.

Without that header pull, the only two things left that
eth_type_trans() does is set the skb->protocol field and
set skb->dev.

And even the latter has to be done already in an else
branch in the suspect AF_PACKET code.

So this eth_type_trans() call is 2/3 duplicate or unnecessary work,
it's the completely the wrong thing to do.

Look, I'm going to fix this myself, because I'm pretty tired of
waiting for the obvious fix.

^ permalink raw reply

* Re: [PATCH RFC 0/5] netfilter: implement netfilter SYN proxy
From: David Miller @ 2013-08-07 23:40 UTC (permalink / raw)
  To: hannes
  Cc: kaber, eric.dumazet, pablo, netfilter-devel, netdev, mph,
	jesper.brouer, as
In-Reply-To: <20130807210540.GE32257@order.stressinduktion.org>

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Wed, 7 Aug 2013 23:05:40 +0200

> It seems, Windows stopped using tcp timestamps at least in windows 8 by
> default.

Thankfully, Android device outnumber Windows 8 installs
by... something like 1,000 to 1, right?

I throw a huge "doesn't matter" to whatever Windows's TCP stack
decides to do.  It absolutely should not dictate whether we decide to
make use of this or that feature of TCP.  It's a bit player at best.

So if Windows 8 is the reason you're saying we shouldn't use
timestamps for anything, you're wrong.

^ permalink raw reply

* Re: low latency/busy poll feedback and bugs
From: David Miller @ 2013-08-07 23:41 UTC (permalink / raw)
  To: eric.dumazet; +Cc: bhutchings, eliezer.tamir, sbohrer, amirv, netdev
In-Reply-To: <1375907005.4004.47.camel@edumazet-glaptop>

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 07 Aug 2013 13:23:25 -0700

> On Wed, 2013-08-07 at 22:05 +0200, Ben Hutchings wrote:
> 
>> It seems like sk_mark_napi_id() should only be called on connected
>> sockets for now.
> 
> I concur.

Me too.

^ permalink raw reply

* Re: linux-next: Tree for Aug 7
From: David Miller @ 2013-08-07 23:36 UTC (permalink / raw)
  To: phil
  Cc: eric.dumazet, johannes, sedat.dilek, sfr, linux-next,
	linux-kernel, netdev, hannes, linux-wireless, linville
In-Reply-To: <20130807.162748.779496444843938176.davem@davemloft.net>

From: David Miller <davem@davemloft.net>
Date: Wed, 07 Aug 2013 16:27:48 -0700 (PDT)

> Look, I'm going to fix this myself, because I'm pretty tired of
> waiting for the obvious fix.

Someone please test this:

diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index c623861..afc02a6 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -29,6 +29,7 @@
 
 #ifdef __KERNEL__
 extern __be16		eth_type_trans(struct sk_buff *skb, struct net_device *dev);
+extern __be16		__eth_type_trans(struct sk_buff *skb, struct net_device *dev);
 extern const struct header_ops eth_header_ops;
 
 extern int eth_header(struct sk_buff *skb, struct net_device *dev,
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index be1f64d..35dc1be 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -146,6 +146,45 @@ int eth_rebuild_header(struct sk_buff *skb)
 EXPORT_SYMBOL(eth_rebuild_header);
 
 /**
+ * __eth_type_trans - only determine the packet's protocol ID.
+ * @skb: packet
+ * @dev: device
+ */
+__be16 __eth_type_trans(struct sk_buff *skb, struct net_device *dev)
+{
+	struct ethhdr *eth = (struct ethhdr *) skb->data;
+
+	/*
+	 * Some variants of DSA tagging don't have an ethertype field
+	 * at all, so we check here whether one of those tagging
+	 * variants has been configured on the receiving interface,
+	 * and if so, set skb->protocol without looking at the packet.
+	 */
+	if (netdev_uses_dsa_tags(dev))
+		return htons(ETH_P_DSA);
+	if (netdev_uses_trailer_tags(dev))
+		return htons(ETH_P_TRAILER);
+
+	if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN)
+		return eth->h_proto;
+
+	/*
+	 *      This is a magic hack to spot IPX packets. Older Novell breaks
+	 *      the protocol design and runs IPX over 802.3 without an 802.2 LLC
+	 *      layer. We look for FFFF which isn't a used 802.2 SSAP/DSAP. This
+	 *      won't work for fault tolerant netware but does for the rest.
+	 */
+	if (skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF)
+		return htons(ETH_P_802_3);
+
+	/*
+	 *      Real 802.2 LLC
+	 */
+	return htons(ETH_P_802_2);
+}
+EXPORT_SYMBOL(__eth_type_trans);
+
+/**
  * eth_type_trans - determine the packet's protocol ID.
  * @skb: received socket data
  * @dev: receiving network device
@@ -184,33 +223,7 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
 			skb->pkt_type = PACKET_OTHERHOST;
 	}
 
-	/*
-	 * Some variants of DSA tagging don't have an ethertype field
-	 * at all, so we check here whether one of those tagging
-	 * variants has been configured on the receiving interface,
-	 * and if so, set skb->protocol without looking at the packet.
-	 */
-	if (netdev_uses_dsa_tags(dev))
-		return htons(ETH_P_DSA);
-	if (netdev_uses_trailer_tags(dev))
-		return htons(ETH_P_TRAILER);
-
-	if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN)
-		return eth->h_proto;
-
-	/*
-	 *      This is a magic hack to spot IPX packets. Older Novell breaks
-	 *      the protocol design and runs IPX over 802.3 without an 802.2 LLC
-	 *      layer. We look for FFFF which isn't a used 802.2 SSAP/DSAP. This
-	 *      won't work for fault tolerant netware but does for the rest.
-	 */
-	if (skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF)
-		return htons(ETH_P_802_3);
-
-	/*
-	 *      Real 802.2 LLC
-	 */
-	return htons(ETH_P_802_2);
+	return __eth_type_trans(skb, dev);
 }
 EXPORT_SYMBOL(eth_type_trans);
 
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 0c0f6c9..ec8e1c3 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2003,7 +2003,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 			return err;
 
 		if (dev->type == ARPHRD_ETHER)
-			skb->protocol = eth_type_trans(skb, dev);
+			skb->protocol = __eth_type_trans(skb, dev);
 
 		data += dev->hard_header_len;
 		to_write -= dev->hard_header_len;
@@ -2332,13 +2332,13 @@ static int packet_snd(struct socket *sock,
 	sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
 
 	if (dev->type == ARPHRD_ETHER) {
-		skb->protocol = eth_type_trans(skb, dev);
+		skb->protocol = __eth_type_trans(skb, dev);
 		if (skb->protocol == htons(ETH_P_8021Q))
 			reserve += VLAN_HLEN;
 	} else {
 		skb->protocol = proto;
-		skb->dev = dev;
 	}
+	skb->dev = dev;
 
 	if (!gso_type && (len > dev->mtu + reserve + extra_len)) {
 		err = -EMSGSIZE;

^ permalink raw reply related

* Re: [PATCH 3/5] netfilter: add SYNPROXY core/target
From: Patrick McHardy @ 2013-08-07 23:37 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: pablo, netfilter-devel, netdev, mph, jesper.brouer, as
In-Reply-To: <1375913514.4004.63.camel@edumazet-glaptop>

On Wed, Aug 07, 2013 at 03:11:54PM -0700, Eric Dumazet wrote:
> On Wed, 2013-08-07 at 19:42 +0200, Patrick McHardy wrote:
> > Add a SYNPROXY for netfilter. The code is split into two parts, the synproxy
> > core with common functions and an address family specific target.
> > 
> > The SYNPROXY receives the connection request from the client, responds with
> > a SYN/ACK containing a SYN cookie and announcing a zero window and checks
> > whether the final ACK from the client contains a valid cookie.
> > 
> > It then establishes a connection to the original destination and, if
> > successful, sends a window update to the client with the window size
> > announced by the server.
> > 
> > Support for timestamps, SACK, window scaling and MSS options can be
> > statically configured as target parameters if the features of the server
> > are known. If timestamps are used, the timestamp value sent back to
> > the client in the SYN/ACK will be different from the real timestamp of
> > the server. In order to now break PAWS, the timestamps are translated in
> > the direction server->client.
> > 
> > Signed-off-by: Patrick McHardy <kaber@trash.net>
> 
> 
> > +static struct iphdr *
> > +synproxy_build_ip(struct sk_buff *skb, u32 saddr, u32 daddr)
> > +{
> > +	struct iphdr *iph;
> > +
> > +	skb_reset_network_header(skb);
> > +	iph = (struct iphdr *)skb_put(skb, sizeof(*iph));
> > +	iph->version	= 4;
> > +	iph->ihl	= sizeof(*iph) / 4;
> > +	iph->tos	= 0;
> > +	iph->id		= 0;
> > +	iph->frag_off	= htons(IP_DF);
> > +	iph->ttl	= 64;
> 
> sysctl_ip_default_ttl ?

Will do, thanks.

> > +static void
> > +synproxy_send_client_synack(const struct sk_buff *skb, const struct tcphdr *th,
> > +			    const struct synproxy_options *opts)
> > +{
> > +	struct sk_buff *nskb;
> > +	struct iphdr *iph, *niph;
> > +	struct tcphdr *nth;
> > +	unsigned int tcp_hdr_size;
> > +	u16 mss = opts->mss;
> > +
> > +	iph = ip_hdr(skb);
> > +
> > +	tcp_hdr_size = sizeof(*nth) + synproxy_options_size(opts);
> > +	nskb = alloc_skb(sizeof(*niph) + tcp_hdr_size + LL_MAX_HEADER,
> > +			 GFP_ATOMIC);
> > +	if (nskb == NULL)
> > +		return;
> > +	skb_reserve(nskb, LL_MAX_HEADER);
> 
> s/LL_MAX_HEADER/MAX_TCP_HEADER ? 

ACK.

> > +	niph = synproxy_build_ip(nskb, iph->daddr, iph->saddr);
> > +
> > +	skb_reset_transport_header(nskb);
> > +	nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
> > +	nth->source	= th->dest;
> > +	nth->dest	= th->source;
> > +	nth->seq	= htonl(__cookie_v4_init_sequence(iph, th, &mss));
> > +	nth->ack_seq	= htonl(ntohl(th->seq) + 1);
> > +	tcp_flag_word(nth) = TCP_FLAG_SYN | TCP_FLAG_ACK;
> > +	if (opts->options & XT_SYNPROXY_OPT_ECN)
> > +		tcp_flag_word(nth) |= TCP_FLAG_ECE;
> > +	nth->doff	= tcp_hdr_size / 4;
> > +	nth->window	= 0;
> > +	nth->check	= 0;
> > +	nth->urg_ptr	= 0;
> > +
> > +	synproxy_build_options(nth, opts);
> > +
> > +	synproxy_send_tcp(skb, nskb, skb->nfct, IP_CT_ESTABLISHED_REPLY,
> > +			  niph, nth, tcp_hdr_size);
> > +}
> 
> Also please check your uses of kfree_skb() .
> 
> Some of them would better be consume_skb() (for example in
> ipv4_synproxy_hook())

I'll look into that.

> I wonder if this code could be generic for IPv4/IPv6, instead of
> duplicating in IPv6

I considered that, in fact I started under that assumption. But it would
result in lots of functions taking 10+ (long) arguments, so in my opinion
the code gets less readable. Alternative would be lots of callbacks.

This seemed like the cleanest way so far, but I'd certainly welcome
concrete proposals for removing duplicated code.

^ permalink raw reply

* Re: [PATCH net-next 1/2] ip_tunnel: embed hash list head
From: David Miller @ 2013-08-07 23:48 UTC (permalink / raw)
  To: stephen; +Cc: pshelar, netdev
In-Reply-To: <20130805225137.6c934d7f@nehalam.linuxnetplumber.net>

From: Stephen Hemminger <stephen@networkplumber.org>
Date: Mon, 5 Aug 2013 22:51:37 -0700

> The IP tunnel hash heads can be embedded in the per-net structure
> since it is a fixed size. Reduce the size so that the total structure
> fits in a page size. The original size was overly large, even NETDEV_HASHBITS
> is only 8 bits!
> 
> Also, add some white space for readability.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Applied, thanks.

^ permalink raw reply

* Re: [net-next 0/6] Reduce code duplication of zerocopy of tun and macvtap
From: David Miller @ 2013-08-07 23:52 UTC (permalink / raw)
  To: jasowang; +Cc: mst, netdev, linux-kernel
In-Reply-To: <1375782308-10565-1-git-send-email-jasowang@redhat.com>

From: Jason Wang <jasowang@redhat.com>
Date: Tue,  6 Aug 2013 17:45:02 +0800

> This series tries to reduce the code duplication of zerocopy of tun and macvtap
> by:
> 
> - Move common helpers iov_pages() and zerocopy_sg_from_iovec() to net/core
> - Use exist helpers instead of open coding the new one
> 
> With this seires, we can do further optimizations on top.

Looks great, applied!

^ permalink raw reply

* Re: [PATCH 1/3] net: phy: micrel: Staticize ksz8873mll_read_status()
From: David Miller @ 2013-08-07 23:59 UTC (permalink / raw)
  To: jg1.han; +Cc: netdev, hector.palacios, david.choi
In-Reply-To: <000101ce927f$15413560$3fc3a020$@samsung.com>

From: Jingoo Han <jg1.han@samsung.com>
Date: Tue, 06 Aug 2013 17:29:35 +0900

> ksz8873mll_read_status() is used only in this file.
> Fix the following sparse warning:
> 
> drivers/net/phy/micrel.c:147:5: warning: symbol 'ksz8873mll_read_status' was not declared. Should it be static?
> 
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>

Applied.

^ permalink raw reply

* Re: [PATCH 2/3] net: phy: mdio: add missing __iomem annotation
From: David Miller @ 2013-08-07 23:59 UTC (permalink / raw)
  To: jg1.han; +Cc: netdev, timur
In-Reply-To: <000201ce927f$8e339490$aa9abdb0$@samsung.com>

From: Jingoo Han <jg1.han@samsung.com>
Date: Tue, 06 Aug 2013 17:32:58 +0900

> Added missing __iomem annotation in order to fix the following
> sparse warnings:
> 
> drivers/net/phy/mdio-mux-mmioreg.c:51:27: warning: incorrect type in initializer (different address spaces)
> drivers/net/phy/mdio-mux-mmioreg.c:51:27:    expected void *p
> drivers/net/phy/mdio-mux-mmioreg.c:51:27:    got void [noderef] <asn:2>*
> drivers/net/phy/mdio-mux-mmioreg.c:57:21: warning: incorrect type in argument 1 (different address spaces)
> drivers/net/phy/mdio-mux-mmioreg.c:57:21:    expected void const volatile [noderef] <asn:2>*addr
> drivers/net/phy/mdio-mux-mmioreg.c:57:21:    got void *p
> drivers/net/phy/mdio-mux-mmioreg.c:60:25: warning: incorrect type in argument 2 (different address spaces)
> drivers/net/phy/mdio-mux-mmioreg.c:60:25:    expected void volatile [noderef] <asn:2>*addr
> drivers/net/phy/mdio-mux-mmioreg.c:60:25:    got void *p
> drivers/net/phy/mdio-mux-mmioreg.c:64:25: warning: incorrect type in argument 1 (different address spaces)
> drivers/net/phy/mdio-mux-mmioreg.c:64:25:    expected void volatile [noderef] <asn:2>*addr
> drivers/net/phy/mdio-mux-mmioreg.c:64:25:    got void *p
> 
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>

Applied.

^ permalink raw reply

* Re: linux-next: Tree for Aug 7
From: Sedat Dilek @ 2013-08-08  0:02 UTC (permalink / raw)
  To: David Miller
  Cc: phil, eric.dumazet, johannes, sfr, linux-next, linux-kernel,
	netdev, hannes, linux-wireless, linville
In-Reply-To: <20130807.163621.84433966934449459.davem@davemloft.net>

On Thu, Aug 8, 2013 at 1:36 AM, David Miller <davem@davemloft.net> wrote:
> From: David Miller <davem@davemloft.net>
> Date: Wed, 07 Aug 2013 16:27:48 -0700 (PDT)
>
>> Look, I'm going to fix this myself, because I'm pretty tired of
>> waiting for the obvious fix.
>
> Someone please test this:
>

Your patch on top of next-20130807 does not fix the issue in my wifi/network.
No working Internet connection (ping etc.).

- Sedat -

> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> index c623861..afc02a6 100644
> --- a/include/linux/etherdevice.h
> +++ b/include/linux/etherdevice.h
> @@ -29,6 +29,7 @@
>
>  #ifdef __KERNEL__
>  extern __be16          eth_type_trans(struct sk_buff *skb, struct net_device *dev);
> +extern __be16          __eth_type_trans(struct sk_buff *skb, struct net_device *dev);
>  extern const struct header_ops eth_header_ops;
>
>  extern int eth_header(struct sk_buff *skb, struct net_device *dev,
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index be1f64d..35dc1be 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -146,6 +146,45 @@ int eth_rebuild_header(struct sk_buff *skb)
>  EXPORT_SYMBOL(eth_rebuild_header);
>
>  /**
> + * __eth_type_trans - only determine the packet's protocol ID.
> + * @skb: packet
> + * @dev: device
> + */
> +__be16 __eth_type_trans(struct sk_buff *skb, struct net_device *dev)
> +{
> +       struct ethhdr *eth = (struct ethhdr *) skb->data;
> +
> +       /*
> +        * Some variants of DSA tagging don't have an ethertype field
> +        * at all, so we check here whether one of those tagging
> +        * variants has been configured on the receiving interface,
> +        * and if so, set skb->protocol without looking at the packet.
> +        */
> +       if (netdev_uses_dsa_tags(dev))
> +               return htons(ETH_P_DSA);
> +       if (netdev_uses_trailer_tags(dev))
> +               return htons(ETH_P_TRAILER);
> +
> +       if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN)
> +               return eth->h_proto;
> +
> +       /*
> +        *      This is a magic hack to spot IPX packets. Older Novell breaks
> +        *      the protocol design and runs IPX over 802.3 without an 802.2 LLC
> +        *      layer. We look for FFFF which isn't a used 802.2 SSAP/DSAP. This
> +        *      won't work for fault tolerant netware but does for the rest.
> +        */
> +       if (skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF)
> +               return htons(ETH_P_802_3);
> +
> +       /*
> +        *      Real 802.2 LLC
> +        */
> +       return htons(ETH_P_802_2);
> +}
> +EXPORT_SYMBOL(__eth_type_trans);
> +
> +/**
>   * eth_type_trans - determine the packet's protocol ID.
>   * @skb: received socket data
>   * @dev: receiving network device
> @@ -184,33 +223,7 @@ __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev)
>                         skb->pkt_type = PACKET_OTHERHOST;
>         }
>
> -       /*
> -        * Some variants of DSA tagging don't have an ethertype field
> -        * at all, so we check here whether one of those tagging
> -        * variants has been configured on the receiving interface,
> -        * and if so, set skb->protocol without looking at the packet.
> -        */
> -       if (netdev_uses_dsa_tags(dev))
> -               return htons(ETH_P_DSA);
> -       if (netdev_uses_trailer_tags(dev))
> -               return htons(ETH_P_TRAILER);
> -
> -       if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN)
> -               return eth->h_proto;
> -
> -       /*
> -        *      This is a magic hack to spot IPX packets. Older Novell breaks
> -        *      the protocol design and runs IPX over 802.3 without an 802.2 LLC
> -        *      layer. We look for FFFF which isn't a used 802.2 SSAP/DSAP. This
> -        *      won't work for fault tolerant netware but does for the rest.
> -        */
> -       if (skb->len >= 2 && *(unsigned short *)(skb->data) == 0xFFFF)
> -               return htons(ETH_P_802_3);
> -
> -       /*
> -        *      Real 802.2 LLC
> -        */
> -       return htons(ETH_P_802_2);
> +       return __eth_type_trans(skb, dev);
>  }
>  EXPORT_SYMBOL(eth_type_trans);
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 0c0f6c9..ec8e1c3 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2003,7 +2003,7 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
>                         return err;
>
>                 if (dev->type == ARPHRD_ETHER)
> -                       skb->protocol = eth_type_trans(skb, dev);
> +                       skb->protocol = __eth_type_trans(skb, dev);
>
>                 data += dev->hard_header_len;
>                 to_write -= dev->hard_header_len;
> @@ -2332,13 +2332,13 @@ static int packet_snd(struct socket *sock,
>         sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
>
>         if (dev->type == ARPHRD_ETHER) {
> -               skb->protocol = eth_type_trans(skb, dev);
> +               skb->protocol = __eth_type_trans(skb, dev);
>                 if (skb->protocol == htons(ETH_P_8021Q))
>                         reserve += VLAN_HLEN;
>         } else {
>                 skb->protocol = proto;
> -               skb->dev = dev;
>         }
> +       skb->dev = dev;
>
>         if (!gso_type && (len > dev->mtu + reserve + extra_len)) {
>                 err = -EMSGSIZE;

^ permalink raw reply

* Re: [PATCH RFC 0/5] netfilter: implement netfilter SYN proxy
From: Hannes Frederic Sowa @ 2013-08-08  0:04 UTC (permalink / raw)
  To: David Miller
  Cc: kaber, eric.dumazet, pablo, netfilter-devel, netdev, mph,
	jesper.brouer, as
In-Reply-To: <20130807.164056.2110811182336568861.davem@davemloft.net>

On Wed, Aug 07, 2013 at 04:40:56PM -0700, David Miller wrote:
> From: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Date: Wed, 7 Aug 2013 23:05:40 +0200
> 
> > It seems, Windows stopped using tcp timestamps at least in windows 8 by
> > default.
> 
> Thankfully, Android device outnumber Windows 8 installs
> by... something like 1,000 to 1, right?

Heh, at minimum. :)

> I throw a huge "doesn't matter" to whatever Windows's TCP stack
> decides to do.  It absolutely should not dictate whether we decide to
> make use of this or that feature of TCP.  It's a bit player at best.
> 
> So if Windows 8 is the reason you're saying we shouldn't use
> timestamps for anything, you're wrong.

Actually, I don't care at all, because I don't do anything with windows
and don't get paid by anyone who wants me to care. ;)

But if we switch to a similar scheme as freebsd we can even care
less because even if some other operating systems or a major provider
decides to disable timestamps on their devices, we would still have
window scaling, sack (and ecn?) under syn dos. So, I do think it is an
improvement and don't see any disadvantages.

So, I don't care as long as the change (and siphash or maybe another
hashing scheme) is secure enough...

Greetings,

  Hannes

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox