[iptables PATCH 0/2] Improve error messages for unsupported extensions

[iptables PATCH 0/2] Improve error messages for unsupported extensions

[Phil Sutter]

Failure to load an extension leads to iptables cmdline parser
complaining about any extension options instead of the extension itself.
This is at least misleading.

This series eliminates the odd error message and instead adds a warning
if a requested extension is not available at all in kernel.

Things are a bit complicated due to the fact that newer kernels not
necessarily support revision 0 of all extensions. So change iptables
first to only register revision 0 if no higher one was accepted earlier.
This allows for a "not even revision 0 is supported" logic.

Phil Sutter (2):
  libxtables: Register only the highest revision extension
  Improve error messages for unsupported extensions

 iptables/nft.c       | 12 +++++++++---
 libxtables/xtables.c | 17 ++++++++++++++---
 2 files changed, 23 insertions(+), 6 deletions(-)

-- 
2.34.1

[iptables PATCH 1/2] libxtables: Register only the highest revision extension

[Phil Sutter]

When fully registering extensions, ignore all consecutive ones with same
name and family value. Since commit b3ac87038f4e4 ("libxtables: Make
sure extensions register in revision order"), one may safely assume the
list of pending extensions has highest revision numbers first. Since
iptables is only interested in the highest revision the kernel supports,
registration and compatibility checks may be skipped once the first
matching extension in pending list has validated.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 libxtables/xtables.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index 50fd6a44b0100..b34d62acf6015 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -697,6 +697,7 @@ xtables_find_match(const char *name, enum xtables_tryload tryload,
 	struct xtables_match **dptr;
 	struct xtables_match *ptr;
 	const char *icmp6 = "icmp6";
+	bool found = false;
 
 	if (strlen(name) >= XT_EXTENSION_MAXNAMELEN)
 		xtables_error(PARAMETER_PROBLEM,
@@ -715,7 +716,9 @@ xtables_find_match(const char *name, enum xtables_tryload tryload,
 		if (extension_cmp(name, (*dptr)->name, (*dptr)->family)) {
 			ptr = *dptr;
 			*dptr = (*dptr)->next;
-			if (xtables_fully_register_pending_match(ptr, prev)) {
+			if (!found &&
+			    xtables_fully_register_pending_match(ptr, prev)) {
+				found = true;
 				prev = ptr;
 				continue;
 			} else if (prev) {
@@ -817,6 +820,7 @@ xtables_find_target(const char *name, enum xtables_tryload tryload)
 	struct xtables_target *prev = NULL;
 	struct xtables_target **dptr;
 	struct xtables_target *ptr;
+	bool found = false;
 
 	/* Standard target? */
 	if (strcmp(name, "") == 0
@@ -831,7 +835,9 @@ xtables_find_target(const char *name, enum xtables_tryload tryload)
 		if (extension_cmp(name, (*dptr)->name, (*dptr)->family)) {
 			ptr = *dptr;
 			*dptr = (*dptr)->next;
-			if (xtables_fully_register_pending_target(ptr, prev)) {
+			if (!found &&
+			    xtables_fully_register_pending_target(ptr, prev)) {
+				found = true;
 				prev = ptr;
 				continue;
 			} else if (prev) {
-- 
2.34.1

[iptables PATCH 2/2] Improve error messages for unsupported extensions

[Phil Sutter]

If a given extension was not supported by the kernel, iptables would
print a rather confusing error message if extension parameters were
given:

| # rm /lib/modules/$(uname -r)/kernel/net/netfilter/xt_LOG.ko
| # iptables -A FORWARD -j LOG --log-prefix foo
| iptables v1.8.7 (legacy): unknown option "--log-prefix"

Avoid this by pretending extension revision 0 is always supported. It is
the same hack as used to successfully print extension help texts as
unprivileged user, extended to all error codes to serve privileged ones
as well.

In addition, print a warning if kernel rejected revision 0 and it's not
a permissions problem. This helps users find out which extension in a
rule the kernel didn't like.

Finally, the above commands result in these messages:

| Warning: Extension LOG revision 0 not supported, missing kernel module?
| iptables: No chain/target/match by that name.

Or, for iptables-nft:

| Warning: Extension LOG revision 0 not supported, missing kernel module?
| iptables v1.8.7 (nf_tables):  RULE_APPEND failed (No such file or directory): rule in chain FORWARD

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c       | 12 +++++++++---
 libxtables/xtables.c |  7 ++++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 041e1b8ccd3e5..d011d7c88da12 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -3514,10 +3514,16 @@ int nft_compatible_revision(const char *name, uint8_t rev, int opt)
 err:
 	mnl_socket_close(nl);
 
-	/* pretend revision 0 is valid if not permitted to check -
-	 * this is required for printing extension help texts as user */
-	if (ret < 0 && errno == EPERM && rev == 0)
+	/* pretend revision 0 is valid -
+	 * this is required for printing extension help texts as user, also
+	 * helps error messaging on unavailable kernel extension */
+	if (ret < 0 && rev == 0) {
+		if (errno != EPERM)
+			fprintf(stderr,
+				"Warning: Extension %s revision 0 not supported, missing kernel module?\n",
+				name);
 		return 1;
+	}
 
 	return ret < 0 ? 0 : 1;
 }
diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index b34d62acf6015..87424d045466b 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -958,7 +958,12 @@ int xtables_compatible_revision(const char *name, uint8_t revision, int opt)
 		/* Definitely don't support this? */
 		if (errno == ENOENT || errno == EPROTONOSUPPORT) {
 			close(sockfd);
-			return 0;
+			/* Pretend revision 0 support for better error messaging */
+			if (revision == 0)
+				fprintf(stderr,
+					"Warning: Extension %s revision 0 not supported, missing kernel module?\n",
+					name);
+			return (revision == 0);
 		} else if (errno == ENOPROTOOPT) {
 			close(sockfd);
 			/* Assume only revision 0 support (old kernel) */
-- 
2.34.1

Re: [iptables PATCH 0/2] Improve error messages for unsupported extensions

[Phil Sutter]

*bump*

On Fri, Feb 11, 2022 at 06:12:09PM +0100, Phil Sutter wrote:
> Failure to load an extension leads to iptables cmdline parser
> complaining about any extension options instead of the extension itself.
> This is at least misleading.
> 
> This series eliminates the odd error message and instead adds a warning
> if a requested extension is not available at all in kernel.
> 
> Things are a bit complicated due to the fact that newer kernels not
> necessarily support revision 0 of all extensions. So change iptables
> first to only register revision 0 if no higher one was accepted earlier.
> This allows for a "not even revision 0 is supported" logic.
> 
> Phil Sutter (2):
>   libxtables: Register only the highest revision extension
>   Improve error messages for unsupported extensions
> 
>  iptables/nft.c       | 12 +++++++++---
>  libxtables/xtables.c | 17 ++++++++++++++---
>  2 files changed, 23 insertions(+), 6 deletions(-)
> 
> -- 
> 2.34.1
> 
> 

Re: [iptables PATCH 0/2] Improve error messages for unsupported extensions

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> *bump*

Looks good to me.  Feel free to push this if it passes the tests on
a normal distro kernel.

Re: [iptables PATCH 0/2] Improve error messages for unsupported extensions

[Phil Sutter]

Hi!

On Wed, Mar 02, 2022 at 04:06:16PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > *bump*
> 
> Looks good to me.  Feel free to push this if it passes the tests on
> a normal distro kernel.

Thanks for the review. Just tested on Fedora 35. All tests pass after
changing the SELinux context string in libxt_SECMARK.t - a known
problem.

Cheers, Phil