diff mbox series

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

Message ID 20200509091141.10619-2-pablo@netfilter.org
State Under Review
Delegated to: Pablo Neira
Headers show
Series [libnetfilter_queue,1/2] pktbuff: add __pktb_setup() | expand

Commit Message

Pablo Neira Ayuso May 9, 2020, 9:11 a.m. UTC
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(+)

Comments

Duncan Roe May 9, 2020, 4:09 p.m. UTC | #1
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.
Pablo Neira Ayuso May 9, 2020, 5:26 p.m. UTC | #2
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.
Duncan Roe May 13, 2020, 6:48 a.m. UTC | #3
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
>
Duncan Roe May 20, 2020, 5:54 a.m. UTC | #4
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.
diff mbox series

Patch

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