[PATCH nf-next v2 1/1] netfilter: SYNPROXY: Return NF_STOLEN instead of NF_DROP during handshaking

[PATCH nf-next v2 1/1] netfilter: SYNPROXY: Return NF_STOLEN instead of NF_DROP during handshaking

[gfree.wind]

From: Gao Feng <fgao@ikuai8.com>

Current SYNPROXY codes return NF_DROP during normal TCP handshaking,
it is not friendly to caller. Because the nf_hook_slow would treat
the NF_DROP as an error, and return -EPERM.
As a result, it may cause the top caller think it meets one error.

So use NF_STOLEN instead of NF_DROP now because there is no error
happened indeed, and free the skb directly.

Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
 v2: Add the check for synproxy_recv_client_ack, per Gao Feng,
 v1: initial version

 net/ipv4/netfilter/ipt_SYNPROXY.c  | 21 ++++++++++++++-------
 net/ipv6/netfilter/ip6t_SYNPROXY.c | 20 ++++++++++++++------
 2 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/net/ipv4/netfilter/ipt_SYNPROXY.c b/net/ipv4/netfilter/ipt_SYNPROXY.c
index 3240a26..ab0f576 100644
--- a/net/ipv4/netfilter/ipt_SYNPROXY.c
+++ b/net/ipv4/netfilter/ipt_SYNPROXY.c
@@ -293,12 +293,16 @@
 					  XT_SYNPROXY_OPT_ECN);
 
 		synproxy_send_client_synack(net, skb, th, &opts);
-		return NF_DROP;
-
+		consume_skb(skb);
+		return NF_STOLEN;
 	} else if (th->ack && !(th->fin || th->rst || th->syn)) {
 		/* ACK from client */
-		synproxy_recv_client_ack(net, skb, th, &opts, ntohl(th->seq));
-		return NF_DROP;
+		if (synproxy_recv_client_ack(net, skb, th, &opts, ntohl(th->seq))) {
+			consume_skb(skb);
+			return NF_STOLEN;
+		} else {
+			return NF_DROP;
+		}
 	}
 
 	return XT_CONTINUE;
@@ -367,10 +371,13 @@ static unsigned int ipv4_synproxy_hook(void *priv,
 			 * number match the one of first SYN.
 			 */
 			if (synproxy_recv_client_ack(net, skb, th, &opts,
-						     ntohl(th->seq) + 1))
+						     ntohl(th->seq) + 1)) {
 				this_cpu_inc(snet->stats->cookie_retrans);
-
-			return NF_DROP;
+				consume_skb(skb);
+				return NF_STOLEN;
+			} else {
+				return NF_DROP;
+			}
 		}
 
 		synproxy->isn = ntohl(th->ack_seq);
diff --git a/net/ipv6/netfilter/ip6t_SYNPROXY.c b/net/ipv6/netfilter/ip6t_SYNPROXY.c
index 4ef1ddd..6a4f49e 100644
--- a/net/ipv6/netfilter/ip6t_SYNPROXY.c
+++ b/net/ipv6/netfilter/ip6t_SYNPROXY.c
@@ -307,12 +307,17 @@
 					  XT_SYNPROXY_OPT_ECN);
 
 		synproxy_send_client_synack(net, skb, th, &opts);
-		return NF_DROP;
+		consume_skb(skb);
+		return NF_STOLEN;
 
 	} else if (th->ack && !(th->fin || th->rst || th->syn)) {
 		/* ACK from client */
-		synproxy_recv_client_ack(net, skb, th, &opts, ntohl(th->seq));
-		return NF_DROP;
+		if (synproxy_recv_client_ack(net, skb, th, &opts, ntohl(th->seq))) {
+			consume_skb(skb);
+			return NF_STOLEN;
+		} else {
+			return NF_DROP;
+		}
 	}
 
 	return XT_CONTINUE;
@@ -388,10 +393,13 @@ static unsigned int ipv6_synproxy_hook(void *priv,
 			 * number match the one of first SYN.
 			 */
 			if (synproxy_recv_client_ack(net, skb, th, &opts,
-						     ntohl(th->seq) + 1))
+						     ntohl(th->seq) + 1)) {
 				this_cpu_inc(snet->stats->cookie_retrans);
-
-			return NF_DROP;
+				consume_skb(skb);
+				return NF_STOLEN;
+			} else {
+				return NF_DROP;
+			}
 		}
 
 		synproxy->isn = ntohl(th->ack_seq);
