2018-11-29, 07:33:11 -0800, Roopa Prabhu wrote:

On Thu, Nov 29, 2018 at 5:56 AM Sabrina Dubroca [off-list ref] wrote:

quoted

2018-11-28, 14:27:57 -0800, Roopa Prabhu wrote:

quoted

From: Roopa Prabhu <redacted>

We started very conservative when supporting changelink
especially because not all attribute changes could be
tested. This patch opens up a few more attributes for
changelink. The reason for choosing this set of attributes
is based on code references for these attributes. I have
tested TTL changes and did some changelink api testing
to sanity test the others.

Signed-off-by: Roopa Prabhu <redacted>
---
 drivers/net/vxlan.c | 36 ++++--------------------------------
 1 file changed, 4 insertions(+), 32 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 9110662..73caa65 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c

@@ -3438,11 +3438,8 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
      if (data[IFLA_VXLAN_TTL])
              conf->ttl = nla_get_u8(data[IFLA_VXLAN_TTL]);

-     if (data[IFLA_VXLAN_TTL_INHERIT]) {
-             if (changelink)
-                     return -EOPNOTSUPP;
+     if (data[IFLA_VXLAN_TTL_INHERIT])
              conf->flags |= VXLAN_F_TTL_INHERIT;
-     }

This doesn't give us an option to disable TTL_INHERIT after it was
enabled once. Same thing with GBP, GPE, REMCSUM_NOPARTIAL.

that is provided by patch3, with the changelink patch. I can squash
them, if thats easier.

Yes, I see it now. Sequential review got me :(
I think you can just add a note in the commit message, "will be fixed
in a further patch introducing the vxlan_nl2flag() helper" or similar.

quoted

quoted

-     if (data[IFLA_VXLAN_GBP]) {
-             if (changelink)
-                     return -EOPNOTSUPP;
+     if (data[IFLA_VXLAN_GBP])
              conf->flags |= VXLAN_F_GBP;
-     }

-     if (data[IFLA_VXLAN_GPE]) {
-             if (changelink)
-                     return -EOPNOTSUPP;
+     if (data[IFLA_VXLAN_GPE])
              conf->flags |= VXLAN_F_GPE;
-     }

GPE implies running a different setup function (vxlan_raw_setup() vs
vxlan_ether_setup()), that vxlan_config_apply() only calls for
!changelink. I think this is incomplete.

I think we'd also end up with mixed tunnel types (GPE/!GPE) on the
same socket, I'm not sure how that would work. Normally, they each try
to create a separate socket, and pass the GPE flag on to the
associated vxlan_sock. I suspect that's also a problem with rx
offload.

that is good to know. I will drop the change to GPE and also the rx
offload flag and let somebody else using it
open it up for changelink. thanks for the review

Looking at vxlan_rcv(), there's also a possible issue with the GBP
flag:

	if (vs->flags & VXLAN_F_GBP)
		vxlan_parse_gbp_hdr(&unparsed, skb, vs->flags, md);

I don't know why that flag must match on all tunnels for a socket, but
given that it was the reason for commit ac5132d1a03f ("vxlan: Only
bind to sockets with compatible flags enabled"), I'd leave it alone as
well.

quoted

quoted

-     if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL]) {
-             if (changelink)
-                     return -EOPNOTSUPP;
+     if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL])
              conf->flags |= VXLAN_F_REMCSUM_NOPARTIAL;
-     }

      if (tb[IFLA_MTU]) {
              if (changelink)
--

-- 
Sabrina