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

[Duncan Roe <duncan_roe@optusnet.com.au>]

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
>