Re: [PATCH iptables v2 0/8] extensions: libxt_NFLOG: use nft back-end for iptables-nft

[Jeremy Sowden <jeremy@azazel.net>]

[-- Attachment #1: Type: text/plain, Size: 2418 bytes --]

On 2022-01-18, at 02:23:46 +0100, Pablo Neira Ayuso wrote:
> On Mon, Jan 17, 2022 at 09:54:52PM +0000, Jeremy Sowden wrote:
> > On 2022-01-17, at 11:40:51 +0100, Phil Sutter wrote:
> > > On Sun, Jan 16, 2022 at 08:08:15PM +0100, Florian Westphal wrote:
> [...]
> > > > Pablo, Phil, others -- what is your take?
> > >
> > > I think the change is OK if existing rulesets will continue to
> > > work just as before and remain compatible with legacy. IMHO, new
> > > rulesets created using iptables-nft may become incompatible if
> > > users explicitly ask for it (e.g. by specifying an exceedingly
> > > long log prefix.
> > >
> > > What about --nflog-range? This series seems to drop support for
> > > it, at least in the sense that ruleset dumps won't contain the
> > > option. In theory, users could depend on identifying a specific
> > > rule via nflog range value.
> >
> > Fair enough.  I'll add a check so that nft is not used for targets
> > that specify `--nflog-range`.
>
> --nflog-range does work?

No.

> --nflog-size is used and can be mapped to 'snaplen' in nft_log.

Correct.

> Manpage also discourages the usage of --nflog-range for long time.
>
> Not sure it is worth to add a different path for this case.

Yes, there have been warnings about `--nflog-range` since `--nflog-size`
was added in 2016.  I wasn't entirely happy dropping `--nflog-range`
support and introducing the divergence between -legacy and -nft as an
incidental side-effect, so when Phil brought it up, I had a look and it
turns out that the code to preserve support for it is quite small:

  --- a/iptables/nft.c
  +++ b/iptables/nft.c
  @@ -1366,6 +1366,12 @@ int add_log(struct nftnl_rule *r, struct iptables_command_state *cs)
          struct nftnl_expr *expr;
          struct xt_nflog_info *info = (struct xt_nflog_info *)cs->target->t->data;

  +       if (info->len && !(info->flags & XT_NFLOG_F_COPY_LEN))
  +               /*
  +                * nft doesn't support --nflog-range
  +                */
  +               return add_target(r, cs->target->t);
  +
          expr = nftnl_expr_alloc("log");
          if (!expr)
                  return -ENOMEM;

Perhaps, we could put this in now, add something to the release notes
for the next release formally deprecating `--nflog-range` and then
remove it altogether in the following release.

Or we could just apply the current patches if you're not that bothered.
:)

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]