Stephen Hemminger
2007-Apr-18 12:34 UTC
[Bridge] Re: [PATCH] fix oops when mangling and brouting and tcpdumping packets
How about this, it is basically Bart's patch with:
* some more cleanups to handle_bridge
* removing extra rcu_read_lock (already done in netif_receive_skb)
* pass bridge port to hook since already dereferenced in handle_bridge
diff -Nru a/include/linux/if_bridge.h b/include/linux/if_bridge.h
--- a/include/linux/if_bridge.h 2004-08-24 11:35:08 -07:00
+++ b/include/linux/if_bridge.h 2004-08-24 11:35:08 -07:00
@@ -105,7 +105,7 @@
#include <linux/netdevice.h>
extern void brioctl_set(int (*ioctl_hook)(unsigned int, void __user *));
-extern int (*br_handle_frame_hook)(struct sk_buff *skb);
+extern int (*br_handle_frame_hook)(struct net_bridge_port *p, struct sk_buff
**pskb);
extern int (*br_should_route_hook)(struct sk_buff **pskb);
#endif
diff -Nru a/net/bridge/br_input.c b/net/bridge/br_input.c
--- a/net/bridge/br_input.c 2004-08-24 11:35:08 -07:00
+++ b/net/bridge/br_input.c 2004-08-24 11:35:08 -07:00
@@ -45,26 +45,15 @@
br_pass_frame_up_finish);
}
+/* note: already called with rcu_read_lock (preempt_disabled) */
int br_handle_frame_finish(struct sk_buff *skb)
{
- struct net_bridge *br;
- unsigned char *dest;
+ const unsigned char *dest = skb->mac.ethernet->h_dest;
+ struct net_bridge_port *p = skb->dev->br_port;
+ struct net_bridge *br = p->br;
struct net_bridge_fdb_entry *dst;
- struct net_bridge_port *p;
- int passedup;
+ int passedup = 0;
- dest = skb->mac.ethernet->h_dest;
-
- rcu_read_lock();
- p = rcu_dereference(skb->dev->br_port);
-
- if (p == NULL || p->state == BR_STATE_DISABLED) {
- kfree_skb(skb);
- goto out;
- }
-
- br = p->br;
- passedup = 0;
if (br->dev->flags & IFF_PROMISC) {
struct sk_buff *skb2;
@@ -99,20 +88,16 @@
br_flood_forward(br, skb, 0);
out:
- rcu_read_unlock();
return 0;
}
-int br_handle_frame(struct sk_buff *skb)
+/* note: already called with rcu_read_lock (preempt_disabled) */
+int br_handle_frame(struct net_bridge_port *p, struct sk_buff **pskb)
{
- unsigned char *dest;
- struct net_bridge_port *p;
-
- dest = skb->mac.ethernet->h_dest;
+ struct sk_buff *skb = *pskb;
+ const unsigned char *dest = skb->mac.ethernet->h_dest;
- rcu_read_lock();
- p = skb->dev->br_port;
- if (p == NULL || p->state == BR_STATE_DISABLED)
+ if (p->state == BR_STATE_DISABLED)
goto err;
if (skb->mac.ethernet->h_source[0] & 1)
@@ -128,28 +113,26 @@
if (!dest[5]) {
NF_HOOK(PF_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev,
NULL, br_stp_handle_bpdu);
- rcu_read_unlock();
return 0;
}
}
else if (p->state == BR_STATE_FORWARDING) {
- if (br_should_route_hook && br_should_route_hook(&skb)) {
- rcu_read_unlock();
+ if (br_should_route_hook && br_should_route_hook(pskb))
return -1;
- }
+
+ skb = *pskb;
+ dest = skb->mac.ethernet->h_dest;
if (!memcmp(p->br->dev->dev_addr, dest, ETH_ALEN))
skb->pkt_type = PACKET_HOST;
NF_HOOK(PF_BRIDGE, NF_BR_PRE_ROUTING, skb, skb->dev, NULL,
br_handle_frame_finish);
- rcu_read_unlock();
return 0;
}
err:
- rcu_read_unlock();
kfree_skb(skb);
return 0;
}
diff -Nru a/net/bridge/br_private.h b/net/bridge/br_private.h
--- a/net/bridge/br_private.h 2004-08-24 11:35:08 -07:00
+++ b/net/bridge/br_private.h 2004-08-24 11:35:08 -07:00
@@ -177,7 +177,7 @@
/* br_input.c */
extern int br_handle_frame_finish(struct sk_buff *skb);
-extern int br_handle_frame(struct sk_buff *skb);
+extern int br_handle_frame(struct net_bridge_port *p, struct sk_buff **pskb);
/* br_ioctl.c */
extern int br_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd);
diff -Nru a/net/core/dev.c b/net/core/dev.c
--- a/net/core/dev.c 2004-08-24 11:35:08 -07:00
+++ b/net/core/dev.c 2004-08-24 11:35:08 -07:00
@@ -1684,35 +1684,26 @@
#if defined(CONFIG_BRIDGE) || defined (CONFIG_BRIDGE_MODULE)
-int (*br_handle_frame_hook)(struct sk_buff *skb);
+int (*br_handle_frame_hook)(struct net_bridge_port *p, struct sk_buff **pskb);
-static __inline__ int handle_bridge(struct sk_buff *skb,
- struct packet_type *pt_prev)
+static __inline__ int handle_bridge(struct sk_buff **pskb,
+ struct packet_type **pt_prev, int *ret)
{
- int ret = NET_RX_DROP;
- if (pt_prev)
- ret = deliver_skb(skb, pt_prev);
-
- return ret;
-}
-
-#endif
+ struct net_bridge_port *port = rcu_dereference((*pskb)->dev->br_port);
-static inline int __handle_bridge(struct sk_buff *skb,
- struct packet_type **pt_prev, int *ret)
-{
-#if defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE)
- if (skb->dev->br_port && skb->pkt_type != PACKET_LOOPBACK) {
- *ret = handle_bridge(skb, *pt_prev);
- if (br_handle_frame_hook(skb) == 0)
- return 1;
+ if (port == NULL || (*pskb)->pkt_type == PACKET_LOOPBACK)
+ return 0;
+ if (*pt_prev) {
+ *ret = deliver_skb(*pskb, *pt_prev);
*pt_prev = NULL;
- }
-#endif
- return 0;
+ }
+
+ return br_handle_frame_hook(port, pskb) == 0;
}
-
+#else
+#define handle_bridge(skb, pt_prev, ret) (0)
+#endif
#ifdef CONFIG_NET_CLS_ACT
/* TODO: Maybe we should just force sch_ingress to be compiled in
@@ -1818,7 +1809,7 @@
handle_diverter(skb);
- if (__handle_bridge(skb, &pt_prev, &ret))
+ if (handle_bridge(&skb, &pt_prev, &ret))
goto out;
type = skb->protocol;
Bart De Schuymer
2007-Apr-18 12:34 UTC
[Bridge] [PATCH] fix oops when mangling and brouting and tcpdumping packets
Hi Stephen,
The ebtables brouting chain, traversed through the call
br_should_route_hook(), can alter a packet. The redirect target
does this, f.e., to change the MAC destination.
This is what ebt_redirect does (among other things):
if (skb_shared(*pskb) || skb_cloned(*pskb)) {
struct sk_buff *nskb;
nskb = skb_copy(*pskb, GFP_ATOMIC);
if (!nskb)
return NF_DROP;
if ((*pskb)->sk)
skb_set_owner_w(nskb, (*pskb)->sk);
kfree_skb(*pskb);
*pskb = nskb;
}
So, if the packet is cloned or shared, we need to copy everything
to a private skb. If br_should_route_hook() returns a non-zero value,
the packet will be brouted and we must make sure the used skb remains
nskb, not the deleted skb. For this, we need the address of the skb used
in netif_receive_skb().
An oops (caused by netif_receive_skb() using a freed skb) is easilly
triggered by this:
ebtables -t broute -A BROUTING -j redirect --redirect-target DROP
along with tcpdump -i bridge_port (and traffic trying to go by)
cheers,
Bart
--- linux-2.6.8.1/net/core/dev.c.old 2004-08-14 22:50:04.000000000 +0200
+++ linux-2.6.8.1/net/core/dev.c 2004-08-14 22:51:39.000000000 +0200
@@ -1685,7 +1685,7 @@ static __inline__ int deliver_skb(struct
#if defined(CONFIG_BRIDGE) || defined (CONFIG_BRIDGE_MODULE)
-int (*br_handle_frame_hook)(struct sk_buff *skb);
+int (*br_handle_frame_hook)(struct sk_buff **pskb);
static __inline__ int handle_bridge(struct sk_buff *skb,
struct packet_type *pt_prev)
@@ -1699,13 +1699,13 @@ static __inline__ int handle_bridge(stru
#endif
-static inline int __handle_bridge(struct sk_buff *skb,
+static inline int __handle_bridge(struct sk_buff **pskb,
struct packet_type **pt_prev, int *ret)
{
#if defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE)
- if (skb->dev->br_port && skb->pkt_type != PACKET_LOOPBACK) {
- *ret = handle_bridge(skb, *pt_prev);
- if (br_handle_frame_hook(skb) == 0)
+ if ((*pskb)->dev->br_port && (*pskb)->pkt_type !=
PACKET_LOOPBACK) {
+ *ret = handle_bridge(*pskb, *pt_prev);
+ if (br_handle_frame_hook(pskb) == 0)
return 1;
*pt_prev = NULL;
@@ -1819,7 +1819,7 @@ ncls:
handle_diverter(skb);
- if (__handle_bridge(skb, &pt_prev, &ret))
+ if (__handle_bridge(&skb, &pt_prev, &ret))
goto out;
type = skb->protocol;
--- linux-2.6.8.1/include/linux/if_bridge.h.old 2004-08-14 22:50:09.000000000
+0200
+++ linux-2.6.8.1/include/linux/if_bridge.h 2004-08-14 22:51:39.000000000 +0200
@@ -105,7 +105,7 @@ struct __fdb_entry
#include <linux/netdevice.h>
extern void brioctl_set(int (*ioctl_hook)(unsigned int, void __user *));
-extern int (*br_handle_frame_hook)(struct sk_buff *skb);
+extern int (*br_handle_frame_hook)(struct sk_buff **pskb);
extern int (*br_should_route_hook)(struct sk_buff **pskb);
#endif
--- linux-2.6.8.1/net/bridge/br_private.h.old 2004-08-14 22:50:15.000000000
+0200
+++ linux-2.6.8.1/net/bridge/br_private.h 2004-08-14 22:51:39.000000000 +0200
@@ -177,7 +177,7 @@ extern int br_min_mtu(const struct net_b
/* br_input.c */
extern int br_handle_frame_finish(struct sk_buff *skb);
-extern int br_handle_frame(struct sk_buff *skb);
+extern int br_handle_frame(struct sk_buff **pskb);
/* br_ioctl.c */
extern int br_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd);
--- linux-2.6.8.1/net/bridge/br_input.c.old 2004-08-14 22:50:21.000000000 +0200
+++ linux-2.6.8.1/net/bridge/br_input.c 2004-08-14 22:51:39.000000000 +0200
@@ -104,30 +104,30 @@ out:
return 0;
}
-int br_handle_frame(struct sk_buff *skb)
+int br_handle_frame(struct sk_buff **pskb)
{
unsigned char *dest;
struct net_bridge_port *p;
- dest = skb->mac.ethernet->h_dest;
+ dest = (*pskb)->mac.ethernet->h_dest;
rcu_read_lock();
- p = skb->dev->br_port;
+ p = (*pskb)->dev->br_port;
if (p == NULL || p->state == BR_STATE_DISABLED)
goto err;
- if (skb->mac.ethernet->h_source[0] & 1)
+ if ((*pskb)->mac.ethernet->h_source[0] & 1)
goto err;
if (p->state == BR_STATE_LEARNING ||
p->state == BR_STATE_FORWARDING)
- br_fdb_insert(p->br, p, skb->mac.ethernet->h_source, 0);
+ br_fdb_insert(p->br, p, (*pskb)->mac.ethernet->h_source, 0);
if (p->br->stp_enabled &&
!memcmp(dest, bridge_ula, 5) &&
!(dest[5] & 0xF0)) {
if (!dest[5]) {
- NF_HOOK(PF_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev,
+ NF_HOOK(PF_BRIDGE, NF_BR_LOCAL_IN, *pskb, (*pskb)->dev,
NULL, br_stp_handle_bpdu);
rcu_read_unlock();
return 0;
@@ -135,15 +135,15 @@ int br_handle_frame(struct sk_buff *skb)
}
else if (p->state == BR_STATE_FORWARDING) {
- if (br_should_route_hook && br_should_route_hook(&skb)) {
+ if (br_should_route_hook && br_should_route_hook(pskb)) {
rcu_read_unlock();
return -1;
}
if (!memcmp(p->br->dev->dev_addr, dest, ETH_ALEN))
- skb->pkt_type = PACKET_HOST;
+ (*pskb)->pkt_type = PACKET_HOST;
- NF_HOOK(PF_BRIDGE, NF_BR_PRE_ROUTING, skb, skb->dev, NULL,
+ NF_HOOK(PF_BRIDGE, NF_BR_PRE_ROUTING, *pskb, (*pskb)->dev, NULL,
br_handle_frame_finish);
rcu_read_unlock();
return 0;
@@ -151,6 +151,6 @@ int br_handle_frame(struct sk_buff *skb)
err:
rcu_read_unlock();
- kfree_skb(skb);
+ kfree_skb(*pskb);
return 0;
}