Hi, On Mon, 19 Sep 2016 13:46:10 -0700 pravin shelar <pshe...@ovn.org> wrote: > On Mon, Sep 19, 2016 at 1:04 PM, Shmulik Ladkani > <shmulik.ladk...@gmail.com> wrote: > > Hi Pravin, > > > > On Sun, 18 Sep 2016 13:26:30 -0700 pravin shelar <pshe...@ovn.org> wrote: > >> > +++ b/net/core/skbuff.c > >> > @@ -4537,7 +4537,7 @@ int skb_vlan_pop(struct sk_buff *skb) > >> > } else { > >> > if (unlikely((skb->protocol != htons(ETH_P_8021Q) && > >> > skb->protocol != htons(ETH_P_8021AD)) || > >> > - skb->len < VLAN_ETH_HLEN)) > >> > + skb->mac_len < VLAN_ETH_HLEN)) > >> > >> There is already check in __skb_vlan_pop() to validate skb for a vlan > >> header. So it is safe to drop this check entirely. > > > > Yep, I submitted a v2 with your suggestion, however I withdrew it, as > > there is a slight behavior difference noticable by 'skb_vlan_pop' callers. > > > > Suppose the rare case where skb->len is too small. > > > > pre: > > skb_vlan_pop returns 0 (at least for the correct tx path). > > Meaning, callers do not see it as a failure. > > post: > > skb_ensure_writable fails (!pskb_may_pull), therefore -ENOMEM returned > > to the callers of 'skb_vlan_pop'. > > > > For ovs, it means do_execute_actions's loop is terminated, no further > > actions are executed, and skb gets freed. > > > > For tc act vlan, it means skb gets dropped. > > > > This actually makes sense, but do we want to present this change? > > > I think this is correct behavior over existing code.
Ok. I'll submit a v3 identical to v2 but with proper statement of this behavior change in the commit log. Thanks.