Re: nftables code size

[Matthias Schiffer]

On 04/11/2018 01:53 PM, Phil Sutter wrote:
> Hi,
> 
> On Wed, Apr 11, 2018 at 11:10:36AM +0200, Matthias Schiffer wrote:
>> On 04/11/2018 10:47 AM, Pablo Neira Ayuso wrote:
>>> On Wed, Apr 11, 2018 at 10:45:27AM +0200, Matthias Schiffer wrote:
 On 04/11/2018 10:03 AM, Pablo Neira Ayuso wrote:
> On Wed, Apr 11, 2018 at 06:40:34AM +0200, Matthias Schiffer wrote:
>> On 03/04/2018 12:16 PM, Matthias Schiffer wrote:
>>> I noticed that more than 25% of binary size of libnftnl are made up of
>>> snprintf functions. Having these in a library with the goal to abstract 
>>> the
>>> netlink interface of nftables seems questionable to me, but I have no 
>>> idea
>>> if it would be viable to move these functions to nft or to a separate 
>>> library.
>>
>> As an experiment, I created a reduced version of libnftnl by ripping out
>> all import/export functions and related code like buffer handling. This
>> reduced the size of libnftnl.so from 155KB to 110KB (on x86-64, -Os,
>> stripped, uncompressed), a reduction of roughly 30%.
>>
>> I would like to look into splitting libnftnl into two parts, which could 
>> be
>> called libnftnl-core and libnftnl, to make nftables more suited for tiny
>> embedded systems. All basic functions that do not deal with textual
>> representations of rules (i.e. the reduced libnftnl I built) would be 
>> moved
>> into libnftnl-core.
>>
>> Does this sound like a good idea, and would such a drastic change be
>> acceptable for upstream inclusion, given the current libnftnl API can be
>> preserved?
>
> Could you wrap the import/export code around the --with-json-parsing?
> I mean, turn this into --with-json or such, so you can just compile
> out that code, which is what is giving you the savings in terms of
> size, right?
>
> I'm trying to keep it simple here :-)
>

 If possible, I'd not only like to get rid of the JSON export support, but
 also the snprintf_default code; in short, anything dealing with
 human-readable rules, as this code is simply not necessary for a firewall
 application that just wants to configure rulesets.

 A libnftables without any printf support (i.e. without
 nftnl_ruleset_fprintf() etc.) would not be sufficient to run the nft CLI
 utility. In consequence, we (OpenWrt/Gluon) would need to ship two
 different flavous of libnftables: A "tiny" version for small devices, and a
 "full" version for users that want to use the nft CLI to read/write the
 low-level rules. Separating libnftnl into a core library and an
 import/export library used by nft seems like better software design to me
 than adding more configuration switches.
>>>
>>> I understand, but probably that json support will be deprecated soon
>>> because IIRC Phil is working on something better.
>>
>> Any details on this replacement? Will it be some kind of binary
>> import/export, or another textual representation?
> 
> I'm currently working on an alternative to the standard syntax in nft
> (or precisely libnftables) which is JSON based. Since with my patches in
> place, people will be able to call 'nft -j list ruleset' and use its
> output as full replacement to 'nft export vm json', we may get rid of
> all JSON export functionality in libnftnl (which the latter command just
> wraps).
> 
> OTOH I don't expect any users for (any) JSON interface on an embedded
> system at all, so I guess with JSON support in libnftnl wrapping related
> functions in #ifdef's should be the right approach.
> 
>>> So I'm not sure it's worth to split this library in two, then to
>>> deprecate the non-core part just weeks later. That's why I think
>>> configure time option will be less work for you and will allow us to
>>> make an step to let this code go away.
>>
>> No problem, we don't have a clear plan for the migration to nftables in
>> OpenWrt or Gluon yet, so if this replacement is likely to be finished (or
>> at least under review) in a few weeks, I can just wait that long and
>> reevaluate my idea before doing unnecessary work :)
> 
> I wonder how you plan to interface with nftables if you consider
> disabling human readable output altogether. Since libnftables is
> completely based on the human readable representation, it is pretty much
> useless without text output (unless you're fine with write-only
> interface ;).
> 
> Then OTOH there's libnftnl, but interfacing with that directly is pretty
> painful. Is this amongst your options to choose from?

I plan to use libnftnl directly. If removing parts of the API from libnftnl
is acceptable at this point, moving all parts in libnftnl dealing with
human-readable or JSON input/output to libnftables would be an alternative
to splitting another library out of libnftnl.

I have not fully decided on my approach yet, but I will probably end up
doing one or both of
1) replacing the iptables 

Re: nftables code size

[Phil Sutter]

Hi,

