On Fri, Jan 29, 2021 at 03:23:31PM +0100, Alexander Bluhm wrote:
> On Fri, Jan 29, 2021 at 10:53:09AM +1000, David Gwynne wrote:
> > > Are you sure that it does not break any use case? I have seen so
> > > much strange stuff. What is the advantage?
> >
> > The current behaviour is lucky at best, and quirky at worst. Usually I
> > would agree with you that breaking stuff isn't great, even if it's
> > wrong, but while I'm changing how route-to etc works I think it's
> > a good chance to clean up some of these edge cases.
>
> I have been developping products based on pf edge cases for 15
> years. I don't know which dragons are in our codebase. This should
> not prevent improvements in OpenBSD. I am just asking not to remove
> anything just because we currently don't know, how it can be used.
i understand.
> Changing syntax like address@interface can easily be adpted. Slight
> semantic changes may cause debugging sessions on productive customer
> systems. And then we might need a quick new solution for a previously
> existing feature. So please be careful.
the regress tests i just updated made it clear that using route-to with
loopback interfaces was not supposed to work. i think blacklisting
RTF_LOCAL routes is in keeping with that idea, and would go a bit
further and drop packets going out IFF_LOOPBACK interfaces too. i can't
think of a good edge case that this would break.
Index: pf.c
===================================================================
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.1107
diff -u -p -r1.1107 pf.c
--- pf.c 3 Feb 2021 07:41:12 -0000 1.1107
+++ pf.c 4 Feb 2021 22:43:11 -0000
@@ -6015,7 +6015,7 @@ pf_route(struct pf_pdesc *pd, struct pf_
rtableid = m0->m_pkthdr.ph_rtableid;
rt = rtalloc(sintosa(dst), RT_RESOLVE, rtableid);
- if (!rtisvalid(rt)) {
+ if (!rtisvalid(rt) || ISSET(rt->rt_flags, RTF_LOCAL)) {
if (s->rt != PF_DUPTO) {
pf_send_icmp(m0, ICMP_UNREACH, ICMP_UNREACH_HOST,
0, pd->af, s->rule.ptr, pd->rdomain);
@@ -6025,7 +6025,7 @@ pf_route(struct pf_pdesc *pd, struct pf_
}
ifp = if_get(rt->rt_ifidx);
- if (ifp == NULL)
+ if (ifp == NULL || ISSET(ifp->if_flags, IFF_LOOPBACK))
goto bad;
/* A locally generated packet may have invalid source address. */
@@ -6159,7 +6159,7 @@ pf_route6(struct pf_pdesc *pd, struct pf
if (IN6_IS_SCOPE_EMBED(&dst->sin6_addr))
dst->sin6_addr.s6_addr16[1] = htons(ifp->if_index);
rt = rtalloc(sin6tosa(dst), RT_RESOLVE, rtableid);
- if (!rtisvalid(rt)) {
+ if (!rtisvalid(rt) || ISSET(rt->rt_flags, RTF_LOCAL)) {
if (s->rt != PF_DUPTO) {
pf_send_icmp(m0, ICMP6_DST_UNREACH,
ICMP6_DST_UNREACH_NOROUTE, 0,
@@ -6170,7 +6170,7 @@ pf_route6(struct pf_pdesc *pd, struct pf
}
ifp = if_get(rt->rt_ifidx);
- if (ifp == NULL)
+ if (ifp == NULL || ISSET(ifp->if_flags, IFF_LOOPBACK))
goto bad;
/* A locally generated packet may have invalid source address. */
>
