Re: [ovs-dev] [PATCH net-next v3 1/6] vxlan: refactor verification and application of configuration

[Johannes Berg]

On Fri, 2017-06-23 at 12:13 +0200, Matthias Schiffer wrote:
> 
> I was told the extended netlink error facilities were not ready yet,
> has that changed since the last release?

Yes, the facility is in the kernel tree now.

> Anyways, I will gladly work on improving the error handling if
> someone can give me a pointer how these extended netlink errors are
> used.

Just grep for 'netlink_ext_ack' :)

johannes
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH net-next v3 1/6] vxlan: refactor verification and application of configuration

[Matthias Schiffer]

On 06/23/2017 10:52 AM, Jiri Benc wrote:
> This patchset looks good overall (would send my Acked-by for most of
> this but I'm late).
> 
> On Mon, 19 Jun 2017 10:03:55 +0200, Matthias Schiffer wrote:
>> Log messages in these
>> functions are removed, as it is generally unexpected to find error output
>> for netlink requests in the kernel log. Userspace should be able to handle
>> errors based on the error codes returned via netlink just fine.
> 
> However, this is not really true. It's impossible to find out what went
> wrong when you use e.g. iproute2 to configure a vxlan link.
> 
> We really need to convert the kernel log messages to the extended
> netlink errors. Since you removed them prematurely, could you please
> work on that?
> 
> Thanks,
> 
>  Jiri
> 

I was told the extended netlink error facilities were not ready yet, has
that changed since the last release?

Off the top of my head, I can't think of any other setting I can do with
iproute2 that will write its errors in the kernel log; but there are quite
a lot settings that will just return a very unspecific error code. Isn't it
more common for the userspace tool to handle diagnostics in such cases?

Anyways, I will gladly work on improving the error handling if someone can
give me a pointer how these extended netlink errors are used.

Matthias

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH net-next v3 1/6] vxlan: refactor verification and application of configuration

[Jiri Benc]

This patchset looks good overall (would send my Acked-by for most of
this but I'm late).

On Mon, 19 Jun 2017 10:03:55 +0200, Matthias Schiffer wrote:
> Log messages in these
> functions are removed, as it is generally unexpected to find error output
> for netlink requests in the kernel log. Userspace should be able to handle
> errors based on the error codes returned via netlink just fine.

However, this is not really true. It's impossible to find out what went
wrong when you use e.g. iproute2 to configure a vxlan link.

We really need to convert the kernel log messages to the extended
netlink errors. Since you removed them prematurely, could you please
work on that?

Thanks,

 Jiri
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH net-next v3 1/6] vxlan: refactor verification and application of configuration

[Matthias Schiffer]

The vxlan_dev_configure function was mixing validation and application of
the vxlan configuration; this could easily lead to bugs with the changelink
operation, as it was hard to see if the function wcould return an error
after parts of the configuration had already been applied.

This commit splits validation and application out of vxlan_dev_configure as
separate functions to make it clearer where error returns are allowed and
where the vxlan_dev or net_device may be configured. Log messages in these
functions are removed, as it is generally unexpected to find error output
for netlink requests in the kernel log. Userspace should be able to handle
errors based on the error codes returned via netlink just fine.

In addition, some validation and initialization is moved to vxlan_validate
and vxlan_setup respectively to improve grouping of similar settings.

Finally, this also fixes two actual bugs:

* if set, conf->mtu would overwrite dev->mtu in each changelink operation,
  reverting other changes of dev->mtu
* the "if (!conf->dst_port)" branch would never be run, as conf->dst_port
  was set in vxlan_setup before. This caused VXLAN-GPE to use the same
  default port as other VXLAN sockets instead of the intended IANA-assigned
  4790.

Signed-off-by: Matthias Schiffer 
---

Notes:
v2: new patch
v3: remove kernel log messages

 drivers/net/vxlan.c | 208 
 1 file changed, 111 insertions(+), 97 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 94ce98229828..9139f15a2ec1 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -2623,6 +2623,10 @@ static void vxlan_setup(struct net_device *dev)
netif_keep_dst(dev);
dev->priv_flags |= IFF_NO_QUEUE;
 
+   /* MTU range: 68 - 65535 */
+   dev->min_mtu = ETH_MIN_MTU;
+   dev->max_mtu = ETH_MAX_MTU;
+
INIT_LIST_HEAD(>next);
spin_lock_init(>hash_lock);
 
@@ -2630,9 +2634,8 @@ static void vxlan_setup(struct net_device *dev)
vxlan->age_timer.function = vxlan_cleanup;
vxlan->age_timer.data = (unsigned long) vxlan;
 
-   vxlan->cfg.dst_port = htons(vxlan_port);
-
vxlan->dev = dev;
+   vxlan->net = dev_net(dev);
 
gro_cells_init(>gro_cells, dev);
 
@@ -2701,11 +2704,19 @@ static int vxlan_validate(struct nlattr *tb[], struct 
nlattr *data[])
}
}
 
+   if (tb[IFLA_MTU]) {
+   u32 mtu = nla_get_u32(data[IFLA_MTU]);
+
+   if (mtu < ETH_MIN_MTU || mtu > ETH_MAX_MTU)
+   return -EINVAL;
+   }
+
if (!data)
return -EINVAL;
 
if (data[IFLA_VXLAN_ID]) {
-   __u32 id = nla_get_u32(data[IFLA_VXLAN_ID]);
+   u32 id = nla_get_u32(data[IFLA_VXLAN_ID]);
+
if (id >= VXLAN_N_VID)
return -ERANGE;
}
@@ -2866,116 +2877,128 @@ static int vxlan_sock_add(struct vxlan_dev *vxlan)
return ret;
 }
 
-static int vxlan_dev_configure(struct net *src_net, struct net_device *dev,
-  struct vxlan_config *conf,
-  bool changelink)
+static int vxlan_config_validate(struct net *src_net, struct vxlan_config 
*conf,
+struct net_device **lower,
+struct vxlan_dev *old)
 {
struct vxlan_net *vn = net_generic(src_net, vxlan_net_id);
-   struct vxlan_dev *vxlan = netdev_priv(dev), *tmp;
-   struct vxlan_rdst *dst = >default_dst;
-   unsigned short needed_headroom = ETH_HLEN;
+   struct vxlan_dev *tmp;
bool use_ipv6 = false;
-   __be16 default_port = vxlan->cfg.dst_port;
-   struct net_device *lowerdev = NULL;
 
-   if (!changelink) {
-   if (conf->flags & VXLAN_F_GPE) {
-   /* For now, allow GPE only together with
-* COLLECT_METADATA. This can be relaxed later; in such
-* case, the other side of the PtP link will have to be
-* provided.
-*/
-   if ((conf->flags & ~VXLAN_F_ALLOWED_GPE) ||
-   !(conf->flags & VXLAN_F_COLLECT_METADATA)) {
-   pr_info("unsupported combination of 
extensions\n");
-   return -EINVAL;
-   }
-   vxlan_raw_setup(dev);
-   } else {
-   vxlan_ether_setup(dev);
+   if (conf->flags & VXLAN_F_GPE) {
+   /* For now, allow GPE only together with
+* COLLECT_METADATA. This can be relaxed later; in such
+* case, the other side of the PtP link will have to be
+* provided.
+*/
+   if ((conf->flags & ~VXLAN_F_ALLOWED_GPE) ||
+   !(conf->flags &