All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net v3] net: bridge: Fix ethernet header pointer before check skb forwardable
@ 2019-01-16 13:04 ` wangyunjian
  0 siblings, 0 replies; 6+ messages in thread
From: wangyunjian @ 2019-01-16 13:04 UTC (permalink / raw)
  To: netdev; +Cc: xudingke, Yunjian Wang, bridge, Nkolay Aleksandrov, Roopa Prabhu

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 packet(Data 1480, UDP 8, IP 20, Ethernet 14, VLAN 4)
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 doesn'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
v3:
 -add detailed description of packet len and construction
---
 net/bridge/br_forward.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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;
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Bridge] [PATCH net v3] net: bridge: Fix ethernet header pointer before check skb forwardable
@ 2019-01-16 13:04 ` wangyunjian
  0 siblings, 0 replies; 6+ messages in thread
From: wangyunjian @ 2019-01-16 13:04 UTC (permalink / raw)
  To: netdev; +Cc: Nkolay Aleksandrov, bridge, Roopa Prabhu, Yunjian Wang, xudingke

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 packet(Data 1480, UDP 8, IP 20, Ethernet 14, VLAN 4)
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 doesn'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
v3:
 -add detailed description of packet len and construction
---
 net/bridge/br_forward.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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;
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net v3] net: bridge: Fix ethernet header pointer before check skb forwardable
  2019-01-16 13:04 ` [Bridge] " wangyunjian
@ 2019-01-16 17:33   ` Stephen Hemminger
  -1 siblings, 0 replies; 6+ messages in thread
From: Stephen Hemminger @ 2019-01-16 17:33 UTC (permalink / raw)
  To: wangyunjian; +Cc: netdev, xudingke, bridge, Nkolay Aleksandrov, Roopa Prabhu

On Wed, 16 Jan 2019 21:04:21 +0800
wangyunjian <wangyunjian@huawei.com> wrote:

>  		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;

The patch looks correct.

On minor style issue is that after moving skb_push out of the if statement,
both branches of the statement now contain a single line and therefore
the brackets are no longer necessary.  Suprised that checkpatch did
not complain about this.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Bridge] [PATCH net v3] net: bridge: Fix ethernet header pointer before check skb forwardable
@ 2019-01-16 17:33   ` Stephen Hemminger
  0 siblings, 0 replies; 6+ messages in thread
From: Stephen Hemminger @ 2019-01-16 17:33 UTC (permalink / raw)
  To: wangyunjian; +Cc: Nkolay Aleksandrov, netdev, Roopa Prabhu, bridge, xudingke

On Wed, 16 Jan 2019 21:04:21 +0800
wangyunjian <wangyunjian@huawei.com> wrote:

>  		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;

The patch looks correct.

On minor style issue is that after moving skb_push out of the if statement,
both branches of the statement now contain a single line and therefore
the brackets are no longer necessary.  Suprised that checkpatch did
not complain about this.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH net v3] net: bridge: Fix ethernet header pointer before check skb forwardable
  2019-01-16 17:33   ` [Bridge] " Stephen Hemminger
@ 2019-01-17  1:40     ` wangyunjian
  -1 siblings, 0 replies; 6+ messages in thread
From: wangyunjian @ 2019-01-17  1:40 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: netdev, xudingke, bridge, Nkolay Aleksandrov, Roopa Prabhu

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, January 17, 2019 1:33 AM
> 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 v3] net: bridge: Fix ethernet header pointer before
> check skb forwardable
> 
> On Wed, 16 Jan 2019 21:04:21 +0800
> wangyunjian <wangyunjian@huawei.com> wrote:
> 
> >  		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;
> 
> The patch looks correct.
> 
> On minor style issue is that after moving skb_push out of the if statement,
> both branches of the statement now contain a single line and therefore the
> brackets are no longer necessary.  Suprised that checkpatch did not complain
> about this.

OK, I'll fix it and send the v4 later.

Thanks
	Yunjian


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Bridge] [PATCH net v3] net: bridge: Fix ethernet header pointer before check skb forwardable
@ 2019-01-17  1:40     ` wangyunjian
  0 siblings, 0 replies; 6+ messages in thread
From: wangyunjian @ 2019-01-17  1:40 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Nkolay Aleksandrov, netdev, Roopa Prabhu, bridge, xudingke

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, January 17, 2019 1:33 AM
> 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 v3] net: bridge: Fix ethernet header pointer before
> check skb forwardable
> 
> On Wed, 16 Jan 2019 21:04:21 +0800
> wangyunjian <wangyunjian@huawei.com> wrote:
> 
> >  		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;
> 
> The patch looks correct.
> 
> On minor style issue is that after moving skb_push out of the if statement,
> both branches of the statement now contain a single line and therefore the
> brackets are no longer necessary.  Suprised that checkpatch did not complain
> about this.

OK, I'll fix it and send the v4 later.

Thanks
	Yunjian


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-01-17  1:41 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-16 13:04 [PATCH net v3] net: bridge: Fix ethernet header pointer before check skb forwardable wangyunjian
2019-01-16 13:04 ` [Bridge] " wangyunjian
2019-01-16 17:33 ` Stephen Hemminger
2019-01-16 17:33   ` [Bridge] " Stephen Hemminger
2019-01-17  1:40   ` wangyunjian
2019-01-17  1:40     ` [Bridge] " wangyunjian

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.