[PATCH][resend] netfilter: ebtables: make broute table work again

[PATCH][resend] netfilter: ebtables: make broute table work again

[Florian Westphal]

broute table init hook sets up the "br_should_route_hook" pointer,
which then gets called from br_input.

commit a386f99025f13b32502fe5dedf223c20d7283826
(bridge: add proper RCU annotation to should_route_hook)
introduced a typedef, and then changed this to:

br_should_route_hook_t *rhook;
[..]
rhook = rcu_dereference(br_should_route_hook);
if (*rhook(skb))

problem is that "br_should_route_hook" contains the address of the function,
so calling *rhook() results in kernel panic.

Can't simply use "if (rhook(skb)", because br_should_route_hook_t *rhook
is not a function.

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Eric, does this patch look sane to you?

 Pablo, only the if_bridge.h change is needed.
 If you do not like the br_input.c change, feel
 free to remove it when applying.

 Thanks!

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index f7e73c3..dd3f201 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -103,7 +103,7 @@ struct __fdb_entry {
 
 extern void brioctl_set(int (*ioctl_hook)(struct net *, unsigned int, void __user *));
 
-typedef int (*br_should_route_hook_t)(struct sk_buff *skb);
+typedef int br_should_route_hook_t(struct sk_buff *skb);
 extern br_should_route_hook_t __rcu *br_should_route_hook;
 
 #endif
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 6f6d8e1..1d677bf 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -178,7 +178,7 @@ forward:
 	case BR_STATE_FORWARDING:
 		rhook = rcu_dereference(br_should_route_hook);
 		if (rhook) {
-			if ((*rhook)(skb))
+			if (rhook(skb))
 				return skb;
 			dest = eth_hdr(skb)->h_dest;
 		}
-- 
1.7.2.2

Re: [PATCH][resend] netfilter: ebtables: make broute table work again

[Eric Dumazet]

Le dimanche 09 janvier 2011 à 22:48 +0100, Florian Westphal a écrit :
> broute table init hook sets up the "br_should_route_hook" pointer,
> which then gets called from br_input.
> 
> commit a386f99025f13b32502fe5dedf223c20d7283826
> (bridge: add proper RCU annotation to should_route_hook)
> introduced a typedef, and then changed this to:
> 
> br_should_route_hook_t *rhook;
> [..]
> rhook = rcu_dereference(br_should_route_hook);
> if (*rhook(skb))
> 
> problem is that "br_should_route_hook" contains the address of the function,
> so calling *rhook() results in kernel panic.
> 
> Can't simply use "if (rhook(skb)", because br_should_route_hook_t *rhook
> is not a function.
> 
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  Eric, does this patch look sane to you?
> 
>  Pablo, only the if_bridge.h change is needed.
>  If you do not like the br_input.c change, feel
>  free to remove it when applying.
> 
>  Thanks!
> 
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index f7e73c3..dd3f201 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -103,7 +103,7 @@ struct __fdb_entry {
>  
>  extern void brioctl_set(int (*ioctl_hook)(struct net *, unsigned int, void __user *));
>  
> -typedef int (*br_should_route_hook_t)(struct sk_buff *skb);
> +typedef int br_should_route_hook_t(struct sk_buff *skb);
>  extern br_should_route_hook_t __rcu *br_should_route_hook;

Hi Florian

It seems I was on crack at that time, but I remember having problem with
sparse on :

net/bridge/netfilter/ebtable_broute.c:90:2: error: cannot dereference this type

Lets ignore sparse for the time being...

Please dont change the invocation of (*rhook) to make clear its an
indirect call. Both syntaxes are valid in C.

You also can remove one un needed cast now.

Soemthing like :

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index f7e73c3..dd3f201 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -103,7 +103,7 @@ struct __fdb_entry {
 
 extern void brioctl_set(int (*ioctl_hook)(struct net *, unsigned int, void __user *));
 
-typedef int (*br_should_route_hook_t)(struct sk_buff *skb);
+typedef int br_should_route_hook_t(struct sk_buff *skb);
 extern br_should_route_hook_t __rcu *br_should_route_hook;
 
 #endif
diff --git a/net/bridge/netfilter/ebtable_broute.c b/net/bridge/netfilter/ebtable_broute.c
index 1bcaf36..ae3f106 100644
--- a/net/bridge/netfilter/ebtable_broute.c
+++ b/net/bridge/netfilter/ebtable_broute.c
@@ -87,8 +87,7 @@ static int __init ebtable_broute_init(void)
 	if (ret < 0)
 		return ret;
 	/* see br_input.c */
-	rcu_assign_pointer(br_should_route_hook,
-			   (br_should_route_hook_t *)ebt_broute);
+	rcu_assign_pointer(br_should_route_hook, ebt_broute);
 	return 0;
 }
 

Thanks !


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

Re: [PATCH][resend] netfilter: ebtables: make broute table work again

[Florian Westphal]

Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> > index f7e73c3..dd3f201 100644
> > --- a/include/linux/if_bridge.h
> > +++ b/include/linux/if_bridge.h
> > @@ -103,7 +103,7 @@ struct __fdb_entry {
> >  
> >  extern void brioctl_set(int (*ioctl_hook)(struct net *, unsigned int, void __user *));
> >  
> > -typedef int (*br_should_route_hook_t)(struct sk_buff *skb);
> > +typedef int br_should_route_hook_t(struct sk_buff *skb);
> >  extern br_should_route_hook_t __rcu *br_should_route_hook;
> 
> Hi Florian
> 
> It seems I was on crack at that time, but I remember having problem with
> sparse on :
> 
> net/bridge/netfilter/ebtable_broute.c:90:2: error: cannot dereference this type
> 
> Lets ignore sparse for the time being...
> 
> Please dont change the invocation of (*rhook) to make clear its an
> indirect call. Both syntaxes are valid in C.

okay.

> You also can remove one un needed cast now.

You're right about the cast, but removing it causes the sparse splat
you quoted above...

I'll resend a v2 containing the change below only.

Thanks for looking at this,
Florian

> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
>  extern void brioctl_set(int (*ioctl_hook)(struct net *, unsigned int, void __user *));
>  
> -typedef int (*br_should_route_hook_t)(struct sk_buff *skb);
> +typedef int br_should_route_hook_t(struct sk_buff *skb);
>  extern br_should_route_hook_t __rcu *br_should_route_hook;