lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20141121190835.GH2251@nanopsycho.orion> Date: Fri, 21 Nov 2014 20:08:35 +0100 From: Jiri Pirko <jiri@...nulli.us> To: Pravin Shelar <pshelar@...ira.com> Cc: Jiri Benc <jbenc@...hat.com>, netdev <netdev@...r.kernel.org>, David Miller <davem@...emloft.net>, Jamal Hadi Salim <jhs@...atatu.com>, Tom Herbert <therbert@...gle.com>, Eric Dumazet <edumazet@...gle.com>, Willem de Bruijn <willemb@...gle.com>, Daniel Borkmann <dborkman@...hat.com>, mst@...hat.com, fw@...len.de, Paul.Durrant@...rix.com, Thomas Graf <tgraf@...g.ch>, Cong Wang <cwang@...pensource.com> Subject: Re: [patch net-next v4 8/9] net: move vlan pop/push functions into common code Fri, Nov 21, 2014 at 07:41:27PM CET, pshelar@...ira.com wrote: >On Fri, Nov 21, 2014 at 10:05 AM, Jiri Pirko <jiri@...nulli.us> wrote: >> Fri, Nov 21, 2014 at 05:11:39PM CET, pshelar@...ira.com wrote: >>>On Wed, Nov 19, 2014 at 5:05 AM, Jiri Pirko <jiri@...nulli.us> wrote: >>>> So it can be used from out of openvswitch code. >>>> Did couple of cosmetic changes on the way, namely variable naming and >>>> adding support for 8021AD proto. >>>> >>>> Signed-off-by: Jiri Pirko <jiri@...nulli.us> >>>> --- >>>> >>>> v3->v4: >>>> - don't try to be smart and copy skb->mac_len manipulation as it is. It can be >>>> adjusted later on. >>> >>>I am fine with moving this code to generic library, as long as we can >>>adjust the mac_len as offset for MPLS header, otherwise this breaks >>>MPLS support. >> >> Pravin, I'm just moving the code without changing it in this aspect. >> Maybe I'm missing something but can you tell me what you see wrong? >> Thanks. >> > >There is bug in the openvswitch code where vlan-pop does mac-length >reset rather than subtracting VLAN_HLEN from mac-len. MPLS code >depends on this. Now this patch moves the bug to common code. I am >fine with this patch and I can send fix to change code vlan-pop code >to adjust mac_len later on. >But I am worried about the comment made by Jiri Benc on earlier >version of this patch where is mentioned that subtracting VLAN_HLEN >can break common case for vlan-pop. In that case we can not change >this common function. Jiri Benc, Can you point me to the code, so the >we can work on solution to fix this issue. Lets move to common code and fix this there with follow-up patches. This common code should work for all users. > >>> >>>> - fix error path in do_execute_actions as suggested by Pravin >>>> v2->v3: >>>> - used previously introduced __vlan_insert_tag helper >>>> - used skb_push/pull to get skb->data into needed point >>>> - fixed skb->mac_len computation in skb_vlan_push pointed out by Pravin >>>> v1->v2: >>>> - adjusted to fix recent ovs changes >>>> - included change to use make_writable suggested by Eric >>>> >>>> include/linux/skbuff.h | 2 + >>>> net/core/skbuff.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++ >>>> net/openvswitch/actions.c | 86 +++++------------------------------------- >>>> 3 files changed, 106 insertions(+), 77 deletions(-) >>>> >>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h >>>> index e045516..78c299f 100644 >>>> --- a/include/linux/skbuff.h >>>> +++ b/include/linux/skbuff.h >>>> @@ -2679,6 +2679,8 @@ unsigned int skb_gso_transport_seglen(const struct sk_buff *skb); >>>> struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features); >>>> struct sk_buff *skb_vlan_untag(struct sk_buff *skb); >>>> int skb_ensure_writable(struct sk_buff *skb, int write_len); >>>> +int skb_vlan_pop(struct sk_buff *skb); >>>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci); >>>> >>>> struct skb_checksum_ops { >>>> __wsum (*update)(const void *mem, int len, __wsum wsum); >>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >>>> index d11bbe0..c906c5f 100644 >>>> --- a/net/core/skbuff.c >>>> +++ b/net/core/skbuff.c >>>> @@ -4163,6 +4163,101 @@ int skb_ensure_writable(struct sk_buff *skb, int write_len) >>>> } >>>> EXPORT_SYMBOL(skb_ensure_writable); >>>> >>>> +/* remove VLAN header from packet and update csum accordingly. */ >>>> +static int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci) >>>> +{ >>>> + struct vlan_hdr *vhdr; >>>> + unsigned int offset = skb->data - skb_mac_header(skb); >>>> + int err; >>>> + >>>> + __skb_push(skb, offset); >>>> + err = skb_ensure_writable(skb, VLAN_ETH_HLEN); >>>> + if (unlikely(err)) >>>> + goto pull; >>>> + >>>> + skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN); >>>> + >>>> + vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN); >>>> + *vlan_tci = ntohs(vhdr->h_vlan_TCI); >>>> + >>>> + memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN); >>>> + __skb_pull(skb, VLAN_HLEN); >>>> + >>>> + vlan_set_encap_proto(skb, vhdr); >>>> + skb->mac_header += VLAN_HLEN; >>>> + >>>> + if (skb_network_offset(skb) < ETH_HLEN) >>>> + skb_set_network_header(skb, ETH_HLEN); >>>> + >>>> + skb_reset_mac_len(skb); >>>> +pull: >>>> + __skb_pull(skb, offset); >>>> + >>>> + return err; >>>> +} >>>> + >>>> +int skb_vlan_pop(struct sk_buff *skb) >>>> +{ >>>> + u16 vlan_tci; >>>> + __be16 vlan_proto; >>>> + int err; >>>> + >>>> + if (likely(vlan_tx_tag_present(skb))) { >>>> + skb->vlan_tci = 0; >>>> + } else { >>>> + if (unlikely((skb->protocol != htons(ETH_P_8021Q) && >>>> + skb->protocol != htons(ETH_P_8021AD)) || >>>> + skb->len < VLAN_ETH_HLEN)) >>>> + return 0; >>>> + >>>> + err = __skb_vlan_pop(skb, &vlan_tci); >>>> + if (err) >>>> + return err; >>>> + } >>>> + /* move next vlan tag to hw accel tag */ >>>> + if (likely((skb->protocol != htons(ETH_P_8021Q) && >>>> + skb->protocol != htons(ETH_P_8021AD)) || >>>> + skb->len < VLAN_ETH_HLEN)) >>>> + return 0; >>>> + >>>> + vlan_proto = skb->protocol; >>>> + err = __skb_vlan_pop(skb, &vlan_tci); >>>> + if (unlikely(err)) >>>> + return err; >>>> + >>>> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci); >>>> + return 0; >>>> +} >>>> +EXPORT_SYMBOL(skb_vlan_pop); >>>> + >>>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci) >>>> +{ >>>> + if (vlan_tx_tag_present(skb)) { >>>> + unsigned int offset = skb->data - skb_mac_header(skb); >>>> + int err; >>>> + >>>> + /* __vlan_insert_tag expect skb->data pointing to mac header. >>>> + * So change skb->data before calling it and change back to >>>> + * original position later >>>> + */ >>>> + __skb_push(skb, offset); >>>> + err = __vlan_insert_tag(skb, skb->vlan_proto, >>>> + vlan_tx_tag_get(skb)); >>>> + if (err) >>>> + return err; >>>> + skb->protocol = skb->vlan_proto; >>>> + skb->mac_len += VLAN_HLEN; >>>> + __skb_pull(skb, offset); >>>> + >>>> + if (skb->ip_summed == CHECKSUM_COMPLETE) >>>> + skb->csum = csum_add(skb->csum, csum_partial(skb->data >>>> + + (2 * ETH_ALEN), VLAN_HLEN, 0)); >>>> + } >>>> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci); >>>> + return 0; >>>> +} >>>> +EXPORT_SYMBOL(skb_vlan_push); >>>> + >>>> /** >>>> * alloc_skb_with_frags - allocate skb with page frags >>>> * >>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c >>>> index 7ffa377..4e05ea1 100644 >>>> --- a/net/openvswitch/actions.c >>>> +++ b/net/openvswitch/actions.c >>>> @@ -206,93 +206,27 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *key, >>>> return 0; >>>> } >>>> >>>> -/* remove VLAN header from packet and update csum accordingly. */ >>>> -static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci) >>>> -{ >>>> - struct vlan_hdr *vhdr; >>>> - int err; >>>> - >>>> - err = skb_ensure_writable(skb, VLAN_ETH_HLEN); >>>> - if (unlikely(err)) >>>> - return err; >>>> - >>>> - skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN); >>>> - >>>> - vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN); >>>> - *current_tci = vhdr->h_vlan_TCI; >>>> - >>>> - memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN); >>>> - __skb_pull(skb, VLAN_HLEN); >>>> - >>>> - vlan_set_encap_proto(skb, vhdr); >>>> - skb->mac_header += VLAN_HLEN; >>>> - >>>> - if (skb_network_offset(skb) < ETH_HLEN) >>>> - skb_set_network_header(skb, ETH_HLEN); >>>> - >>>> - /* Update mac_len for subsequent MPLS actions */ >>>> - skb_reset_mac_len(skb); >>>> - return 0; >>>> -} >>>> - >>>> static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key) >>>> { >>>> - __be16 tci; >>>> int err; >>>> >>>> - if (likely(vlan_tx_tag_present(skb))) { >>>> - skb->vlan_tci = 0; >>>> - } else { >>>> - if (unlikely(skb->protocol != htons(ETH_P_8021Q) || >>>> - skb->len < VLAN_ETH_HLEN)) >>>> - return 0; >>>> - >>>> - err = __pop_vlan_tci(skb, &tci); >>>> - if (err) >>>> - return err; >>>> - } >>>> - /* move next vlan tag to hw accel tag */ >>>> - if (likely(skb->protocol != htons(ETH_P_8021Q) || >>>> - skb->len < VLAN_ETH_HLEN)) { >>>> + err = skb_vlan_pop(skb); >>>> + if (vlan_tx_tag_present(skb)) >>>> + invalidate_flow_key(key); >>>> + else >>>> key->eth.tci = 0; >>>> - return 0; >>>> - } >>>> - >>>> - invalidate_flow_key(key); >>>> - err = __pop_vlan_tci(skb, &tci); >>>> - if (unlikely(err)) >>>> - return err; >>>> - >>>> - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(tci)); >>>> - return 0; >>>> + return err; >>>> } >>>> >>>> static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key, >>>> const struct ovs_action_push_vlan *vlan) >>>> { >>>> - if (unlikely(vlan_tx_tag_present(skb))) { >>>> - u16 current_tag; >>>> - >>>> - /* push down current VLAN tag */ >>>> - current_tag = vlan_tx_tag_get(skb); >>>> - >>>> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto, >>>> - current_tag); >>>> - if (!skb) >>>> - return -ENOMEM; >>>> - /* Update mac_len for subsequent MPLS actions */ >>>> - skb->mac_len += VLAN_HLEN; >>>> - >>>> - if (skb->ip_summed == CHECKSUM_COMPLETE) >>>> - skb->csum = csum_add(skb->csum, csum_partial(skb->data >>>> - + (2 * ETH_ALEN), VLAN_HLEN, 0)); >>>> - >>>> + if (vlan_tx_tag_present(skb)) >>>> invalidate_flow_key(key); >>>> - } else { >>>> + else >>>> key->eth.tci = vlan->vlan_tci; >>>> - } >>>> - __vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT); >>>> - return 0; >>>> + return skb_vlan_push(skb, vlan->vlan_tpid, >>>> + ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT); >>>> } >>>> >>>> static int set_eth_addr(struct sk_buff *skb, struct sw_flow_key *key, >>>> @@ -858,8 +792,6 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, >>>> >>>> case OVS_ACTION_ATTR_PUSH_VLAN: >>>> err = push_vlan(skb, key, nla_data(a)); >>>> - if (unlikely(err)) /* skb already freed. */ >>>> - return err; >>>> break; >>>> >>>> case OVS_ACTION_ATTR_POP_VLAN: >>>> -- >>>> 1.9.3 >>>> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists