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

["Gao Feng" <gfree.wind@foxmail.com>]

> -----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