Documentation question

Documentation question

[Duncan Roe]

Hi Pablo,

Did you follow the email thread
https://www.spinics.net/lists/netfilter/msg60278.html?

In summary, OP asked:
> Good morning! I am using the nf-queue.c example from
> libnetfilter_queue repo. In the queue_cb() function, I am trying to
> get the conntrack info but this condition is always false.
>
> if(attr[NFQA_CT])
>
> I can see the flow in conntrack -L output. Anyone know what I am
> missing? Appreciate your help!

and Florian replied:
> IIRC you need to set NFQA_CFG_F_CONNTRACK in NFQA_CFG_FLAGS when setting
> up the queue.  The example only sets F_GSO, so no conntrack info is
> added.

My question is, where should all this have been documented?

`man nfq_set_queue_flags` documents NFQA_CFG_F_CONNTRACK, but
nfq_set_queue_flags() is deprecated and OP was not using it.

The modern approach is to code
> mnl_attr_put_u32(nlh, NFQA_CFG_MASK, htonl(NFQA_CFG_F_GSO));

NFQA_CFG_MASK is supplied by a libnetfilter_queue header, while
mnl_attr_put_u32() is a libmnl function. What to do?

Cheers ... Duncan.

Re: Documentation question

[Pablo Neira Ayuso]

On Mon, Jul 05, 2021 at 09:45:56AM +1000, Duncan Roe wrote:
> Hi Pablo,
> 
> Did you follow the email thread
> https://www.spinics.net/lists/netfilter/msg60278.html?
> 
> In summary, OP asked:
> > Good morning! I am using the nf-queue.c example from
> > libnetfilter_queue repo. In the queue_cb() function, I am trying to
> > get the conntrack info but this condition is always false.
> >
> > if(attr[NFQA_CT])
> >
> > I can see the flow in conntrack -L output. Anyone know what I am
> > missing? Appreciate your help!
> 
> and Florian replied:
> > IIRC you need to set NFQA_CFG_F_CONNTRACK in NFQA_CFG_FLAGS when setting
> > up the queue.  The example only sets F_GSO, so no conntrack info is
> > added.
> 
> My question is, where should all this have been documented?
> 
> `man nfq_set_queue_flags` documents NFQA_CFG_F_CONNTRACK, but
> nfq_set_queue_flags() is deprecated and OP was not using it.
> 
> The modern approach is to code
> > mnl_attr_put_u32(nlh, NFQA_CFG_MASK, htonl(NFQA_CFG_F_GSO));
>
> NFQA_CFG_MASK is supplied by a libnetfilter_queue header, while
> mnl_attr_put_u32() is a libmnl function. What to do?

NFQA_CFG_MASK is supplied by linux/netfilter/nfnetlink_queue.h

The UAPI header is the main reference, it provides the kernel
definitions for the netlink attributes.

libnetfilter_queue provides a "cache copy" of this header too, that
is: libnetfilter_queue/linux_nfnetlink_queue.h

Re: Documentation question

[Duncan Roe]

On Mon, Jul 05, 2021 at 10:56:10AM +0200, Pablo Neira Ayuso wrote:
> On Mon, Jul 05, 2021 at 09:45:56AM +1000, Duncan Roe wrote:
> > Hi Pablo,
> >
> > Did you follow the email thread
> > https://www.spinics.net/lists/netfilter/msg60278.html?
> >
> > In summary, OP asked:
> > > Good morning! I am using the nf-queue.c example from
> > > libnetfilter_queue repo. In the queue_cb() function, I am trying to
> > > get the conntrack info but this condition is always false.
> > >
> > > if(attr[NFQA_CT])
> > >
> > > I can see the flow in conntrack -L output. Anyone know what I am
> > > missing? Appreciate your help!
> >
> > and Florian replied:
> > > IIRC you need to set NFQA_CFG_F_CONNTRACK in NFQA_CFG_FLAGS when setting
> > > up the queue.  The example only sets F_GSO, so no conntrack info is
> > > added.
> >
> > My question is, where should all this have been documented?
> >
> > `man nfq_set_queue_flags` documents NFQA_CFG_F_CONNTRACK, but
> > nfq_set_queue_flags() is deprecated and OP was not using it.
> >
> > The modern approach is to code
> > > mnl_attr_put_u32(nlh, NFQA_CFG_MASK, htonl(NFQA_CFG_F_GSO));
> >
> > NFQA_CFG_MASK is supplied by a libnetfilter_queue header, while
> > mnl_attr_put_u32() is a libmnl function. What to do?
>
> NFQA_CFG_MASK is supplied by linux/netfilter/nfnetlink_queue.h
>
> The UAPI header is the main reference, it provides the kernel
> definitions for the netlink attributes.
>
> libnetfilter_queue provides a "cache copy" of this header too, that
> is: libnetfilter_queue/linux_nfnetlink_queue.h

