diff mbox series

[net,v2] net: bridge: Fix ethernet header pointer before check skb forwardable

Message ID 1547521959-3736-1-git-send-email-wangyunjian@huawei.com
State Changes Requested
Delegated to: David Miller
Headers show
Series [net,v2] net: bridge: Fix ethernet header pointer before check skb forwardable | expand

Commit Message

wangyunjian Jan. 15, 2019, 3:12 a.m. UTC 
From: Yunjian Wang <wangyunjian@huawei.com>

The skb header should be set to ethernet header before using
is_skb_forwardable. Because the ethernet header length has been
considered in is_skb_forwardable(including dev->hard_header_len
length).

To reproduce the issue:
1, add 2 ports on linux bridge br using following commands:
$ brctl addbr br
$ brctl addif br eth0
$ brctl addif br eth1
2, the mtu of eth0 and eth1 is 1500
3, send a 1504 packet from eth0 to eth1

So the expect result is packet larger than 1500 cannot pass through
eth0 and eth1. But currently, the packet passes through success, it
means eth1's mtu limit donen't take effect.

Fixes: f6367b4660dd ("bridge: use is_skb_forwardable in forward path")
Cc: bridge@lists.linux-foundation.org
Cc: Nkolay Aleksandrov <nikolay@cumulusnetworks.com>
Cc: Roopa Prabhu <roopa@cumulusnetworks.com>
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
v2:
 -fix commit log and cc the bridge maintainers with fixes tags
---
 net/bridge/br_forward.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Toshiaki Makita Jan. 15, 2019, 7:49 a.m. UTC | #1 
On 2019/01/15 12:12, wangyunjian wrote:
> From: Yunjian Wang <wangyunjian@huawei.com>
> 
> The skb header should be set to ethernet header before using
> is_skb_forwardable. Because the ethernet header length has been
> considered in is_skb_forwardable(including dev->hard_header_len
> length).
> 
> To reproduce the issue:
> 1, add 2 ports on linux bridge br using following commands:
> $ brctl addbr br
> $ brctl addif br eth0
> $ brctl addif br eth1
> 2, the mtu of eth0 and eth1 is 1500
> 3, send a 1504 packet from eth0 to eth1
> 
> So the expect result is packet larger than 1500 cannot pass through
> eth0 and eth1. But currently, the packet passes through success, it
> means eth1's mtu limit donen't take effect.

I agree with the change but this test does not look appropriate.
In the first place is_skb_forwardable() accepts 4 bytes extra size (for
VLAN header). Doesn't your test case fail even after the change?
Sergei Shtylyov Jan. 15, 2019, 9:21 a.m. UTC | #2 
On 15.01.2019 6:12, wangyunjian wrote:

> From: Yunjian Wang <wangyunjian@huawei.com>
> 
> The skb header should be set to ethernet header before using
> is_skb_forwardable. Because the ethernet header length has been
> considered in is_skb_forwardable(including dev->hard_header_len
> length).
> 
> To reproduce the issue:
> 1, add 2 ports on linux bridge br using following commands:
> $ brctl addbr br
> $ brctl addif br eth0
> $ brctl addif br eth1
> 2, the mtu of eth0 and eth1 is 1500
> 3, send a 1504 packet from eth0 to eth1
> 
> So the expect result is packet larger than 1500 cannot pass through
> eth0 and eth1. But currently, the packet passes through success, it
> means eth1's mtu limit donen't take effect.

    Doesn't. And MTU.

> Fixes: f6367b4660dd ("bridge: use is_skb_forwardable in forward path")
> Cc: bridge@lists.linux-foundation.org
> Cc: Nkolay Aleksandrov <nikolay@cumulusnetworks.com>
> Cc: Roopa Prabhu <roopa@cumulusnetworks.com>
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
[...]

