| Message ID | 1383130201-6198-1-git-send-email-jiri@resnulli.us |
|---|---|
| State | Not Applicable |
| Headers | show |
Jiri Pirko <jiri@resnulli.us> wrote: > Currently, when ipv6 fragment goes through the netfilter, match > functions are called on them directly. This might cause match function > to fail. So benefit from the fact that nf_defrag_ipv6 constructs > reassembled skb for us and use this reassembled skb for matching. > > This patch fixes for example following situation: > On HOSTA do: > ip6tables -I INPUT -p icmpv6 -j DROP > ip6tables -I INPUT -p icmpv6 -m icmp6 --icmpv6-type 128 -j ACCEPT > > and on HOSTB you do: > ping6 HOSTA -s2000 (MTU is 1500) > > Incoming echo requests will be filtered out on HOSTA. This issue does > not occur with smaller packets than MTU (where fragmentation does not happen). [..] > diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c > index 44400c2..5421beb0 100644 > --- a/net/ipv6/netfilter/ip6_tables.c > +++ b/net/ipv6/netfilter/ip6_tables.c > @@ -328,6 +328,7 @@ ip6t_do_table(struct sk_buff *skb, > const struct xt_table_info *private; > struct xt_action_param acpar; > unsigned int addend; > + struct sk_buff *reasm = skb->nfct_reasm ? skb->nfct_reasm : skb; > > /* Initialization */ > indev = in ? in->name : nulldevname; > @@ -363,7 +364,7 @@ ip6t_do_table(struct sk_buff *skb, > > IP_NF_ASSERT(e); > acpar.thoff = 0; > - if (!ip6_packet_match(skb, indev, outdev, &e->ipv6, > + if (!ip6_packet_match(reasm, indev, outdev, &e->ipv6, > &acpar.thoff, &acpar.fragoff, &acpar.hotdrop)) { [..] This is a bit backwards, I think. - We gather frags - Then we invoke ip6t_do_table for each individual fragment So basically your patch is equivalent to for_each_frag( ) ip6t_do_table(reassembled_skb) Which makes no sense to me - why traverse the ruleset n times with the same packet? -- 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
Wed, Oct 30, 2013 at 02:41:00PM CET, fw@strlen.de wrote: >Jiri Pirko <jiri@resnulli.us> wrote: >> Currently, when ipv6 fragment goes through the netfilter, match >> functions are called on them directly. This might cause match function >> to fail. So benefit from the fact that nf_defrag_ipv6 constructs >> reassembled skb for us and use this reassembled skb for matching. >> >> This patch fixes for example following situation: >> On HOSTA do: >> ip6tables -I INPUT -p icmpv6 -j DROP >> ip6tables -I INPUT -p icmpv6 -m icmp6 --icmpv6-type 128 -j ACCEPT >> >> and on HOSTB you do: >> ping6 HOSTA -s2000 (MTU is 1500) >> >> Incoming echo requests will be filtered out on HOSTA. This issue does >> not occur with smaller packets than MTU (where fragmentation does not happen). > >[..] >> diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c >> index 44400c2..5421beb0 100644 >> --- a/net/ipv6/netfilter/ip6_tables.c >> +++ b/net/ipv6/netfilter/ip6_tables.c >> @@ -328,6 +328,7 @@ ip6t_do_table(struct sk_buff *skb, >> const struct xt_table_info *private; >> struct xt_action_param acpar; >> unsigned int addend; >> + struct sk_buff *reasm = skb->nfct_reasm ? skb->nfct_reasm : skb; >> >> /* Initialization */ >> indev = in ? in->name : nulldevname; >> @@ -363,7 +364,7 @@ ip6t_do_table(struct sk_buff *skb, >> >> IP_NF_ASSERT(e); >> acpar.thoff = 0; >> - if (!ip6_packet_match(skb, indev, outdev, &e->ipv6, >> + if (!ip6_packet_match(reasm, indev, outdev, &e->ipv6, >> &acpar.thoff, &acpar.fragoff, &acpar.hotdrop)) { > >[..] > >This is a bit backwards, I think. >- We gather frags >- Then we invoke ip6t_do_table for each individual fragment > >So basically your patch is equivalent to >for_each_frag( ) > ip6t_do_table(reassembled_skb) > >Which makes no sense to me - why traverse the ruleset n times with the same >packet? Because each fragment need to be pushed through separately. What different approach would you suggest? Thanks Jiri -- 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
Jiri Pirko <jiri@resnulli.us> wrote: > >This is a bit backwards, I think. > >- We gather frags > >- Then we invoke ip6t_do_table for each individual fragment > > > >So basically your patch is equivalent to > >for_each_frag( ) > > ip6t_do_table(reassembled_skb) > > > >Which makes no sense to me - why traverse the ruleset n times with the same > >packet? > > Because each fragment need to be pushed through separately. Why? AFAIU we only need to ensure that (in forwarding case) we send out the original fragments instead of the reassembled packet. > What different approach would you suggest? I am sure that current behaviour is intentional, so I'd first like to understand WHY this was implemented this way. Also, this would change very long standing behaviour so one might argue that this is a no-go anyway. What is the exact problem that this is supposed to solve? -- 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
Wed, Oct 30, 2013 at 03:44:00PM CET, fw@strlen.de wrote: >Jiri Pirko <jiri@resnulli.us> wrote: >> >This is a bit backwards, I think. >> >- We gather frags >> >- Then we invoke ip6t_do_table for each individual fragment >> > >> >So basically your patch is equivalent to >> >for_each_frag( ) >> > ip6t_do_table(reassembled_skb) >> > >> >Which makes no sense to me - why traverse the ruleset n times with the same >> >packet? >> >> Because each fragment need to be pushed through separately. > >Why? AFAIU we only need to ensure that (in forwarding case) we >send out the original fragments instead of the reassembled packet. I don't knot why, that's the way it is done now. From the top of my head I can't think of any scenario why it would hurt to push the reassebled packet instead (and of course send out original fragments at the end of the way for forwarding) > >> What different approach would you suggest? > >I am sure that current behaviour is intentional, so I'd first like to >understand WHY this was implemented this way. > >Also, this would change very long standing behaviour so one might argue that >this is a no-go anyway. Can you think aof any sane use case this change could possible break? > >What is the exact problem that this is supposed to solve? Look at the patch description. There's an example. The problem is that fragments are not correctly matched. -- 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
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c index 44400c2..5421beb0 100644 --- a/net/ipv6/netfilter/ip6_tables.c +++ b/net/ipv6/netfilter/ip6_tables.c @@ -328,6 +328,7 @@ ip6t_do_table(struct sk_buff *skb, const struct xt_table_info *private; struct xt_action_param acpar; unsigned int addend; + struct sk_buff *reasm = skb->nfct_reasm ? skb->nfct_reasm : skb; /* Initialization */ indev = in ? in->name : nulldevname; @@ -363,7 +364,7 @@ ip6t_do_table(struct sk_buff *skb, IP_NF_ASSERT(e); acpar.thoff = 0; - if (!ip6_packet_match(skb, indev, outdev, &e->ipv6, + if (!ip6_packet_match(reasm, indev, outdev, &e->ipv6, &acpar.thoff, &acpar.fragoff, &acpar.hotdrop)) { no_match: e = ip6t_next_entry(e); @@ -373,7 +374,7 @@ ip6t_do_table(struct sk_buff *skb, xt_ematch_foreach(ematch, e) { acpar.match = ematch->u.kernel.match; acpar.matchinfo = ematch->data; - if (!acpar.match->match(skb, &acpar)) + if (!acpar.match->match(reasm, &acpar)) goto no_match; }
Currently, when ipv6 fragment goes through the netfilter, match functions are called on them directly. This might cause match function to fail. So benefit from the fact that nf_defrag_ipv6 constructs reassembled skb for us and use this reassembled skb for matching. This patch fixes for example following situation: On HOSTA do: ip6tables -I INPUT -p icmpv6 -j DROP ip6tables -I INPUT -p icmpv6 -m icmp6 --icmpv6-type 128 -j ACCEPT and on HOSTB you do: ping6 HOSTA -s2000 (MTU is 1500) Incoming echo requests will be filtered out on HOSTA. This issue does not occur with smaller packets than MTU (where fragmentation does not happen). Signed-off-by: Jiri Pirko <jiri@resnulli.us> --- net/ipv6/netfilter/ip6_tables.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)