On Wed, Apr 11, 2018 at 11:10:36AM +0200, Matthias Schiffer wrote:
> On 04/11/2018 10:47 AM, Pablo Neira Ayuso wrote:
> > On Wed, Apr 11, 2018 at 10:45:27AM +0200, Matthias Schiffer wrote:
> >> On 04/11/2018 10:03 AM, Pablo Neira Ayuso wrote:
> >>> On Wed, Apr 11, 2018 at 06:40:34AM +0200, Matthias Schiffer wrote:
>  On 03/04/2018 12:16 PM, Matthias Schiffer wrote:
> > I noticed that more than 25% of binary size of libnftnl are made up of
> > snprintf functions. Having these in a library with the goal to abstract 
> > the
> > netlink interface of nftables seems questionable to me, but I have no 
> > idea
> > if it would be viable to move these functions to nft or to a separate 
> > library.
> 
>  As an experiment, I created a reduced version of libnftnl by ripping out
>  all import/export functions and related code like buffer handling. This
>  reduced the size of libnftnl.so from 155KB to 110KB (on x86-64, -Os,
>  stripped, uncompressed), a reduction of roughly 30%.
> 
>  I would like to look into splitting libnftnl into two parts, which could 
>  be
>  called libnftnl-core and libnftnl, to make nftables more suited for tiny
>  embedded systems. All basic functions that do not deal with textual
>  representations of rules (i.e. the reduced libnftnl I built) would be 
>  moved
>  into libnftnl-core.
> 
>  Does this sound like a good idea, and would such a drastic change be
>  acceptable for upstream inclusion, given the current libnftnl API can be
>  preserved?
> >>>
> >>> Could you wrap the import/export code around the --with-json-parsing?
> >>> I mean, turn this into --with-json or such, so you can just compile
> >>> out that code, which is what is giving you the savings in terms of
> >>> size, right?
> >>>
> >>> I'm trying to keep it simple here :-)
> >>>
> >>
> >> If possible, I'd not only like to get rid of the JSON export support, but
> >> also the snprintf_default code; in short, anything dealing with
> >> human-readable rules, as this code is simply not necessary for a firewall
> >> application that just wants to configure rulesets.
> >>
> >> A libnftables without any printf support (i.e. without
> >> nftnl_ruleset_fprintf() etc.) would not be sufficient to run the nft CLI
> >> utility. In consequence, we (OpenWrt/Gluon) would need to ship two
> >> different flavous of libnftables: A "tiny" version for small devices, and a
> >> "full" version for users that want to use the nft CLI to read/write the
> >> low-level rules. Separating libnftnl into a core library and an
> >> import/export library used by nft seems like better software design to me
> >> than adding more configuration switches.
> > 
> > I understand, but probably that json support will be deprecated soon
> > because IIRC Phil is working on something better.
> 
> Any details on this replacement? Will it be some kind of binary
> import/export, or another textual representation?

I'm currently working on an alternative to the standard syntax in nft
(or precisely libnftables) which is JSON based. Since with my patches in
place, people will be able to call 'nft -j list ruleset' and use its
output as full replacement to 'nft export vm json', we may get rid of
all JSON export functionality in libnftnl (which the latter command just
wraps).

OTOH I don't expect any users for (any) JSON interface on an embedded
system at all, so I guess with JSON support in libnftnl wrapping related
functions in #ifdef's should be the right approach.

> > So I'm not sure it's worth to split this library in two, then to
> > deprecate the non-core part just weeks later. That's why I think
> > configure time option will be less work for you and will allow us to
> > make an step to let this code go away.
> 
> No problem, we don't have a clear plan for the migration to nftables in
> OpenWrt or Gluon yet, so if this replacement is likely to be finished (or
> at least under review) in a few weeks, I can just wait that long and
> reevaluate my idea before doing unnecessary work :)

I wonder how you plan to interface with nftables if you consider
disabling human readable output altogether. Since libnftables is
completely based on the human readable representation, it is pretty much
useless without text output (unless you're fine with write-only
interface ;).

Then OTOH there's libnftnl, but interfacing with that directly is pretty
painful. Is this amongst your options to choose from?

> I'll also gather a few more numbers (savings in code size for compressed
> MIPS16 binaries, which is my platform of interest) and compare the sizes of
> completely removing printf support vs. only removing JSON printing and
> keeping snprintf_default intact.

I'm looking forward to that, thanks!

Cheers, Phil
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  

Re: [nft PATCH 0/8] Convert tests/py to use libnftables directly

[Phil Sutter]

Hi Pablo,

On Wed, Apr 11, 2018 at 09:39:01AM +0200, Pablo Neira Ayuso wrote:
> Two comments, both related to 'struct cookie':
> 
> * How is 'struct cookie' going to be used? Do you have follow up
>   patches to update the codebase to use this?

It's already there, please have a look at
nft_ctx_{un,}buffer_{output,error} functions and those they call
(init_cookie, exit_cookie). The API interface they establish can be used
as such:

| struct nft_ctx *ctx;
| const char *cmd;
| int rc;
| 
| ctx = nft_ctx_new(0);
| nft_buffer_output(ctx);
| nft_buffer_error(ctx);
| 
| cmd = "list ruleset";
| rc = nft_run_cmd_from_buffer(ctx, cmd, sizeof(cmd));
| 
| if (rc) {
|   printf("FAIL: nftables says: %s\n",
|  nft_ctx_get_error_buffer(ctx));
| } else {
|   /* this prints 'list ruleset' output */
|   printf("%s\n", nft_ctx_get_output_buffer(ctx));
| }
| 
| nft_ctx_free(ctx);

So these nft_ctx_buffer_* and nft_ctx_get_*_buffer functions are an
alternative to nft_ctx_set_{output,error} functions for cases where it
is more convenient to have all output available in a char buffer instead
of being written to an open FILE pointer.

> * Can we avoid the union around struct cookie and FILE *fp? I mean, I
>   don't think it makes sense to start exposing lots of toggles through
>   the API, the more we expose, the more combinations are possible and
>   then more chances we get to support all kind of strange combinations
>   in the future.