-- 
1.9.1

Re: [PATCH nf-next v2 1/1] netfilter: SYNPROXY: Return NF_STOLEN instead of NF_DROP during handshaking

[Pablo Neira Ayuso]

On Wed, Apr 12, 2017 at 10:14:50AM +0800, gfree.wind@foxmail.com wrote:
> From: Gao Feng <fgao@ikuai8.com>
> 
> Current SYNPROXY codes return NF_DROP during normal TCP handshaking,
> it is not friendly to caller. Because the nf_hook_slow would treat
> the NF_DROP as an error, and return -EPERM.
> As a result, it may cause the top caller think it meets one error.
> 
> So use NF_STOLEN instead of NF_DROP now because there is no error
> happened indeed, and free the skb directly.

Is this really addressing a real problem? How did you reproduce it?

BTW, your patch title is wrong.

[PATCH nf-next v2 1/1]
                  ^^^

This 1/1 is completely useless, please remove it in your follow up
patches.

Moreover, you should be more careful, *really*, this is not a speed
coding contest. You tend to send me follow up patch version just hours
afterwards because you rush too much.

Be more careful, use the same email address to send your patches. Drop
quote the full email in your replies...

Other than that, I may start ignoring your patches, it's too hard to
keep up with this.

RE: [PATCH nf-next v2 1/1] netfilter: SYNPROXY: Return NF_STOLEN instead of NF_DROP during handshaking

[Gao Feng]

> -----Original Message-----
> From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> 
> On Wed, Apr 12, 2017 at 10:14:50AM +0800, gfree.wind@foxmail.com wrote:
> >
> > Current SYNPROXY codes return NF_DROP during normal TCP handshaking,
> > it is not friendly to caller. Because the nf_hook_slow would treat the
> > NF_DROP as an error, and return -EPERM.
> > As a result, it may cause the top caller think it meets one error.
> >
> > So use NF_STOLEN instead of NF_DROP now because there is no error
> > happened indeed, and free the skb directly.
> 
> Is this really addressing a real problem? How did you reproduce it?

We defined the NF_DROP and NF_STOLEN, I think we should use them clearly.
When NF_DROP happens, it means one error happened.

In this case, when synproxy returns NF_DROP, it would be returned as the
return
value of netif_receive_skb.
And some driver would check the return value, like sb1250-mac.c.
Its function "sbdma_rx_process" checks the return value of
"netif_receive_skb".

