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);
> +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.