RE: [PATCH nf v5 2/3] netfilter: nat_helper: Make sure every proto nat module uses its nat_helper

[Gao Feng]

Hi Pablo,

> From: netfilter-devel-ow...@vger.kernel.org
> [mailto:netfilter-devel-ow...@vger.kernel.org] On Behalf Of Pablo Neira
> Ayuso
> On Fri, Apr 14, 2017 at 08:46:44AM +0800, Gao Feng wrote:
> > > -Original Message-
> > > From: Pablo Neira Ayuso [mailto:pa...@netfilter.org] On Fri, Mar 31,
> > > 2017 at 06:38:20PM +0800, gfree.w...@foxmail.com wrote:
> > > > +static struct nf_ct_nat_helper pptp_nat = {
> > > > +   .name   = "pptp_nat",
> > >
> > > Why all these with "xyz_nat" names?
> >
> > I just used the variable name before.
> > How about rename it to "xyz_nat_helper"?
> > >
> > > This is going to break ctnetlink, as this is the name that
> > > identifies the
> > NAT
> > > helper to be used.
> 
> This name is exposed to userspace, right?
> 
> So FTP uses to rely on the "nat-follow-master" expectfn. But now the name
will
> be "ftp_nat".
> 
> If that is the case, this would break backward.

I am a little confusing.
The " struct nf_ct_nat_helper ftp_nat ftp_nat" is one new variable, while
the original variable 
"struct nf_ct_nat_helper follow_master_nat" name still is "
"nat-follow-master".

I couldn't figure about how could this break backward.
Do you think the name " ftp_nat " should use " ftp_nat_follow_master"?

BTW, how about use nat_helper pointer instead of expect fn?
As I mentioned before, it could avoid two assignment. 
Is it ok?

Regards
Feng

> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel"
in the
> body of a message to majord...@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html



--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nf v5 2/3] netfilter: nat_helper: Make sure every proto nat module uses its nat_helper

[Pablo Neira Ayuso]

On Fri, Apr 14, 2017 at 08:46:44AM +0800, Gao Feng wrote:
> > -Original Message-
> > From: Pablo Neira Ayuso [mailto:pa...@netfilter.org]
> > On Fri, Mar 31, 2017 at 06:38:20PM +0800, gfree.w...@foxmail.com wrote:
> > > +static struct nf_ct_nat_helper pptp_nat = {
> > > + .name   = "pptp_nat",
> > 
> > Why all these with "xyz_nat" names?
> 
> I just used the variable name before.
> How about rename it to "xyz_nat_helper"?
> > 
> > This is going to break ctnetlink, as this is the name that identifies the
> NAT
> > helper to be used.

This name is exposed to userspace, right?

So FTP uses to rely on the "nat-follow-master" expectfn. But now the
name will be "ftp_nat".

If that is the case, this would break backward.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH nf v5 2/3] netfilter: nat_helper: Make sure every proto nat module uses its nat_helper

[Gao Feng]

> -Original Message-
> From: Pablo Neira Ayuso [mailto:pa...@netfilter.org]
> On Fri, Mar 31, 2017 at 06:38:20PM +0800, gfree.w...@foxmail.com wrote:
> > +static struct nf_ct_nat_helper pptp_nat = {
> > +   .name   = "pptp_nat",
> 
> Why all these with "xyz_nat" names?

I just used the variable name before.
How about rename it to "xyz_nat_helper"?
> 
> This is going to break ctnetlink, as this is the name that identifies the
NAT
> helper to be used.
> 
> > +   .expectfn   = pptp_expectfn,
> > +};
> > +
> >  static void pptp_expectfn(struct nf_conn *ct,
> >  struct nf_conntrack_expect *exp)
> >  {
> > @@ -221,7 +229,7 @@ static int exp_gre(struct nf_conn *ct, __be16
callid,
> __be16 peer_callid)
> >   >tuplehash[dir].tuple.src.u3,
> >   >tuplehash[dir].tuple.dst.u3,
> >   IPPROTO_GRE, _callid, );
> > -   exp_orig->expectfn = pptp_expectfn;
> > +   exp_orig->nat_helper = _nat;
> >
> > /* reply direction, PAC->PNS */
> > dir = IP_CT_DIR_REPLY;
> > @@ -230,7 +238,7 @@ static int exp_gre(struct nf_conn *ct, __be16
callid,
> __be16 peer_callid)
> >   >tuplehash[dir].tuple.src.u3,
> >   >tuplehash[dir].tuple.dst.u3,
> >   IPPROTO_GRE, , _callid);
> > -   exp_reply->expectfn = pptp_expectfn;
> > +   exp_reply->nat_helper = _nat;
> 
> Why do we need to attach nat_helper instead of expectfn? I would prefer we
> do not.

