On 08/08/2019 11:04, Zahari Doychev wrote: > On Wed, Aug 07, 2019 at 12:17:43PM +0300, Nikolay Aleksandrov wrote: >> Hi Zahari, >> On 05/08/2019 18:37, Zahari Doychev wrote: >>> The bridge code cannot forward packets from various paths that set up the >>> SKBs in different ways. Some of these packets get corrupted during the >>> forwarding as not always is just ETH_HLEN pulled at the front. This happens >>> e.g. when VLAN tags are pushed bu using tc act_vlan on ingress. >> Overall the patch looks good, I think it shouldn't introduce any regressions >> at least from the codepaths I was able to inspect, but please include more >> details in here from the cover letter, in fact you don't need it just add >> all of >> the details here so we have them, especially the test setup. Also please >> provide >> some details how this patch was tested. It'd be great if you could provide a >> selftest for it so we can make sure it's considered when doing future >> changes. > > Hi Nik, > > Thanks for the reply. I will do the suggested corrections and try creating a > selftest. I assume it should go to the net/forwarding together with the other > bridge tests as a separate patch. > > Regards, > Zahari >
Hi, Yes, the selftest should target net-next and go to net/forwarding. Thanks, Nik >> >> Thank you, >> Nik >> >>> >>> The problem is fixed by using skb->mac_len instead of ETH_HLEN, which makes >>> sure that the skb headers are correctly restored. This usually does not >>> change anything, execpt the local bridge transmits which now need to set >>> the skb->mac_len correctly in br_dev_xmit, as well as the broken case noted >>> above. >>> >>> Signed-off-by: Zahari Doychev <[email protected]> >>> --- >>> net/bridge/br_device.c | 3 ++- >>> net/bridge/br_forward.c | 4 ++-- >>> net/bridge/br_vlan.c | 3 ++- >>> 3 files changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c >>> index 681b72862c16..aeb77ff60311 100644 >>> --- a/net/bridge/br_device.c >>> +++ b/net/bridge/br_device.c >>> @@ -55,8 +55,9 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct >>> net_device *dev) >>> BR_INPUT_SKB_CB(skb)->frag_max_size = 0; >>> >>> skb_reset_mac_header(skb); >>> + skb_reset_mac_len(skb); >>> eth = eth_hdr(skb); >>> - skb_pull(skb, ETH_HLEN); >>> + skb_pull(skb, skb->mac_len); >>> >>> if (!br_allowed_ingress(br, br_vlan_group_rcu(br), skb, &vid)) >>> goto out; >>> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c >>> index 86637000f275..edb4f3533f05 100644 >>> --- a/net/bridge/br_forward.c >>> +++ b/net/bridge/br_forward.c >>> @@ -32,7 +32,7 @@ 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); >>> + skb_push(skb, skb->mac_len); >>> if (!is_skb_forwardable(skb->dev, skb)) >>> goto drop; >>> >>> @@ -94,7 +94,7 @@ 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); >>> + skb_push(skb, skb->mac_len); >>> if (!is_skb_forwardable(skb->dev, skb)) >>> kfree_skb(skb); >>> else >>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c >>> index 021cc9f66804..88244c9cc653 100644 >>> --- a/net/bridge/br_vlan.c >>> +++ b/net/bridge/br_vlan.c >>> @@ -466,13 +466,14 @@ static bool __allowed_ingress(const struct net_bridge >>> *br, >>> /* Tagged frame */ >>> if (skb->vlan_proto != br->vlan_proto) { >>> /* Protocol-mismatch, empty out vlan_tci for new tag */ >>> - skb_push(skb, ETH_HLEN); >>> + skb_push(skb, skb->mac_len); >>> skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto, >>> skb_vlan_tag_get(skb)); >>> if (unlikely(!skb)) >>> return false; >>> >>> skb_pull(skb, ETH_HLEN); >>> + skb_reset_network_header(skb); >>> skb_reset_mac_len(skb); >>> *vid = 0; >>> tagged = false; >>> >>
