diff mbox series

[iptables,1/4] xtables: Do not register matches/targets with incompatible revision

Message ID 1520413843-24456-2-git-send-email-serhe.popovych@gmail.com
State Accepted
Delegated to: Pablo Neira
Headers show
Series iptables: Fix [unsupported revision] for matches/targets after update | expand

Commit Message

Serhey Popovych March 7, 2018, 9:10 a.m. UTC 
If kernel tells revision isn't found/supported at the moment we should
keep entity in pending list, not register or bail to do so later.

Kernel might still load module for entity we asking it for and this
could be slow on some embedded devices.

Catch double registration attempts by checking me->next being non-NULL
in xtables_register_match() and xtables_register_target().

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 libxtables/xtables.c |   66 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 24 deletions(-)

Comments

Phil Sutter Sept. 18, 2020, 2:13 p.m. UTC | #1 
Hi Serhey,

On Wed, Mar 07, 2018 at 11:10:40AM +0200, Serhey Popovych wrote:
> If kernel tells revision isn't found/supported at the moment we should
> keep entity in pending list, not register or bail to do so later.

This causes a problem in particular with conntrack match (but others may
be affected as well): If the kernel doesn't support an older revision of
the match, it stays in pending list and is retried for each new rule
using the match.

> Kernel might still load module for entity we asking it for and this
> could be slow on some embedded devices.

Is this a speculative problem or did you see it in reality? I'm
wondering because kernel uses try_then_request_module() to load the
missing extension which calls __request_module() with 'wait' parameter
set to true. So unless the called usermode helper is behaving unexpected
(e.g. fork and load in background), the call to
compatible_match_revision() should block until the module has been
loaded, no?

> Catch double registration attempts by checking me->next being non-NULL
> in xtables_register_match() and xtables_register_target().

Is this a side-effect of the above or an independent fix?

Cheers, Phil
diff mbox series

Patch

diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index 57a1102..5aaa238 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -198,8 +198,8 @@  struct xtables_match *xtables_matches;
 struct xtables_target *xtables_targets;
 
 /* Fully register a match/target which was previously partially registered. */