Are you saying that the UAPI headers are adequate as documentation of how to use
the system?

If not, where should extra documentation go?

In any case, do we tell the users to use header files in linux/ or
libnetfilter_queue/ (i.e. in the yet-to-be-written SYNOPSIS in man pages)?

Cheers ... Duncan.

Re: Documentation question

[Pablo Neira Ayuso]

On Mon, Jul 05, 2021 at 11:13:36PM +1000, Duncan Roe wrote:
> On Mon, Jul 05, 2021 at 10:56:10AM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Jul 05, 2021 at 09:45:56AM +1000, Duncan Roe wrote:
> > > Hi Pablo,
> > >
> > > Did you follow the email thread
> > > https://www.spinics.net/lists/netfilter/msg60278.html?
> > >
> > > In summary, OP asked:
> > > > Good morning! I am using the nf-queue.c example from
> > > > libnetfilter_queue repo. In the queue_cb() function, I am trying to
> > > > get the conntrack info but this condition is always false.
> > > >
> > > > if(attr[NFQA_CT])
> > > >
> > > > I can see the flow in conntrack -L output. Anyone know what I am
> > > > missing? Appreciate your help!
> > >
> > > and Florian replied:
> > > > IIRC you need to set NFQA_CFG_F_CONNTRACK in NFQA_CFG_FLAGS when setting
> > > > up the queue.  The example only sets F_GSO, so no conntrack info is
> > > > added.
> > >
> > > My question is, where should all this have been documented?
> > >
> > > `man nfq_set_queue_flags` documents NFQA_CFG_F_CONNTRACK, but
> > > nfq_set_queue_flags() is deprecated and OP was not using it.
> > >
> > > The modern approach is to code
> > > > mnl_attr_put_u32(nlh, NFQA_CFG_MASK, htonl(NFQA_CFG_F_GSO));
> > >
> > > NFQA_CFG_MASK is supplied by a libnetfilter_queue header, while
> > > mnl_attr_put_u32() is a libmnl function. What to do?
> >
> > NFQA_CFG_MASK is supplied by linux/netfilter/nfnetlink_queue.h
> >
> > The UAPI header is the main reference, it provides the kernel
> > definitions for the netlink attributes.
> >
> > libnetfilter_queue provides a "cache copy" of this header too, that
> > is: libnetfilter_queue/linux_nfnetlink_queue.h
> 
> Are you saying that the UAPI headers are adequate as documentation of how to use
> the system?

I'm describing that netlink-based subsystems, such as nfnetlink_queue,
define the attributes through UAPI. The attribute definitions tell
you what you might find in the payload of the netlink message. I agree
the semantics of these attributes could be described somewhere.

> If not, where should extra documentation go?

I don't have any suggestion right now.

> In any case, do we tell the users to use header files in linux/ or
> libnetfilter_queue/ (i.e. in the yet-to-be-written SYNOPSIS in man pages)?

There are three choices, actually:

1) third party software keep its own copy of the Linux kernel UAPI
   header in its tree.

2) Use the cache copy of the UAPI header.

3) Use the UAPI header that are installed by the Linux kernel headers.

