git: 5f81426831a6 - main - ipfw: refactor macros around m_pullup()
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Fri, 18 Sep 2026 22:50:28 UTC
The branch main has been updated by glebius:
URL: https://cgit.FreeBSD.org/src/commit/?id=5f81426831a6cc682b87ae85a7e3a39879285260
commit 5f81426831a6cc682b87ae85a7e3a39879285260
Author: Gleb Smirnoff <glebius@FreeBSD.org>
AuthorDate: 2026-09-18 22:45:01 +0000
Commit: Gleb Smirnoff <glebius@FreeBSD.org>
CommitDate: 2026-09-18 22:50:23 +0000
ipfw: refactor macros around m_pullup()
In the prologue, where we check if the argument is a memory or an mbuf
chain, do not set 'struct ip *ip' pointer. However, set the 'struct
ether_header *eh' pointer there and set Etherner header length in 'ehlen'.
Side effect of this refactor is that now Layer 2 hooks may send mbufs with
ETHERTYPE_VLAN frames. However, current network stack doesn't do that.
Write a new PULLUP() macro that would take type of the argument to
determine how much to pull. Unlike PULLUP_TO() this macro can take typed
pointer. This will allow to get rid of 'void *ulp' and bunch of casting
macros in the next change. Use local bool variable to see if we need to
unlock upon jump to pullup_failed. Embed pointer update into the branch
of the macro, where pointers indeed need an update.
Use new PULLUP() macro to pullup initial 'struct ip *ip' and 'struct
ip6_hdr *ip6'. This removes max_protohdr sized pullup, that previously
tried to pull more than an unmapped mbuf could yield, fixing a bug covered
by the testcase sys/netpfil/ipfw/unmapped.
Reviewed by: gallatin
Differential Revision: https://reviews.freebsd.org/D59426
---
sys/netpfil/ipfw/ip_fw2.c | 180 ++++++++++++++++++++--------------------------
1 file changed, 77 insertions(+), 103 deletions(-)
diff --git a/sys/netpfil/ipfw/ip_fw2.c b/sys/netpfil/ipfw/ip_fw2.c
index 133aeeb969a6..b56e4e8ac2d1 100644
--- a/sys/netpfil/ipfw/ip_fw2.c
+++ b/sys/netpfil/ipfw/ip_fw2.c
@@ -1463,6 +1463,7 @@ ipfw_chk(struct ip_fw_args *args)
*/
u_short offset = 0;
u_short ip6f_mf = 0;
+ u_short ehlen;
/*
* Local copies of addresses. They are only valid if we have
@@ -1508,104 +1509,79 @@ ipfw_chk(struct ip_fw_args *args)
int done = 0; /* flag to exit the outer loop */
IPFW_RLOCK_TRACKER;
+ bool locked = false;
bool mem;
bool need_send_reject = false;
int reject_code;
uint16_t reject_mtu;
if ((mem = (args->flags & IPFW_ARGS_LENMASK))) {
- if (args->flags & IPFW_ARGS_ETHER) {
- eh = (struct ether_header *)args->mem;
- if (eh->ether_type == htons(ETHERTYPE_VLAN))
- ip = (struct ip *)
- ((struct ether_vlan_header *)eh + 1);
- else
- ip = (struct ip *)(eh + 1);
- } else {
- eh = NULL;
- ip = (struct ip *)args->mem;
- }
+ eh = args->flags & IPFW_ARGS_ETHER ? args->mem : NULL;
pktlen = IPFW_ARGS_LENGTH(args->flags);
args->f_id.fib = args->ifp->if_fib; /* best guess */
} else {
m = args->m;
if (m->m_flags & M_SKIP_FIREWALL || (! V_ipfw_vnet_ready))
return (IP_FW_PASS); /* accept */
- if (args->flags & IPFW_ARGS_ETHER) {
- /* We need some amount of data to be contiguous. */
- if (m->m_len < min(m->m_pkthdr.len, max_protohdr) &&
- (args->m = m = m_pullup(m, min(m->m_pkthdr.len,
- max_protohdr))) == NULL)
- goto pullup_failed;
- eh = mtod(m, struct ether_header *);
- ip = (struct ip *)(eh + 1);
- } else {
- eh = NULL;
- ip = mtod(m, struct ip *);
- }
+ eh = args->flags & IPFW_ARGS_ETHER ?
+ mtod(m, struct ether_header *) : NULL;
pktlen = m->m_pkthdr.len;
args->f_id.fib = M_GETFIB(m); /* mbuf not altered */
}
+ if (eh != NULL) {
+ if (eh->ether_type == htons(ETHERTYPE_VLAN))
+ ehlen = sizeof(struct ether_vlan_header);
+ else
+ ehlen = sizeof(struct ether_header);
+ } else
+ ehlen = 0;
+
dst_ip.s_addr = 0; /* make sure it is initialized */
src_ip.s_addr = 0; /* make sure it is initialized */
src_port = dst_port = 0;
DYN_INFO_INIT(&dyn_info);
/*
- * PULLUP_TO(len, p, T) makes sure that len + sizeof(T) is contiguous,
- * then it sets p to point at the offset "len" in the mbuf. WARNING: the
- * pointer might become stale after other pullups (but we never use it
- * this way).
+ * PULLUP(p) makes typed pointer 'p' point at contiguous memory sized to
+ * its type, that is offset 'hlen' bytes from the beginning of the packet
+ * The macro reads variables 'hlen', 'ehlen' and 'm' or 'args->mem' in the
+ * current scope. If the macro does m_pullup(), the following current scope
+ * variables will be updated: 'm', 'args->m', 'eh', 'ip'.
*/
-#define PULLUP_TO(_len, p, T) PULLUP_LEN(_len, p, sizeof(T))
-#define EHLEN (eh != NULL ? ((char *)ip - (char *)eh) : 0)
-#define _PULLUP_LOCKED(_len, p, T, unlock) \
-do { \
- int x = (_len) + T + EHLEN; \
- if (mem) { \
- if (__predict_false(pktlen < x)) { \
- unlock; \
- goto pullup_failed; \
- } \
- p = (char *)args->mem + (_len) + EHLEN; \
- } else { \
- if (__predict_false((m)->m_len < x)) { \
- args->m = m = m_pullup(m, x); \
- if (m == NULL) { \
- unlock; \
- goto pullup_failed; \
- } \
- } \
- p = mtod(m, char *) + (_len) + EHLEN; \
- } \
+#define PULLUP(_p) PULLUP_LEN((_p), sizeof(*(_p)))
+#define PULLUP_TO(_p, _T) PULLUP_LEN((_p), sizeof(_T))
+#define PULLUP_LEN(_p, _size) \
+do { \
+ int _max = hlen + ehlen + (_size); \
+ if (mem) { \
+ if (__predict_false(pktlen < _max)) \
+ goto pullup_failed; \
+ (_p) = (__typeof(_p))((char *)args->mem + hlen + ehlen);\
+ } else { \
+ if (__predict_false(m->m_len < _max)) { \
+ args->m = m = m_pullup(m, _max); \
+ if (m == NULL) \
+ goto pullup_failed; \
+ if (eh != NULL) { \
+ eh = mtod(m, struct ether_header *); \
+ ip = (struct ip *)(eh + 1); \
+ } else \
+ ip = mtod(m, struct ip *); \
+ } \
+ (_p) = (__typeof(_p))(mtod(m, char *) + hlen + ehlen); \
+ } \
} while (0)
-#define PULLUP_LEN(_len, p, T) _PULLUP_LOCKED(_len, p, T, )
-#define PULLUP_LEN_LOCKED(_len, p, T) \
- _PULLUP_LOCKED(_len, p, T, IPFW_PF_RUNLOCK(chain)); \
- UPDATE_POINTERS()
-/*
- * In case pointers got stale after pullups, update them.
- */
-#define UPDATE_POINTERS() \
-do { \
- if (!mem) { \
- if (eh != NULL) { \
- eh = mtod(m, struct ether_header *); \
- ip = (struct ip *)(eh + 1); \
- } else \
- ip = mtod(m, struct ip *); \
- args->m = m; \
- } \
-} while (0)
+ PULLUP(ip);
/* Identify IP packets and fill up variables. */
if (pktlen >= sizeof(struct ip6_hdr) &&
(eh == NULL || eh->ether_type == htons(ETHERTYPE_IPV6)) &&
ip->ip_v == 6) {
- struct ip6_hdr *ip6 = (struct ip6_hdr *)ip;
+ struct ip6_hdr *ip6;
+ PULLUP(ip6);
is_ipv6 = 1;
args->flags |= IPFW_ARGS_IP6;
hlen = sizeof(struct ip6_hdr);
@@ -1614,14 +1590,14 @@ do { \
while (ulp == NULL && offset == 0) {
switch (proto) {
case IPPROTO_ICMPV6:
- PULLUP_TO(hlen, ulp, struct icmp6_hdr);
+ PULLUP_TO(ulp, struct icmp6_hdr);
#ifdef INET6
icmp6_type = ICMP6(ulp)->icmp6_type;
#endif
break;
case IPPROTO_TCP:
- PULLUP_TO(hlen, ulp, struct tcphdr);
+ PULLUP_TO(ulp, struct tcphdr);
dst_port = TCP(ulp)->th_dport;
src_port = TCP(ulp)->th_sport;
/* save flags for dynamic rules */
@@ -1632,28 +1608,27 @@ do { \
if (pktlen >= hlen + sizeof(struct sctphdr) +
sizeof(struct sctp_chunkhdr) +
offsetof(struct sctp_init, a_rwnd))
- PULLUP_LEN(hlen, ulp,
+ PULLUP_LEN(ulp,
sizeof(struct sctphdr) +
sizeof(struct sctp_chunkhdr) +
offsetof(struct sctp_init, a_rwnd));
else if (pktlen >= hlen + sizeof(struct sctphdr))
- PULLUP_LEN(hlen, ulp, pktlen - hlen);
+ PULLUP_LEN(ulp, pktlen - hlen);
else
- PULLUP_LEN(hlen, ulp,
- sizeof(struct sctphdr));
+ PULLUP_LEN(ulp, sizeof(struct sctphdr));
src_port = SCTP(ulp)->src_port;
dst_port = SCTP(ulp)->dest_port;
break;
case IPPROTO_UDP:
case IPPROTO_UDPLITE:
- PULLUP_TO(hlen, ulp, struct udphdr);
+ PULLUP_TO(ulp, struct udphdr);
dst_port = UDP(ulp)->uh_dport;
src_port = UDP(ulp)->uh_sport;
break;
case IPPROTO_HOPOPTS: /* RFC 2460 */
- PULLUP_TO(hlen, ulp, struct ip6_hbh);
+ PULLUP_TO(ulp, struct ip6_hbh);
ext_hd |= EXT_HOPOPTS;
hlen += (((struct ip6_hbh *)ulp)->ip6h_len + 1) << 3;
proto = ((struct ip6_hbh *)ulp)->ip6h_nxt;
@@ -1661,7 +1636,7 @@ do { \
break;
case IPPROTO_ROUTING: /* RFC 2460 */
- PULLUP_TO(hlen, ulp, struct ip6_rthdr);
+ PULLUP_TO(ulp, struct ip6_rthdr);
switch (((struct ip6_rthdr *)ulp)->ip6r_type) {
case 0:
ext_hd |= EXT_RTHDR0;
@@ -1686,7 +1661,7 @@ do { \
break;
case IPPROTO_FRAGMENT: /* RFC 2460 */
- PULLUP_TO(hlen, ulp, struct ip6_frag);
+ PULLUP_TO(ulp, struct ip6_frag);
ext_hd |= EXT_FRAGMENT;
hlen += sizeof (struct ip6_frag);
proto = ((struct ip6_frag *)ulp)->ip6f_nxt;
@@ -1709,7 +1684,7 @@ do { \
break;
case IPPROTO_DSTOPTS: /* RFC 2460 */
- PULLUP_TO(hlen, ulp, struct ip6_hbh);
+ PULLUP_TO(ulp, struct ip6_hbh);
ext_hd |= EXT_DSTOPTS;
hlen += (((struct ip6_hbh *)ulp)->ip6h_len + 1) << 3;
proto = ((struct ip6_hbh *)ulp)->ip6h_nxt;
@@ -1717,7 +1692,7 @@ do { \
break;
case IPPROTO_AH: /* RFC 2402 */
- PULLUP_TO(hlen, ulp, struct ip6_ext);
+ PULLUP_TO(ulp, struct ip6_ext);
ext_hd |= EXT_AH;
hlen += (((struct ip6_ext *)ulp)->ip6e_len + 2) << 2;
proto = ((struct ip6_ext *)ulp)->ip6e_nxt;
@@ -1725,7 +1700,7 @@ do { \
break;
case IPPROTO_ESP: /* RFC 2406 */
- PULLUP_TO(hlen, ulp, uint32_t); /* SPI, Seq# */
+ PULLUP_TO(ulp, uint32_t); /* SPI, Seq# */
/* Anything past Seq# is variable length and
* data past this ext. header is encrypted. */
ext_hd |= EXT_ESP;
@@ -1742,21 +1717,21 @@ do { \
case IPPROTO_OSPFIGP:
/* XXX OSPF header check? */
- PULLUP_TO(hlen, ulp, struct ip6_ext);
+ PULLUP_TO(ulp, struct ip6_ext);
break;
case IPPROTO_PIM:
/* XXX PIM header check? */
- PULLUP_TO(hlen, ulp, struct pim);
+ PULLUP_TO(ulp, struct pim);
break;
case IPPROTO_GRE: /* RFC 1701 */
/* XXX GRE header check? */
- PULLUP_TO(hlen, ulp, struct grehdr);
+ PULLUP_TO(ulp, struct grehdr);
break;
case IPPROTO_CARP:
- PULLUP_TO(hlen, ulp, offsetof(
+ PULLUP_TO(ulp, offsetof(
struct carp_header, carp_counter));
if (CARP_ADVERTISEMENT !=
((struct carp_header *)ulp)->carp_type)
@@ -1764,21 +1739,21 @@ do { \
break;
case IPPROTO_IPV6: /* RFC 2893 */
- PULLUP_TO(hlen, ulp, struct ip6_hdr);
+ PULLUP_TO(ulp, struct ip6_hdr);
break;
case IPPROTO_IPV4: /* RFC 2893 */
- PULLUP_TO(hlen, ulp, struct ip);
+ PULLUP_TO(ulp, struct ip);
break;
case IPPROTO_ETHERIP: /* RFC 3378 */
- PULLUP_LEN(hlen, ulp,
+ PULLUP_LEN(ulp,
sizeof(struct etherip_header) +
sizeof(struct ether_header));
break;
case IPPROTO_PFSYNC:
- PULLUP_TO(hlen, ulp, struct pfsync_header);
+ PULLUP_TO(ulp, struct pfsync_header);
break;
default:
@@ -1788,11 +1763,10 @@ do { \
proto, ext_hd);
if (V_fw_deny_unknown_exthdrs)
return (IP_FW_DENY);
- PULLUP_TO(hlen, ulp, struct ip6_ext);
+ PULLUP_TO(ulp, struct ip6_ext);
break;
} /*switch */
}
- UPDATE_POINTERS();
ip6 = (struct ip6_hdr *)ip;
args->f_id.addr_type = 6;
args->f_id.src_ip6 = ip6->ip6_src;
@@ -1818,7 +1792,7 @@ do { \
if (offset == 0) {
switch (proto) {
case IPPROTO_TCP:
- PULLUP_TO(hlen, ulp, struct tcphdr);
+ PULLUP_TO(ulp, struct tcphdr);
dst_port = TCP(ulp)->th_dport;
src_port = TCP(ulp)->th_sport;
/* save flags for dynamic rules */
@@ -1829,28 +1803,27 @@ do { \
if (pktlen >= hlen + sizeof(struct sctphdr) +
sizeof(struct sctp_chunkhdr) +
offsetof(struct sctp_init, a_rwnd))
- PULLUP_LEN(hlen, ulp,
+ PULLUP_LEN(ulp,
sizeof(struct sctphdr) +
sizeof(struct sctp_chunkhdr) +
offsetof(struct sctp_init, a_rwnd));
else if (pktlen >= hlen + sizeof(struct sctphdr))
- PULLUP_LEN(hlen, ulp, pktlen - hlen);
+ PULLUP_LEN(ulp, pktlen - hlen);
else
- PULLUP_LEN(hlen, ulp,
- sizeof(struct sctphdr));
+ PULLUP_LEN(ulp, sizeof(struct sctphdr));
src_port = SCTP(ulp)->src_port;
dst_port = SCTP(ulp)->dest_port;
break;
case IPPROTO_UDP:
case IPPROTO_UDPLITE:
- PULLUP_TO(hlen, ulp, struct udphdr);
+ PULLUP_TO(ulp, struct udphdr);
dst_port = UDP(ulp)->uh_dport;
src_port = UDP(ulp)->uh_sport;
break;
case IPPROTO_ICMP:
- PULLUP_TO(hlen, ulp, struct icmphdr);
+ PULLUP_TO(ulp, struct icmphdr);
//args->f_id.flags = ICMP(ulp)->icmp_type;
break;
@@ -1864,7 +1837,6 @@ do { \
}
}
- UPDATE_POINTERS();
args->f_id.addr_type = 4;
args->f_id.src_ip = ntohl(src_ip.s_addr);
args->f_id.dst_ip = ntohl(dst_ip.s_addr);
@@ -1874,7 +1846,6 @@ do { \
args->f_id.addr_type = 1; /* XXX */
}
-#undef PULLUP_TO
pktlen = iplen < pktlen ? iplen: pktlen;
/* Properly initialize the rest of f_id */
@@ -1887,6 +1858,7 @@ do { \
IPFW_PF_RUNLOCK(chain);
return (IP_FW_PASS); /* accept */
}
+ locked = true;
if (args->flags & IPFW_ARGS_REF) {
/*
* Packet has already been tagged as a result of a previous
@@ -2580,7 +2552,7 @@ do { \
case O_TCPOPTS:
if (proto == IPPROTO_TCP && offset == 0 && ulp){
- PULLUP_LEN_LOCKED(hlen, ulp,
+ PULLUP_LEN(ulp,
(TCP(ulp)->th_off << 2));
match = tcpopts_match(TCP(ulp), cmd);
}
@@ -2605,8 +2577,7 @@ do { \
uint16_t mss, *p;
int i;
- PULLUP_LEN_LOCKED(hlen, ulp,
- (TCP(ulp)->th_off << 2));
+ PULLUP_LEN(ulp, TCP(ulp)->th_off << 2);
if ((tcpopts_parse(TCP(ulp), &mss) &
IP_FW_TCPOPT_MSS) == 0)
break;
@@ -3533,7 +3504,8 @@ do { \
} /* end of inner loop, scan opcodes */
#undef PULLUP_LEN
-#undef PULLUP_LEN_LOCKED
+#undef PULLUP_TO
+#undef PULLUP
if (done)
break;
@@ -3575,6 +3547,8 @@ do { \
return (retval);
pullup_failed:
+ if (locked)
+ IPFW_PF_RUNLOCK(chain);
if (V_fw_verbose)
printf("ipfw: pullup failed\n");
return (IP_FW_DENY);