| Message ID | 20200509091141.10619-2-pablo@netfilter.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [libnetfilter_queue,1/2] pktbuff: add __pktb_setup() | expand |
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.
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.
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 >
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 --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
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(+)