netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [iptables PATCH 0/4] Some misc fixes
@ 2022-05-04 10:34 Phil Sutter
  2022-05-04 10:34 ` [iptables PATCH 1/4] extensions: DNAT: Merge core printing functions Phil Sutter
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Phil Sutter @ 2022-05-04 10:34 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Fix and improve DNAT printing routines, add some missing bits to
extension man page and further improve extension loading for
unprivileged users.

Phil Sutter (4):
  extensions: DNAT: Merge core printing functions
  man: *NAT: Review --random* option descriptions
  extensions: LOG: Document --log-macdecode in man page
  nft: Fix EPERM handling for extensions without rev 0

 extensions/libxt_DNAT.c                       | 58 +++++++++----------
 extensions/libxt_DNAT.man                     |  4 +-
 extensions/libxt_LOG.man                      |  3 +
 extensions/libxt_MASQUERADE.man               | 10 +---
 extensions/libxt_REDIRECT.man                 |  4 +-
 extensions/libxt_SNAT.man                     |  8 +--
 iptables/nft.c                                | 14 +++--
 .../testcases/iptables/0008-unprivileged_0    |  6 ++
 8 files changed, 52 insertions(+), 55 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [iptables PATCH 1/4] extensions: DNAT: Merge core printing functions
  2022-05-04 10:34 [iptables PATCH 0/4] Some misc fixes Phil Sutter
@ 2022-05-04 10:34 ` Phil Sutter
  2022-05-04 10:34 ` [iptables PATCH 2/4] man: *NAT: Review --random* option descriptions Phil Sutter
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Phil Sutter @ 2022-05-04 10:34 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Have a versatile __NAT_print() function providing enough flexibility for
DNAT and REDIRECT, IPv4 and IPv6 and 'print' and 'save' output. Then
define macros to simplify calling it.

As a side effect, this fixes ip6tables DNAT revision 1 print output.

Fixes: 14d77c8aa29a7 ("extensions: Merge IPv4 and IPv6 DNAT targets")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 extensions/libxt_DNAT.c | 58 +++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/extensions/libxt_DNAT.c b/extensions/libxt_DNAT.c
index 5ac8018c12423..5696d31f2b0c5 100644
--- a/extensions/libxt_DNAT.c
+++ b/extensions/libxt_DNAT.c
@@ -312,31 +312,40 @@ static char *sprint_range(const struct nf_nat_range2 *r, int family)
 	return buf;
 }
 
-static void __DNAT_print(const struct nf_nat_range2 *r, bool save, int family)
+static void __NAT_print(const struct nf_nat_range2 *r, int family,
+			const char *rangeopt, const char *flag_pfx,
+			bool skip_colon)
 {
-	const char *dashdash = save ? "--" : "";
+	char *range_str = sprint_range(r, family);
 
-	printf(" %s%s", save ? "--to-destination " : "to:",
-	       sprint_range(r, family));
+	if (strlen(range_str)) {
+		if (range_str[0] == ':' && skip_colon)
+			range_str++;
+		printf(" %s%s", rangeopt, range_str);
+	}
 	if (r->flags & NF_NAT_RANGE_PROTO_RANDOM)
-		printf(" %srandom", dashdash);
+		printf(" %srandom", flag_pfx);
 	if (r->flags & NF_NAT_RANGE_PERSISTENT)
-		printf(" %spersistent", dashdash);
+		printf(" %spersistent", flag_pfx);
 }
+#define __DNAT_print(r, family) __NAT_print(r, family, "to:", "", false)
+#define __DNAT_save(r, family) __NAT_print(r, family, "--to-destination ", "--", false)
+#define __REDIRECT_print(r) __NAT_print(r, AF_INET, "redir ports ", "", true)
+#define __REDIRECT_save(r) __NAT_print(r, AF_INET, "--to-ports ", "--", true)
 
 static void DNAT_print(const void *ip, const struct xt_entry_target *target,
                        int numeric)
 {
 	struct nf_nat_range2 range = RANGE2_INIT_FROM_IPV4_MRC(target->data);
 
-	__DNAT_print(&range, false, AF_INET);
+	__DNAT_print(&range, AF_INET);
 }
 
 static void DNAT_save(const void *ip, const struct xt_entry_target *target)
 {
 	struct nf_nat_range2 range = RANGE2_INIT_FROM_IPV4_MRC(target->data);
 
-	__DNAT_print(&range, true, AF_INET);
+	__DNAT_save(&range, AF_INET);
 }
 
 static int
@@ -387,12 +396,12 @@ static void DNAT_fcheck_v2(struct xt_fcheck_call *cb)
 static void DNAT_print_v2(const void *ip, const struct xt_entry_target *target,
                        int numeric)
 {
-	__DNAT_print((const void *)target->data, false, AF_INET);
+	__DNAT_print((const void *)target->data, AF_INET);
 }
 
 static void DNAT_save_v2(const void *ip, const struct xt_entry_target *target)
 {
-	__DNAT_print((const void *)target->data, true, AF_INET);
+	__DNAT_save((const void *)target->data, AF_INET);
 }
 
 static int DNAT_xlate_v2(struct xt_xlate *xl,
@@ -429,7 +438,7 @@ static void DNAT_print6(const void *ip, const struct xt_entry_target *target,
 	struct nf_nat_range2 range = {};
 
 	memcpy(&range, (const void *)target->data, sizeof(struct nf_nat_range));
-	__DNAT_print(&range, true, AF_INET6);
+	__DNAT_print(&range, AF_INET6);
 }
 
 static void DNAT_save6(const void *ip, const struct xt_entry_target *target)
@@ -437,7 +446,7 @@ static void DNAT_save6(const void *ip, const struct xt_entry_target *target)
 	struct nf_nat_range2 range = {};
 
 	memcpy(&range, (const void *)target->data, sizeof(struct nf_nat_range));
-	__DNAT_print(&range, true, AF_INET6);
+	__DNAT_save(&range, AF_INET6);
 }
 
 static int DNAT_xlate6(struct xt_xlate *xl,
@@ -460,12 +469,12 @@ static void DNAT_parse6_v2(struct xt_option_call *cb)
 static void DNAT_print6_v2(const void *ip, const struct xt_entry_target *target,
 			   int numeric)
 {
-	__DNAT_print((const void *)target->data, true, AF_INET6);
+	__DNAT_print((const void *)target->data, AF_INET6);
 }
 
 static void DNAT_save6_v2(const void *ip, const struct xt_entry_target *target)
 {
-	__DNAT_print((const void *)target->data, true, AF_INET6);
+	__DNAT_save((const void *)target->data, AF_INET6);
 }
 
 static int DNAT_xlate6_v2(struct xt_xlate *xl,
@@ -474,19 +483,6 @@ static int DNAT_xlate6_v2(struct xt_xlate *xl,
 	return __DNAT_xlate(xl, (const void *)params->target->data, AF_INET6);
 }
 
-static void __REDIRECT_print(const struct nf_nat_range2 *range, bool save)
-{
-	char *range_str = sprint_range(range, AF_INET);
-	const char *dashdash = save ? "--" : "";
-
-	if (strlen(range_str))
-		/* range_str starts with colon, skip over them */
-		printf(" %s %s", save ? "--to-ports" : "redir ports",
-		       range_str + 1);
-	if (range->flags & NF_NAT_RANGE_PROTO_RANDOM)
-		printf(" %srandom", dashdash);
-}
-
 static int __REDIRECT_xlate(struct xt_xlate *xl,
 			    const struct nf_nat_range2 *range)
 {
@@ -506,14 +502,14 @@ static void REDIRECT_print(const void *ip, const struct xt_entry_target *target,
 {
 	struct nf_nat_range2 range = RANGE2_INIT_FROM_IPV4_MRC(target->data);
 
-	__REDIRECT_print(&range, false);
+	__REDIRECT_print(&range);
 }
 
 static void REDIRECT_save(const void *ip, const struct xt_entry_target *target)
 {
 	struct nf_nat_range2 range = RANGE2_INIT_FROM_IPV4_MRC(target->data);
 
-	__REDIRECT_print(&range, true);
+	__REDIRECT_save(&range);
 }
 
 static int REDIRECT_xlate(struct xt_xlate *xl,
@@ -531,7 +527,7 @@ static void REDIRECT_print6(const void *ip, const struct xt_entry_target *target
 	struct nf_nat_range2 range = {};
 
 	memcpy(&range, (const void *)target->data, sizeof(struct nf_nat_range));
-	__REDIRECT_print(&range, false);
+	__REDIRECT_print(&range);
 }
 
 static void REDIRECT_save6(const void *ip, const struct xt_entry_target *target)
@@ -539,7 +535,7 @@ static void REDIRECT_save6(const void *ip, const struct xt_entry_target *target)
 	struct nf_nat_range2 range = {};
 
 	memcpy(&range, (const void *)target->data, sizeof(struct nf_nat_range));
-	__REDIRECT_print(&range, true);
+	__REDIRECT_save(&range);
 }
 
 static int REDIRECT_xlate6(struct xt_xlate *xl,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [iptables PATCH 2/4] man: *NAT: Review --random* option descriptions
  2022-05-04 10:34 [iptables PATCH 0/4] Some misc fixes Phil Sutter
  2022-05-04 10:34 ` [iptables PATCH 1/4] extensions: DNAT: Merge core printing functions Phil Sutter
