bridge: send correct MTU value in PMTU

PATCH Applied [email protected] 3 participants 5 messages Jul 30, 2008

Subsystems (4)

Networking Network drivers netfilter Ethernet bridge

lore

Share & tools

lore

[email protected]

author

2008-07-30 19:37 UTC #0 lore raw

PATCH 2/11 bridge: send correct MTU value in PMTU

From: Simon Wunderlich <[email protected]>

When bridging interfaces with different MTUs, the bridge correctly chooses the minimum of the MTUs of the physical devices as the bridges MTU. But when a frame is passed which fits through the incoming, but not through the outgoing interface, a "Fragmentation Needed" packet is generated.

However, the propagated MTU is hardcoded to 1500, which is wrong in this situation. The sender will repeat the packet again with the same frame size, and the same problem will occur again.

Instead of sending 1500, the (correct) MTU value of the bridge is now sent via PMTU. To achieve this, the corresponding rtable structure is stored in its net_bridge structure.

[[email protected]: coding-style fixes]

Signed-off-by Simon Wunderlich <[email protected]>

Cc Stephen Hemminger <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 net/bridge/br_device.c    |    9 +++++-
 net/bridge/br_if.c        |   27 +++++++++++++++++++
 net/bridge/br_netfilter.c |   51 +++++++++++++-----------------------
 net/bridge/br_private.h   |    5 +++
 4 files changed, 59 insertions(+), 33 deletions(-)

net/bridge/br_device.c renamed from net/bridge/br_device.c~bridge-send-correct-mtu-value-in-pmtu +8 -1

