Re: [PATCH] net: skbuff: Fix length validation in skb_vlan_pop()

[Shmulik Ladkani <shmulik.ladkani@gmail.com>]

Hi Pravin,

On Sun, 18 Sep 2016 13:26:30 -0700 pravin shelar <pshelar@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?

Thanks,
Shmulik