I don't see any particular benefit on either of these approach. Well,
downside for the third possibility is that you need to install the
UAPI kernel header files to compile your software. So either 1) or 2)
should be fine.

[PATCH libnetfilter_queue] src: examples: Use libnetfilter_queue cached linux headers throughout

[Duncan Roe]

A user will typically copy nf-queue.c, make changes and compile: picking up
/usr/include/linux/nfnetlink_queue.h rather than
/usr/include/libnetfilter_queue/linux_nfnetlink_queue.h as is recommended.

(Running `make nf-queue` from within libnetfilter_queue/examples will get
the private cached version of nfnetlink_queue.h which is not distributed).

Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
---
 examples/nf-queue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/examples/nf-queue.c b/examples/nf-queue.c
index 5b86e69..7ad631d 100644
--- a/examples/nf-queue.c
+++ b/examples/nf-queue.c
@@ -11,9 +11,9 @@
 #include <linux/netfilter/nfnetlink.h>
 
 #include <linux/types.h>
-#include <linux/netfilter/nfnetlink_queue.h>
 
 #include <libnetfilter_queue/libnetfilter_queue.h>
+#include <libnetfilter_queue/linux_nfnetlink_queue.h>
 
 /* NFQA_CT requires CTA_* attributes defined in nfnetlink_conntrack.h */
 #include <linux/netfilter/nfnetlink_conntrack.h>
-- 
2.17.5

[PATCH libnetfilter_queue v2] src: examples: Use libnetfilter_queue cached linux headers throughout

[Duncan Roe]

A user will typically copy nf-queue.c, make changes and compile: picking up
/usr/include/linux/nfnetlink_queue.h rather than
/usr/include/libnetfilter_queue/linux_nfnetlink_queue.h as is recommended.

libnetfilter_queue.h already includes linux_nfnetlink_queue.h so we only need
to delete the errant line.

(Running `make nf-queue` from within libnetfilter_queue/examples will get
the private cached version of nfnetlink_queue.h which is not distributed).

Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
---
v2: Don't insert a new #include
    Doesn't clash with other nearby patch
 examples/nf-queue.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/examples/nf-queue.c b/examples/nf-queue.c
index 3da2c24..e4b33b5 100644
--- a/examples/nf-queue.c
+++ b/examples/nf-queue.c
@@ -11,7 +11,6 @@
 #include <linux/netfilter/nfnetlink.h>
 
 #include <linux/types.h>
-#include <linux/netfilter/nfnetlink_queue.h>
 
 #include <libnetfilter_queue/libnetfilter_queue.h>
 
-- 
2.17.5

Re: [PATCH libnetfilter_queue v2] src: examples: Use libnetfilter_queue cached linux headers throughout

[Pablo Neira Ayuso]

On Tue, Jul 06, 2021 at 03:36:48PM +1000, Duncan Roe wrote:
> A user will typically copy nf-queue.c, make changes and compile: picking up
> /usr/include/linux/nfnetlink_queue.h rather than
> /usr/include/libnetfilter_queue/linux_nfnetlink_queue.h as is recommended.
> 
> libnetfilter_queue.h already includes linux_nfnetlink_queue.h so we only need
> to delete the errant line.
> 
> (Running `make nf-queue` from within libnetfilter_queue/examples will get
> the private cached version of nfnetlink_queue.h which is not distributed).
> 
> Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
> ---
> v2: Don't insert a new #include
>     Doesn't clash with other nearby patch
>  examples/nf-queue.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/examples/nf-queue.c b/examples/nf-queue.c
> index 3da2c24..e4b33b5 100644
> --- a/examples/nf-queue.c
> +++ b/examples/nf-queue.c
> @@ -11,7 +11,6 @@
>  #include <linux/netfilter/nfnetlink.h>
>  
>  #include <linux/types.h>
> -#include <linux/netfilter/nfnetlink_queue.h>

I remember now what the intention was.

