[PATCH libnetfilter_queue 1/2] pktbuff: add __pktb_setup()

[PATCH libnetfilter_queue 1/2] pktbuff: add __pktb_setup()

[Pablo Neira Ayuso]

Add private helper function to set up the pkt_buff object.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/extra/pktbuff.c | 54 +++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
index 6dd0ca98aff2..118ad898f63b 100644
--- a/src/extra/pktbuff.c
+++ b/src/extra/pktbuff.c
@@ -29,6 +29,34 @@
  * @{
  */
 
+static int __pktb_setup(int family, struct pkt_buff *pktb)
+{
+	struct ethhdr *ethhdr;
+
+	switch (family) {
+	case AF_INET:
+	case AF_INET6:
+		pktb->network_header = pktb->data;
+		break;
+	case AF_BRIDGE:
+		ethhdr = (struct ethhdr *)pktb->data;
+		pktb->mac_header = pktb->data;
+
+		switch(ethhdr->h_proto) {
+		case ETH_P_IP:
+		case ETH_P_IPV6:
+			pktb->network_header = pktb->data + ETH_HLEN;
+			break;
+		default:
+			/* This protocol is unsupported. */
+			return -1;
+		}
+		break;
+	}
+
+	return 0;
+}
+
 /**
  * pktb_alloc - allocate a new packet buffer
  * \param family Indicate what family. Currently supported families are
@@ -52,7 +80,6 @@ EXPORT_SYMBOL
 struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 {
 	struct pkt_buff *pktb;
-	struct ethhdr *ethhdr;
 	void *pkt_data;
 
 	pktb = calloc(1, sizeof(struct pkt_buff) + len + extra);
@@ -68,28 +95,11 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 
 	pktb->data = pkt_data;
 
-	switch(family) {
-	case AF_INET:
-	case AF_INET6:
-		pktb->network_header = pktb->data;
-		break;
-	case AF_BRIDGE:
-		ethhdr = (struct ethhdr *)pktb->data;
-		pktb->mac_header = pktb->data;
-
-		switch(ethhdr->h_proto) {
-		case ETH_P_IP:
-		case ETH_P_IPV6:
-			pktb->network_header = pktb->data + ETH_HLEN;
-			break;
-		default:
-			/* This protocol is unsupported. */
-			errno = EPROTONOSUPPORT;
-			free(pktb);
-			return NULL;
-		}
-		break;
+	if (__pktb_setup(family, pktb) < 0) {
+		errno = EPROTONOSUPPORT;
+		free(pktb);
 	}
+
 	return pktb;
 }
 
-- 
2.20.1

[PATCH libnetfilter_queue 2/2] pktbuff: add pktb_head_alloc(), pktb_setup() and pktb_head_size()

[Pablo Neira Ayuso]

Add two new helper functions, as alternative to pktb_alloc().

* pktb_setup() allows you to skip memcpy()'ing the payload from the
  netlink message.

* pktb_head_size() returns the size of the pkt_buff opaque object.

* pktb_head_alloc() allows you to allocate the pkt_buff in the heap.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/libnetfilter_queue/pktbuff.h |  7 +++++++
 src/extra/pktbuff.c                  | 20 ++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/libnetfilter_queue/pktbuff.h b/include/libnetfilter_queue/pktbuff.h
index 42bc153ec337..a27582b02840 100644
--- a/include/libnetfilter_queue/pktbuff.h
+++ b/include/libnetfilter_queue/pktbuff.h
@@ -6,6 +6,13 @@ struct pkt_buff;
 struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra);
 void pktb_free(struct pkt_buff *pktb);
 
+#define NFQ_BUFFER_SIZE	(0xffff + (MNL_SOCKET_BUFFER_SIZE / 2)
+struct pkt_buff *pktb_setup(struct pkt_buff *pktb, int family, uint8_t *data,
+			    size_t len, size_t extra);
+size_t pktb_head_size(void);
+
+#define pktb_head_alloc()	(struct pkt_buff *)(malloc(pktb_head_size()))
+
 uint8_t *pktb_data(struct pkt_buff *pktb);
 uint32_t pktb_len(struct pkt_buff *pktb);
 
diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
index 118ad898f63b..6acefbe72a9b 100644
--- a/src/extra/pktbuff.c
+++ b/src/extra/pktbuff.c
@@ -103,6 +103,26 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
 	return pktb;
 }
 
+EXPORT_SYMBOL
+struct pkt_buff *pktb_setup(struct pkt_buff *pktb, int family, uint8_t *buf,
+			    size_t len, size_t extra)
+{
+	pktb->data_len = len + extra;
+	pktb->data = buf;
+	pktb->len = len;
+
+	if (__pktb_setup(family, pktb) < 0)
+		return NULL;
+
+	return pktb;
+}
+
+EXPORT_SYMBOL
+size_t pktb_head_size(void)
+{
+	return sizeof(struct pkt_buff);
+}
+
 /**
  * pktb_data - get pointer to network packet
  * \param pktb Pointer to userspace packet buffer
-- 
2.20.1

Re: [PATCH libnetfilter_queue 2/2] pktbuff: add pktb_head_alloc(), pktb_setup() and pktb_head_size()

[Duncan Roe]

On Sat, May 09, 2020 at 11:11:41AM +0200, Pablo Neira Ayuso wrote:
> Add two new helper functions, as alternative to pktb_alloc().
>
> * pktb_setup() allows you to skip memcpy()'ing the payload from the
>   netlink message.
>
> * pktb_head_size() returns the size of the pkt_buff opaque object.
>
> * pktb_head_alloc() allows you to allocate the pkt_buff in the heap.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  include/libnetfilter_queue/pktbuff.h |  7 +++++++
>  src/extra/pktbuff.c                  | 20 ++++++++++++++++++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/include/libnetfilter_queue/pktbuff.h b/include/libnetfilter_queue/pktbuff.h
> index 42bc153ec337..a27582b02840 100644
> --- a/include/libnetfilter_queue/pktbuff.h
> +++ b/include/libnetfilter_queue/pktbuff.h
> @@ -6,6 +6,13 @@ struct pkt_buff;
>  struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra);
>  void pktb_free(struct pkt_buff *pktb);
>
> +#define NFQ_BUFFER_SIZE	(0xffff + (MNL_SOCKET_BUFFER_SIZE / 2)
> +struct pkt_buff *pktb_setup(struct pkt_buff *pktb, int family, uint8_t *data,
> +			    size_t len, size_t extra);
> +size_t pktb_head_size(void);
> +
> +#define pktb_head_alloc()	(struct pkt_buff *)(malloc(pktb_head_size()))
> +
>  uint8_t *pktb_data(struct pkt_buff *pktb);
>  uint32_t pktb_len(struct pkt_buff *pktb);
>
> diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
> index 118ad898f63b..6acefbe72a9b 100644
> --- a/src/extra/pktbuff.c
> +++ b/src/extra/pktbuff.c
> @@ -103,6 +103,26 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
>  	return pktb;
>  }
>
> +EXPORT_SYMBOL
> +struct pkt_buff *pktb_setup(struct pkt_buff *pktb, int family, uint8_t *buf,
> +			    size_t len, size_t extra)
> +{
> +	pktb->data_len = len + extra;

Are you proposing to be able to use extra space in the receive buffer?
I think that is unsafe. mnl_cb_run() steps through that bufffer and needs a
zero following the last message to know there are no more. At least, that's
how it looks to me on stepping through with gdb.

> +	pktb->data = buf;
> +	pktb->len = len;
> +
> +	if (__pktb_setup(family, pktb) < 0)
> +		return NULL;
> +
> +	return pktb;
> +}
> +
> +EXPORT_SYMBOL
> +size_t pktb_head_size(void)
> +{
> +	return sizeof(struct pkt_buff);
> +}
> +
>  /**
>   * pktb_data - get pointer to network packet
>   * \param pktb Pointer to userspace packet buffer
> --
> 2.20.1
>
Will post an alternative in the morning - D.

Re: [PATCH libnetfilter_queue 2/2] pktbuff: add pktb_head_alloc(), pktb_setup() and pktb_head_size()

[Pablo Neira Ayuso]

On Sun, May 10, 2020 at 02:09:03AM +1000, Duncan Roe wrote:
> On Sat, May 09, 2020 at 11:11:41AM +0200, Pablo Neira Ayuso wrote:
> > Add two new helper functions, as alternative to pktb_alloc().
> >
> > * pktb_setup() allows you to skip memcpy()'ing the payload from the
> >   netlink message.
> >
> > * pktb_head_size() returns the size of the pkt_buff opaque object.
> >
> > * pktb_head_alloc() allows you to allocate the pkt_buff in the heap.
> >
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  include/libnetfilter_queue/pktbuff.h |  7 +++++++
> >  src/extra/pktbuff.c                  | 20 ++++++++++++++++++++
> >  2 files changed, 27 insertions(+)
> >
> > diff --git a/include/libnetfilter_queue/pktbuff.h b/include/libnetfilter_queue/pktbuff.h
> > index 42bc153ec337..a27582b02840 100644
> > --- a/include/libnetfilter_queue/pktbuff.h
> > +++ b/include/libnetfilter_queue/pktbuff.h
> > @@ -6,6 +6,13 @@ struct pkt_buff;
> >  struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra);
> >  void pktb_free(struct pkt_buff *pktb);
> >
> > +#define NFQ_BUFFER_SIZE	(0xffff + (MNL_SOCKET_BUFFER_SIZE / 2)
> > +struct pkt_buff *pktb_setup(struct pkt_buff *pktb, int family, uint8_t *data,
> > +			    size_t len, size_t extra);
> > +size_t pktb_head_size(void);
> > +
> > +#define pktb_head_alloc()	(struct pkt_buff *)(malloc(pktb_head_size()))
> > +
> >  uint8_t *pktb_data(struct pkt_buff *pktb);
> >  uint32_t pktb_len(struct pkt_buff *pktb);
> >
> > diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
> > index 118ad898f63b..6acefbe72a9b 100644
> > --- a/src/extra/pktbuff.c
> > +++ b/src/extra/pktbuff.c
> > @@ -103,6 +103,26 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
> >  	return pktb;
> >  }
> >
> > +EXPORT_SYMBOL
> > +struct pkt_buff *pktb_setup(struct pkt_buff *pktb, int family, uint8_t *buf,
> > +			    size_t len, size_t extra)
> > +{
> > +	pktb->data_len = len + extra;
> 
> Are you proposing to be able to use extra space in the receive buffer?
> I think that is unsafe. mnl_cb_run() steps through that bufffer and needs a
> zero following the last message to know there are no more. At least, that's
> how it looks to me on stepping through with gdb.

There are "two buffers":

1) The buffer that you use to receive the netlink message. This buffer
   is parsed via mnl_cb_run().

2) The buffer that stores the pkt_buff structure.

pktb_setup() is called after mnl_cb_run(), once you have already
parsed the buffer that you have received from netlink. You might want
to pass the pointer to the data to mnl_cb_run().

If you would like to mangle the payload, then you can memcpy() the
attr[NFQA_PAYLOAD] and specify how many extra bytes (unused) are
available in the new buffer.

If you use attr[NFQA_PAYLOAD], then extra is zero.

This already allowing you to allocate the data in the stack as you
would like to do.

Re: [PATCH libnetfilter_queue 1/2] pktbuff: add __pktb_setup()

[Duncan Roe]

On Sat, May 09, 2020 at 11:11:40AM +0200, Pablo Neira Ayuso wrote:
> Add private helper function to set up the pkt_buff object.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  src/extra/pktbuff.c | 54 +++++++++++++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 22 deletions(-)
>
> diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
> index 6dd0ca98aff2..118ad898f63b 100644
> --- a/src/extra/pktbuff.c
> +++ b/src/extra/pktbuff.c
> @@ -29,6 +29,34 @@
>   * @{
>   */
>
> +static int __pktb_setup(int family, struct pkt_buff *pktb)

