[PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"

[PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"

[Kai Lüke]

This reverts commit 8dce43919566f06e865f7e8949f5c10d8c2493f5 because it
breaks userspace (e.g., Cilium is affected because it used id 0 for the
dummy state https://github.com/cilium/cilium/pull/18789).

Signed-off-by: Kai Lueke <kailuke@microsoft.com>
---
 net/xfrm/xfrm_interface.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
index 57448fc519fc..41de46b5ffa9 100644
--- a/net/xfrm/xfrm_interface.c
+++ b/net/xfrm/xfrm_interface.c
@@ -637,16 +637,11 @@ static int xfrmi_newlink(struct net *src_net,
struct net_device *dev,
             struct netlink_ext_ack *extack)
 {
     struct net *net = dev_net(dev);
-    struct xfrm_if_parms p = {};
+    struct xfrm_if_parms p;
     struct xfrm_if *xi;
     int err;
 
     xfrmi_netlink_parms(data, &p);
-    if (!p.if_id) {
-        NL_SET_ERR_MSG(extack, "if_id must be non zero");
-        return -EINVAL;
-    }
-
     xi = xfrmi_locate(net, &p);
     if (xi)
         return -EEXIST;
@@ -671,12 +666,7 @@ static int xfrmi_changelink(struct net_device *dev,
struct nlattr *tb[],
 {
     struct xfrm_if *xi = netdev_priv(dev);
     struct net *net = xi->net;
-    struct xfrm_if_parms p = {};
-
-    if (!p.if_id) {
-        NL_SET_ERR_MSG(extack, "if_id must be non zero");
-        return -EINVAL;
-    }
+    struct xfrm_if_parms p;
 
     xfrmi_netlink_parms(data, &p);
     xi = xfrmi_locate(net, &p);
-- 
2.35.1

[PATCH 2/2] Revert "xfrm: state and policy should fail if XFRMA_IF_ID 0"

[Kai Lüke]

This reverts commit 68ac0f3810e76a853b5f7b90601a05c3048b8b54 because it
breaks userspace (e.g., Cilium is affected because it used id 0 for the
dummy state https://github.com/cilium/cilium/pull/18789).

Signed-off-by: Kai Lueke <kailueke@linux.microsoft.com>
---
 net/xfrm/xfrm_user.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 8cd6c8129004..be89a8ac54a4 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -630,13 +630,8 @@ static struct xfrm_state
*xfrm_state_construct(struct net *net,
 
     xfrm_smark_init(attrs, &x->props.smark);
 
-    if (attrs[XFRMA_IF_ID]) {
+    if (attrs[XFRMA_IF_ID])
         x->if_id = nla_get_u32(attrs[XFRMA_IF_ID]);
-        if (!x->if_id) {
-            err = -EINVAL;
-            goto error;
-        }
-    }
 
     err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV]);
     if (err)
@@ -1432,13 +1427,8 @@ static int xfrm_alloc_userspi(struct sk_buff
*skb, struct nlmsghdr *nlh,
 
     mark = xfrm_mark_get(attrs, &m);
 
-    if (attrs[XFRMA_IF_ID]) {
+    if (attrs[XFRMA_IF_ID])
         if_id = nla_get_u32(attrs[XFRMA_IF_ID]);
-        if (!if_id) {
-            err = -EINVAL;
-            goto out_noput;
-        }
-    }
 
     if (p->info.seq) {
         x = xfrm_find_acq_byseq(net, mark, p->info.seq);
@@ -1751,13 +1741,8 @@ static struct xfrm_policy
*xfrm_policy_construct(struct net *net, struct xfrm_us
 
     xfrm_mark_get(attrs, &xp->mark);
 
-    if (attrs[XFRMA_IF_ID]) {
+    if (attrs[XFRMA_IF_ID])
         xp->if_id = nla_get_u32(attrs[XFRMA_IF_ID]);
-        if (!xp->if_id) {
-            err = -EINVAL;
-            goto error;
-        }
-    }
 
     return xp;
  error:
-- 
2.35.1

Re: [PATCH 2/2] Revert "xfrm: state and policy should fail if XFRMA_IF_ID 0"

[Jakub Kicinski]

On Mon, 28 Feb 2022 19:53:59 +0100 Kai Lüke wrote:
> This reverts commit 68ac0f3810e76a853b5f7b90601a05c3048b8b54 because it
> breaks userspace (e.g., Cilium is affected because it used id 0 for the
> dummy state https://github.com/cilium/cilium/pull/18789).
> 
> Signed-off-by: Kai Lueke <kailueke@linux.microsoft.com>

What's the story here? You posted your patches twice and they look
white space damaged (tabs replaced by spaces). Does commit 6d0d95a1c2b0
("xfrm: fix the if_id check in changelink") which is in net/master now
solve the issue for you?

Re: [PATCH 2/2] Revert "xfrm: state and policy should fail if XFRMA_IF_ID 0"

[Kai Lüke]

Hello,
> What's the story here? You posted your patches twice and they look
> white space damaged (tabs replaced by spaces).
yeah, sorry for the spam, now I've set up git send-email and hope it
works ;)
> Does commit 6d0d95a1c2b0
> ("xfrm: fix the if_id check in changelink") which is in net/master now
> solve the issue for you?

I don't think it solves the regression that the kernel now fails
syscalls that use id 0. I reported this to the LTS kernel list and was
told to send it here since it is a regression.

Regards,
Kai

[PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"

[kailueke]

From: Kai Lueke <kailueke@linux.microsoft.com>

This reverts commit 8dce43919566f06e865f7e8949f5c10d8c2493f5 because it
breaks userspace (e.g., Cilium is affected because it used id 0 for the
dummy state https://github.com/cilium/cilium/pull/18789).

Signed-off-by: Kai Lueke <kailueke@linux.microsoft.com>
---
 net/xfrm/xfrm_interface.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
index 57448fc519fc..41de46b5ffa9 100644
--- a/net/xfrm/xfrm_interface.c
+++ b/net/xfrm/xfrm_interface.c
@@ -637,16 +637,11 @@ static int xfrmi_newlink(struct net *src_net, struct net_device *dev,
 			struct netlink_ext_ack *extack)
 {
 	struct net *net = dev_net(dev);
-	struct xfrm_if_parms p = {};
+	struct xfrm_if_parms p;
 	struct xfrm_if *xi;
 	int err;
 
 	xfrmi_netlink_parms(data, &p);
-	if (!p.if_id) {
-		NL_SET_ERR_MSG(extack, "if_id must be non zero");
-		return -EINVAL;
-	}
-
 	xi = xfrmi_locate(net, &p);
 	if (xi)
 		return -EEXIST;
@@ -671,12 +666,7 @@ static int xfrmi_changelink(struct net_device *dev, struct nlattr *tb[],
 {
 	struct xfrm_if *xi = netdev_priv(dev);
 	struct net *net = xi->net;
-	struct xfrm_if_parms p = {};
-
-	if (!p.if_id) {
-		NL_SET_ERR_MSG(extack, "if_id must be non zero");
-		return -EINVAL;
-	}
+	struct xfrm_if_parms p;
 
 	xfrmi_netlink_parms(data, &p);
 	xi = xfrmi_locate(net, &p);
-- 
2.25.1

[PATCH 2/2] Revert "xfrm: state and policy should fail if XFRMA_IF_ID 0"

[kailueke]

From: Kai Lueke <kailueke@linux.microsoft.com>

This reverts commit 68ac0f3810e76a853b5f7b90601a05c3048b8b54 because it
breaks userspace (e.g., Cilium is affected because it used id 0 for the
dummy state https://github.com/cilium/cilium/pull/18789).

Signed-off-by: Kai Lueke <kailueke@linux.microsoft.com>
---
 net/xfrm/xfrm_user.c | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 8cd6c8129004..be89a8ac54a4 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -630,13 +630,8 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
 
 	xfrm_smark_init(attrs, &x->props.smark);
 
-	if (attrs[XFRMA_IF_ID]) {
+	if (attrs[XFRMA_IF_ID])
 		x->if_id = nla_get_u32(attrs[XFRMA_IF_ID]);
-		if (!x->if_id) {
-			err = -EINVAL;
-			goto error;
-		}
-	}
 
 	err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV]);
 	if (err)
@@ -1432,13 +1427,8 @@ static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	mark = xfrm_mark_get(attrs, &m);
 
-	if (attrs[XFRMA_IF_ID]) {
+	if (attrs[XFRMA_IF_ID])
 		if_id = nla_get_u32(attrs[XFRMA_IF_ID]);
-		if (!if_id) {
-			err = -EINVAL;
-			goto out_noput;
-		}
-	}
 
 	if (p->info.seq) {
 		x = xfrm_find_acq_byseq(net, mark, p->info.seq);
@@ -1751,13 +1741,8 @@ static struct xfrm_policy *xfrm_policy_construct(struct net *net, struct xfrm_us
 
 	xfrm_mark_get(attrs, &xp->mark);
 
-	if (attrs[XFRMA_IF_ID]) {
+	if (attrs[XFRMA_IF_ID])
 		xp->if_id = nla_get_u32(attrs[XFRMA_IF_ID]);
-		if (!xp->if_id) {
-			err = -EINVAL;
-			goto error;
-		}
-	}
 
 	return xp;
  error:
-- 
2.25.1