| Message ID | 20210807062106.2563-1-l4stpr0gr4m@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netfilter: remove duplicate code | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 52 this patch: 52 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 80 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 50 this patch: 50 |
| netdev/header_inline | success | Link |
I checked the Changes Requested state in patchwork. But I have not received any review mails. I wonder if there is any problem. I'm sorry if you just review a little late due to a busy schedule. Kangmin Park 2021년 8월 7일 (토) 오후 3:21, Kangmin Park <l4stpr0gr4m@gmail.com>님이 작성: > > nf_nat_ipv4_fn() and nf_nat_ipv6_fn() call nf_nat_inet_fn(). > Those two functions are already contains routine that gets nf_conn > object and checks the untrackable situation. > So, the following code is duplicated. > > ``` > ct = nf_ct_get(skb, &ctinfo); > if (!ct) > return NF_ACCEPT; > ``` > > Therefore, define a function __nf_nat_inet_fn() that has the same > contents as the nf_nat_inet_fn() except for routine gets and checks > the nf_conn object. > Then, separate the nf_nat_inet_fn() into a routine that gets a > nf_conn object and a routine that calls the __nf_nat_inet_fn(). > > Signed-off-by: Kangmin Park <l4stpr0gr4m@gmail.com> > --- > include/net/netfilter/nf_nat.h | 5 +++++ > net/netfilter/nf_nat_core.c | 37 ++++++++++++++++++++++------------ > net/netfilter/nf_nat_proto.c | 4 ++-- > 3 files changed, 31 insertions(+), 15 deletions(-) > > diff --git a/include/net/netfilter/nf_nat.h b/include/net/netfilter/nf_nat.h > index 987111ae5240..a66f617c5054 100644 > --- a/include/net/netfilter/nf_nat.h > +++ b/include/net/netfilter/nf_nat.h > @@ -100,6 +100,11 @@ void nf_nat_ipv6_unregister_fn(struct net *net, const struct nf_hook_ops *ops); > int nf_nat_inet_register_fn(struct net *net, const struct nf_hook_ops *ops); > void nf_nat_inet_unregister_fn(struct net *net, const struct nf_hook_ops *ops); > > +unsigned int > +__nf_nat_inet_fn(void *priv, struct sk_buff *skb, > + const struct nf_hook_state *state, struct nf_conn *ct, > + enum ip_conntrack_info ctinfo); > + > unsigned int > nf_nat_inet_fn(void *priv, struct sk_buff *skb, > const struct nf_hook_state *state); > diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c > index 7de595ead06a..98ebba2c0f6d 100644 > --- a/net/netfilter/nf_nat_core.c > +++ b/net/netfilter/nf_nat_core.c > @@ -682,25 +682,15 @@ unsigned int nf_nat_packet(struct nf_conn *ct, > } > EXPORT_SYMBOL_GPL(nf_nat_packet); > > unsigned int > -nf_nat_inet_fn(void *priv, struct sk_buff *skb, > - const struct nf_hook_state *state) > +__nf_nat_inet_fn(void *priv, struct sk_buff *skb, > + const struct nf_hook_state *state, struct nf_conn *ct, > + enum ip_conntrack_info ctinfo) > { > - struct nf_conn *ct; > - enum ip_conntrack_info ctinfo; > struct nf_conn_nat *nat; > /* maniptype == SRC for postrouting. */ > enum nf_nat_manip_type maniptype = HOOK2MANIP(state->hook); > > - ct = nf_ct_get(skb, &ctinfo); > - /* Can't track? It's not due to stress, or conntrack would > - * have dropped it. Hence it's the user's responsibilty to > - * packet filter it out, or implement conntrack/NAT for that > - * protocol. 8) --RR > - */ > - if (!ct) > - return NF_ACCEPT; > - > nat = nfct_nat(ct); > > switch (ctinfo) { > @@ -755,6 +745,26 @@ nf_nat_inet_fn(void *priv, struct sk_buff *skb, > nf_ct_kill_acct(ct, ctinfo, skb); > return NF_DROP; > } > +EXPORT_SYMBOL_GPL(__nf_nat_inet_fn); > + > +unsigned int > +nf_nat_inet_fn(void *priv, struct sk_buff *skb, > + const struct nf_hook_state *state) > +{ > + struct nf_conn *ct; > + enum ip_conntrack_info ctinfo; > + > + ct = nf_ct_get(skb, &ctinfo); > + /* Can't track? It's not due to stress, or conntrack would > + * have dropped it. Hence it's the user's responsibilty to > + * packet filter it out, or implement conntrack/NAT for that > + * protocol. 8) --RR > + */ > + if (!ct) > + return NF_ACCEPT; > + > + return __nf_nat_inet_fn(priv, skb, state, ct, ctinfo); > +} > EXPORT_SYMBOL_GPL(nf_nat_inet_fn); > > struct nf_nat_proto_clean { > diff --git a/net/netfilter/nf_nat_proto.c b/net/netfilter/nf_nat_proto.c > index 48cc60084d28..897859730078 100644 > --- a/net/netfilter/nf_nat_proto.c > +++ b/net/netfilter/nf_nat_proto.c > @@ -642,7 +642,7 @@ nf_nat_ipv4_fn(void *priv, struct sk_buff *skb, > } > } > > - return nf_nat_inet_fn(priv, skb, state); > + return __nf_nat_inet_fn(priv, skb, state, ct, ctinfo); > } > > static unsigned int > @@ -934,7 +934,7 @@ nf_nat_ipv6_fn(void *priv, struct sk_buff *skb, > } > } > > - return nf_nat_inet_fn(priv, skb, state); > + return __nf_nat_inet_fn(priv, skb, state, ct, ctinfo); > } > > static unsigned int > -- > 2.26.2 >
Kangmin Park <l4stpr0gr4m@gmail.com> wrote: > I checked the Changes Requested state in patchwork. > But I have not received any review mails. I did not see any either. > I wonder if there is any problem. I don't think its worth doing, the 'extra' check avoids the need for two additional function arguments (and those are not free either).
diff --git a/include/net/netfilter/nf_nat.h b/include/net/netfilter/nf_nat.h index 987111ae5240..a66f617c5054 100644 --- a/include/net/netfilter/nf_nat.h +++ b/include/net/netfilter/nf_nat.h @@ -100,6 +100,11 @@ void nf_nat_ipv6_unregister_fn(struct net *net, const struct nf_hook_ops *ops); int nf_nat_inet_register_fn(struct net *net, const struct nf_hook_ops *ops); void nf_nat_inet_unregister_fn(struct net *net, const struct nf_hook_ops *ops); +unsigned int +__nf_nat_inet_fn(void *priv, struct sk_buff *skb, + const struct nf_hook_state *state, struct nf_conn *ct, + enum ip_conntrack_info ctinfo); + unsigned int nf_nat_inet_fn(void *priv, struct sk_buff *skb, const struct nf_hook_state *state); diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c index 7de595ead06a..98ebba2c0f6d 100644 --- a/net/netfilter/nf_nat_core.c +++ b/net/netfilter/nf_nat_core.c @@ -682,25 +682,15 @@ unsigned int nf_nat_packet(struct nf_conn *ct, } EXPORT_SYMBOL_GPL(nf_nat_packet); unsigned int -nf_nat_inet_fn(void *priv, struct sk_buff *skb, - const struct nf_hook_state *state) +__nf_nat_inet_fn(void *priv, struct sk_buff *skb, + const struct nf_hook_state *state, struct nf_conn *ct, + enum ip_conntrack_info ctinfo) { - struct nf_conn *ct; - enum ip_conntrack_info ctinfo; struct nf_conn_nat *nat; /* maniptype == SRC for postrouting. */ enum nf_nat_manip_type maniptype = HOOK2MANIP(state->hook); - ct = nf_ct_get(skb, &ctinfo); - /* Can't track? It's not due to stress, or conntrack would - * have dropped it. Hence it's the user's responsibilty to - * packet filter it out, or implement conntrack/NAT for that - * protocol. 8) --RR - */ - if (!ct) - return NF_ACCEPT; - nat = nfct_nat(ct); switch (ctinfo) { @@ -755,6 +745,26 @@ nf_nat_inet_fn(void *priv, struct sk_buff *skb, nf_ct_kill_acct(ct, ctinfo, skb); return NF_DROP; } +EXPORT_SYMBOL_GPL(__nf_nat_inet_fn); + +unsigned int +nf_nat_inet_fn(void *priv, struct sk_buff *skb, + const struct nf_hook_state *state) +{ + struct nf_conn *ct; + enum ip_conntrack_info ctinfo; + + ct = nf_ct_get(skb, &ctinfo); + /* Can't track? It's not due to stress, or conntrack would + * have dropped it. Hence it's the user's responsibilty to + * packet filter it out, or implement conntrack/NAT for that + * protocol. 8) --RR + */ + if (!ct) + return NF_ACCEPT; + + return __nf_nat_inet_fn(priv, skb, state, ct, ctinfo); +} EXPORT_SYMBOL_GPL(nf_nat_inet_fn); struct nf_nat_proto_clean { diff --git a/net/netfilter/nf_nat_proto.c b/net/netfilter/nf_nat_proto.c index 48cc60084d28..897859730078 100644 --- a/net/netfilter/nf_nat_proto.c +++ b/net/netfilter/nf_nat_proto.c @@ -642,7 +642,7 @@ nf_nat_ipv4_fn(void *priv, struct sk_buff *skb, } } - return nf_nat_inet_fn(priv, skb, state); + return __nf_nat_inet_fn(priv, skb, state, ct, ctinfo); } static unsigned int @@ -934,7 +934,7 @@ nf_nat_ipv6_fn(void *priv, struct sk_buff *skb, } } - return nf_nat_inet_fn(priv, skb, state); + return __nf_nat_inet_fn(priv, skb, state, ct, ctinfo); } static unsigned int
nf_nat_ipv4_fn() and nf_nat_ipv6_fn() call nf_nat_inet_fn(). Those two functions are already contains routine that gets nf_conn object and checks the untrackable situation. So, the following code is duplicated. ``` ct = nf_ct_get(skb, &ctinfo); if (!ct) return NF_ACCEPT; ``` Therefore, define a function __nf_nat_inet_fn() that has the same contents as the nf_nat_inet_fn() except for routine gets and checks the nf_conn object. Then, separate the nf_nat_inet_fn() into a routine that gets a nf_conn object and a routine that calls the __nf_nat_inet_fn(). Signed-off-by: Kangmin Park <l4stpr0gr4m@gmail.com> --- include/net/netfilter/nf_nat.h | 5 +++++ net/netfilter/nf_nat_core.c | 37 ++++++++++++++++++++++------------ net/netfilter/nf_nat_proto.c | 4 ++-- 3 files changed, 31 insertions(+), 15 deletions(-)