[PATCH RFC libnetfilter_queue] src: Simplify struct pkt_buff

[PATCH RFC libnetfilter_queue] src: Simplify struct pkt_buff

[Duncan Roe]

In struct pkt_buff:
- We only ever needed any 2 of len, data and tail.
  This has caused bugs in the past, e.g. commit 8a4316f31.
  Delete len, and where the value of pktb->len was required,
  use new PKTB_LEN macro.
- head and data always had the same value.
  head was in the minority, so replace with data where it was used.

Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
---
 src/extra/ipv4.c    |  2 +-
 src/extra/pktbuff.c | 21 +++++++--------------
 src/internal.h      |  3 +--
 3 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/src/extra/ipv4.c b/src/extra/ipv4.c
index c03f23f..8e36861 100644
--- a/src/extra/ipv4.c
+++ b/src/extra/ipv4.c
@@ -70,7 +70,7 @@ int nfq_ip_set_transport_header(struct pkt_buff *pktb, struct iphdr *iph)
 	int doff = iph->ihl * 4;
 
 	/* Wrong offset to IPv4 payload. */
-	if ((int)pktb->len - doff <= 0)
+	if ((int)PKTB_LEN - doff <= 0)
 		return -1;
 
 	pktb->transport_header = pktb->network_header + doff;
diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
index 37f6bc0..0b67029 100644
--- a/src/extra/pktbuff.c
+++ b/src/extra/pktbuff.c
@@ -62,12 +62,10 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 	pkt_data = (uint8_t *)pktb + sizeof(struct pkt_buff);
 	memcpy(pkt_data, data, len);
 
-	pktb->len = len;
 	pktb->data_len = len + extra;
 
-	pktb->head = pkt_data;
 	pktb->data = pkt_data;
