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
>
> 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 <zahari.doychev@xxxxxxxxx>
> > ---
> > 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;
> >
>