Apart from breakage, this is pktb_setup_family() as I posted in
https://www.spinics.net/lists/netfilter-devel/msg66710.html, with args reversed.

I also added pktb_setup_metadata() to capture 3 other duplicate code lines,
you may not think that worth doing: personal choice I guess.

> +{
> +	struct ethhdr *ethhdr;
> +
> +	switch (family) {
> +	case AF_INET:
> +	case AF_INET6:
> +		pktb->network_header = pktb->data;
> +		break;
> +	case AF_BRIDGE:
> +		ethhdr = (struct ethhdr *)pktb->data;
> +		pktb->mac_header = pktb->data;
> +
> +		switch(ethhdr->h_proto) {
> +		case ETH_P_IP:
> +		case ETH_P_IPV6:
> +			pktb->network_header = pktb->data + ETH_HLEN;
> +			break;
> +		default:
> +			/* This protocol is unsupported. */

Should have moved 'errno = EPROTONOSUPPORT;' here rather than leaving it in
pktb_alloc(). As things stand, pktb_setup() will leave errno unaltered when
this error occurs.

> +			return -1;
> +		}
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * pktb_alloc - allocate a new packet buffer
>   * \param family Indicate what family. Currently supported families are
> @@ -52,7 +80,6 @@ EXPORT_SYMBOL
>  struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
>  {
>  	struct pkt_buff *pktb;
> -	struct ethhdr *ethhdr;
>  	void *pkt_data;
>
>  	pktb = calloc(1, sizeof(struct pkt_buff) + len + extra);
> @@ -68,28 +95,11 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
>
>  	pktb->data = pkt_data;
>
> -	switch(family) {
> -	case AF_INET:
> -	case AF_INET6:
> -		pktb->network_header = pktb->data;
> -		break;
> -	case AF_BRIDGE:
> -		ethhdr = (struct ethhdr *)pktb->data;
> -		pktb->mac_header = pktb->data;
> -
> -		switch(ethhdr->h_proto) {
> -		case ETH_P_IP:
> -		case ETH_P_IPV6:
> -			pktb->network_header = pktb->data + ETH_HLEN;
> -			break;
> -		default:
> -			/* This protocol is unsupported. */
> -			errno = EPROTONOSUPPORT;
> -			free(pktb);
> -			return NULL;
> -		}
> -		break;
> +	if (__pktb_setup(family, pktb) < 0) {
> +		errno = EPROTONOSUPPORT;

As before, above line belongs in __pktb_setup()

> +		free(pktb);

You need 'return NULL;' here, as in the original code.

>  	}
> +
>  	return pktb;
>  }
>
> --
> 2.20.1
>

Re: [PATCH libnetfilter_queue 2/2] pktbuff: add pktb_head_alloc(), pktb_setup() and pktb_head_size()

[Duncan Roe]

On Sat, May 09, 2020 at 11:11:41AM +0200, Pablo Neira Ayuso wrote:
> Add two new helper functions, as alternative to pktb_alloc().
>
> * pktb_setup() allows you to skip memcpy()'ing the payload from the
>   netlink message.
>
> * pktb_head_size() returns the size of the pkt_buff opaque object.
>
> * pktb_head_alloc() allows you to allocate the pkt_buff in the heap.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  include/libnetfilter_queue/pktbuff.h |  7 +++++++
>  src/extra/pktbuff.c                  | 20 ++++++++++++++++++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/include/libnetfilter_queue/pktbuff.h b/include/libnetfilter_queue/pktbuff.h
> index 42bc153ec337..a27582b02840 100644
> --- a/include/libnetfilter_queue/pktbuff.h
> +++ b/include/libnetfilter_queue/pktbuff.h
> @@ -6,6 +6,13 @@ struct pkt_buff;
>  struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra);
>  void pktb_free(struct pkt_buff *pktb);
>
> +#define NFQ_BUFFER_SIZE	(0xffff + (MNL_SOCKET_BUFFER_SIZE / 2)
> +struct pkt_buff *pktb_setup(struct pkt_buff *pktb, int family, uint8_t *data,
> +			    size_t len, size_t extra);

Prototypes in headers are generally on 1 line - see pktb_mangle() below

> +size_t pktb_head_size(void);
> +
> +#define pktb_head_alloc()	(struct pkt_buff *)(malloc(pktb_head_size()))

Users will never know about this. doxygen is configured to only look in .c
files, you wouldn't want it any other way.

Anyway, users can figure this one out for themselves, surely?
> +
>  uint8_t *pktb_data(struct pkt_buff *pktb);
>  uint32_t pktb_len(struct pkt_buff *pktb);
>
> diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
> index 118ad898f63b..6acefbe72a9b 100644
> --- a/src/extra/pktbuff.c
> +++ b/src/extra/pktbuff.c
> @@ -103,6 +103,26 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
>  	return pktb;
>  }
>
> +EXPORT_SYMBOL
> +struct pkt_buff *pktb_setup(struct pkt_buff *pktb, int family, uint8_t *buf,
> +			    size_t len, size_t extra)

This calling sequence shuts out any possibility for pktb_mangle() to decide
whether a memcpy is needed because the packet is being lenghened.

Later in this thread, you write:

> There are "two buffers":
>
> 1) The buffer that you use to receive the netlink message. This buffer
>    is parsed via mnl_cb_run().
>
> 2) The buffer that stores the pkt_buff structure.

