Re: [PATCH nf-next 1/2] netfilter: SYNPROXY: set transport header properly

[Pablo Neira Ayuso]

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

[Serhey Popovych]

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

[Eric Dumazet]

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

[Serhey Popovych]

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

[Eric Dumazet]

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

[PATCH nf-next 1/2] netfilter: SYNPROXY: set transport header properly

[Serhey Popovych]

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;
nth->source = th->dest;
nth->dest   = th->source;
nth->seq= htonl(__cookie_v4_init_sequence(iph, th, ));
@@ -132,8 +132,8 @@
 
niph = synproxy_build_ip(net, nskb, iph->saddr, iph->daddr);
 
-   skb_reset_transport_header(nskb);
nth = skb_put(nskb, tcp_hdr_size);
+   nskb->transport_header = (unsigned char *)nth - nskb->head;
nth->source = th->source;
nth->dest   = th->dest;
nth->seq= htonl(recv_seq - 1);
@@ -177,8 +177,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;
nth->source = th->dest;
nth->dest   = th->source;
nth->seq= htonl(ntohl(th->ack_seq));
@@ -215,8 +215,8 @@
 
niph = synproxy_build_ip(net, nskb, iph->saddr, iph->daddr);
 
-   skb_reset_transport_header(nskb);
nth = skb_put(nskb, tcp_hdr_size);
+   nskb->transport_header = (unsigned char *)nth - nskb->head;
nth->source = th->source;
nth->dest   = th->dest;
nth->seq= htonl(ntohl(th->seq) + 1);
diff --git a/net/ipv6/netfilter/ip6t_SYNPROXY.c 
b/net/ipv6/netfilter/ip6t_SYNPROXY.c
index 437af8c..46c1e28 100644
--- a/net/ipv6/netfilter/ip6t_SYNPROXY.c
+++ b/net/ipv6/netfilter/ip6t_SYNPROXY.c
@@ -104,8 +104,8 @@
 
niph = synproxy_build_ip(net, nskb, >daddr, >saddr);
 
-   skb_reset_transport_header(nskb);
nth = skb_put(nskb, tcp_hdr_size);
+   nskb->transport_header = (unsigned char *)nth - nskb->head;
nth->source = th->dest;
nth->dest   = th->source;
nth->seq= htonl(__cookie_v6_init_sequence(iph, th, ));
@@ -146,8 +146,8 @@
 
niph = synproxy_build_ip(net, nskb, >saddr, >daddr);
 
-   skb_reset_transport_header(nskb);
nth = skb_put(nskb, tcp_hdr_size);
+   nskb->transport_header = (unsigned char *)nth - nskb->head;
nth->source = th->source;
nth->dest   = th->dest;
nth->seq= htonl(recv_seq - 1);
@@ -191,8 +191,8 @@
 
niph = synproxy_build_ip(net, nskb, >daddr, >saddr);
 
-   skb_reset_transport_header(nskb);
nth = skb_put(nskb, tcp_hdr_size);
+   nskb->transport_header = (unsigned char *)nth - nskb->head;
nth->source = th->dest;
nth->dest   = th->source;
nth->seq= htonl(ntohl(th->ack_seq));
@@ -229,8 +229,8 @@
 
niph = synproxy_build_ip(net, nskb, >saddr, >daddr);
 
-   skb_reset_transport_header(nskb);
nth = skb_put(nskb, tcp_hdr_size);
+   nskb->transport_header = (unsigned char *)nth - nskb->head;
nth->source = th->source;
nth->dest   = th->dest;
nth->seq= htonl(ntohl(th->seq) + 1);
-- 
1.8.3.1

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