That union is simply for convenience: {init,exit}_cookie functions take
a pointer to struct cookie as parameter and since it is part of that
union they get access to the "standard" FILE pointer which is used if
no buffering was requested.

That union is introduced by commit 4885331de84cb ("libnftables: Simplify
cookie integration"), a followup to 40d94ca1dcd8e ("libnftables: Support
buffering output and error") - please have a look at the latter to see
how things were without it.

>   So what I'm suggesting is: Can we turn 'struct cookie' into the
>   default way to print messages?

Sure, we could do that. OTOH there is e.g. nft cli, which doesn't care
about what's printed to stdout and stderr. Changing the default would
mean cli has to manually print to stdout and stderr after each command.

Internally, this buffering infrastructure is an add-on to FILE pointer
output we already have (which was introduced as the quickest path from
plain printf's everywhere to giving API users control over library
output).

I really think we should keep the FILE pointer output (i.e. nft_print())
internally, simply because changing everything to snprintf() is tedious
due to buffer length checks everywhere (I'm having libnftnl's
SNPRINTF_BUFFER_SIZE macro in mind ;).

The buffering add-on doesn't necessarily add complexity to API users'
implementations - prior to it, there were two options:

* Don't care and go with the default, i.e. output goes to stdout and
  stderr.

* Provide a FILE pointer to print into, i.e. call nft_ctx_set_output().

The add-on adds a third, completely separate option to them:

* Get output in a buffer, i.e. call nft_ctx_buffer_output() and use
  nft_ctx_get_output_buffer() afterwards to receive a pointer to the
  buffer.

>   Regarding the name 'struct cookie', is this refering to something
>   related to Python? I mean, this name tells me very little. What I
>   can see there is a new class that allows us to do buffering, so
>   probably using 'struct nft_buffer' or such sound more convenient to
>   me?

I chose it simply because it all evolves around fopencookie(3), which
provides the needed functionality (create a FILE pointer with
customizable write() backend). In the documentation, the opaque data
pointer passed around to callbacks is referred to as 'cookie'.

On Wed, Apr 11, 2018 at 09:48:25AM +0200, Pablo Neira Ayuso wrote:
> On Tue, Apr 10, 2018 at 07:00:21PM +0200, Phil Sutter wrote:
[...]
> > diff --git a/src/libnftables.c b/src/libnftables.c
> > index 8bf989b08cc54..d6622d51aba33 100644
> > --- a/src/libnftables.c
> > +++ b/src/libnftables.c
> > @@ -169,6 +169,7 @@ struct nft_ctx *nft_ctx_new(uint32_t flags)
> > init_list_head(>cache.list);
> > ctx->flags = flags;
> > ctx->output.output_fp = stdout;
> > +   ctx->output.error_fp = stderr;
> 
> I understand you may need this new toggle. But can we probably
> reasonable defaults? I mean, if no error_fp is specified, then use the
> one set by output_fp.

This was already the default, see this chunk from commit 4176e24e14f07
("libnftables: Introduce nft_ctx_set_error()"):

| @@ -297,9 +309,7 @@ int nft_run_cmd_from_buffer(struct nft_ctx *nft, char 
*buf, size_t buflen)
| if (nft_run(nft, nft->nf_sock, scanner, , ) != 0)
| rc = -1;
|  
| -   fp = nft_ctx_set_output(nft, stderr);
| erec_print_list(>output, , nft->debug_mask);
| -   nft_ctx_set_output(nft, fp);
| scanner_destroy(scanner);
| iface_cache_release();
| free(nlbuf);

So for 

Re: nftables code size

[Matthias Schiffer]

On 04/11/2018 10:47 AM, Pablo Neira Ayuso wrote:
> On Wed, Apr 11, 2018 at 10:45:27AM +0200, Matthias Schiffer wrote:
>> On 04/11/2018 10:03 AM, Pablo Neira Ayuso wrote:
>>> On Wed, Apr 11, 2018 at 06:40:34AM +0200, Matthias Schiffer wrote:
 On 03/04/2018 12:16 PM, Matthias Schiffer wrote:
> I noticed that more than 25% of binary size of libnftnl are made up of
> snprintf functions. Having these in a library with the goal to abstract 
> the
> netlink interface of nftables seems questionable to me, but I have no idea
> if it would be viable to move these functions to nft or to a separate 
> library.

 As an experiment, I created a reduced version of libnftnl by ripping out
 all import/export functions and related code like buffer handling. This
 reduced the size of libnftnl.so from 155KB to 110KB (on x86-64, -Os,
 stripped, uncompressed), a reduction of roughly 30%.

 I would like to look into splitting libnftnl into two parts, which could be
 called libnftnl-core and libnftnl, to make nftables more suited for tiny
 embedded systems. All basic functions that do not deal with textual
 representations of rules (i.e. the reduced libnftnl I built) would be moved
 into libnftnl-core.

 Does this sound like a good idea, and would such a drastic change be
 acceptable for upstream inclusion, given the current libnftnl API can be
 preserved?
>>>
>>> Could you wrap the import/export code around the --with-json-parsing?
>>> I mean, turn this into --with-json or such, so you can just compile
>>> out that code, which is what is giving you the savings in terms of
>>> size, right?
>>>
>>> I'm trying to keep it simple here :-)
>>>
>>
>> If possible, I'd not only like to get rid of the JSON export support, but
>> also the snprintf_default code; in short, anything dealing with
>> human-readable rules, as this code is simply not necessary for a firewall
>> application that just wants to configure rulesets.
>>
>> A libnftables without any printf support (i.e. without
>> nftnl_ruleset_fprintf() etc.) would not be sufficient to run the nft CLI
>> utility. In consequence, we (OpenWrt/Gluon) would need to ship two
>> different flavous of libnftables: A "tiny" version for small devices, and a
>> "full" version for users that want to use the nft CLI to read/write the
>> low-level rules. Separating libnftnl into a core library and an
>> import/export library used by nft seems like better software design to me
>> than adding more configuration switches.
> 
> I understand, but probably that json support will be deprecated soon
> because IIRC Phil is working on something better.

Any details on this replacement? Will it be some kind of binary
import/export, or another textual representation?

> So I'm not sure it's worth to split this library in two, then to
> deprecate the non-core part just weeks later. That's why I think
> configure time option will be less work for you and will allow us to
> make an step to let this code go away.

No problem, we don't have a clear plan for the migration to nftables in
OpenWrt or Gluon yet, so if this replacement is likely to be finished (or
at least under review) in a few weeks, I can just wait that long and
reevaluate my idea before doing unnecessary work :)