pktb_alloc() can acess both of these. It gets 1 in its arguments and calloc()s
the other. It could be enhanced to pass that information down to pktb_mangle()
via extra elements in struct pktbuff the way pktb_alloc2 does (I'm not
suggesting we should).

BUT, there is no way to do that for pktb_setup() as it is now. You need packet
data address & length, user buffer address & length, and family as a minimum. By
going back to the 'sk_buff' model, you don't need a 'head' argument - packet
data copy just follows the metadata immediately. 5 arguments, as the latest
iteration of pktb_alloc2 has. Suggest something like:

 struct pkt_buff *pktb_setup(int family, void *buf, size_t buflen,
                             void *data, size_t len)

I'm fine with renaming pktb_alloc2 to pktb_setup. Then, with the suggestions
below, the code is pretty much the same.

> +{

Before doing anything, you really need to memset all of pktb to zero. Otherwise
you get scenarios like this:

> (gdb) p *pktb
> $1 = {
>   mac_header = 0xffffffffffffffff <error: Cannot access memory at address 0xffffffffffffffff>,
>   network_header = 0x6052f8 <nlrxbuf+88> "`",
>   transport_header = 0x25252525ffffffff <error: Cannot access memory at address 0x25252525ffffffff>,
>   data = 0x6052f8 <nlrxbuf+88> "`",
>   len = 52,
>   data_len = 52,
>   mangled = 255,
> }

To be fair, that buffer was on the stack. If you do a pktb_head_alloc() early
on, you will likely fluke a bunch of zeroes. But, once a packet is mangled, the
mangle flag will stay on so all subsequent verdicts will contain packet contents
whether the packet was mangled or not.

> +	pktb->data_len = len + extra;
> +	pktb->data = buf;
> +	pktb->len = len;
> +
> +	if (__pktb_setup(family, pktb) < 0)
> +		return NULL;
> +
> +	return pktb;
> +}
> +
> +EXPORT_SYMBOL
> +size_t pktb_head_size(void)
> +{
> +	return sizeof(struct pkt_buff);
> +}
> +
>  /**
>   * pktb_data - get pointer to network packet
>   * \param pktb Pointer to userspace packet buffer
> --
> 2.20.1
>

Re: [PATCH libnetfilter_queue 2/2] pktbuff: add pktb_head_alloc(), pktb_setup() and pktb_head_size()

[Duncan Roe]

On Sun, May 10, 2020 at 02:09:03AM +1000, Duncan Roe wrote:
> On Sat, May 09, 2020 at 11:11:41AM +0200, Pablo Neira Ayuso wrote:
[...]
> > diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
> > index 118ad898f63b..6acefbe72a9b 100644
> > --- a/src/extra/pktbuff.c
> > +++ b/src/extra/pktbuff.c
> > @@ -103,6 +103,26 @@ struct pkt_buff *pktb_alloc(int family, void *data, size_t len, size_t extra)
> >  	return pktb;
> >  }
> >
> > +EXPORT_SYMBOL
> > +struct pkt_buff *pktb_setup(struct pkt_buff *pktb, int family, uint8_t *buf,
> > +			    size_t len, size_t extra)
> > +{
> > +	pktb->data_len = len + extra;
>
> Are you proposing to be able to use extra space in the receive buffer?
> I think that is unsafe. mnl_cb_run() steps through that bufffer and needs a
> zero following the last message to know there are no more. At least, that's
> how it looks to me on stepping through with gdb.

That's wrong - sorry. mnl_cb_run() *does* step through the received buffer but
uses the byte count at the front of each message. When there are not enough
unprocessed bytes for another message header (usually there are zero) then it
returns. So there is room to enlarge the packet in the read buffer if it is in
the last (or only) netlink message in that buffer.

In tests I have never seen more than one netlink message returned in the receive
buffer. Even stopping the nfq program with gdb and then sending 10 or so UDP
messges, each call to mnl_socket_recvfrom() only returns a single netlink
message.

If this always happens, there is no need for memcpy() at all. And even if it
doesn't, one could programatically check whether the struct nlmsghdr passed by
queue_cb() is the last in the receive buffer and memcpy() if not.

Any comments?
>
[...]

Cheers ... Duncan.