[PATCH v2] net/netfilter/x_tables.c: use __seq_open_private()

[Rob Jones]

Reduce boilerplate code by using __seq_open_private() instead of seq_open()
in xt_match_open() and xt_target_open().

Signed-off-by: Rob Jones <***@codethink.co.uk>
---

This patch uses an existing variant of seq_open() to reduce the kernel code
size.

The only significant variation from the pre-existing code is the fact that
__seq_open_private() calls kzalloc() rather than kmalloc(), which could
conceivably have an impact on timing.

This version 2 incorporates a minor initialisation simplification (resulting
from kzalloc() being used) requested by netfilter-***@vger.kernel.org

net/netfilter/x_tables.c | 31 ++++---------------------------
1 file changed, 4 insertions(+), 27 deletions(-)

diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 227aa11..393f17b 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1080,7 +1080,6 @@ static void *xt_mttg_seq_start(struct seq_file *seq, loff_t *pos,
struct nf_mttg_trav *trav = seq->private;
unsigned int j;

- trav->class = MTTG_TRAV_INIT;
for (j = 0; j < *pos; ++j)
if (xt_mttg_seq_next(seq, NULL, NULL, is_target) == NULL)
return NULL;
@@ -1137,22 +1136,11 @@ 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 nf_mttg_trav *trav;
- int ret;
-
- trav = kmalloc(sizeof(*trav), GFP_KERNEL);
- if (trav == NULL)
+ trav = __seq_open_private(file, &xt_match_seq_ops, sizeof(*trav));
+ if (!trav)
return -ENOMEM;

- ret = seq_open(file, &xt_match_seq_ops);
- if (ret < 0) {
- kfree(trav);
- return ret;
- }
-
- seq = file->private_data;
- seq->private = trav;
trav->nfproto = (unsigned long)PDE_DATA(inode);
return 0;
}
@@ -1201,22 +1189,11 @@ static const struct seq_operations xt_target_seq_ops = {

static int xt_target_open(struct inode *inode, struct file *file)
{
- struct seq_file *seq;
struct nf_mttg_trav *trav;
- int ret;
-
- trav = kmalloc(sizeof(*trav), GFP_KERNEL);
- if (trav == NULL)
+ trav = __seq_open_private(file, &xt_target_seq_ops, sizeof(*trav));
+ if (!trav)
return -ENOMEM;

- ret = seq_open(file, &xt_target_seq_ops);
- if (ret < 0) {
- kfree(trav);
- return ret;
- }
-
- seq = file->private_data;
- seq->private = trav;
trav->nfproto = (unsigned long)PDE_DATA(inode);
return 0;
}

[Ben Hutchings]

Post by Rob Jones
Reduce boilerplate code by using __seq_open_private() instead of seq_open()
in xt_match_open() and xt_target_open().
---
This patch uses an existing variant of seq_open() to reduce the kernel code
size.
The only significant variation from the pre-existing code is the fact that
__seq_open_private() calls kzalloc() rather than kmalloc(), which could
conceivably have an impact on timing.
This version 2 incorporates a minor initialisation simplification (resulting

[...]

Post by Rob Jones
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1080,7 +1080,6 @@ static void *xt_mttg_seq_start(struct seq_file *seq, loff_t *pos,
struct nf_mttg_trav *trav = seq->private;
unsigned int j;
- trav->class = MTTG_TRAV_INIT;
for (j = 0; j < *pos; ++j)
if (xt_mttg_seq_next(seq, NULL, NULL, is_target) == NULL)
return NULL;

[...]

I'm fairly sure this simplification is wrong, as xt_mttg_seq_start() is
potentially called multiple times on the same file handle.

Ben.

[Pablo Neira Ayuso]

Post by Ben Hutchings

Post by Rob Jones
Reduce boilerplate code by using __seq_open_private() instead of seq_open()
in xt_match_open() and xt_target_open().
---
This patch uses an existing variant of seq_open() to reduce the kernel code
size.
The only significant variation from the pre-existing code is the fact that
__seq_open_private() calls kzalloc() rather than kmalloc(), which could
conceivably have an impact on timing.
This version 2 incorporates a minor initialisation simplification (resulting

[...]

Post by Rob Jones
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1080,7 +1080,6 @@ static void *xt_mttg_seq_start(struct seq_file *seq, loff_t *pos,
struct nf_mttg_trav *trav = seq->private;
unsigned int j;
- trav->class = MTTG_TRAV_INIT;
for (j = 0; j < *pos; ++j)
if (xt_mttg_seq_next(seq, NULL, NULL, is_target) == NULL)
return NULL;

[...]
I'm fairly sure this simplification is wrong, as xt_mttg_seq_start() is
potentially called multiple times on the same file handle.

Right. I'm going to take v1 of this patch instead, sorry for the
inconvenience Rob.

[Rob Jones]

Post by Pablo Neira Ayuso

Post by Ben Hutchings

Post by Rob Jones
Reduce boilerplate code by using __seq_open_private() instead of seq_open()
in xt_match_open() and xt_target_open().
---
This patch uses an existing variant of seq_open() to reduce the kernel code
size.
The only significant variation from the pre-existing code is the fact that
__seq_open_private() calls kzalloc() rather than kmalloc(), which could
conceivably have an impact on timing.
This version 2 incorporates a minor initialisation simplification (resulting

[...]

Post by Rob Jones
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1080,7 +1080,6 @@ static void *xt_mttg_seq_start(struct seq_file *seq, loff_t *pos,
struct nf_mttg_trav *trav = seq->private;
unsigned int j;
- trav->class = MTTG_TRAV_INIT;
for (j = 0; j < *pos; ++j)
if (xt_mttg_seq_next(seq, NULL, NULL, is_target) == NULL)
return NULL;

[...]
I'm fairly sure this simplification is wrong, as xt_mttg_seq_start() is
potentially called multiple times on the same file handle.

Well spotted Ben.

Post by Pablo Neira Ayuso
Right. I'm going to take v1 of this patch instead, sorry for the
inconvenience Rob.

No problem at all. It gave me something to do on the train home last
night :-)