@ 2022-05-04 10:34 ` Phil Sutter
  2022-05-04 10:34 ` [iptables PATCH 3/4] extensions: LOG: Document --log-macdecode in man page Phil Sutter
  2022-05-04 10:34 ` [iptables PATCH 4/4] nft: Fix EPERM handling for extensions without rev 0 Phil Sutter
  3 siblings, 0 replies; 9+ messages in thread
From: Phil Sutter @ 2022-05-04 10:34 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Stating the option again in the first (single?) sentence is pointless.
Get rid of that initial half-sentence in MASQUERADE options and unify
the texts a bit.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 extensions/libxt_DNAT.man       |  4 +---
 extensions/libxt_MASQUERADE.man | 10 ++--------
 extensions/libxt_REDIRECT.man   |  4 +---
 extensions/libxt_SNAT.man       |  8 ++------
 4 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/extensions/libxt_DNAT.man b/extensions/libxt_DNAT.man
index 12d334af5a479..af9a3f06f6aaf 100644
--- a/extensions/libxt_DNAT.man
+++ b/extensions/libxt_DNAT.man
@@ -25,9 +25,7 @@ For a single port or \fIbaseport\fP, a service name as listed in
 \fB/etc/services\fP may be used.
 .TP
 \fB\-\-random\fP
-If option
-\fB\-\-random\fP
-is used then port mapping will be randomized (kernel >= 2.6.22).
+Randomize source port mapping (kernel >= 2.6.22).
 .TP
 \fB\-\-persistent\fP
 Gives a client the same source-/destination-address for each connection.
diff --git a/extensions/libxt_MASQUERADE.man b/extensions/libxt_MASQUERADE.man
index 7746f4734a31a..26d91ddba6b15 100644
--- a/extensions/libxt_MASQUERADE.man
+++ b/extensions/libxt_MASQUERADE.man
@@ -20,16 +20,10 @@ if the rule also specifies one of the following protocols:
 \fBtcp\fP, \fBudp\fP, \fBdccp\fP or \fBsctp\fP.
 .TP
 \fB\-\-random\fP
-Randomize source port mapping
-If option
-\fB\-\-random\fP
-is used then port mapping will be randomized (kernel >= 2.6.21).
+Randomize source port mapping (kernel >= 2.6.21).
 Since kernel 5.0, \fB\-\-random\fP is identical to \fB\-\-random-fully\fP.
 .TP
 \fB\-\-random-fully\fP
-Full randomize source port mapping
-If option
-\fB\-\-random-fully\fP
-is used then port mapping will be fully randomized (kernel >= 3.13).
+Fully randomize source port mapping (kernel >= 3.13).
 .TP
 IPv6 support available since Linux kernels >= 3.7.
diff --git a/extensions/libxt_REDIRECT.man b/extensions/libxt_REDIRECT.man
index 10305597f87a3..1cbdb9bae988c 100644
--- a/extensions/libxt_REDIRECT.man
+++ b/extensions/libxt_REDIRECT.man
@@ -19,8 +19,6 @@ if the rule also specifies one of the following protocols:
 For a single port, a service name as listed in \fB/etc/services\fP may be used.
 .TP
 \fB\-\-random\fP
-If option
-\fB\-\-random\fP
-is used then port mapping will be randomized (kernel >= 2.6.22).
+Randomize source port mapping (kernel >= 2.6.22).
 .TP
 IPv6 support available starting Linux kernels >= 3.7.
diff --git a/extensions/libxt_SNAT.man b/extensions/libxt_SNAT.man
index 087664471d110..80a698a64738b 100644
--- a/extensions/libxt_SNAT.man
+++ b/extensions/libxt_SNAT.man
@@ -21,14 +21,10 @@ will be mapped to ports below 1024, and other ports will be mapped to
 1024 or above. Where possible, no port alteration will occur.
 .TP
 \fB\-\-random\fP
-If option
-\fB\-\-random\fP
-is used then port mapping will be randomized through a hash-based algorithm (kernel >= 2.6.21).
+Randomize source port mapping through a hash-based algorithm (kernel >= 2.6.21).
 .TP
 \fB\-\-random-fully\fP
-If option
-\fB\-\-random-fully\fP
-is used then port mapping will be fully randomized through a PRNG (kernel >= 3.14).
+Fully randomize source port mapping through a PRNG (kernel >= 3.14).
 .TP
 \fB\-\-persistent\fP
 Gives a client the same source-/destination-address for each connection.
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [iptables PATCH 3/4] extensions: LOG: Document --log-macdecode in man page
  2022-05-04 10:34 [iptables PATCH 0/4] Some misc fixes Phil Sutter
  2022-05-04 10:34 ` [iptables PATCH 1/4] extensions: DNAT: Merge core printing functions Phil Sutter
  2022-05-04 10:34 ` [iptables PATCH 2/4] man: *NAT: Review --random* option descriptions Phil Sutter
@ 2022-05-04 10:34 ` Phil Sutter
  2022-05-04 10:34 ` [iptables PATCH 4/4] nft: Fix EPERM handling for extensions without rev 0 Phil Sutter
  3 siblings, 0 replies; 9+ messages in thread
From: Phil Sutter @ 2022-05-04 10:34 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Help text already contains it, so no update needed there.

Fixes: 127647892c7ca ("extensions: libipt_LOG/libip6t_LOG: support macdecode option")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 extensions/libxt_LOG.man | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/extensions/libxt_LOG.man b/extensions/libxt_LOG.man
index 354edf4cc2916..1d5071ba720b9 100644
--- a/extensions/libxt_LOG.man
+++ b/extensions/libxt_LOG.man
@@ -30,3 +30,6 @@ Log options from the IP/IPv6 packet header.
 .TP
 \fB\-\-log\-uid\fP
 Log the userid of the process which generated the packet.
+.TP
+\fB\-\-log\-macdecode\fP
+Log MAC addresses and protocol.
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [iptables PATCH 4/4] nft: Fix EPERM handling for extensions without rev 0
  2022-05-04 10:34 [iptables PATCH 0/4] Some misc fixes Phil Sutter
                   ` (2 preceding siblings ...)
  2022-05-04 10:34 ` [iptables PATCH 3/4] extensions: LOG: Document --log-macdecode in man page Phil Sutter
@ 2022-05-04 10:34 ` Phil Sutter
  2022-05-04 20:17   ` Pablo Neira Ayuso
  2022-05-06 11:43   ` [iptables PATCH v2 " Phil Sutter
  3 siblings, 2 replies; 9+ messages in thread
From: Phil Sutter @ 2022-05-04 10:34 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Treating revision 0 as compatible in EPERM case works fine as long as
there is a revision 0 of that extension defined in DSO. Fix the code for
others: Extend the EPERM handling to all revisions and keep the existing
warning for revision 0.

Fixes: 17534cb18ed0a ("Improve error messages for unsupported extensions")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft.c                                     | 14 ++++++++++----
 .../shell/testcases/iptables/0008-unprivileged_0   |  6 ++++++
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 33813ce1b9202..95e6c222682c0 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -3510,15 +3510,21 @@ int nft_compatible_revision(const char *name, uint8_t rev, int opt)
 err:
 	mnl_socket_close(nl);
 
-	/* pretend revision 0 is valid -
+	/* ignore EPERM and errors for revision 0 -
 	 * 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)
+	if (ret < 0) {
+		if (errno == EPERM) {
+			fprintf(stderr,
+				"%s: Could not determine whether revision %u is supported, assuming it is.\n",
+				name, rev);
+			return 1;
+		} else if (rev == 0) {
 			fprintf(stderr,
 				"Warning: Extension %s revision 0 not supported, missing kernel module?\n",
 				name);
-		return 1;
+			return 1;
+		}
 	}
 
 	return ret < 0 ? 0 : 1;
diff --git a/iptables/tests/shell/testcases/iptables/0008-unprivileged_0 b/iptables/tests/shell/testcases/iptables/0008-unprivileged_0
index 43e3bc8721dbd..983531fef4720 100755
--- a/iptables/tests/shell/testcases/iptables/0008-unprivileged_0
+++ b/iptables/tests/shell/testcases/iptables/0008-unprivileged_0
@@ -35,6 +35,12 @@ let "rc+=$?"
 grep_or_rc "DNAT target options:" <<< "$out"
 let "rc+=$?"
 
+# TEE has no revision 0
+out=$(run $XT_MULTI iptables -j TEE --help)
+let "rc+=$?"
+grep_or_rc "TEE target options:" <<< "$out"
+let "rc+=$?"
+
 out=$(run $XT_MULTI iptables -p tcp -j DNAT --help)
 let "rc+=$?"
 grep_or_rc "tcp match options:" <<< "$out"
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [iptables PATCH 4/4] nft: Fix EPERM handling for extensions without rev 0
  2022-05-04 10:34 ` [iptables PATCH 4/4] nft: Fix EPERM handling for extensions without rev 0 Phil Sutter
@ 2022-05-04 20:17   ` Pablo Neira Ayuso
  2022-05-05 12:09     ` Phil Sutter
  2022-05-06 11:43   ` [iptables PATCH v2 " Phil Sutter
  1 sibling, 1 reply; 9+ messages in thread
From: Pablo Neira Ayuso @ 2022-05-04 20:17 UTC (permalink / raw)
  To: Phil Sutter; +Cc: netfilter-devel

On Wed, May 04, 2022 at 12:34:16PM +0200, Phil Sutter wrote:
> Treating revision 0 as compatible in EPERM case works fine as long as
> there is a revision 0 of that extension defined in DSO. Fix the code for
> others: Extend the EPERM handling to all revisions and keep the existing
> warning for revision 0.
> 
> Fixes: 17534cb18ed0a ("Improve error messages for unsupported extensions")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  iptables/nft.c                                     | 14 ++++++++++----
>  .../shell/testcases/iptables/0008-unprivileged_0   |  6 ++++++
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/iptables/nft.c b/iptables/nft.c
> index 33813ce1b9202..95e6c222682c0 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
> @@ -3510,15 +3510,21 @@ int nft_compatible_revision(const char *name, uint8_t rev, int opt)
>  err:
>  	mnl_socket_close(nl);
>  
> -	/* pretend revision 0 is valid -
> +	/* ignore EPERM and errors for revision 0 -
>  	 * 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)
> +	if (ret < 0) {
> +		if (errno == EPERM) {
> +			fprintf(stderr,
> +				"%s: Could not determine whether revision %u is supported, assuming it is.\n",

I'm not sure the user can do much about this error message, to me the
revisions concept are developer-only, I don't think we expose this
implementation detail in the documentation.

Why warn users in this case?

> +				name, rev);
> +			return 1;
> +		} else if (rev == 0) {
>  			fprintf(stderr,
>  				"Warning: Extension %s revision 0 not supported, missing kernel module?\n",
>  				name);
> -		return 1;
> +			return 1;
> +		}
>  	}
>  
>  	return ret < 0 ? 0 : 1;
> diff --git a/iptables/tests/shell/testcases/iptables/0008-unprivileged_0 b/iptables/tests/shell/testcases/iptables/0008-unprivileged_0
> index 43e3bc8721dbd..983531fef4720 100755
> --- a/iptables/tests/shell/testcases/iptables/0008-unprivileged_0
> +++ b/iptables/tests/shell/testcases/iptables/0008-unprivileged_0
> @@ -35,6 +35,12 @@ let "rc+=$?"
>  grep_or_rc "DNAT target options:" <<< "$out"
>  let "rc+=$?"
>  
> +# TEE has no revision 0
> +out=$(run $XT_MULTI iptables -j TEE --help)
> +let "rc+=$?"
> +grep_or_rc "TEE target options:" <<< "$out"
> +let "rc+=$?"
> +
>  out=$(run $XT_MULTI iptables -p tcp -j DNAT --help)
>  let "rc+=$?"
>  grep_or_rc "tcp match options:" <<< "$out"
> -- 
> 2.34.1
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [iptables PATCH 4/4] nft: Fix EPERM handling for extensions without rev 0
  2022-05-04 20:17   ` Pablo Neira Ayuso
@ 2022-05-05 12:09     ` Phil Sutter
  2022-05-05 13:30       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 9+ messages in thread
From: Phil Sutter @ 2022-05-05 12:09 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

On Wed, May 04, 2022 at 10:17:21PM +0200, Pablo Neira Ayuso wrote:
> On Wed, May 04, 2022 at 12:34:16PM +0200, Phil Sutter wrote:
> > Treating revision 0 as compatible in EPERM case works fine as long as
> > there is a revision 0 of that extension defined in DSO. Fix the code for
> > others: Extend the EPERM handling to all revisions and keep the existing
> > warning for revision 0.
> > 
> > Fixes: 17534cb18ed0a ("Improve error messages for unsupported extensions")
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  iptables/nft.c                                     | 14 ++++++++++----
> >  .../shell/testcases/iptables/0008-unprivileged_0   |  6 ++++++
> >  2 files changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/iptables/nft.c b/iptables/nft.c
> > index 33813ce1b9202..95e6c222682c0 100644
> > --- a/iptables/nft.c
> > +++ b/iptables/nft.c
> > @@ -3510,15 +3510,21 @@ int nft_compatible_revision(const char *name, uint8_t rev, int opt)
> >  err:
> >  	mnl_socket_close(nl);
> >  
> > -	/* pretend revision 0 is valid -
> > +	/* ignore EPERM and errors for revision 0 -
> >  	 * 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)
> > +	if (ret < 0) {
> > +		if (errno == EPERM) {
> > +			fprintf(stderr,
> > +				"%s: Could not determine whether revision %u is supported, assuming it is.\n",
> 
> I'm not sure the user can do much about this error message, to me the
> revisions concept are developer-only, I don't think we expose this
> implementation detail in the documentation.
> 
> Why warn users in this case?

You're right, it does not make much sense to be verbose here. I copied
that error message from libxtables, iptables-legacy does the same if
socket() fails with EPERM during compatibility check for revisions != 0.

WDYT, drop both? Leave the one in libxtables alone "for legacy
purposes"?

I'd make them debug output, but nft_compatible_revision() does not have
access to nft_handle which I can't easily change since it is a callback
in xtables_globals.

Thanks, Phil

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [iptables PATCH 4/4] nft: Fix EPERM handling for extensions without rev 0
  2022-05-05 12:09     ` Phil Sutter
@ 2022-05-05 13:30       ` Pablo Neira Ayuso
  0 siblings, 0 replies; 9+ messages in thread
From: Pablo Neira Ayuso @ 2022-05-05 13:30 UTC (permalink / raw)
  To: Phil Sutter, netfilter-devel

On Thu, May 05, 2022 at 02:09:52PM +0200, Phil Sutter wrote:
> On Wed, May 04, 2022 at 10:17:21PM +0200, Pablo Neira Ayuso wrote:
> > On Wed, May 04, 2022 at 12:34:16PM +0200, Phil Sutter wrote:
> > > Treating revision 0 as compatible in EPERM case works fine as long as
> > > there is a revision 0 of that extension defined in DSO. Fix the code for
> > > others: Extend the EPERM handling to all revisions and keep the existing
> > > warning for revision 0.
> > > 
> > > Fixes: 17534cb18ed0a ("Improve error messages for unsupported extensions")
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > ---
> > >  iptables/nft.c                                     | 14 ++++++++++----
> > >  .../shell/testcases/iptables/0008-unprivileged_0   |  6 ++++++
> > >  2 files changed, 16 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/iptables/nft.c b/iptables/nft.c
> > > index 33813ce1b9202..95e6c222682c0 100644
> > > --- a/iptables/nft.c
> > > +++ b/iptables/nft.c
> > > @@ -3510,15 +3510,21 @@ int nft_compatible_revision(const char *name, uint8_t rev, int opt)
> > >  err:
> > >  	mnl_socket_close(nl);
> > >  
> > > -	/* pretend revision 0 is valid -
> > > +	/* ignore EPERM and errors for revision 0 -
> > >  	 * 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)
> > > +	if (ret < 0) {
> > > +		if (errno == EPERM) {
> > > +			fprintf(stderr,
> > > +				"%s: Could not determine whether revision %u is supported, assuming it is.\n",
> > 
> > I'm not sure the user can do much about this error message, to me the
> > revisions concept are developer-only, I don't think we expose this
> > implementation detail in the documentation.
> > 
> > Why warn users in this case?
> 
> You're right, it does not make much sense to be verbose here. I copied
> that error message from libxtables, iptables-legacy does the same if
> socket() fails with EPERM during compatibility check for revisions != 0.
> 
> WDYT, drop both? Leave the one in libxtables alone "for legacy
> purposes"?
> 
> I'd make them debug output, but nft_compatible_revision() does not have
> access to nft_handle which I can't easily change since it is a callback
> in xtables_globals.

If you needs this maybe you can do this before release, there is a
bump in libversion for recent updates in iptables, unless some recent
updates are reworked to be added into xshared.c (at the cost of
slightly increasing size of xtables-multi binaries).

Your call.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [iptables PATCH v2 4/4] nft: Fix EPERM handling for extensions without rev 0
  2022-05-04 10:34 ` [iptables PATCH 4/4] nft: Fix EPERM handling for extensions without rev 0 Phil Sutter
  2022-05-04 20:17   ` Pablo Neira Ayuso
@ 2022-05-06 11:43   ` Phil Sutter
  1 sibling, 0 replies; 9+ messages in thread
From: Phil Sutter @ 2022-05-06 11:43 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

Treating revision 0 as compatible in EPERM case works fine as long as
there is a revision 0 of that extension defined in DSO. Fix the code for
others: Extend the EPERM handling to all revisions and keep the existing
warning for revision 0.

Fixes: 17534cb18ed0a ("Improve error messages for unsupported extensions")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Drop useless error message.
---
 iptables/nft.c                                        | 11 +++++++----
 .../shell/testcases/iptables/0008-unprivileged_0      |  6 ++++++
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 4ab59b12d00b1..21dfbd32540e0 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -3551,15 +3551,18 @@ int nft_compatible_revision(const char *name, uint8_t rev, int opt)
 err:
 	mnl_socket_close(nl);
 
-	/* pretend revision 0 is valid -
+	/* ignore EPERM and errors for revision 0 -
 	 * 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)
+	if (ret < 0) {
+		if (errno == EPERM)
+			return 1;
+		if (rev == 0) {
 			fprintf(stderr,
 				"Warning: Extension %s revision 0 not supported, missing kernel module?\n",
 				name);
-		return 1;
+			return 1;
+		}
 	}
 
 	return ret < 0 ? 0 : 1;
diff --git a/iptables/tests/shell/testcases/iptables/0008-unprivileged_0 b/iptables/tests/shell/testcases/iptables/0008-unprivileged_0
index 43e3bc8721dbd..983531fef4720 100755
--- a/iptables/tests/shell/testcases/iptables/0008-unprivileged_0
+++ b/iptables/tests/shell/testcases/iptables/0008-unprivileged_0
@@ -35,6 +35,12 @@ let "rc+=$?"
 grep_or_rc "DNAT target options:" <<< "$out"
 let "rc+=$?"
 
+# TEE has no revision 0
+out=$(run $XT_MULTI iptables -j TEE --help)
+let "rc+=$?"
+grep_or_rc "TEE target options:" <<< "$out"
+let "rc+=$?"
+
 out=$(run $XT_MULTI iptables -p tcp -j DNAT --help)
 let "rc+=$?"
 grep_or_rc "tcp match options:" <<< "$out"
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-05-06 11:43 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-04 10:34 [iptables PATCH 0/4] Some misc fixes Phil Sutter
2022-05-04 10:34 ` [iptables PATCH 1/4] extensions: DNAT: Merge core printing functions Phil Sutter
2022-05-04 10:34 ` [iptables PATCH 2/4] man: *NAT: Review --random* option descriptions Phil Sutter
2022-05-04 10:34 ` [iptables PATCH 3/4] extensions: LOG: Document --log-macdecode in man page Phil Sutter
2022-05-04 10:34 ` [iptables PATCH 4/4] nft: Fix EPERM handling for extensions without rev 0 Phil Sutter
2022-05-04 20:17   ` Pablo Neira Ayuso
2022-05-05 12:09     ` Phil Sutter
2022-05-05 13:30       ` Pablo Neira Ayuso
2022-05-06 11:43   ` [iptables PATCH v2 " Phil Sutter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).