I'll also gather a few more numbers (savings in code size for compressed
MIPS16 binaries, which is my platform of interest) and compare the sizes of
completely removing printf support vs. only removing JSON printing and
keeping snprintf_default intact.



signature.asc
Description: OpenPGP digital signature

Re: nftables code size

[Pablo Neira Ayuso]

On Wed, Apr 11, 2018 at 10:45:27AM +0200, Matthias Schiffer wrote:
> On 04/11/2018 10:03 AM, Pablo Neira Ayuso wrote:
> > On Wed, Apr 11, 2018 at 06:40:34AM +0200, Matthias Schiffer wrote:
> >> On 03/04/2018 12:16 PM, Matthias Schiffer wrote:
> >>> I noticed that more than 25% of binary size of libnftnl are made up of
> >>> snprintf functions. Having these in a library with the goal to abstract 
> >>> the
> >>> netlink interface of nftables seems questionable to me, but I have no idea
> >>> if it would be viable to move these functions to nft or to a separate 
> >>> library.
> >>
> >> As an experiment, I created a reduced version of libnftnl by ripping out
> >> all import/export functions and related code like buffer handling. This
> >> reduced the size of libnftnl.so from 155KB to 110KB (on x86-64, -Os,
> >> stripped, uncompressed), a reduction of roughly 30%.
> >>
> >> I would like to look into splitting libnftnl into two parts, which could be
> >> called libnftnl-core and libnftnl, to make nftables more suited for tiny
> >> embedded systems. All basic functions that do not deal with textual
> >> representations of rules (i.e. the reduced libnftnl I built) would be moved
> >> into libnftnl-core.
> >>
> >> Does this sound like a good idea, and would such a drastic change be
> >> acceptable for upstream inclusion, given the current libnftnl API can be
> >> preserved?
> > 
> > Could you wrap the import/export code around the --with-json-parsing?
> > I mean, turn this into --with-json or such, so you can just compile
> > out that code, which is what is giving you the savings in terms of
> > size, right?
> > 
> > I'm trying to keep it simple here :-)
> > 
> 
> If possible, I'd not only like to get rid of the JSON export support, but
> also the snprintf_default code; in short, anything dealing with
> human-readable rules, as this code is simply not necessary for a firewall
> application that just wants to configure rulesets.
> 
> A libnftables without any printf support (i.e. without
> nftnl_ruleset_fprintf() etc.) would not be sufficient to run the nft CLI
> utility. In consequence, we (OpenWrt/Gluon) would need to ship two
> different flavous of libnftables: A "tiny" version for small devices, and a
> "full" version for users that want to use the nft CLI to read/write the
> low-level rules. Separating libnftnl into a core library and an
> import/export library used by nft seems like better software design to me
> than adding more configuration switches.

I understand, but probably that json support will be deprecated soon
because IIRC Phil is working on something better.

So I'm not sure it's worth to split this library in two, then to
deprecate the non-core part just weeks later. That's why I think
configure time option will be less work for you and will allow us to
make an step to let this code go away.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: nftables code size

[Matthias Schiffer]

On 04/11/2018 10:03 AM, Pablo Neira Ayuso wrote:
> On Wed, Apr 11, 2018 at 06:40:34AM +0200, Matthias Schiffer wrote:
>> On 03/04/2018 12:16 PM, Matthias Schiffer wrote:
>>> I noticed that more than 25% of binary size of libnftnl are made up of
>>> snprintf functions. Having these in a library with the goal to abstract the
>>> netlink interface of nftables seems questionable to me, but I have no idea
>>> if it would be viable to move these functions to nft or to a separate 
>>> library.
>>
>> As an experiment, I created a reduced version of libnftnl by ripping out
>> all import/export functions and related code like buffer handling. This
>> reduced the size of libnftnl.so from 155KB to 110KB (on x86-64, -Os,
>> stripped, uncompressed), a reduction of roughly 30%.
>>
>> I would like to look into splitting libnftnl into two parts, which could be
>> called libnftnl-core and libnftnl, to make nftables more suited for tiny
>> embedded systems. All basic functions that do not deal with textual
>> representations of rules (i.e. the reduced libnftnl I built) would be moved
>> into libnftnl-core.
>>
>> Does this sound like a good idea, and would such a drastic change be
>> acceptable for upstream inclusion, given the current libnftnl API can be
>> preserved?
> 
> Could you wrap the import/export code around the --with-json-parsing?
> I mean, turn this into --with-json or such, so you can just compile
> out that code, which is what is giving you the savings in terms of
> size, right?
> 
> I'm trying to keep it simple here :-)
> 

