Documentation question

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.

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.

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: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

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

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

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.

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.

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.