| Message ID | 1479994128-16654-1-git-send-email-zlpnobody@163.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
Liping Zhang <zlpnobody@163.com> wrote: > In general, we haven't do routing lookup in PREROUTING hook, so it's > very likely that fib4/6_is_local will not be met. loopback packets retain skb->dst (and thats what this test is about). > Then the *dest will > be set to 0 because we do nothing when the fib result is RTN_LOCAL. Yes. > So if the user want to drop all packets which cannot be routed, > and input the following nft rule: > # nft add rule filter prerouting fib daddr oif eq 0 drop > > Then all the packets which destinate to local will be dropped > incorrectly. but in "saddr oif eq 0 drop" case they really should have no oif, the address should not be considered routeable. Pablo, please don't apply this; I would like to look at this next week. Msybe this needs a check if we're testing daddr or saddr. -- 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
Hi Florian, At 2016-11-24 21:50:14, "Florian Westphal" <fw@strlen.de> wrote: >Liping Zhang <zlpnobody@163.com> wrote: >> In general, we haven't do routing lookup in PREROUTING hook, so it's >> very likely that fib4/6_is_local will not be met. > >loopback packets retain skb->dst (and thats what this test is about). Yes, so I use the words "very likely" :) [...] >but in "saddr oif eq 0 drop" case they really should have no oif, the >address should not be considered routeable. Yes, I read the ipt_rpfilter.c's source codes, and I find that there's a test flag XT_RPFILTER_ACCEPT_LOCAL, so I guess your initial intention is (just my guess, maybe I'm wrong): 0 - no route 1 - local route others - routing oif > >Pablo, please don't apply this; I would like to look at this next week. > >Msybe this needs a check if we're testing daddr or saddr.
Liping Zhang <zlpnobody@163.com> wrote: > At 2016-11-24 21:50:14, "Florian Westphal" <fw@strlen.de> wrote: > >Liping Zhang <zlpnobody@163.com> wrote: > >> In general, we haven't do routing lookup in PREROUTING hook, so it's > >> very likely that fib4/6_is_local will not be met. [..] > Yes, so I use the words "very likely" :) > [...] > >but in "saddr oif eq 0 drop" case they really should have no oif, the > >address should not be considered routeable. > > Yes, I read the ipt_rpfilter.c's source codes, and I find that there's a test flag > XT_RPFILTER_ACCEPT_LOCAL, so I guess your initial intention is (just my > guess, maybe I'm wrong): > 0 - no route > 1 - local route > others - routing oif Yes, thats right. "1" should only appear if lookup-up address is configured on this machine. For saddr, I don't think its good idea, because it will pass oif ne 0 accept For ACCEPT_LOCAL i think its easier to combine this with the addrtype check of just add explicit accept rules that make it bypass nft_fib rule. What do you think? I agree that for your prerouting daddr example 0 makes no sense and 1 would indeed be a better option. -- 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
At 2016-11-24 22:48:59, "Florian Westphal" <fw@strlen.de> wrote: >Liping Zhang <zlpnobody@163.com> wrote: [...] >"1" should only appear if lookup-up address is configured on this machine. >For saddr, I don't think its good idea, because it will pass > >oif ne 0 accept Yes, my patch will break this. > >For ACCEPT_LOCAL i think its easier to combine this with the addrtype >check of just add explicit accept rules that make it bypass nft_fib >rule. Yes, combine this with addrtype will be easier. My first thought was that we can also use "fib saddr oif eq 1" to simulate the ACCECPT_LOCAL, but I'm wrong, it will become more complicated. > >What do you think? > >I agree that for your prerouting daddr example 0 makes no sense and 1 >would indeed be a better option. >
Liping Zhang <zlpnobody@163.com> wrote: > From: Liping Zhang <zlpnobody@gmail.com> > > In general, we haven't do routing lookup in PREROUTING hook, so it's > very likely that fib4/6_is_local will not be met. Then the *dest will > be set to 0 because we do nothing when the fib result is RTN_LOCAL. > > So if the user want to drop all packets which cannot be routed, > and input the following nft rule: > # nft add rule filter prerouting fib daddr oif eq 0 drop > > Then all the packets which destinate to local will be dropped > incorrectly. > > Fixes: f6d0cbcf09c5 ("netfilter: nf_tables: add fib expression") > Signed-off-by: Liping Zhang <zlpnobody@gmail.com> > --- > net/ipv4/netfilter/nft_fib_ipv4.c | 3 ++- > net/ipv6/netfilter/nft_fib_ipv6.c | 8 ++++++-- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/netfilter/nft_fib_ipv4.c b/net/ipv4/netfilter/nft_fib_ipv4.c > index 2581363..2107775 100644 > --- a/net/ipv4/netfilter/nft_fib_ipv4.c > +++ b/net/ipv4/netfilter/nft_fib_ipv4.c > @@ -130,7 +130,8 @@ void nft_fib4_eval(const struct nft_expr *expr, struct nft_regs *regs, > switch (res.type) { > case RTN_UNICAST: > break; > - case RTN_LOCAL: /* should not appear here, see fib4_is_local() above */ > + case RTN_LOCAL: > + nft_fib_store_result(dest, priv->result, pkt, LOOPBACK_IFINDEX); Liping, what about doing: case RTN_LOCAL: if (priv->flags & NFTA_FIB_F_DADDR) nft_fib_store_result(dest, priv->result, pkt, LOOPBACK_IFINDEX); AFAICS this will make above rule work while the saddr test will still appear to not have a route at all. What do you think? -- 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
Hi Florian, 2016-11-28 20:25 GMT+08:00 Florian Westphal <fw@strlen.de>: [...] >> diff --git a/net/ipv4/netfilter/nft_fib_ipv4.c b/net/ipv4/netfilter/nft_fib_ipv4.c >> index 2581363..2107775 100644 >> --- a/net/ipv4/netfilter/nft_fib_ipv4.c >> +++ b/net/ipv4/netfilter/nft_fib_ipv4.c >> @@ -130,7 +130,8 @@ void nft_fib4_eval(const struct nft_expr *expr, struct nft_regs *regs, >> switch (res.type) { >> case RTN_UNICAST: >> break; >> - case RTN_LOCAL: /* should not appear here, see fib4_is_local() above */ >> + case RTN_LOCAL: >> + nft_fib_store_result(dest, priv->result, pkt, LOOPBACK_IFINDEX); > > Liping, what about doing: > > case RTN_LOCAL: > if (priv->flags & NFTA_FIB_F_DADDR) > nft_fib_store_result(dest, priv->result, pkt, LOOPBACK_IFINDEX); > > > AFAICS this will make above rule work while the saddr test will > still appear to not have a route at all. > > What do you think? Yes, this will work both for *rpfilter* and my user case. It seems a little ugly but I cannot find a better solution now ... -- 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/ipv4/netfilter/nft_fib_ipv4.c b/net/ipv4/netfilter/nft_fib_ipv4.c index 2581363..2107775 100644 --- a/net/ipv4/netfilter/nft_fib_ipv4.c +++ b/net/ipv4/netfilter/nft_fib_ipv4.c @@ -130,7 +130,8 @@ void nft_fib4_eval(const struct nft_expr *expr, struct nft_regs *regs, switch (res.type) { case RTN_UNICAST: break; - case RTN_LOCAL: /* should not appear here, see fib4_is_local() above */ + case RTN_LOCAL: + nft_fib_store_result(dest, priv->result, pkt, LOOPBACK_IFINDEX); return; default: break; diff --git a/net/ipv6/netfilter/nft_fib_ipv6.c b/net/ipv6/netfilter/nft_fib_ipv6.c index c947aad..5e2de1b 100644 --- a/net/ipv6/netfilter/nft_fib_ipv6.c +++ b/net/ipv6/netfilter/nft_fib_ipv6.c @@ -175,8 +175,12 @@ void nft_fib6_eval(const struct nft_expr *expr, struct nft_regs *regs, if (rt->dst.error) goto put_rt_err; - /* Should not see RTF_LOCAL here */ - if (rt->rt6i_flags & (RTF_REJECT | RTF_ANYCAST | RTF_LOCAL)) + if (rt->rt6i_flags & RTF_LOCAL) { + nft_fib_store_result(dest, priv->result, pkt, LOOPBACK_IFINDEX); + goto put_rt_err; + } + + if (rt->rt6i_flags & (RTF_REJECT | RTF_ANYCAST)) goto put_rt_err; if (oif && oif != rt->rt6i_idev->dev) {