Documentation question

Documentation question

[Duncan Roe]

Hi Pablo,

In pktbuff.c, the doc for pktb_mangle states that "It is appropriate to use
pktb_mangle to change the MAC header".

This is not true. pktb_mangle always mangles from the network header onwards.

I can either:

Whithdraw the offending doc items

OR:

Adjust pktb_mangle to make the doc correct. This involves changing pktb_mangle,
nfq_ip_mangle and (soon) nfq_ip6_mangle. The changes would be a no-op for
AF_INET and AF_INET6 packet buffers.

What do you think?

Cheers ... Duncan.

Re: Documentation question

[Pablo Neira Ayuso]

On Sun, Dec 15, 2019 at 01:02:20PM +1100, Duncan Roe wrote:
> Hi Pablo,
> 
> In pktbuff.c, the doc for pktb_mangle states that "It is appropriate to use
> pktb_mangle to change the MAC header".
> 
> This is not true. pktb_mangle always mangles from the network header onwards.
> 
> I can either:
> 
> Whithdraw the offending doc items
>
> OR:
> 
> Adjust pktb_mangle to make the doc correct. This involves changing pktb_mangle,
> nfq_ip_mangle and (soon) nfq_ip6_mangle. The changes would be a no-op for
> AF_INET and AF_INET6 packet buffers.
> 
> What do you think?

You could fix it through signed int dataoff. So the users could
specify a negative offset to mangle the MAC address.

This function was made to update layer 7 payload information to
implement the helpers. So dataoff usually contains the transport
header size.

Let me know, thanks.

Re: Documentation question

[Duncan Roe]

On Fri, Dec 20, 2019 at 01:29:53AM +0100, Pablo Neira Ayuso wrote:
> On Sun, Dec 15, 2019 at 01:02:20PM +1100, Duncan Roe wrote:
> > Hi Pablo,
> >
> > In pktbuff.c, the doc for pktb_mangle states that "It is appropriate to use
> > pktb_mangle to change the MAC header".
> >
> > This is not true. pktb_mangle always mangles from the network header onwards.
> >
> > I can either:
> >
> > Whithdraw the offending doc items
> >
> > OR:
> >
> > Adjust pktb_mangle to make the doc correct. This involves changing pktb_mangle,
> > nfq_ip_mangle and (soon) nfq_ip6_mangle. The changes would be a no-op for
> > AF_INET and AF_INET6 packet buffers.
> >
> > What do you think?
>
> You could fix it through signed int dataoff. So the users could
> specify a negative offset to mangle the MAC address.
>
> This function was made to update layer 7 payload information to
> implement the helpers. So dataoff usually contains the transport
> header size.
>
> Let me know, thanks.
>
-ve offsets? There has to be a better way.

When I added documentation for pktb_mangle, I assumed it mangled from
pktb->data, rather than checking the source.

That is the function I documented, and I think we need a function like that.

Rather than change the behaviour of pktb_mangle when a MAC header is present, I
propose a new function pktb_mangle2 which mangles from pktb->data onwards.

pktb_mangle would call this new function, with dataoff incremented by
pktb->network_header - pktb->data (only nonzero for AF_BRIDGE)

Ok?

Cheers ... Duncan.

Re: Documentation question

[Duncan Roe]

