Message ID | 3b9c5ffd8085c0af7072960865883a23be0d0a32.1462974886.git.jbenc@redhat.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Hi Jiri, On Wed, 11 May 2016 15:55:06 +0200 Jiri Benc <jbenc@redhat.com> wrote: > For VXLAN-GPE, the interface is ARPHRD_NONE, thus we need to reset > mac_header after pulling the outer header. > > Fixes: e1e5314de08b ("vxlan: implement GPE") > Signed-off-by: Jiri Benc <jbenc@redhat.com> > --- > drivers/net/vxlan.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c > index 2f29d20aa08f..e030a804b772 100644 > --- a/drivers/net/vxlan.c > +++ b/drivers/net/vxlan.c > @@ -1338,6 +1338,8 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) > if (__iptunnel_pull_header(skb, VXLAN_HLEN, protocol, raw_proto, > !net_eq(vxlan->net, dev_net(vxlan->dev)))) > goto drop; > + if (raw_proto) > + skb_reset_mac_header(skb); Would it make sense to perform this within __iptunnel_pull_header (in case raw_proto is true) for all __iptunnel_pull_header callers? If not (meaning vxlan specific), in next few lines we have: if (!raw_proto) { if (!vxlan_set_mac(vxlan, vs, skb)) goto drop; } else { skb->dev = vxlan->dev; skb->pkt_type = PACKET_HOST; } Seems more appropriate to place the missing 'skb_reset_mac_header' within the "else" part, which logically holds all "things to initialize in the skb if raw_proto is true, thus havn't called vxlan_set_mac". Regards, Shmulik
On Thu, 12 May 2016 09:47:58 +0300, Shmulik Ladkani wrote: > Would it make sense to perform this within __iptunnel_pull_header (in > case raw_proto is true) for all __iptunnel_pull_header callers? raw_proto just denotes that inner_proto of ETH_P_TEB should not be treated specially. Guessing any other special meaning based on this flag would be unexpected and confusing. Specifically, __iptunnel_pull_header doesn't have and should not have any notion of the device type; although all current users of raw_proto == true are of ARPHRD_NONE type, it's not a requirement. > If not (meaning vxlan specific), in next few lines we have: > > if (!raw_proto) { > if (!vxlan_set_mac(vxlan, vs, skb)) > goto drop; > } else { > skb->dev = vxlan->dev; > skb->pkt_type = PACKET_HOST; > } > > Seems more appropriate to place the missing 'skb_reset_mac_header' > within the "else" part, which logically holds all "things to initialize > in the skb if raw_proto is true, thus havn't called vxlan_set_mac". I agree. I'll respin the patch. Thanks! Jiri
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c index 2f29d20aa08f..e030a804b772 100644 --- a/drivers/net/vxlan.c +++ b/drivers/net/vxlan.c @@ -1338,6 +1338,8 @@ static int vxlan_rcv(struct sock *sk, struct sk_buff *skb) if (__iptunnel_pull_header(skb, VXLAN_HLEN, protocol, raw_proto, !net_eq(vxlan->net, dev_net(vxlan->dev)))) goto drop; + if (raw_proto) + skb_reset_mac_header(skb); if (vxlan_collect_metadata(vs)) { __be32 vni = vxlan_vni(vxlan_hdr(skb)->vx_vni);
For VXLAN-GPE, the interface is ARPHRD_NONE, thus we need to reset mac_header after pulling the outer header. Fixes: e1e5314de08b ("vxlan: implement GPE") Signed-off-by: Jiri Benc <jbenc@redhat.com> --- drivers/net/vxlan.c | 2 ++ 1 file changed, 2 insertions(+)