MBR, Sergei
wangyunjian Jan. 16, 2019, 9:09 a.m. UTC | #3 
> -----Original Message-----
> From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com]
> Sent: Tuesday, January 15, 2019 5:22 PM
> To: wangyunjian <wangyunjian@huawei.com>; netdev@vger.kernel.org
> Cc: xudingke <xudingke@huawei.com>; bridge@lists.linux-foundation.org;
> Nkolay Aleksandrov <nikolay@cumulusnetworks.com>; Roopa Prabhu
> <roopa@cumulusnetworks.com>
> Subject: Re: [PATCH net v2] net: bridge: Fix ethernet header pointer before
> check skb forwardable
> 
> On 15.01.2019 6:12, wangyunjian wrote:
> 
> > From: Yunjian Wang <wangyunjian@huawei.com>
> >
> > The skb header should be set to ethernet header before using
> > is_skb_forwardable. Because the ethernet header length has been
> > considered in is_skb_forwardable(including dev->hard_header_len
> > length).
> >
> > To reproduce the issue:
> > 1, add 2 ports on linux bridge br using following commands:
> > $ brctl addbr br
> > $ brctl addif br eth0
> > $ brctl addif br eth1
> > 2, the mtu of eth0 and eth1 is 1500
> > 3, send a 1504 packet from eth0 to eth1
> >
> > So the expect result is packet larger than 1500 cannot pass through
> > eth0 and eth1. But currently, the packet passes through success, it
> > means eth1's mtu limit donen't take effect.
> 
>     Doesn't. And MTU.

Thanks, I will fix them and send a new patch later.

Best regards,
	Yunjian
> 
> > Fixes: f6367b4660dd ("bridge: use is_skb_forwardable in forward path")
> > Cc: bridge@lists.linux-foundation.org
> > Cc: Nkolay Aleksandrov <nikolay@cumulusnetworks.com>
> > Cc: Roopa Prabhu <roopa@cumulusnetworks.com>
> > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> [...]
> 
> MBR, Sergei
wangyunjian Jan. 16, 2019, 12:44 p.m. UTC | #4 
> -----Original Message-----
> From: Toshiaki Makita [mailto:makita.toshiaki@lab.ntt.co.jp]
> Sent: Tuesday, January 15, 2019 3:50 PM
> To: wangyunjian <wangyunjian@huawei.com>
> Cc: netdev@vger.kernel.org; xudingke <xudingke@huawei.com>;
> bridge@lists.linux-foundation.org; Nkolay Aleksandrov
> <nikolay@cumulusnetworks.com>; Roopa Prabhu
> <roopa@cumulusnetworks.com>
> Subject: Re: [PATCH net v2] net: bridge: Fix ethernet header pointer before
> check skb forwardable
> 
> On 2019/01/15 12:12, wangyunjian wrote:
> > From: Yunjian Wang <wangyunjian@huawei.com>
> >
> > The skb header should be set to ethernet header before using
> > is_skb_forwardable. Because the ethernet header length has been
> > considered in is_skb_forwardable(including dev->hard_header_len
> > length).
> >
> > To reproduce the issue:
> > 1, add 2 ports on linux bridge br using following commands:
> > $ brctl addbr br
> > $ brctl addif br eth0
> > $ brctl addif br eth1
> > 2, the mtu of eth0 and eth1 is 1500
> > 3, send a 1504 packet from eth0 to eth1
> >
> > So the expect result is packet larger than 1500 cannot pass through
> > eth0 and eth1. But currently, the packet passes through success, it
> > means eth1's mtu limit donen't take effect.
> 
> I agree with the change but this test does not look appropriate.
> In the first place is_skb_forwardable() accepts 4 bytes extra size (for VLAN
> header). Doesn't your test case fail even after the change?
> 
> --
> Toshiaki Makita

Before executing the functions is_skb_forwardable() and skb_push(), the actual
packet consist of the follwing parts, Data 1480, UDP 8, IP 20, Ethernet 14, VLAN 4.
And the value of variables are dev->mtu=1500, dev->hard_header_len=14 and
skb->len=1508. The packet cannot pass through.

I will fix the previous description of packet len.

Thanks,
	Yunjian
diff mbox series

Patch

diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index 5372e20..74b688b 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -36,10 +36,10 @@  static inline int should_deliver(const struct net_bridge_port *p,
 
 int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb)
 {
+	skb_push(skb, ETH_HLEN);
 	if (!is_skb_forwardable(skb->dev, skb))
 		goto drop;
 
-	skb_push(skb, ETH_HLEN);
 	br_drop_fake_rtable(skb);
 
 	if (skb->ip_summed == CHECKSUM_PARTIAL &&
@@ -97,10 +97,10 @@  static void __br_forward(const struct net_bridge_port *to,
 		net = dev_net(indev);
 	} else {
 		if (unlikely(netpoll_tx_running(to->br->dev))) {
+			skb_push(skb, ETH_HLEN);
 			if (!is_skb_forwardable(skb->dev, skb)) {
 				kfree_skb(skb);
 			} else {
-				skb_push(skb, ETH_HLEN);
 				br_netpoll_send_skb(to, skb);
 			}
 			return;