netdev - Re: [PATCH net-next v2] vxlan: change vxlan_[config_]validate() to use netlink_ext

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 11 Aug 2017 10:21:12 -0700
From:   Girish Moodalbail <girish.moodalbail@...cle.com>
To:     Jiri Benc <jbenc@...hat.com>,
        Roopa Prabhu <roopa@...ulusnetworks.com>
Cc:     pravin shelar <pshelar@....org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Matthias Schiffer <mschiffer@...verse-factory.net>
Subject: Re: [PATCH net-next v2] vxlan: change vxlan_[config_]validate() to
 use netlink_ext_ack for error reporting

[snip]

>>> @@ -2933,6 +2945,8 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf,
>>>                   */
>>>                  if ((conf->flags & ~VXLAN_F_ALLOWED_GPE) ||
>>>                      !(conf->flags & VXLAN_F_COLLECT_METADATA)) {
>>> +                       NL_SET_ERR_MSG(extack,
>>> +                                      "VXLAN GPE does not support this combination of attributes");
>>>                          return -EINVAL;
>>>                  }
>>
>> "collect metadata not supported with vxlan gpe"

Actually, VXLAN GPE is only supported together with external keyword. From 
ip-link(8)...

       gpe - enables the Generic Protocol extension (VXLAN-GPE). Currently, this
            is only supported together with the external keyword.

> That's completely wrong message. Not saying that the capitalization is
> wrong, too. Girish's wording precisely explains what went wrong.

Furthermore, the condition not only checks for collect metadata but also it 
checks to see if any attribute specified is outside of what is allowed by 
VXLAN_F_ALLOWED_GPE.

> 
>>>                  lowerdev = __dev_get_by_index(src_net, conf->remote_ifindex);
>>> -               if (!lowerdev)
>>> +               if (!lowerdev) {
>>> +                       NL_SET_ERR_MSG(extack,
>>> +                                      "Specified interface for tunnel endpoint communications not found");
>>>                          return -ENODEV;
>>
>> "invalid vxlan remote link interface, device not found"
> 
> Finally one that looks better :-) Modulo the missing capitalization,
> though.

It is not the remote link interface. It is the local interface itself that needs 
to be specified.

> 
>>> -               if (vxlan_addr_multicast(&conf->remote_ip))
>>> +               if (vxlan_addr_multicast(&conf->remote_ip)) {
>>> +                       NL_SET_ERR_MSG(extack,
>>> +                                      "Interface need to be specified for multicast destination");
>>
>> "vxlan remote link interface required for multicast remote destination"
> 
> I like this one better, too.
> 
>>>   #if IS_ENABLED(CONFIG_IPV6)
>>> -               if (conf->flags & VXLAN_F_IPV6_LINKLOCAL)
>>> +               if (conf->flags & VXLAN_F_IPV6_LINKLOCAL) {
>>> +                       NL_SET_ERR_MSG(extack,
>>> +                                      "Interface need to be specified for link-local local/remote addresses");
>>>                          return -EINVAL;
>>
>> "vxlan link interface required for link-local local/remote addresses"
> 
> Okay but to be consistent (and more clear), it should be "remote link
> interface".

There is no remote link while configuring VXLAN. It is the local interface 
through which VXLAN packets will be send out.

Thanks all for the review,
~Girish