Because we need to introduce another member "expectfn_module" in the third
patch.
If insisted on the current style, the codes would look like the following:
exp_reply->expectfn = pptp_expectfn;
exp_reply->expectfn_module = THIS_MODULE;

There are two assignments when using expect_fn.
It isn't only a little duplicated, but also it is easy to lost the module
assignment and bring one bug.

If attached the nat_helper instead of expectfn, there would be only one
assignment.

Regards
Feng





--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nf v5 2/3] netfilter: nat_helper: Make sure every proto nat module uses its nat_helper

[Pablo Neira Ayuso]

On Fri, Mar 31, 2017 at 06:38:20PM +0800, gfree.w...@foxmail.com wrote:
> +static struct nf_ct_nat_helper pptp_nat = {
> + .name   = "pptp_nat",

Why all these with "xyz_nat" names?

This is going to break ctnetlink, as this is the name that identifies
the NAT helper to be used.

> + .expectfn   = pptp_expectfn,
> +};
> +
>  static void pptp_expectfn(struct nf_conn *ct,
>struct nf_conntrack_expect *exp)
>  {
> @@ -221,7 +229,7 @@ static int exp_gre(struct nf_conn *ct, __be16 callid, 
> __be16 peer_callid)
> >tuplehash[dir].tuple.src.u3,
> >tuplehash[dir].tuple.dst.u3,
> IPPROTO_GRE, _callid, );
> - exp_orig->expectfn = pptp_expectfn;
> + exp_orig->nat_helper = _nat;
>  
>   /* reply direction, PAC->PNS */
>   dir = IP_CT_DIR_REPLY;
> @@ -230,7 +238,7 @@ static int exp_gre(struct nf_conn *ct, __be16 callid, 
> __be16 peer_callid)
> >tuplehash[dir].tuple.src.u3,
> >tuplehash[dir].tuple.dst.u3,
> IPPROTO_GRE, , _callid);
> - exp_reply->expectfn = pptp_expectfn;
> + exp_reply->nat_helper = _nat;

Why do we need to attach nat_helper instead of expectfn? I would
prefer we do not.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH nf v5 2/3] netfilter: nat_helper: Make sure every proto nat module uses its nat_helper

[gfree . wind]

From: Gao Feng 

Make sure every proto nat module owns one struct nat_helper at least,
and it only uses its nat_helper.

1. Every proto nat module registers one nat_helper at least;
2. Replace the expectfn with nat_helper in the nf_conntrack_expect;
It is helpful to maintain the nat_helper codes
3. Make sure the nat module only uses its nat_helper;
4. Remove nf_ct_nat_helper_find_by_symbol, it is useless now.

Signed-off-by: Gao Feng 
---
 v5: Register one nat_helper for every nat module, per Pablo
 v4: Cover the nat_module assignment in dataplane, per Pablo
 v3: Rename the nf_ct_helper_expectfn, func, and member, per Pablo
 v2: Use the module as the identifier when flush expect
 v1: initial version

 include/net/netfilter/nf_conntrack_expect.h |  5 ++--
 include/net/netfilter/nf_conntrack_helper.h |  2 --
 net/ipv4/netfilter/nf_nat_h323.c| 44 ++---
 net/netfilter/ipvs/ip_vs_nfct.c |  7 -
 net/netfilter/nf_conntrack_broadcast.c  |  2 +-
 net/netfilter/nf_conntrack_core.c   |  4 +--
 net/netfilter/nf_conntrack_expect.c |  2 +-
 net/netfilter/nf_conntrack_netlink.c| 14 -
 net/netfilter/nf_conntrack_pptp.c   | 14 +++--
 net/netfilter/nf_nat_amanda.c   |  9 +-
 net/netfilter/nf_nat_ftp.c  |  9 +-
 net/netfilter/nf_nat_irc.c  |  9 +-
 net/netfilter/nf_nat_sip.c  | 18 ++--
 net/netfilter/nf_nat_tftp.c |  9 +-
 14 files changed, 101 insertions(+), 47 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_expect.h 