dropped = netif_receive_skb(sb);
if (dropped == NET_RX_DROP) {
    dev->stats.rx_dropped++;
........

The "NET_RX_DROP" is same as NF_DROP. When return NF_DROP, it would add the 
dropped counter.


> 
> BTW, your patch title is wrong.
> 
> [PATCH nf-next v2 1/1]
>                   ^^^
> 
> This 1/1 is completely useless, please remove it in your follow up
patches.

Sorry, I always use one command "git format-patch -s -n master..XX"
according to one document
whose title is "HOWTO: Create and submit your first Linux kernel patch using
GIT".

It generate the "1/1" by default.

I will try to lookup other documents about the patch rule, and correct the
current command.

> 
> Moreover, you should be more careful, *really*, this is not a speed coding
> contest. You tend to send me follow up patch version just hours afterwards
> because you rush too much.

Yes, it is my fault. I would pay more attention on it.
I treated the patch commit as the power which could push me investigate the
kernel codes.
When one commit is accept, it would excite me a lot.

>From now on, I would use the power more carefully, and don't rush more.

> 
> Be more careful, use the same email address to send your patches. Drop
quote
> the full email in your replies...

I would drop the quote by manual carefully. 
As mentioned in other email, I have to change my email from gmail, and try
to find one which could support text email well recently

More carefully, and don't rush more.
I must follow it.

Best Regards
Feng

> 
> Other than that, I may start ignoring your patches, it's too hard to keep
up with
> this.

Re: [PATCH nf-next v2 1/1] netfilter: SYNPROXY: Return NF_STOLEN instead of NF_DROP during handshaking

[Pablo Neira Ayuso]

On Fri, Apr 14, 2017 at 07:04:44AM +0800, Gao Feng wrote:
> > -----Original Message-----
> > From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> > 
> > On Wed, Apr 12, 2017 at 10:14:50AM +0800, gfree.wind@foxmail.com wrote:
> > >
> > > Current SYNPROXY codes return NF_DROP during normal TCP handshaking,
> > > it is not friendly to caller. Because the nf_hook_slow would treat the
> > > NF_DROP as an error, and return -EPERM.
> > > As a result, it may cause the top caller think it meets one error.
> > >
> > > So use NF_STOLEN instead of NF_DROP now because there is no error
> > > happened indeed, and free the skb directly.
> > 
> > Is this really addressing a real problem? How did you reproduce it?
> 
> We defined the NF_DROP and NF_STOLEN, I think we should use them clearly.
> When NF_DROP happens, it means one error happened.

That's a valid concern. How did you tested this change?

[...]
> Sorry, I always use one command "git format-patch -s -n master..XX"
> according to one document
> whose title is "HOWTO: Create and submit your first Linux kernel patch using
> GIT".
> 
> It generate the "1/1" by default.

There are ways to avoid that. Only you send 1/1 patches.

> I will try to lookup other documents about the patch rule, and correct the
> current command.

OK.

[...]
> More carefully, and don't rush more.

Thank you.

RE: [PATCH nf-next v2 1/1] netfilter: SYNPROXY: Return NF_STOLEN instead of NF_DROP during handshaking

[Gao Feng]

> -----Original Message-----
> From: netfilter-devel-owner@vger.kernel.org
> On Fri, Apr 14, 2017 at 07:04:44AM +0800, Gao Feng wrote:
> > > -----Original Message-----
> > > From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> > >
> > > On Wed, Apr 12, 2017 at 10:14:50AM +0800, gfree.wind@foxmail.com
> wrote:
> > > >
> > > > Current SYNPROXY codes return NF_DROP during normal TCP
> > > > handshaking, it is not friendly to caller. Because the
> > > > nf_hook_slow would treat the NF_DROP as an error, and return -EPERM.
> > > > As a result, it may cause the top caller think it meets one error.
> > > >
> > > > So use NF_STOLEN instead of NF_DROP now because there is no error
> > > > happened indeed, and free the skb directly.
> > >
> > > Is this really addressing a real problem? How did you reproduce it?
> >
> > We defined the NF_DROP and NF_STOLEN, I think we should use them
> clearly.
> > When NF_DROP happens, it means one error happened.
> 
> That's a valid concern. How did you tested this change?

The test is a little hacker. The following is my whole test process.
1. Add one "print" member in the struct sk_buff; it would be zero by
default;
2. Add one log in the netif_receive_skb_internal 
+       if (skb->print)
+               pr_info("skb(%p) ret is %d\n", skb, ret);
3. the iptable rule is "iptables -A INPUT -p tcp --dport 12345 -m conntrack
--ctstate NEW -j SYNPROXY"

Test NF_DROP with the original codes:
1. I comment out the "kfree_skb" in the NF_DROP handler of nf_hook_slow.
2. Add one log before return NF_DROP in the synproxy codes
pr_info("skb(%p) is dropped\n", skb);

The result is following:
[   71.765035] skb(ffff9a6208bd7500) is dropped
[   71.765049] skb(ffff9a6208bd7500) ret is -1

Test NF_STOLEN with the patch:
1. I comment out the "consume_skb" before return NF_STOLEN;
2. Add the log before return NF_STOLEN in the synproxy codes
pr_info("skb(%p) is stolen\n", skb);

The test result is following:
[  221.564370] skb(ffff9a01214a8c00) is stolen
[  221.564383] skb(ffff9a01214a8c00) ret is 0

To summary, netif_receive_skb would return -EPERM when Netfilter returns
NF_DROP,
but NF_STOLEN not.
For the caller which cares about the return value of netif_receive_skb would
treat it
as one error.
Like cfv_rx_poll() in drivers/net/caif/caif_virtio.c.
                err = netif_receive_skb(skb);
                if (unlikely(err)) {
                        ++cfv->ndev->stats.rx_dropped;
                } else {
                        ++cfv->ndev->stats.rx_packets;
                        cfv->ndev->stats.rx_bytes += skb_len;
                }
It would cause the driver increase the dropped counter.

Best Regards
Feng

> 
> [...]
> > Sorry, I always use one command "git format-patch -s -n master..XX"
> > according to one document
> > whose title is "HOWTO: Create and submit your first Linux kernel patch
> > using GIT".
> >
> > It generate the "1/1" by default.
> 
> There are ways to avoid that. Only you send 1/1 patches.
> 
> > I will try to lookup other documents about the patch rule, and correct
> > the current command.
> 
> OK.
> 
> [...]
> > More carefully, and don't rush more.
> 
> Thank you.
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel"
in the
> body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

Re: [PATCH nf-next v2 1/1] netfilter: SYNPROXY: Return NF_STOLEN instead of NF_DROP during handshaking

[Pablo Neira Ayuso]

On Fri, Apr 14, 2017 at 12:52:05PM +0800, Gao Feng wrote:
> > -----Original Message-----
> > From: netfilter-devel-owner@vger.kernel.org
> > On Fri, Apr 14, 2017 at 07:04:44AM +0800, Gao Feng wrote:
> > > > -----Original Message-----
> > > > From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> > > >
> > > > On Wed, Apr 12, 2017 at 10:14:50AM +0800, gfree.wind@foxmail.com
> > wrote:
> > > > >
> > > > > Current SYNPROXY codes return NF_DROP during normal TCP
> > > > > handshaking, it is not friendly to caller. Because the
> > > > > nf_hook_slow would treat the NF_DROP as an error, and return -EPERM.
> > > > > As a result, it may cause the top caller think it meets one error.
> > > > >
> > > > > So use NF_STOLEN instead of NF_DROP now because there is no error
> > > > > happened indeed, and free the skb directly.
> > > >
> > > > Is this really addressing a real problem? How did you reproduce it?
> > >
> > > We defined the NF_DROP and NF_STOLEN, I think we should use them
> > clearly.
> > > When NF_DROP happens, it means one error happened.
> > 
> > That's a valid concern. How did you tested this change?
> 
> The test is a little hacker. The following is my whole test process.
> 1. Add one "print" member in the struct sk_buff; it would be zero by
> default;
> 2. Add one log in the netif_receive_skb_internal 
> +       if (skb->print)
> +               pr_info("skb(%p) ret is %d\n", skb, ret);
> 3. the iptable rule is "iptables -A INPUT -p tcp --dport 12345 -m conntrack
> --ctstate NEW -j SYNPROXY"
> 
> Test NF_DROP with the original codes:
> 1. I comment out the "kfree_skb" in the NF_DROP handler of nf_hook_slow.
> 2. Add one log before return NF_DROP in the synproxy codes
> pr_info("skb(%p) is dropped\n", skb);
> 
> The result is following:
> [   71.765035] skb(ffff9a6208bd7500) is dropped
> [   71.765049] skb(ffff9a6208bd7500) ret is -1
> 
> Test NF_STOLEN with the patch:
> 1. I comment out the "consume_skb" before return NF_STOLEN;
> 2. Add the log before return NF_STOLEN in the synproxy codes
> pr_info("skb(%p) is stolen\n", skb);
> 
> The test result is following:
> [  221.564370] skb(ffff9a01214a8c00) is stolen
> [  221.564383] skb(ffff9a01214a8c00) ret is 0
> 
> To summary, netif_receive_skb would return -EPERM when Netfilter returns
> NF_DROP,
> but NF_STOLEN not.
> For the caller which cares about the return value of netif_receive_skb would
> treat it
> as one error.
> Like cfv_rx_poll() in drivers/net/caif/caif_virtio.c.
>                 err = netif_receive_skb(skb);
>                 if (unlikely(err)) {
>                         ++cfv->ndev->stats.rx_dropped;
>                 } else {
>                         ++cfv->ndev->stats.rx_packets;
>                         cfv->ndev->stats.rx_bytes += skb_len;
>                 }
> It would cause the driver increase the dropped counter.

OK, please resubmit. Thanks.