Re: [PATCH net 02/10] netfilter: ebt_nflog: pin the NFLOG backend

Pablo Neira Ayuso <[email protected]> wrote:
> > >  	info->prefix[EBT_NFLOG_PREFIX_SIZE - 1] = '\0';
> > > -	return 0;
> > > +
> > > +	ret = nf_logger_find_get(par->family, NF_LOG_TYPE_ULOG);
> > 
> > [Severity: Medium]
> > Should the commit message mention that this can reject rulesets that used
> > to load?  ebt_nflog_tg_check() previously returned 0 unconditionally, so an
> > ebtables nflog rule installed fine and simply logged nothing when
> > loggers[pf][NF_LOG_TYPE_ULOG] was NULL.  Now the raw -ENOENT from
> > nf_logger_find_get() is propagated out through xt_check_target(), so
> > ebtables -A ... --nflog and ebtables-restore fail.  This seems worth
> > spelling out given the Cc: stable.
> > 
> > Also, is the stated parity with xt_NFLOG complete?  NETFILTER_XT_TARGET_NFLOG
> > carries select NETFILTER_NETLINK_LOG in net/netfilter/Kconfig, while
> > BRIDGE_EBT_NFLOG has no select or depends:
> > 
> > net/bridge/netfilter/Kconfig:
> > config BRIDGE_EBT_NFLOG
> > 	tristate "ebt: nflog support"
> > 	help
> > 	  This option enables the nflog watcher, which allows to LOG
> > 	  messages through the netfilter logging API, which can use
> > 	  either the old LOG target, the old ULOG target or nfnetlink_log
> > 	  as backend.
> > 
> > On a kernel with CONFIG_BRIDGE_EBT_NFLOG set and
> > CONFIG_NETFILTER_NETLINK_LOG=n, the request_module() fallback below can
> > never succeed, so the target becomes unusable.  Should a
> > select NETFILTER_NETLINK_LOG be added here?  The help text claiming the
> > watcher can use the old LOG or ULOG targets as backend also no longer
> > matches the code.
> 
> This sounds again like a pre-existing issue in ebt_nflog's Kconfig.

Yes, this is ancient by kernel standards.

> > > +	if (ret != 0 && !par->nft_compat) {
> > > +		request_module("%s", "nfnetlink_log");
> > > +
> > > +		ret = nf_logger_find_get(par->family, NF_LOG_TYPE_ULOG);
> > > +	}
> > > +
> > > +	return ret;
> > 
> > [Severity: Medium]
> > Who loads nfnetlink_log for the nft_compat case?
> 
> This is again a pre-existing issue that should be handled in a follow
> up, it affects xt_LOG and xt_NFLOG too.

Its not, actually: in ebtables case, user has to do it manually:
rule did load but doesn't work, now rule load would fail.

I think the best fix is to just add
MODULE_SOFTDEP("pre: nfnetlink_log");

like in xt_NFLOG and let modprobe handle it.
I don't think we should bother extending the workarounds for
modprobe we carry in nft_compat for this.

And also add a kconfig dependency to be sure.

And I think it should be done in a different commit.