[PATCH] TPROXY cleanups

[PATCH] TPROXY cleanups

[Alexey Dobriyan]

* drop version.h -- unneeded and file will be needlessly rebuilt.
* make tproxy_core unloadable
* use "in" device outright, -- target and match are only in PRE_ROUTING,
  so should avoid dereference.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 net/netfilter/nf_tproxy_core.c |    5 ++++-
 net/netfilter/xt_TPROXY.c      |    2 +-
 net/netfilter/xt_socket.c      |    2 +-
 3 files changed, 6 insertions(+), 3 deletions(-)

--- a/net/netfilter/nf_tproxy_core.c
+++ b/net/netfilter/nf_tproxy_core.c
@@ -10,7 +10,6 @@
  *
  */
 
-#include <linux/version.h>
 #include <linux/module.h>
 
 #include <linux/net.h>
@@ -89,7 +88,11 @@ static int __init nf_tproxy_init(void)
 	return 0;
 }
 
+static void __exit nf_tproxy_exit(void)
+{
+}
 module_init(nf_tproxy_init);
+module_exit(nf_tproxy_exit);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Krisztian Kovacs");
--- a/net/netfilter/xt_TPROXY.c
+++ b/net/netfilter/xt_TPROXY.c
@@ -41,7 +41,7 @@ tproxy_tg(struct sk_buff *skb,
 	if (hp == NULL)
 		return NF_DROP;
 
-	sk = nf_tproxy_get_sock_v4(dev_net(skb->dev), iph->protocol,
+	sk = nf_tproxy_get_sock_v4(dev_net(in), iph->protocol,
 				   iph->saddr, tgi->laddr ? tgi->laddr : iph->daddr,
 				   hp->source, tgi->lport ? tgi->lport : hp->dest,
 				   in, true);
--- a/net/netfilter/xt_socket.c
+++ b/net/netfilter/xt_socket.c
@@ -145,7 +145,7 @@ socket_mt(const struct sk_buff *skb,
 	}
 #endif
 
-	sk = nf_tproxy_get_sock_v4(dev_net(skb->dev), protocol,
+	sk = nf_tproxy_get_sock_v4(dev_net(in), protocol,
 				   saddr, daddr, sport, dport, in, false);
 	if (sk != NULL) {
 		bool wildcard = (inet_sk(sk)->rcv_saddr == 0);

Re: [PATCH] TPROXY cleanups

[KOVACS Krisztian]

Hi,

On h, okt 06, 2008 at 04:19:44 +0400, Alexey Dobriyan wrote:
> * drop version.h -- unneeded and file will be needlessly rebuilt.
> * make tproxy_core unloadable
> * use "in" device outright, -- target and match are only in PRE_ROUTING,
>   so should avoid dereference.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
> 
>  net/netfilter/nf_tproxy_core.c |    5 ++++-
>  net/netfilter/xt_TPROXY.c      |    2 +-
>  net/netfilter/xt_socket.c      |    2 +-
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> --- a/net/netfilter/nf_tproxy_core.c
> +++ b/net/netfilter/nf_tproxy_core.c
> @@ -10,7 +10,6 @@
>   *
>   */
>  
> -#include <linux/version.h>
>  #include <linux/module.h>
>  
>  #include <linux/net.h>
> @@ -89,7 +88,11 @@ static int __init nf_tproxy_init(void)
>  	return 0;
>  }
>  
> +static void __exit nf_tproxy_exit(void)
> +{
> +}
>  module_init(nf_tproxy_init);
> +module_exit(nf_tproxy_exit);

Please don't do this. The module was made unloadable because it attaches a
function pointer to skb's and removing the module while there's an skb in
flight would cause a kernel crash.

Thanks for the other cleanups, though.

-- 
KOVACS Krisztian

Re: [PATCH] TPROXY cleanups

[Alexey Dobriyan]

On Mon, Oct 06, 2008 at 02:26:03PM +0200, KOVACS Krisztian wrote:
> On h, okt 06, 2008 at 04:19:44 +0400, Alexey Dobriyan wrote:
> > * drop version.h -- unneeded and file will be needlessly rebuilt.
> > * make tproxy_core unloadable
> > * use "in" device outright, -- target and match are only in PRE_ROUTING,
> >   so should avoid dereference.

> > @@ -89,7 +88,11 @@ static int __init nf_tproxy_init(void)
> >  	return 0;
> >  }
> >  
> > +static void __exit nf_tproxy_exit(void)
> > +{
> > +}
> >  module_init(nf_tproxy_init);
> > +module_exit(nf_tproxy_exit);
> 
> Please don't do this. The module was made unloadable because it attaches a
> function pointer to skb's and removing the module while there's an skb in
> flight would cause a kernel crash.

Have you actually seen it?

xt_TPROXY will pin it because it uses a symbol from it, so it won't
dissapear.

Anyway, banning rmmod is absolutely wrong thing regardless.

Re: [PATCH] TPROXY cleanups

[KOVACS Krisztian]

Hi,

On h, okt 06, 2008 at 04:39:21 +0400, Alexey Dobriyan wrote:
> On Mon, Oct 06, 2008 at 02:26:03PM +0200, KOVACS Krisztian wrote:
> > On h, okt 06, 2008 at 04:19:44 +0400, Alexey Dobriyan wrote:
> > > * drop version.h -- unneeded and file will be needlessly rebuilt.
> > > * make tproxy_core unloadable
> > > * use "in" device outright, -- target and match are only in PRE_ROUTING,
> > >   so should avoid dereference.
> 
> > > @@ -89,7 +88,11 @@ static int __init nf_tproxy_init(void)
> > >  	return 0;
> > >  }
> > >  
> > > +static void __exit nf_tproxy_exit(void)
> > > +{
> > > +}
> > >  module_init(nf_tproxy_init);
> > > +module_exit(nf_tproxy_exit);
> > 
> > Please don't do this. The module was made unloadable because it attaches a
> > function pointer to skb's and removing the module while there's an skb in
> > flight would cause a kernel crash.
> 
> Have you actually seen it?

No.

> xt_TPROXY will pin it because it uses a symbol from it, so it won't
> dissapear.

Yeah, that's true, and I think that it's impossible to remove the rule
attaching the socket references while the skb's in flight. Ok, so let's
add module_exit() then.

-- 
KOVACS Krisztian

Re: [PATCH] TPROXY cleanups

[Patrick McHardy]

KOVACS Krisztian wrote:
> On h, okt 06, 2008 at 04:39:21 +0400, Alexey Dobriyan wrote:
>> xt_TPROXY will pin it because it uses a symbol from it, so it won't
>> dissapear.
> 
> Yeah, that's true, and I think that it's impossible to remove the rule
> attaching the socket references while the skb's in flight. Ok, so let's
> add module_exit() then.

So Alexey's patch is fine for applying?

Re: [PATCH] TPROXY cleanups

[KOVACS Krisztian]

Hi,

On h, okt 06, 2008 at 05:41:59 +0200, Patrick McHardy wrote:
> KOVACS Krisztian wrote:
> >On h, okt 06, 2008 at 04:39:21 +0400, Alexey Dobriyan wrote:
> >>xt_TPROXY will pin it because it uses a symbol from it, so it won't
> >>dissapear.
> >
> >Yeah, that's true, and I think that it's impossible to remove the rule
> >attaching the socket references while the skb's in flight. Ok, so let's
> >add module_exit() then.
> 
> So Alexey's patch is fine for applying?

My only fear was that you can remove the core module while there's a
function pointer attached to the skb.

The TPROXY target is the only one actually attaching the pointer and you
obviously can't remove the core module while you have a rule referring to
TPROXY. The question is wheter or not it's possible that an skb still has
the TPROXY-assigned socket (and destructor function pointer) after the
referring iptables rule has been removed.

I'm still not 100% sure that this is not possible... Making the module
unloadable is not the proper solution, though.

-- 
KOVACS Krisztian