On Fri, Apr 14, 2017 at 12:52:05PM +0800, Gao Feng wrote: > > -----Original Message----- > > From: netfilter-devel-owner@xxxxxxxxxxxxxxx > > On Fri, Apr 14, 2017 at 07:04:44AM +0800, Gao Feng wrote: > > > > -----Original Message----- > > > > From: Pablo Neira Ayuso [mailto:pablo@xxxxxxxxxxxxx] > > > > > > > > On Wed, Apr 12, 2017 at 10:14:50AM +0800, gfree.wind@xxxxxxxxxxx > > 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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
