Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH RFC libnetfilter_queue] src: Simplify struct pkt_buff
@ 2020-01-10  4:09 Duncan Roe
  2020-01-10 11:12 ` Pablo Neira Ayuso
  0 siblings, 1 reply; 6+ messages in thread
From: Duncan Roe @ 2020-01-10  4:09 UTC (permalink / raw)
  To: pablo; +Cc: netfilter-devel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH RFC libnetfilter_queue] src: Simplify struct pkt_buff
  2020-01-10  4:09 [PATCH RFC libnetfilter_queue] src: Simplify struct pkt_buff Duncan Roe
@ 2020-01-10 11:12 ` Pablo Neira Ayuso
  2020-01-17 11:32   ` [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove head Duncan Roe
  0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2020-01-10 11:12 UTC (permalink / raw)
  To: Duncan Roe; +Cc: netfilter-devel

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove head
  2020-01-10 11:12 ` Pablo Neira Ayuso
@ 2020-01-17 11:32   ` Duncan Roe
  2020-01-17 14:09     ` [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove tail Duncan Roe
  2020-01-18 20:40     ` [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove head Pablo Neira Ayuso
  0 siblings, 2 replies; 6+ messages in thread
From: Duncan Roe @ 2020-01-17 11:32 UTC (permalink / raw)
  To: pablo; +Cc: netfilter-devel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove tail
  2020-01-17 11:32   ` [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove head Duncan Roe
@ 2020-01-17 14:09     ` Duncan Roe
  2020-01-18 20:43       ` Pablo Neira Ayuso
  2020-01-18 20:40     ` [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove head Pablo Neira Ayuso
  1 sibling, 1 reply; 6+ messages in thread
From: Duncan Roe @ 2020-01-17 14:09 UTC (permalink / raw)
  To: pablo; +Cc: netfilter-devel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove head
  2020-01-17 11:32   ` [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove head Duncan Roe
  2020-01-17 14:09     ` [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove tail Duncan Roe
@ 2020-01-18 20:40     ` Pablo Neira Ayuso
  1 sibling, 0 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2020-01-18 20:40 UTC (permalink / raw)
  To: Duncan Roe; +Cc: netfilter-devel

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove tail
  2020-01-17 14:09     ` [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove tail Duncan Roe
@ 2020-01-18 20:43       ` Pablo Neira Ayuso
  0 siblings, 0 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2020-01-18 20:43 UTC (permalink / raw)
  To: Duncan Roe; +Cc: netfilter-devel

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-10  4:09 [PATCH RFC libnetfilter_queue] src: Simplify struct pkt_buff Duncan Roe
2020-01-10 11:12 ` Pablo Neira Ayuso
2020-01-17 11:32   ` [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove head Duncan Roe
2020-01-17 14:09     ` [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove tail Duncan Roe
2020-01-18 20:43       ` Pablo Neira Ayuso
2020-01-18 20:40     ` [PATCH libnetfilter_queue] src: Simplify struct pkt_buff: remove head Pablo Neira Ayuso

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git