b/include/net/netfilter/nf_conntrack_expect.h
index 5ed33ea..f665a6b 100644
--- a/include/net/netfilter/nf_conntrack_expect.h
+++ b/include/net/netfilter/nf_conntrack_expect.h
@@ -23,9 +23,8 @@ struct nf_conntrack_expect {
struct nf_conntrack_tuple tuple;
struct nf_conntrack_tuple_mask mask;
 
-   /* Function to call after setup and insertion */
-   void (*expectfn)(struct nf_conn *new,
-struct nf_conntrack_expect *this);
+   /* Expectation function owner */
+   struct nf_ct_nat_helper *nat_helper;
 
/* Helper to assign to new connection */
struct nf_conntrack_helper *helper;
diff --git a/include/net/netfilter/nf_conntrack_helper.h 
b/include/net/netfilter/nf_conntrack_helper.h
index d14fe493..e8d31ca 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -125,8 +125,6 @@ void nf_ct_helper_log(struct sk_buff *skb, const struct 
nf_conn *ct,
 void nf_ct_nat_helper_unregister(struct nf_ct_nat_helper *n);
 struct nf_ct_nat_helper *
 nf_ct_nat_helper_find_by_name(const char *name);
-struct nf_ct_nat_helper *
-nf_ct_nat_helper_find_by_symbol(const void *symbol);
 
 extern struct hlist_head *nf_ct_helper_hash;
 extern unsigned int nf_ct_helper_hsize;
diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c
index 346e764..9101c48 100644
--- a/net/ipv4/netfilter/nf_nat_h323.c
+++ b/net/ipv4/netfilter/nf_nat_h323.c
@@ -21,6 +21,26 @@
 #include 
 
 //
+static void ip_nat_q931_expect(struct nf_conn *new,
+  struct nf_conntrack_expect *this);
+static void ip_nat_callforwarding_expect(struct nf_conn *new,
+struct nf_conntrack_expect *this);
+
+static struct nf_ct_nat_helper q931_nat = {
+   .name   = "Q.931",
+   .expectfn   = ip_nat_q931_expect,
+};
+
+static struct nf_ct_nat_helper callforwarding_nat = {
+   .name   = "callforwarding",
+   .expectfn   = ip_nat_callforwarding_expect,
+};
+
+static struct nf_ct_nat_helper follow_master_nat = {
+   .name   = "h323_follow_master",
+   .expectfn   = nf_nat_follow_master,
+};
+
 static int set_addr(struct sk_buff *skb, unsigned int protoff,
unsigned char **data, int dataoff,
unsigned int addroff, __be32 ip, __be16 port)
@@ -187,10 +207,10 @@ static int nat_rtp_rtcp(struct sk_buff *skb, struct 
nf_conn *ct,
 
/* Set expectations for NAT */
rtp_exp->saved_proto.udp.port = rtp_exp->tuple.dst.u.udp.port;
-   rtp_exp->expectfn = nf_nat_follow_master;
+   rtp_exp->nat_helper = _master_nat;
rtp_exp->dir = !dir;
rtcp_exp->saved_proto.udp.port = rtcp_exp->tuple.dst.u.udp.port;
-   rtcp_exp->expectfn = nf_nat_follow_master;
+   rtcp_exp->nat_helper = _master_nat;
rtcp_exp->dir = !dir;
 
/* Lookup existing expects */
@@ -289,7 +309,7 @@ static int nat_t120(struct sk_buff *skb, struct nf_conn *ct,
 
/* Set expectations for NAT */
exp->saved_proto.tcp.port = exp->tuple.dst.u.tcp.port;
-   exp->expectfn = nf_nat_follow_master;
+   exp->nat_helper = _master_nat;