-static void xtables_fully_register_pending_match(struct xtables_match *me);
-static void xtables_fully_register_pending_target(struct xtables_target *me);
+static bool xtables_fully_register_pending_match(struct xtables_match *me);
+static bool xtables_fully_register_pending_target(struct xtables_target *me);
 
 void xtables_init(void)
 {
@@ -638,11 +638,11 @@  xtables_find_match(const char *name, enum xtables_tryload tryload,
 		if (extension_cmp(name, (*dptr)->name, (*dptr)->family)) {
 			ptr = *dptr;
 			*dptr = (*dptr)->next;
-			ptr->next = NULL;
-			xtables_fully_register_pending_match(ptr);
-		} else {
-			dptr = &((*dptr)->next);
+			if (xtables_fully_register_pending_match(ptr))
+				continue;
+			*dptr = ptr;
 		}
+		dptr = &((*dptr)->next);
 	}
 
 	for (ptr = xtables_matches; ptr; ptr = ptr->next) {
@@ -728,11 +728,11 @@  xtables_find_target(const char *name, enum xtables_tryload tryload)
 		if (extension_cmp(name, (*dptr)->name, (*dptr)->family)) {
 			ptr = *dptr;
 			*dptr = (*dptr)->next;
-			ptr->next = NULL;
-			xtables_fully_register_pending_target(ptr);
-		} else {
-			dptr = &((*dptr)->next);
+			if (xtables_fully_register_pending_target(ptr))
+				continue;
+			*dptr = ptr;
 		}
+		dptr = &((*dptr)->next);
 	}
 
 	for (ptr = xtables_targets; ptr; ptr = ptr->next) {
@@ -846,6 +846,12 @@  static void xtables_check_options(const char *name, const struct option *opt)
 
 void xtables_register_match(struct xtables_match *me)
 {
+	if (me->next) {
+		fprintf(stderr, "%s: match \"%s\" already registered\n",
+			xt_params->program_name, me->name);
+		exit(1);
+	}
+
 	if (me->version == NULL) {
 		fprintf(stderr, "%s: match %s<%u> is missing a version\n",
 		        xt_params->program_name, me->name, me->revision);
@@ -947,12 +953,17 @@  static int xtables_target_prefer(const struct xtables_target *a,
 				 b->revision, b->family);
 }
 
-static void xtables_fully_register_pending_match(struct xtables_match *me)
+static bool xtables_fully_register_pending_match(struct xtables_match *me)
 {
 	struct xtables_match **i, *old;
 	const char *rn;
 	int compare;
 
+	/* See if new match can be used. */
+	rn = (me->real_name != NULL) ? me->real_name : me->name;
+	if (!compatible_match_revision(rn, me->revision))
+		return false;
+
 	old = xtables_find_match(me->name, XTF_DURING_LOAD, NULL);
 	if (old) {
 		compare = xtables_match_prefer(old, me);
@@ -967,12 +978,7 @@  static void xtables_fully_register_pending_match(struct xtables_match *me)
 		rn = (old->real_name != NULL) ? old->real_name : old->name;
 		if (compare > 0 &&
 		    compatible_match_revision(rn, old->revision))
-			return;
-
-		/* See if new match can be used. */
-		rn = (me->real_name != NULL) ? me->real_name : me->name;
-		if (!compatible_match_revision(rn, me->revision))
-			return;
+			return true;
 
 		/* Delete old one. */
 		for (i = &xtables_matches; *i!=old; i = &(*i)->next);
@@ -993,6 +999,8 @@  static void xtables_fully_register_pending_match(struct xtables_match *me)
 
 	me->m = NULL;
 	me->mflags = 0;
+
+	return true;
 }
 
 void xtables_register_matches(struct xtables_match *match, unsigned int n)
@@ -1004,6 +1012,12 @@  void xtables_register_matches(struct xtables_match *match, unsigned int n)
 
 void xtables_register_target(struct xtables_target *me)
 {
+	if (me->next) {
+		fprintf(stderr, "%s: target \"%s\" already registered\n",
+			xt_params->program_name, me->name);
+		exit(1);
+	}
+
 	if (me->version == NULL) {
 		fprintf(stderr, "%s: target %s<%u> is missing a version\n",
 		        xt_params->program_name, me->name, me->revision);
@@ -1044,12 +1058,19 @@  void xtables_register_target(struct xtables_target *me)
 	xtables_pending_targets = me;
 }
 
-static void xtables_fully_register_pending_target(struct xtables_target *me)
+static bool xtables_fully_register_pending_target(struct xtables_target *me)
 {
 	struct xtables_target *old;
 	const char *rn;
 	int compare;
 
+	if (strcmp(me->name, "standard") != 0) {
+		/* See if new target can be used. */
+		rn = (me->real_name != NULL) ? me->real_name : me->name;
+		if (!compatible_target_revision(rn, me->revision))
+			return false;
+	}
+
 	old = xtables_find_target(me->name, XTF_DURING_LOAD);
 	if (old) {
 		struct xtables_target **i;
@@ -1066,12 +1087,7 @@  static void xtables_fully_register_pending_target(struct xtables_target *me)
 		rn = (old->real_name != NULL) ? old->real_name : old->name;
 		if (compare > 0 &&
 		    compatible_target_revision(rn, old->revision))
-			return;
-
-		/* See if new target can be used. */
-		rn = (me->real_name != NULL) ? me->real_name : me->name;
-		if (!compatible_target_revision(rn, me->revision))
-			return;
+			return true;
 
 		/* Delete old one. */
 		for (i = &xtables_targets; *i!=old; i = &(*i)->next);
@@ -1090,6 +1106,8 @@  static void xtables_fully_register_pending_target(struct xtables_target *me)
 	xtables_targets = me;
 	me->t = NULL;
 	me->tflags = 0;
+
+	return true;
 }
 
 void xtables_register_targets(struct xtables_target *target, unsigned int n)