On Sat, Dec 21, 2019 at 09:43:45PM +1100, Duncan Roe wrote:
> On Fri, Dec 20, 2019 at 01:29:53AM +0100, Pablo Neira Ayuso wrote:
> > On Sun, Dec 15, 2019 at 01:02:20PM +1100, Duncan Roe wrote:
> > > Hi Pablo,
> > >
> > > In pktbuff.c, the doc for pktb_mangle states that "It is appropriate to use
> > > pktb_mangle to change the MAC header".
> > >
> > > This is not true. pktb_mangle always mangles from the network header onwards.
> > >
> > > I can either:
> > >
> > > Whithdraw the offending doc items
> > >
> > > OR:
> > >
> > > Adjust pktb_mangle to make the doc correct. This involves changing pktb_mangle,
> > > nfq_ip_mangle and (soon) nfq_ip6_mangle. The changes would be a no-op for
> > > AF_INET and AF_INET6 packet buffers.
> > >
> > > What do you think?
> >
> > You could fix it through signed int dataoff. So the users could
> > specify a negative offset to mangle the MAC address.
> >
> > This function was made to update layer 7 payload information to
> > implement the helpers. So dataoff usually contains the transport
> > header size.
> >
> > Let me know, thanks.
> >
> -ve offsets? There has to be a better way.
>
> When I added documentation for pktb_mangle, I assumed it mangled from
> pktb->data, rather than checking the source.
>
> That is the function I documented, and I think we need a function like that.
>
> Rather than change the behaviour of pktb_mangle when a MAC header is present, I
> propose a new function pktb_mangle2 which mangles from pktb->data onwards.
>
> pktb_mangle would call this new function, with dataoff incremented by
> pktb->network_header - pktb->data (only nonzero for AF_BRIDGE)
>
> Ok?
>
> Cheers ... Duncan.
>
On second thoughts, I'll just do the signed offset thing and have done with it.
Hope you can accept it quickly: I'll base it on master so you can apply it
before considering the pktb_usebuf() patch.

[PATCH libnetfilter_queue] src: pktb_mangle has signed offset arg so can mangle MAC header with -ve one

[Duncan Roe]

- Update prototype
- Update doxygen documentation
- Update declaration

Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
---
 include/libnetfilter_queue/pktbuff.h | 2 +-
 src/extra/pktbuff.c                  | 8 +++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/libnetfilter_queue/pktbuff.h b/include/libnetfilter_queue/pktbuff.h
index b15ee1e..5bcc3e5 100644
--- a/include/libnetfilter_queue/pktbuff.h
+++ b/include/libnetfilter_queue/pktbuff.h
@@ -19,7 +19,7 @@ uint8_t *pktb_mac_header(struct pkt_buff *pktb);
 uint8_t *pktb_network_header(struct pkt_buff *pktb);
 uint8_t *pktb_transport_header(struct pkt_buff *pktb);
 
-int pktb_mangle(struct pkt_buff *pkt, unsigned int dataoff, unsigned int match_offset, unsigned int match_len, const char *rep_buffer, unsigned int rep_len);
+int pktb_mangle(struct pkt_buff *pkt, int dataoff, unsigned int match_offset, unsigned int match_len, const char *rep_buffer, unsigned int rep_len);
 
 bool pktb_mangled(const struct pkt_buff *pktb);
 
diff --git a/src/extra/pktbuff.c b/src/extra/pktbuff.c
index c4f3da3..6250fbf 100644
--- a/src/extra/pktbuff.c
+++ b/src/extra/pktbuff.c
@@ -299,8 +299,10 @@ static int enlarge_pkt(struct pkt_buff *pkt, unsigned int extra)
 /**
  * pktb_mangle - adjust contents of a packet
  * \param pktb Pointer to userspace packet buffer
- * \param dataoff Offset to layer 4 header. Specify zero to access layer 3 (IP)
- * header (layer 2 for family \b AF_BRIDGE)
+ * \param dataoff Supplementary offset, usually offset from layer 3 (IP) header
+ * to the layer 4 (TCP or UDP) header. Specify zero to access the layer 3
+ * header. If \b pktb was created in family \b AF_BRIDGE, specify
+ * \b -ETH_HLEN (a negative offset) to access the layer 2 (MAC) header.
  * \param match_offset Further offset to content that you want to mangle
  * \param match_len Length of the existing content you want to mangle
  * \param rep_buffer Pointer to data you want to use to replace current content
@@ -316,7 +318,7 @@ static int enlarge_pkt(struct pkt_buff *pkt, unsigned int extra)
  */
 EXPORT_SYMBOL
 int pktb_mangle(struct pkt_buff *pktb,
-		unsigned int dataoff,
+		int dataoff,
 		unsigned int match_offset,
 		unsigned int match_len,
 		const char *rep_buffer,
-- 
2.14.5

Re: [PATCH libnetfilter_queue] src: pktb_mangle has signed offset arg so can mangle MAC header with -ve one

[Pablo Neira Ayuso]

Applied, thanks.