diff -puN net/bridge/br_device.c~bridge-send-correct-mtu-value-in-pmtu net/bridge/br_device.c
@@ -68,10 +68,17 @@ static int br_dev_stop(struct net_device
 
 static int br_change_mtu(struct net_device *dev, int new_mtu)
 {
-	if (new_mtu < 68 || new_mtu > br_min_mtu(netdev_priv(dev)))
+	struct net_bridge *br = netdev_priv(dev);
+	if (new_mtu < 68 || new_mtu > br_min_mtu(br))
 		return -EINVAL;
 
 	dev->mtu = new_mtu;
+
+#ifdef CONFIG_BRIDGE_NETFILTER
+	/* remember the MTU in the rtable for PMTU */
+	br->fake_rtable.u.dst.metrics[RTAX_MTU - 1] = new_mtu;
+#endif
+
 	return 0;
 }

net/bridge/br_if.c renamed from net/bridge/br_if.c~bridge-send-correct-mtu-value-in-pmtu +27 -0

diff -puN net/bridge/br_if.c~bridge-send-correct-mtu-value-in-pmtu net/bridge/br_if.c
@@ -168,6 +168,23 @@ static void del_br(struct net_bridge *br
 	unregister_netdevice(br->dev);
 }
 
+#ifdef CONFIG_BRIDGE_NETFILTER
+/* We need these fake structures to make netfilter happy --
+ * lots of places assume that skb->dst != NULL, which isn't
+ * all that unreasonable.
+ *
+ * Currently, we fill in the PMTU entry because netfilter
+ * refragmentation needs it, and the rt_flags entry because
+ * ipt_REJECT needs it.  Future netfilter modules might
+ * require us to fill additional fields. */
+struct net_device __fake_net_device = {
+	.hard_header_len	= ETH_HLEN,
+#ifdef CONFIG_NET_NS
+	.nd_net			= &init_net,
+#endif
+};
+#endif
+
 static struct net_device *new_bridge_dev(const char *name)
 {
 	struct net_bridge *br;

@@ -202,6 +219,16 @@ static struct net_device *new_bridge_dev
 	br->topology_change = 0;
 	br->topology_change_detected = 0;
 	br->ageing_time = 300 * HZ;
+
+#ifdef CONFIG_BRIDGE_NETFILTER
+	atomic_set(&br->fake_rtable.u.dst.__refcnt, 1);
+	br->fake_rtable.u.dst.dev = &__fake_net_device;
+	br->fake_rtable.u.dst.path = &br->fake_rtable.u.dst;
+	br->fake_rtable.u.dst.metrics[RTAX_MTU - 1] = 1500;
+	br->fake_rtable.u.dst.flags	= DST_NOXFRM;
+	br->fake_rtable.rt_flags = 0;
+#endif
+
 	INIT_LIST_HEAD(&br->age_list);
 
 	br_stp_timer_init(br);

net/bridge/br_netfilter.c renamed from net/bridge/br_netfilter.c~bridge-send-correct-mtu-value-in-pmtu +19 ~~-32~~

76 lines not on this page · raw message

net/bridge/br_private.h renamed from net/bridge/br_private.h~bridge-send-correct-mtu-value-in-pmtu +5 -0

diff -puN net/bridge/br_private.h~bridge-send-correct-mtu-value-in-pmtu net/bridge/br_private.h
@@ -15,6 +15,7 @@
 
 #include <linux/netdevice.h>
 #include <linux/if_bridge.h>
+#include <net/route.h>	/* struct rtable, RTAX_MTU */
 
 #define BR_HASH_BITS 8
 #define BR_HASH_SIZE (1 << BR_HASH_BITS)

@@ -92,6 +93,9 @@ struct net_bridge
 	struct hlist_head		hash[BR_HASH_SIZE];
 	struct list_head		age_list;
 	unsigned long			feature_mask;
+#ifdef CONFIG_BRIDGE_NETFILTER
+	struct rtable 			fake_rtable;
+#endif
 	unsigned long			flags;
 #define BR_SET_MAC_ADDR		0x00000001

@@ -200,6 +204,7 @@ extern void br_netfilter_fini(void);
 #else
 #define br_netfilter_init()	(0)
 #define br_netfilter_fini()	do { } while(0)
+extern struct net_device __fake_net_device;
 #endif
 
 /* br_stp.c */

Stephen Hemminger

2008-07-30 20:33 UTC #1 lore raw

PATCH 1/1 bridge: send correct MTU value in PMTU (revised)

However, the propagated MTU is hardcoded to 1500, which is wrong in this situation. The sender will repeat the packet again with the same frame size, and the same problem will occur again.

Instead of sending 1500, the (correct) MTU value of the bridge is now sent via PMTU. To achieve this, the corresponding rtable structure is stored in its net_bridge structure.

Modified to get rid of fake_net_device as well.

Signed-off-by Simon Wunderlich <[email protected]>

Signed-off-by Stephen Hemminger <[email protected]>

---
 net/bridge/br_device.c    |    9 +++++-
 net/bridge/br_if.c        |    3 ++
 net/bridge/br_netfilter.c |   64 +++++++++++++++++++++++++---------------------
 net/bridge/br_private.h   |    6 ++++
 4 files changed, 53 insertions(+), 29 deletions(-)

net/bridge/br_device.c +8 -1

@@ -68,10 +68,17 @@ static int br_dev_stop(struct net_device
 
 static int br_change_mtu(struct net_device *dev, int new_mtu)
 {
-	if (new_mtu < 68 || new_mtu > br_min_mtu(netdev_priv(dev)))
+	struct net_bridge *br = netdev_priv(dev);
+	if (new_mtu < 68 || new_mtu > br_min_mtu(br))
 		return -EINVAL;
 
 	dev->mtu = new_mtu;
+
+#ifdef CONFIG_BRIDGE_NETFILTER
+	/* remember the MTU in the rtable for PMTU */
+	br->fake_rtable.u.dst.metrics[RTAX_MTU - 1] = new_mtu;
+#endif
+
 	return 0;
 }

net/bridge/br_if.c +3 -0

@@ -202,6 +202,9 @@ static struct net_device *new_bridge_dev
 	br->topology_change = 0;
 	br->topology_change_detected = 0;
 	br->ageing_time = 300 * HZ;
+
+	br_netfilter_rtable_init(br);
+
 	INIT_LIST_HEAD(&br->age_list);
 
 	br_stp_timer_init(br);

net/bridge/br_netfilter.c +34 ~~-29~~

91 lines not on this page · raw message

net/bridge/br_private.h +6 -0

@@ -15,6 +15,7 @@
 
 #include <linux/netdevice.h>
 #include <linux/if_bridge.h>
+#include <net/route.h>
 
 #define BR_HASH_BITS 8
 #define BR_HASH_SIZE (1 << BR_HASH_BITS)

@@ -92,6 +93,9 @@ struct net_bridge
 	struct hlist_head		hash[BR_HASH_SIZE];
 	struct list_head		age_list;
 	unsigned long			feature_mask;
+#ifdef CONFIG_BRIDGE_NETFILTER
+	struct rtable 			fake_rtable;
+#endif
 	unsigned long			flags;
 #define BR_SET_MAC_ADDR		0x00000001

@@ -197,9 +201,11 @@ extern int br_ioctl_deviceless_stub(stru
 #ifdef CONFIG_BRIDGE_NETFILTER
 extern int br_netfilter_init(void);
 extern void br_netfilter_fini(void);
+extern void br_netfilter_rtable_init(struct net_bridge *);
 #else
 #define br_netfilter_init()	(0)
 #define br_netfilter_fini()	do { } while(0)
+#define br_netfilter_rtable_init(x)
 #endif
 
 /* br_stp.c */

Andrew Morton

author

2008-07-30 20:50 UTC #2 lore raw

Re: [PATCH] bridge: send correct MTU value in PMTU (revised)

Quoted 9 lines · On Wed, 30 Jul 2008 13:32:59 -0700 Stephen Hemminger…

When bridging interfaces with different MTUs, the bridge correctly chooses the minimum of the MTUs of the physical devices as the bridges MTU. But when a frame is passed which fits through the incoming, but not through the outgoing interface, a "Fragmentation Needed" packet is generated.
However, the propagated MTU is hardcoded to 1500, which is wrong in this situation. The sender will repeat the packet again with the same frame size, and the same problem will occur again.

Quoted 8 lines

Instead of sending 1500, the (correct) MTU value of the bridge is now sent via PMTU. To achieve this, the corresponding rtable structure is stored in its net_bridge structure.
Modified to get rid of fake_net_device as well.
Signed-off-by Simon Wunderlich <[email protected]>
Signed-off-by Stephen Hemminger <[email protected]>

um.

a) Why was this To:me and not To:davem? I'm trying to get rid of it ;)

b) The way you sent this email indicates that you wrote the patch. To preserve correct attribution, please put a From: line right at the top of the changelog.

c) For the record, here are the changes which you (I assume) made to Simon's patch:

(br_netfilter_rtable_init() no longer initialises fake_rtable.rt_flags. Deliberate?)

 net/bridge/br_if.c        |   26 +-------------------------
 net/bridge/br_netfilter.c |   18 ++++++++++++++++++
 net/bridge/br_private.h   |    5 +++--
 3 files changed, 22 insertions(+), 27 deletions(-)

net/bridge/br_if.c renamed from net/bridge/br_if.c~bridge-send-correct-mtu-value-in-pmtu-revised +1 ~~-25~~

diff -puN net/bridge/br_if.c~bridge-send-correct-mtu-value-in-pmtu-revised net/bridge/br_if.c
@@ -168,23 +168,6 @@ static void del_br(struct net_bridge *br
 	unregister_netdevice(br->dev);
 }
 
-#ifdef CONFIG_BRIDGE_NETFILTER
-/* We need these fake structures to make netfilter happy --
- * lots of places assume that skb->dst != NULL, which isn't
- * all that unreasonable.
- *
- * Currently, we fill in the PMTU entry because netfilter
- * refragmentation needs it, and the rt_flags entry because
- * ipt_REJECT needs it.  Future netfilter modules might
- * require us to fill additional fields. */
-struct net_device __fake_net_device = {
-	.hard_header_len	= ETH_HLEN,
-#ifdef CONFIG_NET_NS
-	.nd_net			= &init_net,
-#endif
-};
-#endif
-
 static struct net_device *new_bridge_dev(const char *name)
 {
 	struct net_bridge *br;

@@ -220,14 +203,7 @@ static struct net_device *new_bridge_dev
 	br->topology_change_detected = 0;
 	br->ageing_time = 300 * HZ;
 
-#ifdef CONFIG_BRIDGE_NETFILTER
-	atomic_set(&br->fake_rtable.u.dst.__refcnt, 1);
-	br->fake_rtable.u.dst.dev = &__fake_net_device;
-	br->fake_rtable.u.dst.path = &br->fake_rtable.u.dst;
-	br->fake_rtable.u.dst.metrics[RTAX_MTU - 1] = 1500;
-	br->fake_rtable.u.dst.flags	= DST_NOXFRM;
-	br->fake_rtable.rt_flags = 0;
-#endif
+	br_netfilter_rtable_init(br);
 
 	INIT_LIST_HEAD(&br->age_list);

net/bridge/br_netfilter.c renamed from net/bridge/br_netfilter.c~bridge-send-correct-mtu-value-in-pmtu-revised +18 -0

diff -puN net/bridge/br_netfilter.c~bridge-send-correct-mtu-value-in-pmtu-revised net/bridge/br_netfilter.c
@@ -101,6 +101,24 @@ static inline __be16 pppoe_proto(const s
 	 pppoe_proto(skb) == htons(PPP_IPV6) && \
 	 brnf_filter_pppoe_tagged)
 
+/*
+ * Initialize bogus route table used to keep netfilter happy.
+ * Currently, we fill in the PMTU entry because netfilter
+ * refragmentation needs it, and the rt_flags entry because
+ * ipt_REJECT needs it.  Future netfilter modules might
+ * require us to fill additional fields.
+ */
+void br_netfilter_rtable_init(struct net_bridge *br)
+{
+	struct rtable *rt = &br->fake_rtable;
+
+	atomic_set(&rt->u.dst.__refcnt, 1);
+	rt->u.dst.dev = &br->dev;
+	rt->u.dst.path = &rt->u.dst;
+	rt->u.dst.metrics[RTAX_MTU - 1] = 1500;
+	rt->u.dst.flags	= DST_NOXFRM;
+}
+
 static inline struct rtable *bridge_parent_rtable(const struct net_device *dev)
 {
 	struct net_bridge_port *port = rcu_dereference(dev->br_port);

net/bridge/br_private.h renamed from net/bridge/br_private.h~bridge-send-correct-mtu-value-in-pmtu-revised +3 -2

diff -puN net/bridge/br_private.h~bridge-send-correct-mtu-value-in-pmtu-revised net/bridge/br_private.h
@@ -15,7 +15,7 @@
 
 #include <linux/netdevice.h>
 #include <linux/if_bridge.h>
-#include <net/route.h>	/* struct rtable, RTAX_MTU */
+#include <net/route.h>
 
 #define BR_HASH_BITS 8
 #define BR_HASH_SIZE (1 << BR_HASH_BITS)

@@ -201,10 +201,11 @@ extern int br_ioctl_deviceless_stub(stru
 #ifdef CONFIG_BRIDGE_NETFILTER
 extern int br_netfilter_init(void);
 extern void br_netfilter_fini(void);
+extern void br_netfilter_rtable_init(struct net_bridge *);
 #else
 #define br_netfilter_init()	(0)
 #define br_netfilter_fini()	do { } while(0)
-extern struct net_device __fake_net_device;
+#define br_netfilter_rtable_init(x)
 #endif
 
 /* br_stp.c */

Stephen Hemminger

2008-07-30 21:10 UTC #3 lore raw

Re: [PATCH] bridge: send correct MTU value in PMTU (revised)

Quoted 29 lines · On Wed, 30 Jul 2008 13:50:08 -0700 Andrew Morton <[email protected]>…

Quoted 9 lines · On Wed, 30 Jul 2008 13:32:59 -0700 Stephen Hemminger…
When bridging interfaces with different MTUs, the bridge correctly chooses the minimum of the MTUs of the physical devices as the bridges MTU. But when a frame is passed which fits through the incoming, but not through the outgoing interface, a "Fragmentation Needed" packet is generated.
However, the propagated MTU is hardcoded to 1500, which is wrong in this situation. The sender will repeat the packet again with the same frame size, and the same problem will occur again.
Quoted 8 lines
Instead of sending 1500, the (correct) MTU value of the bridge is now sent via PMTU. To achieve this, the corresponding rtable structure is stored in its net_bridge structure.
Modified to get rid of fake_net_device as well.
Signed-off-by Simon Wunderlich <[email protected]>
Signed-off-by Stephen Hemminger <[email protected]>
um.
a) Why was this To:me and not To:davem? I'm trying to get rid of it ;)
b) The way you sent this email indicates that you wrote the patch. To preserve correct attribution, please put a From: line right at the top of the changelog.

Quoted 6 lines

c) For the record, here are the changes which you (I assume) made to Simon's patch:
(br_netfilter_rtable_init() no longer initialises fake_rtable.rt_flags. Deliberate?)

a) didn't want you holding onto old version, and it leaking upstream.
b) didn't know how to spoof git's attribution
c) fake_rtable gets initialized to zero already.
Since it is part of bridge structure and the whole bridge structure is the private part of the bridge pseudo network device which is initialized to zero as part of alloc_netdev.

David Miller

maintainer

2008-07-30 23:28 UTC #4 lore raw

Re: [PATCH] bridge: send correct MTU value in PMTU (revised)

From: Stephen Hemminger <[email protected]>
Date: Wed, 30 Jul 2008 14:10:17 -0700

Quoted 6 lines

a) didn't want you holding onto old version, and it leaking upstream.
b) didn't know how to spoof git's attribution
c) fake_rtable gets initialized to zero already.
Since it is part of bridge structure and the whole bridge structure is the private part of the bridge pseudo network device which is initialized to zero as part of alloc_netdev.

I've applied Stephen's updated version of Simon's patch.

Thanks.