This include is fine as is: new applications should cache a copy of
nfnetlink_queue.h in their own tree, that's the recommended way to go.
This is the approach that we follow in other existing userspace
netfilter codebase (ie. the userspace program caches the kernel UAPI
header in the tree). The linux_nfnetlink_queue.h header is a legacy
file only for backward compatibility, it should not be used for new
software. This is not documented, the use of this include in
examples/nf-queue.c was intentional.

This approach also allows to fall back to the UAPI kernel headers that
are installed in your system.

Thanks.

Re: [PATCH libnetfilter_queue v2] src: examples: Use libnetfilter_queue cached linux headers throughout

[Duncan Roe]

On Wed, Jul 07, 2021 at 12:52:31AM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jul 06, 2021 at 03:36:48PM +1000, Duncan Roe wrote:
> > A user will typically copy nf-queue.c, make changes and compile: picking up
> > /usr/include/linux/nfnetlink_queue.h rather than
> > /usr/include/libnetfilter_queue/linux_nfnetlink_queue.h as is recommended.
> >
> > libnetfilter_queue.h already includes linux_nfnetlink_queue.h so we only need
> > to delete the errant line.
> >
> > (Running `make nf-queue` from within libnetfilter_queue/examples will get
> > the private cached version of nfnetlink_queue.h which is not distributed).
> >
> > Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
> > ---
> > v2: Don't insert a new #include
> >     Doesn't clash with other nearby patch
> >  examples/nf-queue.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/examples/nf-queue.c b/examples/nf-queue.c
> > index 3da2c24..e4b33b5 100644
> > --- a/examples/nf-queue.c
> > +++ b/examples/nf-queue.c
> > @@ -11,7 +11,6 @@
> >  #include <linux/netfilter/nfnetlink.h>
> >
> >  #include <linux/types.h>
> > -#include <linux/netfilter/nfnetlink_queue.h>
>
> I remember now what the intention was.
>
> This include is fine as is: new applications should cache a copy of
> nfnetlink_queue.h in their own tree, that's the recommended way to go.
> This is the approach that we follow in other existing userspace
> netfilter codebase (ie. the userspace program caches the kernel UAPI
> header in the tree). The linux_nfnetlink_queue.h header is a legacy
> file only for backward compatibility, it should not be used for new
> software. This is not documented, the use of this include in
> examples/nf-queue.c was intentional.
>
> This approach also allows to fall back to the UAPI kernel headers that
> are installed in your system.
>
> Thanks.

Thanks for explaining. But I do see a glitch: if you put
> #include <libnetfilter_queue/libnetfilter_queue.h>
before
> #include <linux/netfilter/nfnetlink_queue.h>
then you will get libnetfilter_queue/linux_nfnetlink_queue.h because
libnetfilter_queue.h includes it.

IMHO it's highly undesirable for the order of #include stmts to make a
difference and we should do something about it.

If you like, I can submit a patch to remove the linux_nfnetlink_queue.h include
from libnetfilter_queue.h and add nfnetlink_queue.h to any sources which then
fail to compile.

Should I?

Cheers ... Duncan.

Re: [PATCH libnetfilter_queue v2] src: examples: Use libnetfilter_queue cached linux headers throughout

[Duncan Roe]

