* [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).