If possible, I'd not only like to get rid of the JSON export support, but
also the snprintf_default code; in short, anything dealing with
human-readable rules, as this code is simply not necessary for a firewall
application that just wants to configure rulesets.

A libnftables without any printf support (i.e. without
nftnl_ruleset_fprintf() etc.) would not be sufficient to run the nft CLI
utility. In consequence, we (OpenWrt/Gluon) would need to ship two
different flavous of libnftables: A "tiny" version for small devices, and a
"full" version for users that want to use the nft CLI to read/write the
low-level rules. Separating libnftnl into a core library and an
import/export library used by nft seems like better software design to me
than adding more configuration switches.

Kind regards,
Matthias



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2] iptables: constify option struct

[Pablo Neira Ayuso]

On Wed, Mar 21, 2018 at 03:20:28PM +0530, Arushi Singhal wrote:
> The struct of type option is only used to initialise a field and
> is not modified anywhere.

I have placed these bits into iptables.git, thanks Arushi.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ulogd PATCH] ulogd2: cleanup downstream files

[Pablo Neira Ayuso]

On Tue, Apr 03, 2018 at 12:08:09PM +0200, Arturo Borrero Gonzalez wrote:
> These files are outdated and they belong to downstream users (distributions).
> Providing outdated and unmaintained files here serves no purpose other than
> confusing users and annoy packagers.
> 
> If an user is using ulogd2 directly from the source tarball, I would expect it
> to be proficient enough to generate these files by itself.

Looks good, applied, thanks Arturo.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xt_connmark: Add bit mapping for bit-shift operation.

[Pablo Neira Ayuso]

On Fri, Apr 06, 2018 at 11:14:12AM +0200, Florian Westphal wrote:
> Jack Ma  wrote:
> > With the additiona of bit-shift operations, we are able
> > to shift ct/skbmark based on user requirements. However,
> > this change might also cause the most left/right hand-
> > side mark to be accidentially lost during shift operations.
> > 
> > This patch adds the ability to 'grep' ceratin bits based
> > on ctmask or nfmask out of the original mark. Then apply
> > shift operations to achieve a new mapping between ctmark
> > and skb->mark.
> > 
> > For example.
> > 
> > If someone would like save the fourth F bits of ctmark 0xFFF(F)000F
> > into the seventh hexadecimal (0) skb->mark 0xABC000(0)E.
> > 
> > new_targetmark = (ctmark & ctmask) >> 12;
> > (new) skb->mark = (skb->mark &~nfmask) ^
> >new_targetmark;
> > 
> > This will preserve the other bits that are not related to
> > this operation.
> > 
> > Reviewed-by: Florian Westphal 
> 
> I don't recall having seen this patch before.
> 
> That being said, it looks ok to me.
> 
> You might have mentioned that this patch is for 'nf' tree, after
> having merged -next, as this patch changes user-visible behaviour
> (which should be fine in this case as the original change isn't part
>  of 4.16).

Applied to nf.git including the Fixes: tag.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH iptables] ebtables-compat: add initial translations

[Pablo Neira Ayuso]

On Wed, Apr 11, 2018 at 10:24:37AM +0200, Florian Westphal wrote:
> add translations for ip, limit, log, mark, mark_m, nflog.

LGTM!

> Signed-off-by: Florian Westphal 
> ---
>  NB: No tests yet, I need to implement 'ebtables-translate' first :-)

:-)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH iptables] ebtables-compat: add initial translations

[Florian Westphal]

add translations for ip, limit, log, mark, mark_m, nflog.

Signed-off-by: Florian Westphal 
---
 NB: No tests yet, I need to implement 'ebtables-translate' first :-)

 extensions/libebt_ip.c | 128 +
 extensions/libebt_limit.c  |  26 +
 extensions/libebt_log.c|  22 
 extensions/libebt_mark.c   |  44 
 extensions/libebt_mark_m.c |  25 +
 extensions/libebt_nflog.c  |  25 +
 6 files changed, 270 insertions(+)

diff --git a/extensions/libebt_ip.c b/extensions/libebt_ip.c
index 4ca63e939066..1a87585c533f 100644
--- a/extensions/libebt_ip.c
+++ b/extensions/libebt_ip.c
@@ -291,6 +291,133 @@ static void brip_print(const void *ip, const struct 
xt_entry_match *match,
}
 }
 