On Wed, Jul 07, 2021 at 11:58:23AM +1000, Duncan Roe wrote:
> On Wed, Jul 07, 2021 at 12:52:31AM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Jul 06, 2021 at 03:36:48PM +1000, Duncan Roe wrote:
> > > A user will typically copy nf-queue.c, make changes and compile: picking up
> > > /usr/include/linux/nfnetlink_queue.h rather than
> > > /usr/include/libnetfilter_queue/linux_nfnetlink_queue.h as is recommended.
> > >
> > > libnetfilter_queue.h already includes linux_nfnetlink_queue.h so we only need
> > > to delete the errant line.
> > >
> > > (Running `make nf-queue` from within libnetfilter_queue/examples will get
> > > the private cached version of nfnetlink_queue.h which is not distributed).
> > >
> > > Signed-off-by: Duncan Roe <duncan_roe@optusnet.com.au>
> > > ---
> > > v2: Don't insert a new #include
> > >     Doesn't clash with other nearby patch
> > >  examples/nf-queue.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/examples/nf-queue.c b/examples/nf-queue.c
> > > index 3da2c24..e4b33b5 100644
> > > --- a/examples/nf-queue.c
> > > +++ b/examples/nf-queue.c
> > > @@ -11,7 +11,6 @@
> > >  #include <linux/netfilter/nfnetlink.h>
> > >
> > >  #include <linux/types.h>
> > > -#include <linux/netfilter/nfnetlink_queue.h>
> >
> > I remember now what the intention was.
> >
> > This include is fine as is: new applications should cache a copy of
> > nfnetlink_queue.h in their own tree, that's the recommended way to go.
> > This is the approach that we follow in other existing userspace
> > netfilter codebase (ie. the userspace program caches the kernel UAPI
> > header in the tree). The linux_nfnetlink_queue.h header is a legacy
> > file only for backward compatibility, it should not be used for new
> > software. This is not documented, the use of this include in
> > examples/nf-queue.c was intentional.
> >
> > This approach also allows to fall back to the UAPI kernel headers that
> > are installed in your system.
> >
> > Thanks.
>
> Thanks for explaining. But I do see a glitch: if you put
> > #include <libnetfilter_queue/libnetfilter_queue.h>
> before
> > #include <linux/netfilter/nfnetlink_queue.h>
> then you will get libnetfilter_queue/linux_nfnetlink_queue.h because
> libnetfilter_queue.h includes it.
>
> IMHO it's highly undesirable for the order of #include stmts to make a
> difference and we should do something about it.
>
> If you like, I can submit a patch to remove the linux_nfnetlink_queue.h include
> from libnetfilter_queue.h and add nfnetlink_queue.h to any sources which then
> fail to compile.
>
> Should I?
>
> Cheers ... Duncan.

Sent patch but forgot in-reply-to.

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.

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

[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.

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

[Duncan Roe]

On Thu, Nov 21, 2019 at 12:26:17AM +0100, Florian Westphal wrote:
> Duncan Roe <duncan_roe@optusnet.com.au> wrote:
> > Deprecated nfq_set_queue_flags documents flag NFQA_CFG_F_FAIL_OPEN for kernel to
> > accept packets if the kernel queue gets full.
> >
> > Does this still work with libmnl?
>
> Yes.
> > I'm thinking we need a new "Library Setup
> > [CURRENT]" section to document available flags (including e.g. NFQA_CFG_F_GSO
> > that examples/nf-queue.c uses).
>
> Makes sense, thanks.
>
> > Maybe we need Attribute helper functions as well? (documentation *and* new
> > code).
>
> If you think it makes it easier, sure, why not.
> But it would be something like this:
>
> void nfq_nlmsg_cfg_put_flags(struct nlmsghdr *nlh, uint32_t flags)
> {
>         mnl_attr_put_u32(nlh, NFQA_CFG_FLAGS, htonl(flags));
>         mnl_attr_put_u32(nlh, NFQA_CFG_MASK, htonl(flags));
> }
>
> I'm not sure that warrants a library helper.

Many of the existing helper functions are 2-liners, some even 1 line. These
little functions often have more lines of doxygen documentation than of code.

So I think the extra helpers would fit in fine.

Cheers ... Duncan.

Re: Documentation question

[Florian Westphal]

Duncan Roe <duncan_roe@optusnet.com.au> wrote:
> Deprecated nfq_set_queue_flags documents flag NFQA_CFG_F_FAIL_OPEN for kernel to
> accept packets if the kernel queue gets full.
> 
> Does this still work with libmnl?

Yes.
> I'm thinking we need a new "Library Setup
> [CURRENT]" section to document available flags (including e.g. NFQA_CFG_F_GSO
> that examples/nf-queue.c uses).

Makes sense, thanks.

> Maybe we need Attribute helper functions as well? (documentation *and* new
> code).

If you think it makes it easier, sure, why not.
But it would be something like this:

void nfq_nlmsg_cfg_put_flags(struct nlmsghdr *nlh, uint32_t flags)
{
        mnl_attr_put_u32(nlh, NFQA_CFG_FLAGS, htonl(flags));
        mnl_attr_put_u32(nlh, NFQA_CFG_MASK, htonl(flags));
}

I'm not sure that warrants a library helper.

Documentation question

[Duncan Roe]

Hi Pablo,

Deprecated nfq_set_queue_flags documents flag NFQA_CFG_F_FAIL_OPEN for kernel to
accept packets if the kernel queue gets full.

Does this still work with libmnl?I'm thinking we need a new "Library Setup
[CURRENT]" section to document available flags (including e.g. NFQA_CFG_F_GSO
that examples/nf-queue.c uses).

Maybe we need Attribute helper functions as well? (documentation *and* new
code).

Cheers ... Duncan.

Re: Documentation question

[Pablo Neira Ayuso]

On Wed, Oct 30, 2019 at 08:38:02PM +1100, Duncan Roe wrote:
> On Wed, Oct 30, 2019 at 10:15:21AM +0100, Pablo Neira Ayuso wrote:
> > On Wed, Oct 30, 2019 at 08:07:07PM +1100, Duncan Roe wrote:
> > > Hi Pablo,
> > >
> > > When setting verdicts, does sending amended packet contents imply to accept the
> > > packet? In my app I have assumed not and that seems to work fine, but I'd like
> > > to be sure for the doco.
> >
> > If you set the verdict to NF_ACCEPT and the packet that you send back
> > to the kernel is mangled, then the kernel takes your mangled packet
> > contents.
> >
> > Thanks.
> 
> Thanks Pablo I knew that, but what happens if you send back mangled contents
> and no NF_ACCEPT or NF_DROP?
> 
> Does the kernel keep waiting until you send one of these?

If you don't specify the verdict attribute, then kernel says -EINVAL.
For reference, the function to handle the netlink message that comes
from userspace is nfqnl_recv_verdict() [1].

The nfqueue netlink protocol forces the user to send the verdict along
with the packet contents (only relevent if the contents have been
updated, if packet is left untouched, you can skip sending the packets
contents so the kernel assumes packet is not altered).

Setting verbosity mode on here, many of this information you might
already know, but I prefer this for clarity.

Thanks.

[1]
https://elixir.bootlin.com/linux/latest/source/net/netfilter/nfnetlink_queue.c#L1167

Re: Documentation question

[Duncan Roe]

On Wed, Oct 30, 2019 at 10:15:21AM +0100, Pablo Neira Ayuso wrote:
> On Wed, Oct 30, 2019 at 08:07:07PM +1100, Duncan Roe wrote:
> > Hi Pablo,
> >
> > When setting verdicts, does sending amended packet contents imply to accept the
> > packet? In my app I have assumed not and that seems to work fine, but I'd like
> > to be sure for the doco.
>
> If you set the verdict to NF_ACCEPT and the packet that you send back
> to the kernel is mangled, then the kernel takes your mangled packet
> contents.
>
> Thanks.

Thanks Pablo I knew that, but what happens if you send back mangled contents
and no NF_ACCEPT or NF_DROP?

Does the kernel keep waiting until you send one of these?

Cheers ... Duncan.

Re: Documentation question

[Pablo Neira Ayuso]

On Wed, Oct 30, 2019 at 08:07:07PM +1100, Duncan Roe wrote:
> Hi Pablo,
> 
> When setting verdicts, does sending amended packet contents imply to accept the
> packet? In my app I have assumed not and that seems to work fine, but I'd like
> to be sure for the doco.

If you set the verdict to NF_ACCEPT and the packet that you send back
to the kernel is mangled, then the kernel takes your mangled packet
contents.

Thanks.

Documentation question

[Duncan Roe]

Hi Pablo,

When setting verdicts, does sending amended packet contents imply to accept the
packet? In my app I have assumed not and that seems to work fine, but I'd like
to be sure for the doco.

Cheers ... Duncan.