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