+static const char *brip_xlate_proto_to_name(uint8_t proto)
+{
+   switch (proto) {
+   case IPPROTO_TCP:
+   return "tcp";
+   case IPPROTO_UDP:
+   return "udp";
+   case IPPROTO_UDPLITE:
+   return "udplite";
+   case IPPROTO_SCTP:
+   return "sctp";
+   case IPPROTO_DCCP:
+   return "dccp";
+   default:
+   return NULL;
+   }
+}
+
+static void brip_xlate_th(struct xt_xlate *xl,
+ const struct ebt_ip_info *info, int bit,
+ const char *pname)
+{
+   const uint16_t *ports;
+
+   if ((info->bitmask & bit) == 0)
+   return;
+
+   switch (bit) {
+   case EBT_IP_SPORT:
+   if (pname)
+   xt_xlate_add(xl, "%s sport ", pname);
+   else
+   xt_xlate_add(xl, "@th,0,16 ");
+
+   ports = info->sport;
+   break;
+   case EBT_IP_DPORT:
+   if (pname)
+   xt_xlate_add(xl, "%s dport ", pname);
+   else
+   xt_xlate_add(xl, "@th,16,16 ");
+
+   ports = info->dport;
+   break;
+   default:
+   return;
+   }
+
+   if (info->invflags & bit)
+   xt_xlate_add(xl, "!= ");
+
+   if (ports[0] == ports[1])
+   xt_xlate_add(xl, "%d ", ports[0]);
+   else
+   xt_xlate_add(xl, "%d-%d ", ports[0], ports[1]);
+}
+
+static void brip_xlate_nh(struct xt_xlate *xl,
+ const struct ebt_ip_info *info, int bit)
+{
+   struct in_addr *addrp, *maskp;
+
+   if ((info->bitmask & bit) == 0)
+   return;
+
+   switch (bit) {
+   case EBT_IP_SOURCE:
+   xt_xlate_add(xl, "ip saddr ");
+   addrp = (struct in_addr *)>saddr;
+   maskp = (struct in_addr *)>smsk;
+   break;
+   case EBT_IP_DEST:
+   xt_xlate_add(xl, "ip daddr ");
+   addrp = (struct in_addr *)>daddr;
+   maskp = (struct in_addr *)>dmsk;
+   break;
+   default:
+   return;
+   }
+
+   if (info->invflags & bit)
+   xt_xlate_add(xl, "!= ");
+
+   xt_xlate_add(xl, "%s%s ", xtables_ipaddr_to_numeric(addrp),
+ xtables_ipmask_to_numeric(maskp));
+}
+
+static int brip_xlate(struct xt_xlate *xl,
+ const struct xt_xlate_mt_params *params)
+{
+   const struct ebt_ip_info *info = (const void *)params->match->data;
+   const char *pname = NULL;
+
+   brip_xlate_nh(xl, info, EBT_IP_SOURCE);
+   brip_xlate_nh(xl, info, EBT_IP_DEST);
+
+   if (info->bitmask & EBT_IP_TOS) {
+   xt_xlate_add(xl, "ip dscp ");
+   if (info->invflags & EBT_IP_TOS)
+   xt_xlate_add(xl, "!= ");
+   xt_xlate_add(xl, "0x%02X ", info->tos & ~0x3); /* remove ECN 
bits */
+   }
+   if (info->bitmask & EBT_IP_PROTO) {
+   struct protoent *pe;
+
+   if (info->bitmask & (EBT_IP_SPORT|EBT_IP_DPORT) &&
+   (info->invflags & EBT_IP_PROTO) == 0) {
+   /* port number given and not inverted, no need to print 
this */
+   pname = brip_xlate_proto_to_name(info->protocol);
+   } else {
+   xt_xlate_add(xl, "ip protocol ");
+   if (info->invflags & EBT_IP_PROTO)
+   xt_xlate_add(xl, "!= ");
+   pe = getprotobynumber(info->protocol);
+   if (pe == NULL)
+   xt_xlate_add(xl, "%d ", info->protocol);
+   else
+   xt_xlate_add(xl, "%s ", pe->p_name);
+   }
+   }
+
+   brip_xlate_th(xl, info, EBT_IP_SPORT, pname);
+   brip_xlate_th(xl, info, EBT_IP_DPORT, pname);
+
+   return 1;
+}
+
 static struct xtables_match brip_match = {
.name   = "ip",
.revision   = 0,
@@ -303,6 +430,7 @@ static struct 

Re: [nft PATCH] cli: Drop String termination workaround

[Pablo Neira Ayuso]

On Wed, Apr 11, 2018 at 10:21:35AM +0200, Phil Sutter wrote:
> This spot was missed by commit 2b3f18e0cf7a7 ("libnftables: Fix for
> input without trailing newline") - since line termination is now added
> in nft_run_cmd_from_buffer(), cli is relieved from doing so.

Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[nft PATCH] cli: Drop String termination workaround

[Phil Sutter]

This spot was missed by commit 2b3f18e0cf7a7 ("libnftables: Fix for
input without trailing newline") - since line termination is now added
in nft_run_cmd_from_buffer(), cli is relieved from doing so.

Fixes: 2b3f18e0cf7a7 ("libnftables: Fix for input without trailing newline")
Signed-off-by: Phil Sutter 
---
 src/cli.c | 10 +-
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/src/cli.c b/src/cli.c
index eb60d01dad3dc..241ea0105512f 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -83,8 +83,6 @@ static void cli_complete(char *line)
const HIST_ENTRY *hist;
const char *c;
LIST_HEAD(msgs);
-   int len;
-   char *s;
 
if (line == NULL) {
printf("\n");
@@ -112,13 +110,7 @@ static void cli_complete(char *line)
if (hist == NULL || strcmp(hist->line, line))
add_history(line);
 
-   len = strlen(line);
-   s = xmalloc(len + 2);
-   snprintf(s, len + 2, "%s\n", line);
-   xfree(line);
-   line = s;
-
-   nft_run_cmd_from_buffer(cli_nft, line, len + 2);
+   nft_run_cmd_from_buffer(cli_nft, line, strlen(line) + 1);
xfree(line);
 }
 
-- 
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH ebtables 1/4] include: sync linux/netfilter_bridge/ebt_ip.h with kernel

[Pablo Neira Ayuso]

Applied this series (from 1/4 to 4/4), thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH iptables] ebtables-compat: don't make failing extension load fatal

[Pablo Neira Ayuso]

On Tue, Apr 10, 2018 at 12:54:16PM +0200, Florian Westphal wrote:
> We will fail later when we can't parse the option, but that
> failure only happens if the is actually used.
> 
> So in some cases things will work fine even if an extension
> doesn't exist.
> 
> Signed-off-by: Florian Westphal 

Acked-by: Pablo Neira Ayuso 
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: nftables code size (was: Re: [PATCH nf-next 0/2] ebtables: add support for ICMP and IGMP type/code matching)

[Pablo Neira Ayuso]

On Wed, Apr 11, 2018 at 06:40:34AM +0200, Matthias Schiffer wrote:
> On 03/04/2018 12:16 PM, Matthias Schiffer wrote:
> > I noticed that more than 25% of binary size of libnftnl are made up of
> > snprintf functions. Having these in a library with the goal to abstract the
> > netlink interface of nftables seems questionable to me, but I have no idea
> > if it would be viable to move these functions to nft or to a separate 
> > library.
> 
> As an experiment, I created a reduced version of libnftnl by ripping out
> all import/export functions and related code like buffer handling. This
> reduced the size of libnftnl.so from 155KB to 110KB (on x86-64, -Os,
> stripped, uncompressed), a reduction of roughly 30%.
> 
> I would like to look into splitting libnftnl into two parts, which could be
> called libnftnl-core and libnftnl, to make nftables more suited for tiny
> embedded systems. All basic functions that do not deal with textual
> representations of rules (i.e. the reduced libnftnl I built) would be moved
> into libnftnl-core.
> 
> Does this sound like a good idea, and would such a drastic change be
> acceptable for upstream inclusion, given the current libnftnl API can be
> preserved?

Could you wrap the import/export code around the --with-json-parsing?
I mean, turn this into --with-json or such, so you can just compile
out that code, which is what is giving you the savings in terms of
size, right?

I'm trying to keep it simple here :-)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH iptables] ebtables-compat: load mark target

[Pablo Neira Ayuso]

On Tue, Apr 10, 2018 at 12:53:38PM +0200, Florian Westphal wrote:
> Its already there but it did not work because it wasn't loaded.
> 
> Signed-off-by: Florian Westphal 

Acked-by: Pablo Neira Ayuso 

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [nft PATCH 3/8] libnftables: Introduce nft_ctx_set_error()

[Pablo Neira Ayuso]

On Tue, Apr 10, 2018 at 07:00:21PM +0200, Phil Sutter wrote:
> Analogous to nft_ctx_set_output(), this allows to set a custom file
> pointer for writing error messages to.
> 
> Signed-off-by: Phil Sutter 
> ---
>  include/nftables.h  |  1 +
>  include/nftables/nftables.h |  2 ++
>  src/erec.c  |  2 +-
>  src/libnftables.c   | 16 +---
>  4 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/include/nftables.h b/include/nftables.h
> index 3d96f2e3f10f4..1b368971bee9a 100644
> --- a/include/nftables.h
> +++ b/include/nftables.h
> @@ -14,6 +14,7 @@ struct output_ctx {
>   unsigned int handle;
>   unsigned int echo;
>   FILE *output_fp;
> + FILE *error_fp;
>  };
>  
>  struct nft_cache {
> diff --git a/include/nftables/nftables.h b/include/nftables/nftables.h
> index 8e59f2b2a59ab..1e9306822eb7e 100644
> --- a/include/nftables/nftables.h
> +++ b/include/nftables/nftables.h
> @@ -57,6 +57,8 @@ bool nft_ctx_output_get_echo(struct nft_ctx *ctx);
>  void nft_ctx_output_set_echo(struct nft_ctx *ctx, bool val);
>  
>  FILE *nft_ctx_set_output(struct nft_ctx *ctx, FILE *fp);
> +FILE *nft_ctx_set_error(struct nft_ctx *ctx, FILE *fp);
> +
>  int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path);
>  void nft_ctx_clear_include_paths(struct nft_ctx *ctx);
>  
> diff --git a/src/erec.c b/src/erec.c
> index 3e1b7fd108a7e..226c51f552267 100644
> --- a/src/erec.c
> +++ b/src/erec.c
> @@ -124,7 +124,7 @@ void erec_print(struct output_ctx *octx, const struct 
> error_record *erec,
>   unsigned int i, end;
>   int l;
>   off_t orig_offset = 0;
> - FILE *f = octx->output_fp;
> + FILE *f = octx->error_fp;
>  
>   if (!f)
>   return;
> diff --git a/src/libnftables.c b/src/libnftables.c
> index 8bf989b08cc54..d6622d51aba33 100644
> --- a/src/libnftables.c
> +++ b/src/libnftables.c
> @@ -169,6 +169,7 @@ struct nft_ctx *nft_ctx_new(uint32_t flags)
>   init_list_head(>cache.list);
>   ctx->flags = flags;
>   ctx->output.output_fp = stdout;
> + ctx->output.error_fp = stderr;

I understand you may need this new toggle. But can we probably
reasonable defaults? I mean, if no error_fp is specified, then use the
one set by output_fp.

As said, it would be good if you can review the existing toggles and
see if we can stop exposing some of those through API if possible. I
would just expose the bare minimum and provide reasonable defaults so
we most people in the world will not need to call n+1 different setter
functions.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [nft PATCH 0/8] Convert tests/py to use libnftables directly

[Pablo Neira Ayuso]

Hi Phil,

On Tue, Apr 10, 2018 at 07:00:18PM +0200, Phil Sutter wrote:
> This series of patches results in tests/py/nft-test.py using libnftables
> API directly via ctypes module which leads to significant performance
> improvement of the test script.

Series looks good, applied.

Two comments, both related to 'struct cookie':

* How is 'struct cookie' going to be used? Do you have follow up
  patches to update the codebase to use this?

* Can we avoid the union around struct cookie and FILE *fp? I mean, I
  don't think it makes sense to start exposing lots of toggles through
  the API, the more we expose, the more combinations are possible and
  then more chances we get to support all kind of strange combinations
  in the future.

  So what I'm suggesting is: Can we turn 'struct cookie' into the
  default way to print messages?

  Regarding the name 'struct cookie', is this refering to something
  related to Python? I mean, this name tells me very little. What I
  can see there is a new class that allows us to do buffering, so
  probably using 'struct nft_buffer' or such sound more convenient to
  me?

If you think these two comments are relevant, please follow up with
patches.

Thanks.

> The first five patches are preparational work:
> 
> * Patch 1 fixes a missed conversion to nft_print() in src/ct.c which
>   became obvious after nft-test.py started using libnftables API and
>   therefore ignored anything not written into output_fp.
> 
> * Patch 2 moves a necessary workaround from main() into
>   run_cmd_from_buffer() which is required to accept non-newline
>   terminated strings. While being at it, reading input from files
>   without trailing newline is fixed as well. (Yes, this never worked.)
> 
> * Patch 3 makes error output configurable by users, which is required if
>   one wants to redirect stderr without dirty hacks.
> 
> * Patch 4 adds convenience functions for using fopencookie() to buffer
>   standard and error output. Doing so from within Python code is
>   non-trivial and therefore an unnecessary burden on (already chastened)
>   Python programmers.
> 
> * Patch 5 simplifies the code introduced in patch 4. Though since it also
>   increases the size of struct nft_ctx quite a bit, it is kept separate
>   in case that side-effect is unwanted upstream.
> 
> Finally, patch 6 introduces a simple Python class Nftables and changes
> nft-test.py accordingly to make good use of it. Since we might at some
> point want to ship that class as a Python module, it is placed in a new
> topdir subdirectory named 'py'.
> 
> Patches 7 and 8 are more or less fall-out from the actual
> implementation. A bit of cleanup in the first one and a minor
> enhancement in the latter.
> 
> Phil Sutter (8):
>   ct: Fix output_fp bypass in ct_print()
>   libnftables: Fix for input without trailing newline
>   libnftables: Introduce nft_ctx_set_error()
>   libnftables: Support buffering output and error
>   libnftables: Simplify cookie integration
>   tests/py: Use libnftables instead of calling nft binary
>   tests/py: Review print statements in nft-test.py
>   tests/py: Allow passing multiple files to nft-test.py
> 
>  include/nftables.h|  17 ++-
>  include/nftables/nftables.h   |   9 ++
>  py/.gitignore |   1 +
>  py/nftables.py| 224 ++
>  src/ct.c  |   2 +-
>  src/erec.c|   2 +-
>  src/libnftables.c | 135 -
>  src/main.c|   5 +-
>  src/scanner.l |   2 +-
>  tests/py/any/ct.t |  14 +--
>  tests/py/any/ct.t.payload |  12 +-
>  tests/py/any/log.t|   2 +-
>  tests/py/any/log.t.payload|   2 +-
>  tests/py/any/meta.t   |   4 +-
>  tests/py/arp/arp.t|   2 +-
>  tests/py/arp/arp.t.payload|   2 +-
>  tests/py/arp/arp.t.payload.netdev |   2 +-
>  tests/py/inet/tcp.t   |   2 +-
>  tests/py/inet/tcp.t.payload   |   2 +-
>  tests/py/ip/ip.t  |   6 +-
>  tests/py/ip/ip.t.payload  |   6 +-
>  tests/py/ip/ip.t.payload.bridge   |   6 +-
>  tests/py/ip/ip.t.payload.inet |   6 +-
>  tests/py/ip/ip.t.payload.netdev   |   6 +-
>  tests/py/ip/objects.t |   4 +-
>  tests/py/nft-test.py  | 247 
> ++
>  26 files changed, 545 insertions(+), 177 deletions(-)
>  create mode 100644 py/.gitignore
>  create mode 100644 py/nftables.py
> 
> -- 
> 2.16.1
> 
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: nftables code size (was: Re: [PATCH nf-next 0/2] ebtables: add support for ICMP and IGMP type/code matching)

[Florian Westphal]

Matthias Schiffer  wrote:
> As an experiment, I created a reduced version of libnftnl by ripping out
> all import/export functions and related code like buffer handling. This
> reduced the size of libnftnl.so from 155KB to 110KB (on x86-64, -Os,
> stripped, uncompressed), a reduction of roughly 30%.

[..]

> Does this sound like a good idea, and would such a drastic change be
> acceptable for upstream inclusion, given the current libnftnl API can be
> preserved?

Seems like a good idea to split this up.

I think as first step you could even send a patch that
just excludes all unneeded snprintf etc. code from the build.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html