Re: tiny pf_route{,6} tweak
On Wed, Jan 27, 2021 at 11:14:51AM +1000, David Gwynne wrote:
> On Wed, Jan 27, 2021 at 11:13:12AM +1000, David Gwynne wrote:
> > when pf_route (and pf_route6) are supposed to handle forwarding the
> > packet (ie, for route-to or reply-to rules), they take the mbuf
> > away from the calling code path. this is done by clearing the mbuf
> > pointer in the pf_pdesc struct. it doesn't do this for dup-to rules
> > though.
> >
> > at the moment pf_route clears that pointer on the way out, but it could
> > take the mbuf away up front in the same place that it already checks if
> > it's a dup-to rule or not.
> >
> > it's a small change. i've bumped up the number of lines of context so
> > it's easier to read too.
> >
> > ok?
>
> sigh. here's the diff with the extra context.
Usually you want all information including the mbuf in pd.
Here it does not matter, pd->m is not accessed.
I see no advantage in changing, so I would leave it as it is.
If others think new code is better, I have no objections.
bluhm
> Index: pf.c
> ===
> RCS file: /cvs/src/sys/net/pf.c,v
> retrieving revision 1.1101
> diff -u -p -U8 -r1.1101 pf.c
> --- pf.c 19 Jan 2021 22:22:23 - 1.1101
> +++ pf.c 27 Jan 2021 01:10:52 -
> @@ -5983,16 +5983,17 @@ pf_route(struct pf_pdesc *pd, struct pf_
>
> if (r->rt == PF_DUPTO) {
> if ((m0 = m_dup_pkt(pd->m, max_linkhdr, M_NOWAIT)) == NULL)
> return;
> } else {
> if ((r->rt == PF_REPLYTO) == (r->direction == pd->dir))
> return;
> m0 = pd->m;
> + pd->m = NULL;
> }
>
> if (m0->m_len < sizeof(struct ip)) {
> DPFPRINTF(LOG_ERR,
> "%s: m0->m_len < sizeof(struct ip)", __func__);
> goto bad;
> }
>
> @@ -6103,18 +6104,16 @@ pf_route(struct pf_pdesc *pd, struct pf_
> else
> m_freem(m0);
> }
>
> if (error == 0)
> ipstat_inc(ips_fragmented);
>
> done:
> - if (r->rt != PF_DUPTO)
> - pd->m = NULL;
> rtfree(rt);
> return;
>
> bad:
> m_freem(m0);
> goto done;
> }
>
> @@ -6141,16 +6140,17 @@ pf_route6(struct pf_pdesc *pd, struct pf
>
> if (r->rt == PF_DUPTO) {
> if ((m0 = m_dup_pkt(pd->m, max_linkhdr, M_NOWAIT)) == NULL)
> return;
> } else {
> if ((r->rt == PF_REPLYTO) == (r->direction == pd->dir))
> return;
> m0 = pd->m;
> + pd->m = NULL;
> }
>
> if (m0->m_len < sizeof(struct ip6_hdr)) {
> DPFPRINTF(LOG_ERR,
> "%s: m0->m_len < sizeof(struct ip6_hdr)", __func__);
> goto bad;
> }
> ip6 = mtod(m0, struct ip6_hdr *);
> @@ -6232,18 +6232,16 @@ pf_route6(struct pf_pdesc *pd, struct pf
> ip6stat_inc(ip6s_cantfrag);
> if (r->rt != PF_DUPTO)
> pf_send_icmp(m0, ICMP6_PACKET_TOO_BIG, 0,
> ifp->if_mtu, pd->af, r, pd->rdomain);
> goto bad;
> }
>
> done:
> - if (r->rt != PF_DUPTO)
> - pd->m = NULL;
> rtfree(rt);
> return;
>
> bad:
> m_freem(m0);
> goto done;
> }
> #endif /* INET6 */
Re: [External] : tiny pf_route{,6} tweak
On Wed, Jan 27, 2021 at 11:13:12AM +1000, David Gwynne wrote:
> when pf_route (and pf_route6) are supposed to handle forwarding the
> packet (ie, for route-to or reply-to rules), they take the mbuf
> away from the calling code path. this is done by clearing the mbuf
> pointer in the pf_pdesc struct. it doesn't do this for dup-to rules
> though.
>
> at the moment pf_route clears that pointer on the way out, but it could
> take the mbuf away up front in the same place that it already checks if
> it's a dup-to rule or not.
>
> it's a small change. i've bumped up the number of lines of context so
> it's easier to read too.
>
> ok?
OK sashan@
>
> Index: pf.c
> ===
> RCS file: /cvs/src/sys/net/pf.c,v
> retrieving revision 1.1101
> diff -u -p -r1.1101 pf.c
> --- pf.c 19 Jan 2021 22:22:23 - 1.1101
> +++ pf.c 27 Jan 2021 01:05:29 -
> @@ -5988,6 +5988,7 @@ pf_route(struct pf_pdesc *pd, struct pf_
> if ((r->rt == PF_REPLYTO) == (r->direction == pd->dir))
> return;
> m0 = pd->m;
> + pd->m = NULL;
> }
>
> if (m0->m_len < sizeof(struct ip)) {
> @@ -6108,8 +6109,6 @@ pf_route(struct pf_pdesc *pd, struct pf_
> ipstat_inc(ips_fragmented);
>
> done:
> - if (r->rt != PF_DUPTO)
> - pd->m = NULL;
> rtfree(rt);
> return;
>
> @@ -6146,6 +6145,7 @@ pf_route6(struct pf_pdesc *pd, struct pf
> if ((r->rt == PF_REPLYTO) == (r->direction == pd->dir))
> return;
> m0 = pd->m;
> + pd->m = NULL;
> }
>
> if (m0->m_len < sizeof(struct ip6_hdr)) {
> @@ -6237,8 +6237,6 @@ pf_route6(struct pf_pdesc *pd, struct pf
> }
>
> done:
> - if (r->rt != PF_DUPTO)
> - pd->m = NULL;
> rtfree(rt);
> return;
>
>
>
Re: tiny pf_route{,6} tweak
On Wed, Jan 27, 2021 at 11:13:12AM +1000, David Gwynne wrote:
> when pf_route (and pf_route6) are supposed to handle forwarding the
> packet (ie, for route-to or reply-to rules), they take the mbuf
> away from the calling code path. this is done by clearing the mbuf
> pointer in the pf_pdesc struct. it doesn't do this for dup-to rules
> though.
>
> at the moment pf_route clears that pointer on the way out, but it could
> take the mbuf away up front in the same place that it already checks if
> it's a dup-to rule or not.
>
> it's a small change. i've bumped up the number of lines of context so
> it's easier to read too.
>
> ok?
sigh. here's the diff with the extra context.
Index: pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.1101
diff -u -p -U8 -r1.1101 pf.c
--- pf.c19 Jan 2021 22:22:23 - 1.1101
+++ pf.c27 Jan 2021 01:10:52 -
@@ -5983,16 +5983,17 @@ pf_route(struct pf_pdesc *pd, struct pf_
if (r->rt == PF_DUPTO) {
if ((m0 = m_dup_pkt(pd->m, max_linkhdr, M_NOWAIT)) == NULL)
return;
} else {
if ((r->rt == PF_REPLYTO) == (r->direction == pd->dir))
return;
m0 = pd->m;
+ pd->m = NULL;
}
if (m0->m_len < sizeof(struct ip)) {
DPFPRINTF(LOG_ERR,
"%s: m0->m_len < sizeof(struct ip)", __func__);
goto bad;
}
@@ -6103,18 +6104,16 @@ pf_route(struct pf_pdesc *pd, struct pf_
else
m_freem(m0);
}
if (error == 0)
ipstat_inc(ips_fragmented);
done:
- if (r->rt != PF_DUPTO)
- pd->m = NULL;
rtfree(rt);
return;
bad:
m_freem(m0);
goto done;
}
@@ -6141,16 +6140,17 @@ pf_route6(struct pf_pdesc *pd, struct pf
if (r->rt == PF_DUPTO) {
if ((m0 = m_dup_pkt(pd->m, max_linkhdr, M_NOWAIT)) == NULL)
return;
} else {
if ((r->rt == PF_REPLYTO) == (r->direction == pd->dir))
return;
m0 = pd->m;
+ pd->m = NULL;
}
if (m0->m_len < sizeof(struct ip6_hdr)) {
DPFPRINTF(LOG_ERR,
"%s: m0->m_len < sizeof(struct ip6_hdr)", __func__);
goto bad;
}
ip6 = mtod(m0, struct ip6_hdr *);
@@ -6232,18 +6232,16 @@ pf_route6(struct pf_pdesc *pd, struct pf
ip6stat_inc(ip6s_cantfrag);
if (r->rt != PF_DUPTO)
pf_send_icmp(m0, ICMP6_PACKET_TOO_BIG, 0,
ifp->if_mtu, pd->af, r, pd->rdomain);
goto bad;
}
done:
- if (r->rt != PF_DUPTO)
- pd->m = NULL;
rtfree(rt);
return;
bad:
m_freem(m0);
goto done;
}
#endif /* INET6 */
tiny pf_route{,6} tweak
when pf_route (and pf_route6) are supposed to handle forwarding the
packet (ie, for route-to or reply-to rules), they take the mbuf
away from the calling code path. this is done by clearing the mbuf
pointer in the pf_pdesc struct. it doesn't do this for dup-to rules
though.
at the moment pf_route clears that pointer on the way out, but it could
take the mbuf away up front in the same place that it already checks if
it's a dup-to rule or not.
it's a small change. i've bumped up the number of lines of context so
it's easier to read too.
ok?
Index: pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.1101
diff -u -p -r1.1101 pf.c
--- pf.c19 Jan 2021 22:22:23 - 1.1101
+++ pf.c27 Jan 2021 01:05:29 -
@@ -5988,6 +5988,7 @@ pf_route(struct pf_pdesc *pd, struct pf_
if ((r->rt == PF_REPLYTO) == (r->direction == pd->dir))
return;
m0 = pd->m;
+ pd->m = NULL;
}
if (m0->m_len < sizeof(struct ip)) {
@@ -6108,8 +6109,6 @@ pf_route(struct pf_pdesc *pd, struct pf_
ipstat_inc(ips_fragmented);
done:
- if (r->rt != PF_DUPTO)
- pd->m = NULL;
rtfree(rt);
return;
@@ -6146,6 +6145,7 @@ pf_route6(struct pf_pdesc *pd, struct pf
if ((r->rt == PF_REPLYTO) == (r->direction == pd->dir))
return;
m0 = pd->m;
+ pd->m = NULL;
}
if (m0->m_len < sizeof(struct ip6_hdr)) {
@@ -6237,8 +6237,6 @@ pf_route6(struct pf_pdesc *pd, struct pf
}
done:
- if (r->rt != PF_DUPTO)
- pd->m = NULL;
rtfree(rt);
return;
