pass netns to ->match or to not pass

pass netns to ->match or to not pass

[Alexey Dobriyan]

xt_recent wants netns inside ->match hook (and xt_hashlimit probably,
haven't looked closely):

	recent_mt
	recent_table_lookup
	[use per-netns tables list]

Now, I can drag netns through struct xt_match_param, or use
dev_net(skb->dev ? skb->dev : skb->dst->dev) trick.

And the latter should be better, because only xt_recent module will be
affected.

Comments?

Re: pass netns to ->match or to not pass

[Jan Engelhardt]

On Thursday 2008-10-23 19:16, Alexey Dobriyan wrote:

>xt_recent wants netns inside ->match hook (and xt_hashlimit probably,
>haven't looked closely):
>
>	recent_mt
>	recent_table_lookup
>	[use per-netns tables list]
>
>Now, I can drag netns through struct xt_match_param, or use
>dev_net(skb->dev ? skb->dev : skb->dst->dev) trick.
>
>And the latter should be better, because only xt_recent module will be
>affected.
>
>Comments?

What exactly needs netns? This?

	if (par->out != NULL && skb->sk == NULL)
		ttl++;

Re: pass netns to ->match or to not pass

[Alexey Dobriyan]

On Thu, Oct 23, 2008 at 07:17:29PM -0400, Jan Engelhardt wrote:
> 
> On Thursday 2008-10-23 19:16, Alexey Dobriyan wrote:
> 
> >xt_recent wants netns inside ->match hook (and xt_hashlimit probably,
> >haven't looked closely):
> >
> >	recent_mt
> >	recent_table_lookup
> >	[use per-netns tables list]
> >
> >Now, I can drag netns through struct xt_match_param, or use
> >dev_net(skb->dev ? skb->dev : skb->dst->dev) trick.
> >
> >And the latter should be better, because only xt_recent module will be
> >affected.
> >
> >Comments?
> 
> What exactly needs netns? This?
> 
> 	if (par->out != NULL && skb->sk == NULL)
> 		ttl++;

Nope, something like below, and if I'm reading correctly,
struct xt_recent_mtinfo is immutable because of ABI, hence the question.


--- a/net/netfilter/xt_recent.c
+++ b/net/netfilter/xt_recent.c
@@ -180,11 +180,12 @@ static void recent_entry_update(struct recent_table *t, struct recent_entry *e)
 	list_move_tail(&e->lru_list, &t->lru_list);
 }
 
-static struct recent_table *recent_table_lookup(const char *name)
+static struct recent_table *recent_table_lookup(struct net *net, const char *name)
 {
+	struct netns_recent *net_recent = net_generic(net, recent_net_id);
 	struct recent_table *t;
 
-	list_for_each_entry(t, &tables, list)
+	list_for_each_entry(t, &net_recent->tables, list)
 		if (!strcmp(t->name, name))
 			return t;
 	return NULL;
@@ -203,6 +204,7 @@ static void recent_table_flush(struct recent_table *t)
 static bool
 recent_mt(const struct sk_buff *skb, const struct xt_match_param *par)
 {
+	struct net *net = ???;
 	const struct xt_recent_mtinfo *info = par->matchinfo;
 	struct recent_table *t;
 	struct recent_entry *e;
@@ -235,7 +237,7 @@ recent_mt(const struct sk_buff *skb, const struct xt_match_param *par)
 		ttl++;
 
 	spin_lock_bh(&recent_lock);
-	t = recent_table_lookup(info->name);
+	t = recent_table_lookup(net, info->name);
 	e = recent_entry_lookup(t, &addr, par->match->family,
 				(info->check_set & XT_RECENT_TTL) ? ttl : 0);
 	if (e == NULL) {

Re: pass netns to ->match or to not pass

[Jan Engelhardt]

On Thursday 2008-10-23 19:36, Alexey Dobriyan wrote:
>> 
>> >xt_recent wants netns inside ->match hook (and xt_hashlimit probably,
>> >haven't looked closely):
>> >
>> >	recent_mt
>> >	recent_table_lookup
>> >	[use per-netns tables list]
>> >
>> >Now, I can drag netns through struct xt_match_param, or use
>> >dev_net(skb->dev ? skb->dev : skb->dst->dev) trick.
>> >
>> >And the latter should be better, because only xt_recent module will be
>> >affected.
>> >
>> >Comments?
>> 
>> What exactly needs netns? This?
>> 
>> 	if (par->out != NULL && skb->sk == NULL)
>> 		ttl++;
>
>Nope, something like below, and if I'm reading correctly,
>struct xt_recent_mtinfo is immutable because of ABI, hence the question.

What info would you pass thorugh xt_recent_mtinfo anyway?

>@@ -203,6 +204,7 @@ static void recent_table_flush(struct recent_table *t)
> static bool
> recent_mt(const struct sk_buff *skb, const struct xt_match_param *par)
> {
>+	struct net *net = ???;

Either add a net in match_param or use dev_net.
The latter seems nicer because it does not use another 8 bytes
in match_param.

Re: pass netns to ->match or to not pass

[Patrick McHardy]

Alexey Dobriyan wrote:
> xt_recent wants netns inside ->match hook (and xt_hashlimit probably,
> haven't looked closely):
> 
> 	recent_mt
> 	recent_table_lookup
> 	[use per-netns tables list]
> 
> Now, I can drag netns through struct xt_match_param, or use
> dev_net(skb->dev ? skb->dev : skb->dst->dev) trick.
> 
> And the latter should be better, because only xt_recent module will be
> affected.
> 
> Comments?

I prefer the later. Using the supplied device parameters would
be better though to keep the device handling centralized.

Re: pass netns to ->match or to not pass

[Alexey Dobriyan]

On Thu, Oct 23, 2008 at 08:23:10PM -0400, Jan Engelhardt wrote:
> 
> On Thursday 2008-10-23 19:36, Alexey Dobriyan wrote:
> >> 
> >> >xt_recent wants netns inside ->match hook (and xt_hashlimit probably,
> >> >haven't looked closely):
> >> >
> >> >	recent_mt
> >> >	recent_table_lookup
> >> >	[use per-netns tables list]
> >> >
> >> >Now, I can drag netns through struct xt_match_param, or use
> >> >dev_net(skb->dev ? skb->dev : skb->dst->dev) trick.
> >> >
> >> >And the latter should be better, because only xt_recent module will be
> >> >affected.
> >> >
> >> >Comments?
> >> 
> >> What exactly needs netns? This?
> >> 
> >> 	if (par->out != NULL && skb->sk == NULL)
> >> 		ttl++;
> >
> >Nope, something like below, and if I'm reading correctly,
> >struct xt_recent_mtinfo is immutable because of ABI, hence the question.
> 
> What info would you pass thorugh xt_recent_mtinfo anyway?

Well, if it would have a pointer like xt_connlimit has, I could take
netns from ->matchinfo. Here is xt_connlimit patch, btw.


commit d922d559ee30336579f56e7f45b4eac977643f29
Author: Alexey Dobriyan <adobriyan@gmail.com>
Date:   Fri Oct 24 02:37:00 2008 +0400

    netns nf: xt_connlimit in netns

diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index 7f404cc..ef01431 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -38,6 +38,7 @@ struct xt_connlimit_conn {
 struct xt_connlimit_data {
 	struct list_head iphash[256];
 	spinlock_t lock;
+	struct net *net;
 };
 
 static u_int32_t connlimit_rnd;
@@ -105,6 +106,7 @@ static int count_them(struct xt_connlimit_data *data,
 		      const union nf_inet_addr *mask,
 		      const struct xt_match *match)
 {
+	struct net *net = data->net;
 	const struct nf_conntrack_tuple_hash *found;
 	struct xt_connlimit_conn *conn;
 	struct xt_connlimit_conn *tmp;
@@ -123,7 +125,7 @@ static int count_them(struct xt_connlimit_data *data,
 
 	/* check the saved connections */
 	list_for_each_entry_safe(conn, tmp, hash, list) {
-		found    = __nf_conntrack_find(&init_net, &conn->tuple);
+		found    = __nf_conntrack_find(net, &conn->tuple);
 		found_ct = NULL;
 
 		if (found != NULL)
@@ -242,6 +244,7 @@ static bool connlimit_mt_check(const struct xt_mtchk_param *par)
 	spin_lock_init(&info->data->lock);
 	for (i = 0; i < ARRAY_SIZE(info->data->iphash); ++i)
 		INIT_LIST_HEAD(&info->data->iphash[i]);
+	info->data->net = par->net;
 
 	return true;
 }

Re: pass netns to ->match or to not pass

[Jan Engelhardt]

On Friday 2008-10-24 05:45, Alexey Dobriyan wrote:
>
>Well, if it would have a pointer like xt_connlimit has, I could take
>netns from ->matchinfo. Here is xt_connlimit patch, btw.
>
>diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
>index 7f404cc..ef01431 100644
>--- a/net/netfilter/xt_connlimit.c
>+++ b/net/netfilter/xt_connlimit.c
>@@ -38,6 +38,7 @@ struct xt_connlimit_conn {
> struct xt_connlimit_data {
> 	struct list_head iphash[256];
> 	spinlock_t lock;
>+	struct net *net;
> };

And why should this be better than dev_net(par->in)/dev_net(par_out)?

Re: pass netns to ->match or to not pass

[Alexey Dobriyan]

On Fri, Oct 24, 2008 at 09:31:28AM -0400, Jan Engelhardt wrote:
> 
> On Friday 2008-10-24 05:45, Alexey Dobriyan wrote:
> >
> >Well, if it would have a pointer like xt_connlimit has, I could take
> >netns from ->matchinfo. Here is xt_connlimit patch, btw.
> >
> >diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
> >index 7f404cc..ef01431 100644
> >--- a/net/netfilter/xt_connlimit.c
> >+++ b/net/netfilter/xt_connlimit.c
> >@@ -38,6 +38,7 @@ struct xt_connlimit_conn {
> > struct xt_connlimit_data {
> > 	struct list_head iphash[256];
> > 	spinlock_t lock;
> >+	struct net *net;
> > };
> 
> And why should this be better than dev_net(par->in)/dev_net(par_out)?

par->in doesn't exist, not sure what do you mean.

Re: pass netns to ->match or to not pass

[Jan Engelhardt]

On Friday 2008-10-24 10:10, Alexey Dobriyan wrote:
>On Fri, Oct 24, 2008 at 09:31:28AM -0400, Jan Engelhardt wrote:
>> 
>> >Well, if it would have a pointer like xt_connlimit has, I could take
>> >netns from ->matchinfo. Here is xt_connlimit patch, btw.
>> >
>> >diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
>> >index 7f404cc..ef01431 100644
>> >--- a/net/netfilter/xt_connlimit.c
>> >+++ b/net/netfilter/xt_connlimit.c
>> >@@ -38,6 +38,7 @@ struct xt_connlimit_conn {
>> > struct xt_connlimit_data {
>> > 	struct list_head iphash[256];
>> > 	spinlock_t lock;
>> >+	struct net *net;
>> > };
>> 
>> And why should this be better than dev_net(par->in)/dev_net(par_out)?
>
>par->in doesn't exist, not sure what do you mean.
>
Of course that exists when you get a packet. (If not, then it's par->out
in case of forwarded/output traffic.)

static bool
connlimit_mt(const struct sk_buff *skb, const struct xt_match_param *par)
{
	...
}

struct xt_match_param {
	const struct net_device *in, *out;   <- THAT ONE
	const struct xt_match *match;
	const void *matchinfo;
	int fragoff;
	unsigned int thoff;
	bool *hotdrop;
	u_int8_t family;
};

Re: pass netns to ->match or to not pass

[Alexey Dobriyan]

On Fri, Oct 24, 2008 at 11:06:25AM -0400, Jan Engelhardt wrote:
> 
> On Friday 2008-10-24 10:10, Alexey Dobriyan wrote:
> >On Fri, Oct 24, 2008 at 09:31:28AM -0400, Jan Engelhardt wrote:
> >> 
> >> >Well, if it would have a pointer like xt_connlimit has, I could take
> >> >netns from ->matchinfo. Here is xt_connlimit patch, btw.
> >> >
> >> >diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
> >> >index 7f404cc..ef01431 100644
> >> >--- a/net/netfilter/xt_connlimit.c
> >> >+++ b/net/netfilter/xt_connlimit.c
> >> >@@ -38,6 +38,7 @@ struct xt_connlimit_conn {
> >> > struct xt_connlimit_data {
> >> > 	struct list_head iphash[256];
> >> > 	spinlock_t lock;
> >> >+	struct net *net;
> >> > };
> >> 
> >> And why should this be better than dev_net(par->in)/dev_net(par_out)?
> >
> >par->in doesn't exist, not sure what do you mean.
> >
> Of course that exists when you get a packet. (If not, then it's par->out
> in case of forwarded/output traffic.)
> 
> static bool
> connlimit_mt(const struct sk_buff *skb, const struct xt_match_param *par)
> {
> 	...
> }
> 
> struct xt_match_param {
> 	const struct net_device *in, *out;   <- THAT ONE

Ah, these ones!