Re: [PATCH nf-next 1/2] netfilter: SYNPROXY: set transport header properly
On Thu, Mar 08, 2018 at 05:01:26PM +0200, Serhey Popovych wrote: > Eric Dumazet wrote: > > > > > > On 03/08/2018 02:08 AM, Serhey Popovych wrote: > >> We can't use skb_reset_transport_header() together with skb_put() to set > >> skb->transport_header field because skb_put() does not touch skb->data. > >> > >> Do this same way as we did for csum_data in code: substract skb->head > >> from tcph. > >> > >> Signed-off-by: Serhey Popovych > >> --- > >> net/ipv4/netfilter/ipt_SYNPROXY.c | 8 > >> net/ipv6/netfilter/ip6t_SYNPROXY.c | 8 > >> 2 files changed, 8 insertions(+), 8 deletions(-) > >> > >> diff --git a/net/ipv4/netfilter/ipt_SYNPROXY.c > >> b/net/ipv4/netfilter/ipt_SYNPROXY.c > >> index f75fc6b..4953390 100644 > >> --- a/net/ipv4/netfilter/ipt_SYNPROXY.c > >> +++ b/net/ipv4/netfilter/ipt_SYNPROXY.c > >> @@ -90,8 +90,8 @@ > >> niph = synproxy_build_ip(net, nskb, iph->daddr, iph->saddr); > >> - skb_reset_transport_header(nskb); > >> nth = skb_put(nskb, tcp_hdr_size); > >> + nskb->transport_header = (unsigned char *)nth - nskb->head; > > > > I have no idea why you believe the current code is not correct. > > > alloc_skb(): >skb->head = skb->data = skb_tail_pointer(skb) > > synproxy_build_ip(): >skb_reset_network_header() /* skb_network_header(skb) == skb->data */ >skb_put(skb, len); /* skb->tail += len; skb->len += len */ > > > nth = skb_put() : nth is equal to skb->data > > Yes it is equal to skb->data, but in my opinion should be set to > skb->data + ip_hdrlen(skb) to point to TCP header. Netfilter hooks are placed at the network layer, existing codebase in the Linux networking stack assumes network header offset == transport header offset from the network layer. So this is OK even if it looks confusing at first glance. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH nf-next 1/2] netfilter: SYNPROXY: set transport header properly
Eric Dumazet wrote: > > > On 03/08/2018 07:01 AM, Serhey Popovych wrote: >> Eric Dumazet wrote: >>> >>> >>> On 03/08/2018 02:08 AM, Serhey Popovych wrote: We can't use skb_reset_transport_header() together with skb_put() to set skb->transport_header field because skb_put() does not touch skb->data. Do this same way as we did for csum_data in code: substract skb->head from tcph. Signed-off-by: Serhey Popovych --- net/ipv4/netfilter/ipt_SYNPROXY.c | 8 net/ipv6/netfilter/ip6t_SYNPROXY.c | 8 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/net/ipv4/netfilter/ipt_SYNPROXY.c b/net/ipv4/netfilter/ipt_SYNPROXY.c index f75fc6b..4953390 100644 --- a/net/ipv4/netfilter/ipt_SYNPROXY.c +++ b/net/ipv4/netfilter/ipt_SYNPROXY.c @@ -90,8 +90,8 @@ niph = synproxy_build_ip(net, nskb, iph->daddr, iph->saddr); - skb_reset_transport_header(nskb); nth = skb_put(nskb, tcp_hdr_size); + nskb->transport_header = (unsigned char *)nth - nskb->head; >>> >>> I have no idea why you believe the current code is not correct. >>> >> alloc_skb(): >> skb->head = skb->data = skb_tail_pointer(skb) >> >> synproxy_build_ip(): >> skb_reset_network_header() /* skb_network_header(skb) == skb->data */ >> skb_put(skb, len); /* skb->tail += len; skb->len += len */ >> >>> nth = skb_put() : nth is equal to skb->data >> >> Yes it is equal to skb->data, but in my opinion should be set to >> skb->data + ip_hdrlen(skb) to point to TCP header. > > Is it really needed to set transport header offset ? We do not intend to > inject this packet to a local TCP stack ? > > Nothing will care of transport header for non GSO packet. I think no, it works with transport header pointing to network header right now. > > Anyway, maybe the confusion would be avoided if the normal way of > pushing headers would be used (using skb_push()) Using skb_push() for SYNPROXY is easy, but for nf_reject we have nf_reject_iphdr_put() and nf_reject_ip_tcphdr_put() exported via EXPORT_SYMBOL_GPL. If I switch them to skb_push() this will broke compatibility with out of tree code that uses them. Should I change it to skb_push() or to something like skb->transport_header = skb_tail_pointer(skb) - skb->head; skb = skb_put(); Should these changes be single tone or I can use skb_push() for SYNPROXY and explicit skb->transport_header manipulations for nf_reject? > > No casts requested ;) signature.asc Description: OpenPGP digital signature
Re: [PATCH nf-next 1/2] netfilter: SYNPROXY: set transport header properly
On 03/08/2018 07:01 AM, Serhey Popovych wrote: Eric Dumazet wrote: On 03/08/2018 02:08 AM, Serhey Popovych wrote: We can't use skb_reset_transport_header() together with skb_put() to set skb->transport_header field because skb_put() does not touch skb->data. Do this same way as we did for csum_data in code: substract skb->head from tcph. Signed-off-by: Serhey Popovych --- net/ipv4/netfilter/ipt_SYNPROXY.c | 8 net/ipv6/netfilter/ip6t_SYNPROXY.c | 8 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/net/ipv4/netfilter/ipt_SYNPROXY.c b/net/ipv4/netfilter/ipt_SYNPROXY.c index f75fc6b..4953390 100644 --- a/net/ipv4/netfilter/ipt_SYNPROXY.c +++ b/net/ipv4/netfilter/ipt_SYNPROXY.c @@ -90,8 +90,8 @@ niph = synproxy_build_ip(net, nskb, iph->daddr, iph->saddr); - skb_reset_transport_header(nskb); nth = skb_put(nskb, tcp_hdr_size); + nskb->transport_header = (unsigned char *)nth - nskb->head; I have no idea why you believe the current code is not correct. alloc_skb(): skb->head = skb->data = skb_tail_pointer(skb) synproxy_build_ip(): skb_reset_network_header() /* skb_network_header(skb) == skb->data */ skb_put(skb, len); /* skb->tail += len; skb->len += len */ nth = skb_put() : nth is equal to skb->data Yes it is equal to skb->data, but in my opinion should be set to skb->data + ip_hdrlen(skb) to point to TCP header. Is it really needed to set transport header offset ? We do not intend to inject this packet to a local TCP stack ? Nothing will care of transport header for non GSO packet. Anyway, maybe the confusion would be avoided if the normal way of pushing headers would be used (using skb_push()) No casts requested ;) -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH nf-next 1/2] netfilter: SYNPROXY: set transport header properly
Eric Dumazet wrote: > > > On 03/08/2018 02:08 AM, Serhey Popovych wrote: >> We can't use skb_reset_transport_header() together with skb_put() to set >> skb->transport_header field because skb_put() does not touch skb->data. >> >> Do this same way as we did for csum_data in code: substract skb->head >> from tcph. >> >> Signed-off-by: Serhey Popovych >> --- >> net/ipv4/netfilter/ipt_SYNPROXY.c | 8 >> net/ipv6/netfilter/ip6t_SYNPROXY.c | 8 >> 2 files changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/net/ipv4/netfilter/ipt_SYNPROXY.c >> b/net/ipv4/netfilter/ipt_SYNPROXY.c >> index f75fc6b..4953390 100644 >> --- a/net/ipv4/netfilter/ipt_SYNPROXY.c >> +++ b/net/ipv4/netfilter/ipt_SYNPROXY.c >> @@ -90,8 +90,8 @@ >> niph = synproxy_build_ip(net, nskb, iph->daddr, iph->saddr); >> - skb_reset_transport_header(nskb); >> nth = skb_put(nskb, tcp_hdr_size); >> + nskb->transport_header = (unsigned char *)nth - nskb->head; > > I have no idea why you believe the current code is not correct. > alloc_skb(): skb->head = skb->data = skb_tail_pointer(skb) synproxy_build_ip(): skb_reset_network_header() /* skb_network_header(skb) == skb->data */ skb_put(skb, len); /* skb->tail += len; skb->len += len */ > nth = skb_put() : nth is equal to skb->data Yes it is equal to skb->data, but in my opinion should be set to skb->data + ip_hdrlen(skb) to point to TCP header. > > So skb_reset_transport_header(nskb); does exactly the same thing than > your code, but in a more readable way, without ugly cast. Yes I also not happy with this cast. > signature.asc Description: OpenPGP digital signature
Re: [PATCH nf-next 1/2] netfilter: SYNPROXY: set transport header properly
On 03/08/2018 02:08 AM, Serhey Popovych wrote: We can't use skb_reset_transport_header() together with skb_put() to set skb->transport_header field because skb_put() does not touch skb->data. Do this same way as we did for csum_data in code: substract skb->head from tcph. Signed-off-by: Serhey Popovych --- net/ipv4/netfilter/ipt_SYNPROXY.c | 8 net/ipv6/netfilter/ip6t_SYNPROXY.c | 8 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/net/ipv4/netfilter/ipt_SYNPROXY.c b/net/ipv4/netfilter/ipt_SYNPROXY.c index f75fc6b..4953390 100644 --- a/net/ipv4/netfilter/ipt_SYNPROXY.c +++ b/net/ipv4/netfilter/ipt_SYNPROXY.c @@ -90,8 +90,8 @@ niph = synproxy_build_ip(net, nskb, iph->daddr, iph->saddr); - skb_reset_transport_header(nskb); nth = skb_put(nskb, tcp_hdr_size); + nskb->transport_header = (unsigned char *)nth - nskb->head; I have no idea why you believe the current code is not correct. nth = skb_put() : nth is equal to skb->data So skb_reset_transport_header(nskb); does exactly the same thing than your code, but in a more readable way, without ugly cast. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html