netdev - Re: [PATCH net-next V15 3/3] 802.1AD: Flow handling, actions, vlan parsing and netlink attributes

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <561FA113.6050304@gmail.com>
Date:	Thu, 15 Oct 2015 08:50:27 -0400
From:	Thomas F Herbert <thomasfherbert@...il.com>
To:	Pravin Shelar <pshelar@...ira.com>
Cc:	netdev <netdev@...r.kernel.org>, therbert@...hat.com,
	"dev@...nvswitch.org" <dev@...nvswitch.org>
Subject: Re: [PATCH net-next V15 3/3] 802.1AD: Flow handling, actions, vlan
 parsing and netlink attributes

On 10/13/15 2:14 PM, Pravin Shelar wrote:
> On Tue, Oct 13, 2015 at 10:39 AM, Thomas F Herbert
> <thomasfherbert@...il.com>  wrote:
>> Pravin,
>>
>> Thanks for the review.
>>
>>
>> On 10/13/15 7:47 AM, Pravin Shelar wrote:
>>> On Sat, Oct 10, 2015 at 4:40 PM, Thomas F Herbert
>>> <thomasfherbert@...il.com>  wrote:
>>>> Add support for 802.1ad including the ability to push and pop double
>>>> tagged vlans. Add support for 802.1ad to netlink parsing and flow
>>>> conversion. Uses double nested encap attributes to represent double
>>>> tagged vlan. Inner TPID encoded along with ctci in nested attributes.
>>>>
>>>> Signed-off-by: Thomas F Herbert<thomasfherbert@...il.com>
>>>> ---
>>>>    net/openvswitch/actions.c      |   6 +-
>>>>    net/openvswitch/flow.c         |  92 +++++++++++++++++++----
>>>>    net/openvswitch/flow.h         |  11 ++-
>>>>    net/openvswitch/flow_netlink.c | 166
>>>> +++++++++++++++++++++++++++++++++++++----
>>>>    net/openvswitch/vport-netdev.c |   4 +-
>>>>    5 files changed, 245 insertions(+), 34 deletions(-)
>>>>
>>> ...
>>>
> ...
>
>>> I see lot of duplicate code here. How about code below:
>>>
>>> struct qtag_prefix {
>>>           __be16 eth_type; /* ETH_P_8021Q  or ETH_P_8021AD */
>>>           __be16 tci;
>>> };
>>>
>>> /* Return  < 0 on memory error
>>>    * Return   == 0 on non vlan or incomplete packet packet
>>>    * Return > 0 on successfully parsing vlan tag.
>>>    */
>>> static int parse_vlan_tag(__be16 vlan_proto, struct sk_buff *skb,
>>>                             struct vlan_tag *cvlan)
>>> {
>>>           if (likely(!eth_type_vlan(skb->vlan_proto)))
>>>                   return 0;
>>>
>>>           if (unlikely(skb->len < sizeof(struct qtag_prefix) +
>>> sizeof(__be16))) {
>>>                   vlan->tci = 0;
>>>                   return 0;
>>>           }
>>>
>>>           if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
>>> sizeof(__be16))))
>>>                           return -ENOMEM;
>>>
>>>           qp = (struct qtag_prefix *)skb->data;
>>>           key->eth.cvlan.tci = qp->tci | htons(VLAN_TAG_PRESENT);
>>>           key->eth.cvlan.tpid = qp->eth_type;
>>>
>>>           __skb_pull(skb, sizeof(struct qtag_prefix));
>>>           return 1;
>>> }
>> This makes for cleaner code and certainly better for maintainability so I
>> have just implemented it for this next revision. However, note that with
>> this change, we incur the overhead of an additional function call for single
>> tagged vlan packets.
>>
> If there is any performance issue we can fix the code later.
>
>>> static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>>> {
>>>           struct qtag_prefix *qp = (struct qtag_prefix *)skb->data;
> ...
>
>>>> +               u64 mask_v_attrs = 0;
>>>> +
>>>> +               err = parse_flow_mask_nlattrs(*nla, a, &mask_v_attrs,
>>>> log);
>>>> +               if (err)
>>>> +                       return err;
>>>> +
>>>> +               if (mask_v_attrs & 1 << OVS_KEY_ATTR_ENCAP) {
>>>> +                       if (!*ie_valid) {
>>>> +                               OVS_NLERR(log, "Encap mask attribute is
>>>> set for non-CVLAN frame.");
>>>> +                               err = -EINVAL;
>>>> +                               return err;
>>>> +                       }
>>>> +                       encap = a[OVS_KEY_ATTR_ENCAP];
>>>> +
>>>> +                       err = cust_vlan_from_nlattrs(match, a, is_mask,
>>>> log);
>>>> +                       if (err)
>>>> +                               return err;
>>>> +                       *nla = encap;
>>>> +
>>> There is no checking for ATTR_VLAN or ATTR_ETHERTYPE. This can result
>>> in null pointer deference in cust_vlan_from_nlattrs().
>> The original vlan code does not check for these attribs in the masked case.
>> It does check for them in the non-masked case and then sets a boolean and
>> checks it in the masked case. I do the same thing for the inner vlan. I
>> check for the attributes in the non-masked case and set a boolean and check
>> the boolean in the masked case. Why is this not sufficient?
> Original code is checking for attributes before referencing them. For
> example  in function ovs_nla_get_match() before extracting eth_type,
> it does check a[OVS_KEY_ATTR_ETHERTYPE]. But If you spot bug in
> current code please send fix for net tree.
> Regarding the Boolean, it is for presence of inner vlan for key
> attribute, mask attribute still could be missing vlan attribute.
> For vlan mask, we can keep check sanity check as outer vlan. It means
> eth_type must be specified and should be 0xffff, and tci mask is
> optional and by default initialized to 0xffff.
You are correct. I was thinking of something else. I had this but must 
have lost it one of the patch revisions. Fixed it in V16.

--
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