netfilter: half a patch for the pseudoregression

netfilter: half a patch for the pseudoregression

[Jan Engelhardt]

Here is the sample patch that fixes ip_tables_matches, but I still 
challenge the usefulness of /proc/net/ip_tables_* altogether.
Those files should have been removed, or appropriately upgraded when 
revision support was introduced.

http://picpaste.de/usefulornot_1.png

I am not fond of it, at all. Doing the same for targets - code
replication here we come. The LOC for it would have been
better spent at the previously-posted pretty-printer (knowing
that text printing meets the usual resistance).

---8<---
parent d8bf62a172295f26d960cea92aead0ce78ca524c (v2.6.29-rc2-21-gd8bf62a)
commit 3f1d7c73afeeb637ef05eba0ad7ccb55305ab8f2
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Tue Jan 27 23:21:16 2009 +0100

netfilter: make proc/net/ip_tables_matches print names from foreign NFPROTO

The ugliness this code adds could have been better spent at
a full and much more helpful dump.
---
 net/netfilter/x_tables.c |  115 +++++++++++++++++++++++++++++--------
 1 files changed, 90 insertions(+), 25 deletions(-)

diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index bfbf521..329c01c 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -827,39 +827,96 @@ static const struct file_operations xt_table_ops = {
 	.release = seq_release_net,
 };
 
-static void *xt_match_seq_start(struct seq_file *seq, loff_t *pos)
+struct match_trav {
+	struct list_head *head, *curr;
+	uint8_t class, nfproto;
+};
+
+enum {
+	MATCHTRAV_INIT,
+	MATCHTRAV_NFP_UNSPEC,
+	MATCHTRAV_NFP_SPEC,
+	MATCHTRAV_DONE,
+};
+
+static void *xt_match_seq_next(struct seq_file *seq, void *v, loff_t *ppos)
 {
-	struct proc_dir_entry *pde = (struct proc_dir_entry *)seq->private;
-	u_int16_t af = (unsigned long)pde->data;
+	static const uint8_t next_class[] = {
+		[MATCHTRAV_NFP_UNSPEC] = MATCHTRAV_NFP_SPEC,
+		[MATCHTRAV_NFP_SPEC]   = MATCHTRAV_DONE,
+	};
+	struct match_trav *trav = seq->private;
+
+	switch (trav->class) {
+	case MATCHTRAV_INIT:
+		trav->class = MATCHTRAV_NFP_UNSPEC;
+		mutex_lock(&xt[NFPROTO_UNSPEC].mutex);
+		trav->head = trav->curr = &xt[NFPROTO_UNSPEC].match;
+		break;
+	case MATCHTRAV_NFP_UNSPEC:
+		trav->curr = trav->curr->next;
+		if (trav->curr != trav->head)
+			break;
+		mutex_unlock(&xt[NFPROTO_UNSPEC].mutex);
+		mutex_lock(&xt[trav->nfproto].mutex);
+		trav->head = trav->curr = &xt[trav->nfproto].match;
+		trav->class = next_class[trav->class];
+		break;
+	case MATCHTRAV_NFP_SPEC:
+		trav->curr = trav->curr->next;
+		if (trav->curr != trav->head)
+			break;
+		/* fallthru, _stop will unlock */
+	default:
+		return NULL;
+	}
 
-	mutex_lock(&xt[af].mutex);
-	return seq_list_start(&xt[af].match, *pos);
+	if (ppos != NULL)
+		++*ppos;
+	return trav;
 }
 
-static void *xt_match_seq_next(struct seq_file *seq, void *v, loff_t *pos)
+static void *xt_match_seq_start(struct seq_file *seq, loff_t *pos)
 {
-	struct proc_dir_entry *pde = (struct proc_dir_entry *)seq->private;
-	u_int16_t af = (unsigned long)pde->data;
+	struct match_trav *trav = seq->private;
+	unsigned int j;
 
-	return seq_list_next(v, &xt[af].match, pos);
+	trav->class = MATCHTRAV_INIT;
+	for (j = 0; j < *pos; ++j)
+		if (xt_match_seq_next(seq, NULL, NULL) == NULL)
+			return NULL;
+	return trav;
 }
 
 static void xt_match_seq_stop(struct seq_file *seq, void *v)
 {
-	struct proc_dir_entry *pde = seq->private;
-	u_int16_t af = (unsigned long)pde->data;
-
-	mutex_unlock(&xt[af].mutex);
+	struct match_trav *trav = seq->private;
+
+	switch (trav->class) {
+	case MATCHTRAV_NFP_UNSPEC:
+		mutex_unlock(&xt[NFPROTO_UNSPEC].mutex);
+		break;
+	case MATCHTRAV_NFP_SPEC:
+		mutex_unlock(&xt[trav->nfproto].mutex);
+		break;
+	}
 }
 
 static int xt_match_seq_show(struct seq_file *seq, void *v)
 {
-	struct xt_match *match = list_entry(v, struct xt_match, list);
-
-	if (strlen(match->name))
-		return seq_printf(seq, "%s\n", match->name);
-	else
-		return 0;
+	const struct match_trav *trav = seq->private;
+	const struct xt_match *match;
+
+	switch (trav->class) {
+	case MATCHTRAV_NFP_UNSPEC:
+	case MATCHTRAV_NFP_SPEC:
+		if (trav->curr == trav->head)
+			return 0;
+		match = list_entry(trav->curr, struct xt_match, list);
+		return (*match->name == '\0') ? 0 :
+		       seq_printf(seq, "%s\n", match->name);
+	}
+	return 0;
 }
 
 static const struct seq_operations xt_match_seq_ops = {
@@ -871,15 +928,23 @@ static const struct seq_operations xt_match_seq_ops = {
 
 static int xt_match_open(struct inode *inode, struct file *file)
 {
+	struct seq_file *seq;
+	struct match_trav *trav;
 	int ret;
 
-	ret = seq_open(file, &xt_match_seq_ops);
-	if (!ret) {
-		struct seq_file *seq = file->private_data;
+	trav = kmalloc(sizeof(*trav), GFP_KERNEL);
+	if (trav == NULL)
+		return -ENOMEM;
 
-		seq->private = PDE(inode);
+	ret = seq_open(file, &xt_match_seq_ops);
+	if (ret < 0) {
+		kfree(trav);
+		return ret;
 	}
-	return ret;
+
+	seq = file->private_data;
+	seq->private = trav;
+	return 0;
 }
 
 static const struct file_operations xt_match_ops = {
@@ -887,7 +952,7 @@ static const struct file_operations xt_match_ops = {
 	.open	 = xt_match_open,
 	.read	 = seq_read,
 	.llseek	 = seq_lseek,
-	.release = seq_release,
+	.release = seq_release_private,
 };
 
 static void *xt_target_seq_start(struct seq_file *seq, loff_t *pos)
-- 
# Created with git-export-patch

Re: netfilter: half a patch for the pseudoregression

[Patrick McHardy]

Jan Engelhardt wrote:
> 
> Here is the sample patch that fixes ip_tables_matches, but I still 
> challenge the usefulness of /proc/net/ip_tables_* altogether.
> Those files should have been removed, or appropriately upgraded when 
> revision support was introduced.

Thats not really the question right now since it exists and people
are using it. Feel free to start a discussion on removing it once
we've fixed the regression.

> netfilter: make proc/net/ip_tables_matches print names from foreign NFPROTO
> 
> The ugliness this code adds could have been better spent at
> a full and much more helpful dump.

This might get easier by simply using the same mutex for all families.
Not sure if its worth it, in any case not for now.

The patch looks fine, please resubmit when you have fixed the second
half with proper changelog (stating that it fixes a regression and
the commit introducing it).

Re: netfilter: half a patch for the pseudoregression

[Jan Engelhardt]

On Monday 2009-02-09 17:20, Patrick McHardy wrote:
>
>> netfilter: make proc/net/ip_tables_matches print names from foreign NFPROTO
>>
>> The ugliness this code adds could have been better spent at
>> a full and much more helpful dump.
>
> This might get easier by simply using the same mutex for all families.

There is (probably) a reason this mutex exists - should not touch
it before analysis to make sure performance does not go down the
gutter just because cross-nfproto mutex was unified. Maybe Stephen's
RCU work makes it less of a problem. Have not looked into that.

> The patch looks fine, please resubmit when you have fixed the second
> half with proper changelog (stating that it fixes a regression and
> the commit introducing it).

parent de9662219c15ec76867815e1ea4ee6e3bd2d5783 (v2.6.29-rc4-24-gde96622)
commit 6919d7bc8bc5e0639b6699e06cbb5af1be2d9523
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Thu Feb 12 05:27:45 2009 +0100

netfilter: make proc/net/ip* print names from foreign NFPROTO

When extensions were moved to the NFPROTO_UNSPEC wildcard in
ab4f21e6fb1c09b13c4c3cb8357babe8223471bd, they disappeared from the
procfs files.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 net/netfilter/x_tables.c |  199 +++++++++++++++++++++++++++-----------
 1 files changed, 142 insertions(+), 57 deletions(-)

diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index bfbf521..5baccfa 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -827,59 +827,143 @@ static const struct file_operations xt_table_ops = {
 	.release = seq_release_net,
 };
 
-static void *xt_match_seq_start(struct seq_file *seq, loff_t *pos)
+/*
+ * Traverse state for ip{,6}_{tables,matches} for helping crossing
+ * the multi-AF mutexes.
+ */
+struct nf_mttg_trav {
+	struct list_head *head, *curr;
+	uint8_t class, nfproto;
+};
+
+enum {
+	MTTG_TRAV_INIT,
+	MTTG_TRAV_NFP_UNSPEC,
+	MTTG_TRAV_NFP_SPEC,
+	MTTG_TRAV_DONE,
+};
+
+static void *xt_mttg_seq_next(struct seq_file *seq, void *v, loff_t *ppos,
+    bool is_target)
 {
-	struct proc_dir_entry *pde = (struct proc_dir_entry *)seq->private;
-	u_int16_t af = (unsigned long)pde->data;
+	static const uint8_t next_class[] = {
+		[MTTG_TRAV_NFP_UNSPEC] = MTTG_TRAV_NFP_SPEC,
+		[MTTG_TRAV_NFP_SPEC]   = MTTG_TRAV_DONE,
+	};
+	struct nf_mttg_trav *trav = seq->private;
+
+	switch (trav->class) {
+	case MTTG_TRAV_INIT:
+		trav->class = MTTG_TRAV_NFP_UNSPEC;
+		mutex_lock(&xt[NFPROTO_UNSPEC].mutex);
+		trav->head = trav->curr = is_target ?
+			&xt[NFPROTO_UNSPEC].target : &xt[NFPROTO_UNSPEC].match;
+ 		break;
+	case MTTG_TRAV_NFP_UNSPEC:
+		trav->curr = trav->curr->next;
+		if (trav->curr != trav->head)
+			break;
+		mutex_unlock(&xt[NFPROTO_UNSPEC].mutex);
+		mutex_lock(&xt[trav->nfproto].mutex);
+		trav->head = trav->curr = is_target ?
+			&xt[trav->nfproto].target : &xt[trav->nfproto].match;
+		trav->class = next_class[trav->class];
+		break;
+	case MTTG_TRAV_NFP_SPEC:
+		trav->curr = trav->curr->next;
+		if (trav->curr != trav->head)
+			break;
+		/* fallthru, _stop will unlock */
+	default:
+		return NULL;
+	}
 
-	mutex_lock(&xt[af].mutex);
-	return seq_list_start(&xt[af].match, *pos);
+	if (ppos != NULL)
+		++*ppos;
+	return trav;
 }
 
-static void *xt_match_seq_next(struct seq_file *seq, void *v, loff_t *pos)
+static void *xt_mttg_seq_start(struct seq_file *seq, loff_t *pos,
+    bool is_target)
 {
-	struct proc_dir_entry *pde = (struct proc_dir_entry *)seq->private;
-	u_int16_t af = (unsigned long)pde->data;
+	struct nf_mttg_trav *trav = seq->private;
+	unsigned int j;
 
-	return seq_list_next(v, &xt[af].match, pos);
+	trav->class = MTTG_TRAV_INIT;
+	for (j = 0; j < *pos; ++j)
+		if (xt_mttg_seq_next(seq, NULL, NULL, is_target) == NULL)
+			return NULL;
+	return trav;
 }
 
-static void xt_match_seq_stop(struct seq_file *seq, void *v)
+static void xt_mttg_seq_stop(struct seq_file *seq, void *v)
 {
-	struct proc_dir_entry *pde = seq->private;
-	u_int16_t af = (unsigned long)pde->data;
+	struct nf_mttg_trav *trav = seq->private;
+
+	switch (trav->class) {
+	case MTTG_TRAV_NFP_UNSPEC:
+		mutex_unlock(&xt[NFPROTO_UNSPEC].mutex);
+		break;
+	case MTTG_TRAV_NFP_SPEC:
+		mutex_unlock(&xt[trav->nfproto].mutex);
+		break;
+	}
+}
 
-	mutex_unlock(&xt[af].mutex);
+static void *xt_match_seq_start(struct seq_file *seq, loff_t *pos)
+{
+	return xt_mttg_seq_start(seq, pos, false);
 }
 
-static int xt_match_seq_show(struct seq_file *seq, void *v)
+static void *xt_match_seq_next(struct seq_file *seq, void *v, loff_t *ppos)
 {
-	struct xt_match *match = list_entry(v, struct xt_match, list);
+	return xt_mttg_seq_next(seq, v, ppos, false);
+}
 
-	if (strlen(match->name))
-		return seq_printf(seq, "%s\n", match->name);
-	else
-		return 0;
+static int xt_match_seq_show(struct seq_file *seq, void *v)
+{
+	const struct nf_mttg_trav *trav = seq->private;
+	const struct xt_match *match;
+
+	switch (trav->class) {
+	case MTTG_TRAV_NFP_UNSPEC:
+	case MTTG_TRAV_NFP_SPEC:
+		if (trav->curr == trav->head)
+			return 0;
+		match = list_entry(trav->curr, struct xt_match, list);
+		return (*match->name == '\0') ? 0 :
+		       seq_printf(seq, "%s\n", match->name);
+	}
+	return 0;
 }
 
 static const struct seq_operations xt_match_seq_ops = {
 	.start	= xt_match_seq_start,
 	.next	= xt_match_seq_next,
-	.stop	= xt_match_seq_stop,
+	.stop	= xt_mttg_seq_stop,
 	.show	= xt_match_seq_show,
 };
 
 static int xt_match_open(struct inode *inode, struct file *file)
 {
+	struct seq_file *seq;
+	struct nf_mttg_trav *trav;
 	int ret;
 
-	ret = seq_open(file, &xt_match_seq_ops);
-	if (!ret) {
-		struct seq_file *seq = file->private_data;
+	trav = kmalloc(sizeof(*trav), GFP_KERNEL);
+	if (trav == NULL)
+		return -ENOMEM;
 
-		seq->private = PDE(inode);
+	ret = seq_open(file, &xt_match_seq_ops);
+	if (ret < 0) {
+		kfree(trav);
+		return ret;
 	}
-	return ret;
+
+	seq = file->private_data;
+	seq->private = trav;
+	trav->nfproto = (unsigned long)PDE(inode)->data;
+	return 0;
 }
 
 static const struct file_operations xt_match_ops = {
@@ -887,62 +971,63 @@ static const struct file_operations xt_match_ops = {
 	.open	 = xt_match_open,
 	.read	 = seq_read,
 	.llseek	 = seq_lseek,
-	.release = seq_release,
+	.release = seq_release_private,
 };
 
 static void *xt_target_seq_start(struct seq_file *seq, loff_t *pos)
 {
-	struct proc_dir_entry *pde = (struct proc_dir_entry *)seq->private;
-	u_int16_t af = (unsigned long)pde->data;
-
-	mutex_lock(&xt[af].mutex);
-	return seq_list_start(&xt[af].target, *pos);
+	return xt_mttg_seq_start(seq, pos, true);
 }
 
-static void *xt_target_seq_next(struct seq_file *seq, void *v, loff_t *pos)
+static void *xt_target_seq_next(struct seq_file *seq, void *v, loff_t *ppos)
 {
-	struct proc_dir_entry *pde = (struct proc_dir_entry *)seq->private;
-	u_int16_t af = (unsigned long)pde->data;
-
-	return seq_list_next(v, &xt[af].target, pos);
-}
-
-static void xt_target_seq_stop(struct seq_file *seq, void *v)
-{
-	struct proc_dir_entry *pde = seq->private;
-	u_int16_t af = (unsigned long)pde->data;
-
-	mutex_unlock(&xt[af].mutex);
+	return xt_mttg_seq_next(seq, v, ppos, true);
 }
 
 static int xt_target_seq_show(struct seq_file *seq, void *v)
 {
-	struct xt_target *target = list_entry(v, struct xt_target, list);
-
-	if (strlen(target->name))
-		return seq_printf(seq, "%s\n", target->name);
-	else
-		return 0;
+	const struct nf_mttg_trav *trav = seq->private;
+	const struct xt_target *target;
+
+	switch (trav->class) {
+	case MTTG_TRAV_NFP_UNSPEC:
+	case MTTG_TRAV_NFP_SPEC:
+		if (trav->curr == trav->head)
+			return 0;
+		target = list_entry(trav->curr, struct xt_target, list);
+		return (*target->name == '\0') ? 0 :
+		       seq_printf(seq, "%s\n", target->name);
+	}
+	return 0;
 }
 
 static const struct seq_operations xt_target_seq_ops = {
 	.start	= xt_target_seq_start,
 	.next	= xt_target_seq_next,
-	.stop	= xt_target_seq_stop,
+	.stop	= xt_mttg_seq_stop,
 	.show	= xt_target_seq_show,
 };
 
 static int xt_target_open(struct inode *inode, struct file *file)
 {
+	struct seq_file *seq;
+	struct nf_mttg_trav *trav;
 	int ret;
 
-	ret = seq_open(file, &xt_target_seq_ops);
-	if (!ret) {
-		struct seq_file *seq = file->private_data;
+	trav = kmalloc(sizeof(*trav), GFP_KERNEL);
+	if (trav == NULL)
+		return -ENOMEM;
 
-		seq->private = PDE(inode);
+	ret = seq_open(file, &xt_target_seq_ops);
+	if (ret < 0) {
+		kfree(trav);
+		return ret;
 	}
-	return ret;
+
+	seq = file->private_data;
+	seq->private = trav;
+	trav->nfproto = (unsigned long)PDE(inode)->data;
+	return 0;
 }
 
 static const struct file_operations xt_target_ops = {
@@ -950,7 +1035,7 @@ static const struct file_operations xt_target_ops = {
 	.open	 = xt_target_open,
 	.read	 = seq_read,
 	.llseek	 = seq_lseek,
-	.release = seq_release,
+	.release = seq_release_private,
 };
 
 #define FORMAT_TABLES	"_tables_names"
-- 
# Created with git-export-patch

Re: netfilter: half a patch for the pseudoregression

[Jan Engelhardt]

On Monday 2009-02-09 17:20, Patrick McHardy wrote:
> Jan Engelhardt wrote:
>>
>> Here is the sample patch that fixes ip_tables_matches, but I still challenge
>> the usefulness of /proc/net/ip_tables_* altogether.
>> Those files should have been removed, or appropriately upgraded when revision
>> support was introduced.
>
> Thats not really the question right now since it exists and people
> are using it. Feel free to start a discussion on removing it once
> we've fixed the regression.

I've also send the complete thing (twice now). Did you receive it?

Re: netfilter: half a patch for the pseudoregression

[Patrick McHardy]

Jan Engelhardt wrote:
> On Monday 2009-02-09 17:20, Patrick McHardy wrote:
>> Jan Engelhardt wrote:
>>> Here is the sample patch that fixes ip_tables_matches, but I still challenge
>>> the usefulness of /proc/net/ip_tables_* altogether.
>>> Those files should have been removed, or appropriately upgraded when revision
>>> support was introduced.
>> Thats not really the question right now since it exists and people
>> are using it. Feel free to start a discussion on removing it once
>> we've fixed the regression.
> 
> I've also send the complete thing (twice now). Did you receive it?

Applied, thanks. Will pass it upstream after some testing.