-	pktb->tail = pktb->head + len;
+	pktb->tail = pktb->data + len;
 
 	switch(family) {
 	case AF_INET:
@@ -121,7 +119,7 @@ uint8_t *pktb_data(struct pkt_buff *pktb)
 EXPORT_SYMBOL
 uint32_t pktb_len(struct pkt_buff *pktb)
 {
-	return pktb->len;
+	return PKTB_LEN;
 }
 
 /**
@@ -169,7 +167,6 @@ EXPORT_SYMBOL
 void pktb_push(struct pkt_buff *pktb, unsigned int len)
 {
 	pktb->data -= len;
-	pktb->len += len;
 }
 
 /**
@@ -181,7 +178,6 @@ EXPORT_SYMBOL
 void pktb_pull(struct pkt_buff *pktb, unsigned int len)
 {
 	pktb->data += len;
-	pktb->len -= len;
 }
 
 /**
@@ -193,7 +189,6 @@ EXPORT_SYMBOL
 void pktb_put(struct pkt_buff *pktb, unsigned int len)
 {
 	pktb->tail += len;
-	pktb->len += len;
 }
 
 /**
@@ -204,8 +199,7 @@ void pktb_put(struct pkt_buff *pktb, unsigned int len)
 EXPORT_SYMBOL
 void pktb_trim(struct pkt_buff *pktb, unsigned int len)
 {
-	pktb->len = len;
-	pktb->tail = pktb->head + len;
+	pktb->tail = pktb->data + len;
 }
 
 /**
@@ -224,7 +218,7 @@ void pktb_trim(struct pkt_buff *pktb, unsigned int len)
 EXPORT_SYMBOL
 unsigned int pktb_tailroom(struct pkt_buff *pktb)
 {
-	return pktb->data_len - pktb->len;
+	return pktb->data_len - PKTB_LEN;
 }
 
 /**
@@ -277,17 +271,16 @@ static int pktb_expand_tail(struct pkt_buff *pktb, int extra)
 	 * reallocation. Instead, increase the size of the extra room in
 	 * the tail in pktb_alloc.
 	 */
-	if (pktb->len + extra > pktb->data_len)
+	if (PKTB_LEN + extra > pktb->data_len)
 		return 0;
 
-	pktb->len += extra;
 	pktb->tail = pktb->tail + extra;
 	return 1;
 }
 
 static int enlarge_pkt(struct pkt_buff *pktb, unsigned int extra)
 {
-	if (pktb->len + extra > 65535)
+	if (PKTB_LEN + extra > 65535)
 		return 0;
 
 	if (!pktb_expand_tail(pktb, extra - pktb_tailroom(pktb)))
@@ -346,7 +339,7 @@ int pktb_mangle(struct pkt_buff *pktb,
 	if (rep_len > match_len)
 		pktb_put(pktb, rep_len - match_len);
 	else
-		pktb_trim(pktb, pktb->len + rep_len - match_len);
+		pktb_trim(pktb, PKTB_LEN + rep_len - match_len);
 
 	pktb->mangled = true;
 	return 1;
diff --git a/src/internal.h b/src/internal.h
index d968325..95ac03e 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -9,6 +9,7 @@
 #else
 #	define EXPORT_SYMBOL
 #endif
+#define PKTB_LEN (pktb->tail - pktb->data)
 
 struct iphdr;
 struct ip6_hdr;
@@ -23,11 +24,9 @@ struct pkt_buff {
 	uint8_t *network_header;
 	uint8_t *transport_header;
 
-	uint8_t *head;
 	uint8_t *data;
 	uint8_t *tail;
 
-	uint32_t len;
 	uint32_t data_len;
 
 	bool	mangled;
-- 
2.14.5

Re: [PATCH RFC libnetfilter_queue] src: Simplify struct pkt_buff

[Pablo Neira Ayuso]

On Fri, Jan 10, 2020 at 03:09:25PM +1100, Duncan Roe wrote:
> In struct pkt_buff:
> - We only ever needed any 2 of len, data and tail.

You can remove ->tail.

>   This has caused bugs in the past, e.g. commit 8a4316f31.
>   Delete len, and where the value of pktb->len was required,
>   use new PKTB_LEN macro.

I would leave pktb->len in please, it keeps things simple. Otherwise,
I would suggest to remove pktb->data_len, keep pktb->len in place and
reallocate the data buffer only in case of mangling.

> - head and data always had the same value.
>   head was in the minority, so replace with data where it was used.

This item above also looks fine.

Probably you can split this in several patches, one for each item.

Thanks.

[PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove head

[Duncan Roe]

head and data always had the same value.
head was in the minority, so replace with data where it was used.

Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
---
 src/extra/pktbuff.c | 5 ++---
 src/internal.h      | 1 -
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
index f013cfe..c95384f 100644
--- a/src/extra/pktbuff.c
+++ b/src/extra/pktbuff.c
@@ -66,9 +66,8 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 	pktb->len = len;
 	pktb->data_len = len + extra;
 
-	pktb->head = pkt_data;
 	pktb->data = pkt_data;
-	pktb->tail = pktb->head + len;
+	pktb->tail = pktb->data + len;
 
 	switch(family) {
 	case AF_INET:
@@ -204,7 +203,7 @@ EXPORT_SYMBOL
 void pktb_trim(struct pkt_buff *pktb, unsigned int len)
 {
 	pktb->len = len;
-	pktb->tail = pktb->head + len;
+	pktb->tail = pktb->data + len;
 }
 
 /**
diff --git a/src/internal.h b/src/internal.h
index d968325..0cfa425 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -23,7 +23,6 @@ struct pkt_buff {
 	uint8_t *network_header;
 	uint8_t *transport_header;
 
-	uint8_t *head;
 	uint8_t *data;
 	uint8_t *tail;
 
-- 
2.14.5

[PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove tail

[Duncan Roe]

In struct pkt_buff, we only ever needed any 2 of len, data and tail.
This has caused bugs in the past, e.g. commit 8a4316f31.
Delete tail, and where the value of pktb->tail was required,
use new PKTB_TAIL macro.

Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
---
 src/extra/ipv4.c    | 4 ++--
 src/extra/ipv6.c    | 8 ++++----
 src/extra/pktbuff.c | 6 +-----
 src/extra/tcp.c     | 6 +++---
 src/extra/udp.c     | 6 +++---
 src/internal.h      | 2 +-
 6 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/src/extra/ipv4.c b/src/extra/ipv4.c
index caafd37..bd5da22 100644
--- a/src/extra/ipv4.c
+++ b/src/extra/ipv4.c
@@ -40,7 +40,7 @@ EXPORT_SYMBOL
 struct iphdr *nfq_ip_get_hdr(struct pkt_buff *pktb)
 {
 	struct iphdr *iph;
-	unsigned int pktlen = pktb->tail - pktb->network_header;
+	unsigned int pktlen = PKTB_TAIL - pktb->network_header;
 
 	/* Not enough room for IPv4 header. */
 	if (pktlen < sizeof(struct iphdr))
@@ -135,7 +135,7 @@ int nfq_ip_mangle(struct pkt_buff *pktb, unsigned int dataoff,
 		return 0;
 
 	/* fix IP hdr checksum information */
-	iph->tot_len = htons(pktb->tail - pktb->network_header);
+	iph->tot_len = htons(PKTB_TAIL - pktb->network_header);
 	nfq_ip_set_checksum(iph);
 
 	return 1;
diff --git a/src/extra/ipv6.c b/src/extra/ipv6.c
index 6e8820c..b6542f9 100644
--- a/src/extra/ipv6.c
+++ b/src/extra/ipv6.c
@@ -36,7 +36,7 @@ EXPORT_SYMBOL
 struct ip6_hdr *nfq_ip6_get_hdr(struct pkt_buff *pktb)
 {
 	struct ip6_hdr *ip6h;
-	unsigned int pktlen = pktb->tail - pktb->network_header;
+	unsigned int pktlen = PKTB_TAIL - pktb->network_header;
 
 	/* Not enough room for IPv6 header. */
 	if (pktlen < sizeof(struct ip6_hdr))
@@ -77,7 +77,7 @@ int nfq_ip6_set_transport_header(struct pkt_buff *pktb, struct ip6_hdr *ip6h,
 			break;
 		}
 		/* No room for extension, bad packet. */
-		if (pktb->tail - cur < sizeof(struct ip6_ext)) {
+		if (PKTB_TAIL - cur < sizeof(struct ip6_ext)) {
 			cur = NULL;
 			break;
 		}
@@ -87,7 +87,7 @@ int nfq_ip6_set_transport_header(struct pkt_buff *pktb, struct ip6_hdr *ip6h,
 			uint16_t *frag_off;
 
 			/* No room for full fragment header, bad packet. */
-			if (pktb->tail - cur < sizeof(struct ip6_frag)) {
+			if (PKTB_TAIL - cur < sizeof(struct ip6_frag)) {
 				cur = NULL;
 				break;
 			}
@@ -140,7 +140,7 @@ int nfq_ip6_mangle(struct pkt_buff *pktb, unsigned int dataoff,
 
 	/* Fix IPv6 hdr length information */
 	ip6h->ip6_plen =
-		htons(pktb->tail - pktb->network_header - sizeof *ip6h);
+		htons(PKTB_TAIL - pktb->network_header - sizeof *ip6h);
 
 	return 1;
 }
diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
index c95384f..f97750f 100644
--- a/src/extra/pktbuff.c
+++ b/src/extra/pktbuff.c
@@ -67,7 +67,6 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 	pktb->data_len = len + extra;
 
 	pktb->data = pkt_data;
-	pktb->tail = pktb->data + len;
 
 	switch(family) {
 	case AF_INET:
@@ -190,7 +189,6 @@ void pktb_pull(struct pkt_buff *pktb, unsigned int len)
 EXPORT_SYMBOL
 void pktb_put(struct pkt_buff *pktb, unsigned int len)
 {
-	pktb->tail += len;
 	pktb->len += len;
 }
 
@@ -203,7 +201,6 @@ EXPORT_SYMBOL
 void pktb_trim(struct pkt_buff *pktb, unsigned int len)
 {
 	pktb->len = len;
-	pktb->tail = pktb->data + len;
 }
 
 /**
@@ -279,7 +276,6 @@ static int pktb_expand_tail(struct pkt_buff *pktb, int extra)
 		return 0;
 
 	pktb->len += extra;
-	pktb->tail = pktb->tail + extra;
 	return 1;
 }
 
@@ -334,7 +330,7 @@ int pktb_mangle(struct pkt_buff *pktb,
 	/* move post-replacement */
 	memmove(data + match_offset + rep_len,
 		data + match_offset + match_len,
-		pktb->tail - (pktb->network_header + dataoff +
+		PKTB_TAIL - (pktb->network_header + dataoff +
 			     match_offset + match_len));
 
 	/* insert data from buffer */
diff --git a/src/extra/tcp.c b/src/extra/tcp.c
index cca20e7..eb43067 100644
--- a/src/extra/tcp.c
+++ b/src/extra/tcp.c
@@ -46,7 +46,7 @@ struct tcphdr *nfq_tcp_get_hdr(struct pkt_buff *pktb)
 		return NULL;
 
 	/* No room for the TCP header. */
-	if (pktb->tail - pktb->transport_header < sizeof(struct tcphdr))
+	if (PKTB_TAIL - pktb->transport_header < sizeof(struct tcphdr))
 		return NULL;
 
 	return (struct tcphdr *)pktb->transport_header;
@@ -68,7 +68,7 @@ void *nfq_tcp_get_payload(struct tcphdr *tcph, struct pkt_buff *pktb)
 		return NULL;
 
 	/* malformed TCP data offset. */
-	if (pktb->transport_header + len > pktb->tail)
+	if (pktb->transport_header + len > PKTB_TAIL)
 		return NULL;
 
 	return pktb->transport_header + len;
@@ -83,7 +83,7 @@ void *nfq_tcp_get_payload(struct tcphdr *tcph, struct pkt_buff *pktb)
 EXPORT_SYMBOL
 unsigned int nfq_tcp_get_payload_len(struct tcphdr *tcph, struct pkt_buff *pktb)
 {
-	return pktb->tail - pktb->transport_header - (tcph->doff * 4);
+	return PKTB_TAIL - pktb->transport_header - (tcph->doff * 4);
 }
 
 /**
diff --git a/src/extra/udp.c b/src/extra/udp.c
index dc476d4..4b7e3cc 100644
--- a/src/extra/udp.c
+++ b/src/extra/udp.c
@@ -46,7 +46,7 @@ struct udphdr *nfq_udp_get_hdr(struct pkt_buff *pktb)
 		return NULL;
 
 	/* No room for the UDP header. */
-	if (pktb->tail - pktb->transport_header < sizeof(struct udphdr))
+	if (PKTB_TAIL - pktb->transport_header < sizeof(struct udphdr))
 		return NULL;
 
 	return (struct udphdr *)pktb->transport_header;
@@ -68,7 +68,7 @@ void *nfq_udp_get_payload(struct udphdr *udph, struct pkt_buff *pktb)
 		return NULL;
 
 	/* malformed UDP packet. */
-	if (pktb->transport_header + len > pktb->tail)
+	if (pktb->transport_header + len > PKTB_TAIL)
 		return NULL;
 
 	return pktb->transport_header + sizeof(struct udphdr);
@@ -83,7 +83,7 @@ void *nfq_udp_get_payload(struct udphdr *udph, struct pkt_buff *pktb)
 EXPORT_SYMBOL
 unsigned int nfq_udp_get_payload_len(struct udphdr *udph, struct pkt_buff *pktb)
 {
-	return pktb->tail - pktb->transport_header - sizeof(struct udphdr);
+	return PKTB_TAIL - pktb->transport_header - sizeof(struct udphdr);
 }
 
 /**
diff --git a/src/internal.h b/src/internal.h
index 0cfa425..dafb33a 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -9,6 +9,7 @@
 #else
 #	define EXPORT_SYMBOL
 #endif
+#define PKTB_TAIL (pktb->data + pktb->len)
 
 struct iphdr;
 struct ip6_hdr;
@@ -24,7 +25,6 @@ struct pkt_buff {
 	uint8_t *transport_header;
 
 	uint8_t *data;
-	uint8_t *tail;
 
 	uint32_t len;
 	uint32_t data_len;
-- 
2.14.5

Re: [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove head

[Pablo Neira Ayuso]

On Fri, Jan 17, 2020 at 10:32:03PM +1100, Duncan Roe wrote:
> head and data always had the same value.
> head was in the minority, so replace with data where it was used.

Applied, thanks.

Re: [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove tail

[Pablo Neira Ayuso]

On Sat, Jan 18, 2020 at 01:09:55AM +1100, Duncan Roe wrote:
[...]
> diff --git a/src/internal.h b/src/internal.h
> index 0cfa425..dafb33a 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -9,6 +9,7 @@
>  #else
>  #	define EXPORT_SYMBOL
>  #endif
> +#define PKTB_TAIL (pktb->data + pktb->len)

Instead of a macro, I'd suggest you add (something like):

static inline uint8_t *pktb_tail(struct pktbuff *pktb)
{
        return pktb->data + pktb->len;
}

Thanks.

[PATCH libnetfilter_queue v2 0/1] Simplify struct pkt_buff: remove tail

[Duncan Roe]

Hi Pablo,

This patch uses a static inline function as you requested.

I thought the macro would result in fewer instructions, but haven't checked.

Anyway you have both versions now.

Cheers ... Duncan.

Duncan Roe (1):
  Simplify struct pkt_buff: remove tail

 src/extra/ipv4.c    | 4 ++--
 src/extra/ipv6.c    | 8 ++++----
 src/extra/pktbuff.c | 6 +-----
 src/extra/tcp.c     | 6 +++---
 src/extra/udp.c     | 6 +++---
 src/internal.h      | 5 ++++-
 6 files changed, 17 insertions(+), 18 deletions(-)

-- 
2.14.5

[PATCH libnetfilter_queue v2 1/1] Simplify struct pkt_buff: remove tail

[Duncan Roe]

In struct pkt_buff, we only ever needed any 2 of len, data and tail.
This has caused bugs in the past, e.g. commit 8a4316f31.
Delete tail, and where the value of pktb->tail was required,
use new pktb_tail() function.

Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
---
 src/extra/ipv4.c    | 4 ++--
 src/extra/ipv6.c    | 8 ++++----
 src/extra/pktbuff.c | 6 +-----
 src/extra/tcp.c     | 6 +++---
 src/extra/udp.c     | 6 +++---
 src/internal.h      | 5 ++++-
 6 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/src/extra/ipv4.c b/src/extra/ipv4.c
index caafd37..797bab1 100644
--- a/src/extra/ipv4.c
+++ b/src/extra/ipv4.c
@@ -40,7 +40,7 @@ EXPORT_SYMBOL
 struct iphdr *nfq_ip_get_hdr(struct pkt_buff *pktb)
 {
 	struct iphdr *iph;
-	unsigned int pktlen = pktb->tail - pktb->network_header;
+	unsigned int pktlen = pktb_tail(pktb) - pktb->network_header;
 
 	/* Not enough room for IPv4 header. */
 	if (pktlen < sizeof(struct iphdr))
@@ -135,7 +135,7 @@ int nfq_ip_mangle(struct pkt_buff *pktb, unsigned int dataoff,
 		return 0;
 
 	/* fix IP hdr checksum information */
-	iph->tot_len = htons(pktb->tail - pktb->network_header);
+	iph->tot_len = htons(pktb_tail(pktb) - pktb->network_header);
 	nfq_ip_set_checksum(iph);
 
 	return 1;
diff --git a/src/extra/ipv6.c b/src/extra/ipv6.c
index 6e8820c..42c5e25 100644
--- a/src/extra/ipv6.c
+++ b/src/extra/ipv6.c
@@ -36,7 +36,7 @@ EXPORT_SYMBOL
 struct ip6_hdr *nfq_ip6_get_hdr(struct pkt_buff *pktb)
 {
 	struct ip6_hdr *ip6h;
-	unsigned int pktlen = pktb->tail - pktb->network_header;
+	unsigned int pktlen = pktb_tail(pktb) - pktb->network_header;
 
 	/* Not enough room for IPv6 header. */
 	if (pktlen < sizeof(struct ip6_hdr))
@@ -77,7 +77,7 @@ int nfq_ip6_set_transport_header(struct pkt_buff *pktb, struct ip6_hdr *ip6h,
 			break;
 		}
 		/* No room for extension, bad packet. */
-		if (pktb->tail - cur < sizeof(struct ip6_ext)) {
+		if (pktb_tail(pktb) - cur < sizeof(struct ip6_ext)) {
 			cur = NULL;
 			break;
 		}
@@ -87,7 +87,7 @@ int nfq_ip6_set_transport_header(struct pkt_buff *pktb, struct ip6_hdr *ip6h,
 			uint16_t *frag_off;
 
 			/* No room for full fragment header, bad packet. */
-			if (pktb->tail - cur < sizeof(struct ip6_frag)) {
+			if (pktb_tail(pktb) - cur < sizeof(struct ip6_frag)) {
 				cur = NULL;
 				break;
 			}
@@ -140,7 +140,7 @@ int nfq_ip6_mangle(struct pkt_buff *pktb, unsigned int dataoff,
 
 	/* Fix IPv6 hdr length information */
 	ip6h->ip6_plen =
-		htons(pktb->tail - pktb->network_header - sizeof *ip6h);
+		htons(pktb_tail(pktb) - pktb->network_header - sizeof *ip6h);
 
 	return 1;
 }
diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
index c95384f..6dd0ca9 100644
--- a/src/extra/pktbuff.c
+++ b/src/extra/pktbuff.c
@@ -67,7 +67,6 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 	pktb->data_len = len + extra;
 
 	pktb->data = pkt_data;
-	pktb->tail = pktb->data + len;
 
 	switch(family) {
 	case AF_INET:
@@ -190,7 +189,6 @@ void pktb_pull(struct pkt_buff *pktb, unsigned int len)
 EXPORT_SYMBOL
 void pktb_put(struct pkt_buff *pktb, unsigned int len)
 {
-	pktb->tail += len;
 	pktb->len += len;
 }
 
@@ -203,7 +201,6 @@ EXPORT_SYMBOL
 void pktb_trim(struct pkt_buff *pktb, unsigned int len)
 {
 	pktb->len = len;
-	pktb->tail = pktb->data + len;
 }
 
 /**
@@ -279,7 +276,6 @@ static int pktb_expand_tail(struct pkt_buff *pktb, int extra)
 		return 0;
 
 	pktb->len += extra;
-	pktb->tail = pktb->tail + extra;
 	return 1;
 }
 
@@ -334,7 +330,7 @@ int pktb_mangle(struct pkt_buff *pktb,
 	/* move post-replacement */
 	memmove(data + match_offset + rep_len,
 		data + match_offset + match_len,
-		pktb->tail - (pktb->network_header + dataoff +
+		pktb_tail(pktb) - (pktb->network_header + dataoff +
 			     match_offset + match_len));
 
 	/* insert data from buffer */
diff --git a/src/extra/tcp.c b/src/extra/tcp.c
index cca20e7..933c6ee 100644
--- a/src/extra/tcp.c
+++ b/src/extra/tcp.c
@@ -46,7 +46,7 @@ struct tcphdr *nfq_tcp_get_hdr(struct pkt_buff *pktb)
 		return NULL;
 
 	/* No room for the TCP header. */
-	if (pktb->tail - pktb->transport_header < sizeof(struct tcphdr))
+	if (pktb_tail(pktb) - pktb->transport_header < sizeof(struct tcphdr))
 		return NULL;
 
 	return (struct tcphdr *)pktb->transport_header;
@@ -68,7 +68,7 @@ void *nfq_tcp_get_payload(struct tcphdr *tcph, struct pkt_buff *pktb)
 		return NULL;
 
 	/* malformed TCP data offset. */
-	if (pktb->transport_header + len > pktb->tail)
+	if (pktb->transport_header + len > pktb_tail(pktb))
 		return NULL;
 
 	return pktb->transport_header + len;
@@ -83,7 +83,7 @@ void *nfq_tcp_get_payload(struct tcphdr *tcph, struct pkt_buff *pktb)
 EXPORT_SYMBOL
 unsigned int nfq_tcp_get_payload_len(struct tcphdr *tcph, struct pkt_buff *pktb)
 {
-	return pktb->tail - pktb->transport_header - (tcph->doff * 4);
+	return pktb_tail(pktb) - pktb->transport_header - (tcph->doff * 4);
 }
 
 /**
diff --git a/src/extra/udp.c b/src/extra/udp.c
index dc476d4..f232127 100644
--- a/src/extra/udp.c
+++ b/src/extra/udp.c
@@ -46,7 +46,7 @@ struct udphdr *nfq_udp_get_hdr(struct pkt_buff *pktb)
 		return NULL;
 
 	/* No room for the UDP header. */
-	if (pktb->tail - pktb->transport_header < sizeof(struct udphdr))
+	if (pktb_tail(pktb) - pktb->transport_header < sizeof(struct udphdr))
 		return NULL;
 
 	return (struct udphdr *)pktb->transport_header;
@@ -68,7 +68,7 @@ void *nfq_udp_get_payload(struct udphdr *udph, struct pkt_buff *pktb)
 		return NULL;
 
 	/* malformed UDP packet. */
-	if (pktb->transport_header + len > pktb->tail)
+	if (pktb->transport_header + len > pktb_tail(pktb))
 		return NULL;
 
 	return pktb->transport_header + sizeof(struct udphdr);
@@ -83,7 +83,7 @@ void *nfq_udp_get_payload(struct udphdr *udph, struct pkt_buff *pktb)
 EXPORT_SYMBOL
 unsigned int nfq_udp_get_payload_len(struct udphdr *udph, struct pkt_buff *pktb)
 {
-	return pktb->tail - pktb->transport_header - sizeof(struct udphdr);
+	return pktb_tail(pktb) - pktb->transport_header - sizeof(struct udphdr);
 }
 
 /**
diff --git a/src/internal.h b/src/internal.h
index 0cfa425..ae849d6 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -24,7 +24,6 @@ struct pkt_buff {
 	uint8_t *transport_header;
 
 	uint8_t *data;
-	uint8_t *tail;
 
 	uint32_t len;
 	uint32_t data_len;
@@ -32,4 +31,8 @@ struct pkt_buff {
 	bool	mangled;
 };
 
+static inline uint8_t *pktb_tail(struct pkt_buff *pktb)
+{
+	return pktb->data + pktb->len;
+}
 #endif
-- 
2.14.5

Re: [PATCH libnetfilter_queue v2 1/1] Simplify struct pkt_buff: remove tail

[Pablo Neira Ayuso]

On Sun, Jan 26, 2020 at 03:02:02PM +1100, Duncan Roe wrote:
> In struct pkt_buff, we only ever needed any 2 of len, data and tail.
> This has caused bugs in the past, e.g. commit 8a4316f31.
> Delete tail, and where the value of pktb->tail was required,
> use new pktb_tail() function.

Applied, thanks.