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.