Date: Thu, 19 Oct 2023 22:03:02 +0200 From: Kristof Provost <kp@FreeBSD.org> To: Gleb Smirnoff <glebius@freebsd.org> Cc: Igor Ostapenko <pm@igoro.pro>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: fabf705f4b5a - main - pf: fix pf divert-to loop Message-ID: <86D6E74C-3062-4718-816E-07CBA4F2903F@FreeBSD.org> In-Reply-To: <ZTFV74-C4amIm0ru@FreeBSD.org> References: <202310191237.39JCbdXp094554@gitrepo.freebsd.org> <ZTFV74-C4amIm0ru@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--=_MailMate_C0B0683C-C6CE-412C-88A4-DEEE8300C497_=
Content-Type: text/plain; format=flowed
On 19 Oct 2023, at 18:14, Gleb Smirnoff wrote:
> On Thu, Oct 19, 2023 at 12:37:39PM +0000, Kristof Provost wrote:
> K> +++ b/sys/netinet/ip_var.h
> ...
> K> +/* pf specific mtag for divert(4) support */
> K> +enum { PF_DIVERT_MTAG_DIR_IN=1, PF_DIVERT_MTAG_DIR_OUT=2 };
> K> +struct pf_divert_mtag {
> K> + uint16_t idir; // initial pkt direction
> K> + uint16_t ndir; // a) divert(4) port upon initial diversion
> K> + // b) new direction upon pkt re-enter
> K> +};
>
> This can be written as:
>
> typedef enum {
> PF_DIVERT_MTAG_DIR_IN = 1,
> PF_DIVERT_MTAG_DIR_OUT = 2,
> } pf_mtag_dir;
> struct pf_divert_mtag {
> pf_mtag_dir idir; /* Initial packet direction. */
> union {
> pf_mtag_dir ndir; /* New direction after re-enter.
> */
> uint16_t port; /* Initial divert(4) port. */
> };
> };
>
> The benefit is that in the debugger you will see PF_DIVERT_MTAG_DIR_IN
> instead
> of 1 when looking at a structure. And compilation time failure if
> anybody sets
> it to a wrong value. Using "port" instead of "ndir" when assigning a
> port
> improves readability of code.
>
> This will grow structure from 4 bytes to 8, as enum is always an int.
> However,
> uma allocator backing m_tag_alloc() will allocate same amount of
> memory.
>
Something like this?
diff --git a/sys/netinet/ip_divert.c b/sys/netinet/ip_divert.c
index ad95a1ce0d76..78ca36fc2a0f 100644
--- a/sys/netinet/ip_divert.c
+++ b/sys/netinet/ip_divert.c
@@ -182,7 +182,7 @@ divert_packet(struct mbuf *m, bool incoming)
(((struct ipfw_rule_ref *)(mtag+1))->info));
} else if ((mtag = m_tag_locate(m, MTAG_PF_DIVERT, 0, NULL)) !=
NULL) {
cookie = ((struct pf_divert_mtag *)(mtag+1))->idir;
- nport = htons(((struct pf_divert_mtag
*)(mtag+1))->ndir);
+ nport = htons(((struct pf_divert_mtag
*)(mtag+1))->port);
} else {
m_freem(m);
return;
diff --git a/sys/netinet/ip_var.h b/sys/netinet/ip_var.h
index a8c687682af9..0f3facc54d4e 100644
--- a/sys/netinet/ip_var.h
+++ b/sys/netinet/ip_var.h
@@ -328,11 +328,17 @@ extern int (*ip_dn_ctl_ptr)(struct sockopt
*);
extern int (*ip_dn_io_ptr)(struct mbuf **, struct ip_fw_args *);
/* pf specific mtag for divert(4) support */
-enum { PF_DIVERT_MTAG_DIR_IN=1, PF_DIVERT_MTAG_DIR_OUT=2 };
+__enum_uint8_decl(pf_mtag_dir) {
+ PF_DIVERT_MTAG_DIR_IN = 1,
+ PF_DIVERT_MTAG_DIR_OUT = 2
+};
struct pf_divert_mtag {
uint16_t idir; // initial pkt direction
- uint16_t ndir; // a) divert(4) port upon initial diversion
- // b) new direction upon pkt re-enter
+ union {
+ __enum_uint8(pf_mtag_dir) ndir; // a) divert(4) port
upon initial diversion
+ // b) new direction upon pkt re-enter
+ uint16_t port; /* Initial divert(4) port */
+ };
};
#define MTAG_PF_DIVERT 1262273569
diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index a6c7ee359416..1cd8412193dc 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -8022,7 +8022,7 @@ pf_test(int dir, int pflags, struct ifnet *ifp,
struct mbuf **m0,
mtag = m_tag_alloc(MTAG_PF_DIVERT, 0,
sizeof(struct pf_divert_mtag), M_NOWAIT | M_ZERO);
if (mtag != NULL) {
- ((struct pf_divert_mtag *)(mtag+1))->ndir =
+ ((struct pf_divert_mtag *)(mtag+1))->port =
ntohs(r->divert.port);
((struct pf_divert_mtag *)(mtag+1))->idir =
(dir == PF_IN) ? PF_DIVERT_MTAG_DIR_IN :
Best regards,
Kristof
--=_MailMate_C0B0683C-C6CE-412C-88A4-DEEE8300C497_=
Content-Type: text/html
Content-Transfer-Encoding: quoted-printable
<!DOCTYPE html>
<html>
<head>
<meta http-equiv=3D"Content-Type" content=3D"text/xhtml; charset=3Dutf-8"=
>
</head>
<body><div style=3D"font-family: sans-serif;"><div class=3D"markdown" sty=
le=3D"white-space: normal;">
<p dir=3D"auto">On 19 Oct 2023, at 18:14, Gleb Smirnoff wrote:</p>
</div><div class=3D"plaintext" style=3D"white-space: normal;"><blockquote=
style=3D"margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #136=
BCE; color: #136BCE;"><p dir=3D"auto">On Thu, Oct 19, 2023 at 12:37:39PM =
+0000, Kristof Provost wrote:
<br>
K> +++ b/sys/netinet/ip_var.h
<br>
=2E..
<br>
K> +/* pf specific mtag for divert(4) support */
<br>
K> +enum { PF_DIVERT_MTAG_DIR_IN=3D1, PF_DIVERT_MTAG_DIR_OUT=3D2 };
<br>
K> +struct pf_divert_mtag {
<br>
K> + uint16_t idir; // initial pkt direction
<br>
K> + uint16_t ndir; // a) divert(4) port upon initial diversion
<br>
K> + // b) new direction upon pkt re-enter
<br>
K> +};</p>
<p dir=3D"auto">This can be written as:</p>
<p dir=3D"auto">typedef enum {
<br>
PF_DIVERT_MTAG_DIR_IN =3D 1,
<br>
PF_DIVERT_MTAG_DIR_OUT =3D 2,
<br>
} pf_mtag_dir;
<br>
struct pf_divert_mtag {
<br>
pf_mtag_dir idir; /* Initial packet direction. */
<br>
union {
<br>
pf_mtag_dir ndir; /* New direction after re-enter. */=
<br>
uint16_t port; /* Initial divert(4) port. */
<br>
};
<br>
};</p>
<p dir=3D"auto">The benefit is that in the debugger you will see PF_DIVER=
T_MTAG_DIR_IN instead
<br>
of 1 when looking at a structure. And compilation time failure if anybody=
sets
<br>
it to a wrong value. Using "port" instead of "ndir" when assigning a port=
<br>
improves readability of code.</p>
<p dir=3D"auto">This will grow structure from 4 bytes to 8, as enum is al=
ways an int. However,
<br>
uma allocator backing m_tag_alloc() will allocate same amount of memory.<=
/p>
<br></blockquote></div>
<div class=3D"markdown" style=3D"white-space: normal;">
<p dir=3D"auto">Something like this?</p>
<pre style=3D"margin-left: 15px; margin-right: 15px; padding: 5px; border=
: thin solid gray; overflow-x: auto; max-width: 90vw; background-color: #=
E4E4E4;"><code style=3D"padding: 0 0.25em; background-color: #E4E4E4;">di=
ff --git a/sys/netinet/ip_divert.c b/sys/netinet/ip_divert.c
index ad95a1ce0d76..78ca36fc2a0f 100644
--- a/sys/netinet/ip_divert.c
+++ b/sys/netinet/ip_divert.c
@@ -182,7 +182,7 @@ divert_packet(struct mbuf *m, bool incoming)
(((struct ipfw_rule_ref *)(mtag+1))->info));
} else if ((mtag =3D m_tag_locate(m, MTAG_PF_DIVERT, 0, NULL)) !=3D=
NULL) {
cookie =3D ((struct pf_divert_mtag *)(mtag+1))->idir;
- nport =3D htons(((struct pf_divert_mtag *)(mtag+1))->n=
dir);
+ nport =3D htons(((struct pf_divert_mtag *)(mtag+1))->p=
ort);
} else {
m_freem(m);
return;
diff --git a/sys/netinet/ip_var.h b/sys/netinet/ip_var.h
index a8c687682af9..0f3facc54d4e 100644
--- a/sys/netinet/ip_var.h
+++ b/sys/netinet/ip_var.h
@@ -328,11 +328,17 @@ extern int (*ip_dn_ctl_ptr)(struct sockopt *=
);
extern int (*ip_dn_io_ptr)(struct mbuf **, struct ip_fw_args *);
/* pf specific mtag for divert(4) support */
-enum { PF_DIVERT_MTAG_DIR_IN=3D1, PF_DIVERT_MTAG_DIR_OUT=3D2 };
+__enum_uint8_decl(pf_mtag_dir) {
+ PF_DIVERT_MTAG_DIR_IN =3D 1,
+ PF_DIVERT_MTAG_DIR_OUT =3D 2
+};
struct pf_divert_mtag {
uint16_t idir; // initial pkt direction
- uint16_t ndir; // a) divert(4) port upon initial diversion
- // b) new direction upon pkt re-enter
+ union {
+ __enum_uint8(pf_mtag_dir) ndir; // a) divert(4) port upon=
initial diversion
+ // b) new direction upon pkt re-enter
+ uint16_t port; /* Initial divert(4) port */
+ };
};
#define MTAG_PF_DIVERT 1262273569
diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index a6c7ee359416..1cd8412193dc 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -8022,7 +8022,7 @@ pf_test(int dir, int pflags, struct ifnet *ifp, str=
uct mbuf **m0,
mtag =3D m_tag_alloc(MTAG_PF_DIVERT, 0,
sizeof(struct pf_divert_mtag), M_NOWAIT | M_ZERO);
if (mtag !=3D NULL) {
- ((struct pf_divert_mtag *)(mtag+1))->ndir =3D
+ ((struct pf_divert_mtag *)(mtag+1))->port =3D
ntohs(r->divert.port);
((struct pf_divert_mtag *)(mtag+1))->idir =3D
(dir =3D=3D PF_IN) ? PF_DIVERT_MTAG_DIR_IN :
</code></pre>
<p dir=3D"auto">Best regards,<br>
Kristof</p>
</div>
</div>
</body>
</html>
--=_MailMate_C0B0683C-C6CE-412C-88A4-DEEE8300C497_=--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?86D6E74C-3062-4718-816E-07CBA4F2903F>