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

[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

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

[Eyal Birger]

Hi Kai,

On Tue, Mar 1, 2022 at 3:15 PM <kailueke@linux.microsoft.com> wrote:
>
> 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>

From the pull request you mentioned I understand the commit which affected
Cilium userspace is 68ac0f3810e7
("xfrm: state and policy should fail if XFRMA_IF_ID 0").

Whereas 8dce43919566 ("xfrm: interface with if_id 0 should return error")
involves xfrm interfaces which don't appear in the pull request.

In which case, why should that commit be reverted?

Eyal.

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

[Kai Lüke]

Hi,
> Whereas 8dce43919566 ("xfrm: interface with if_id 0 should return error")
> involves xfrm interfaces which don't appear in the pull request.
>
> In which case, why should that commit be reverted?

Correct me if I misunderstood this but reading the commit message it is
explicitly labeled as a behavior change for userspace:

    With this commit:
     ip link add ipsec0  type xfrm dev lo  if_id 0
     Error: if_id must be non zero.

Changing behavior this way is from my understanding a regression because
it breaks programs that happened to work before, even if they worked
incorrect (cf. https://lwn.net/Articles/726021/ "The current process for
Linux development says that kernel patches cannot break programs that
rely on the ABI. That means a program that runs on the 4.0 kernel should
be able to run on the 5.0 kernel, Levin said.").

Regards,
Kai

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

[Eyal Birger]

Hi Kai,

On Tue, Mar 1, 2022 at 4:17 PM Kai Lüke <kailueke@linux.microsoft.com> wrote:
>
> Hi,
> > Whereas 8dce43919566 ("xfrm: interface with if_id 0 should return error")
> > involves xfrm interfaces which don't appear in the pull request.
> >
> > In which case, why should that commit be reverted?
>
> Correct me if I misunderstood this but reading the commit message it is
> explicitly labeled as a behavior change for userspace:
>
>     With this commit:
>      ip link add ipsec0  type xfrm dev lo  if_id 0
>      Error: if_id must be non zero.
>
> Changing behavior this way is from my understanding a regression because
> it breaks programs that happened to work before, even if they worked
> incorrect (cf. https://lwn.net/Articles/726021/ "The current process for
> Linux development says that kernel patches cannot break programs that
> rely on the ABI. That means a program that runs on the 4.0 kernel should
> be able to run on the 5.0 kernel, Levin said.").

Well to some extent, but the point was that xfrm interfaces with if_id=0
were already broken, so returning an error to userspace in such case
would be a better behavior.
So I'm not sure this is a regression but it's not up to me to decide these
things.

Eyal

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

[Paul Chaignon]

On Tue, Mar 01, 2022 at 04:34:52PM +0200, Eyal Birger wrote:
> Hi Kai,
> 
> On Tue, Mar 1, 2022 at 4:17 PM Kai Lüke <kailueke@linux.microsoft.com> wrote:
> >
> > Hi,
> > > Whereas 8dce43919566 ("xfrm: interface with if_id 0 should return error")
> > > involves xfrm interfaces which don't appear in the pull request.
> > >
> > > In which case, why should that commit be reverted?
> >
> > Correct me if I misunderstood this but reading the commit message it is
> > explicitly labeled as a behavior change for userspace:
> >
> >     With this commit:
> >      ip link add ipsec0  type xfrm dev lo  if_id 0
> >      Error: if_id must be non zero.
> >
> > Changing behavior this way is from my understanding a regression because
> > it breaks programs that happened to work before, even if they worked
> > incorrect (cf. https://lwn.net/Articles/726021/ "The current process for
> > Linux development says that kernel patches cannot break programs that
> > rely on the ABI. That means a program that runs on the 4.0 kernel should
> > be able to run on the 5.0 kernel, Levin said.").
> 
> Well to some extent, but the point was that xfrm interfaces with if_id=0
> were already broken, so returning an error to userspace in such case
> would be a better behavior.
> So I'm not sure this is a regression but it's not up to me to decide these
> things.

I agree with Eyal here.  As far as Cilium is concerned, this is not
causing any regression.  Only the second commit, 68ac0f3810e7 ("xfrm:
state and policy should fail if XFRMA_IF_ID 0") causes issues in a
previously-working setup in Cilium.  We don't use xfrm interfaces.

Paul

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

[Kai Lüke]

> I agree with Eyal here.  As far as Cilium is concerned, this is not
> causing any regression.  Only the second commit, 68ac0f3810e7 ("xfrm:
> state and policy should fail if XFRMA_IF_ID 0") causes issues in a
> previously-working setup in Cilium.  We don't use xfrm interfaces.
>
I see this as a very generic question of changing userspace behavior or
not, regardless if we know how many users are affected, and from what I
know there are similar cases in the kernel where the response was that
breaking userspace is a no go - even if the intention was to be helpful
by having early errors.

Greets,
Kai

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

[Steffen Klassert]

On Tue, Mar 01, 2022 at 04:48:38PM +0100, Kai Lüke wrote:
> > I agree with Eyal here.  As far as Cilium is concerned, this is not
> > causing any regression.  Only the second commit, 68ac0f3810e7 ("xfrm:
> > state and policy should fail if XFRMA_IF_ID 0") causes issues in a
> > previously-working setup in Cilium.  We don't use xfrm interfaces.
> >
> I see this as a very generic question of changing userspace behavior or
> not, regardless if we know how many users are affected, and from what I
> know there are similar cases in the kernel where the response was that
> breaking userspace is a no go - even if the intention was to be helpful
> by having early errors.

In general I agree that the userspace ABI has to be stable, but
this never worked. We changed the behaviour from silently broken to
notify userspace about a misconfiguration.

It is the question what is more annoying for the users. A bug that
we can never fix, or changing a broken behaviour to something that
tells you at least why it is not working.

In such a case we should gauge what's the better solution. Here
I tend to keep it as it is.

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

[Kai Lueke]

Hi,
> In general I agree that the userspace ABI has to be stable, but
> this never worked. We changed the behaviour from silently broken to
> notify userspace about a misconfiguration.
>
> It is the question what is more annoying for the users. A bug that
> we can never fix, or changing a broken behaviour to something that
> tells you at least why it is not working.
>
> In such a case we should gauge what's the better solution. Here
> I tend to keep it as it is.

alternatives are: docs to ensure the API is used the right way, maybe a
dmesg log entry if wrong usage is detected, and filing bugs where the
API is used wrong.

The chosen way led to having this change being introduced as part of an
LTS kernel bugfix update, breaking user's clusters:
https://github.com/flatcar-linux/Flatcar/issues/626

Please rethink how you want to handle this, also for not making a
precedent here so that this repeats.

Regards,
Kai (Flatcar Container Linux team)

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

[Nicolas Dichtel]

Le 01/03/2022 à 17:44, Kai Lueke a écrit :
> Hi,
>> In general I agree that the userspace ABI has to be stable, but
>> this never worked. We changed the behaviour from silently broken to
>> notify userspace about a misconfiguration.
>>
>> It is the question what is more annoying for the users. A bug that
>> we can never fix, or changing a broken behaviour to something that
>> tells you at least why it is not working.
>>
>> In such a case we should gauge what's the better solution. Here
>> I tend to keep it as it is.
> 
> alternatives are: docs to ensure the API is used the right way, maybe a
> dmesg log entry if wrong usage is detected, and filing bugs where the
> API is used wrong.
I agree with that proposal (dmesg log). Breaking an existing script, even if it
made something wrong is really painful. And maybe this broken xfrm interface was
unused, so everything worked well before the patch and is now broken.

My two cents,
Nicolas

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

[Jakub Kicinski]

On Tue, 1 Mar 2022 17:10:01 +0100 Steffen Klassert wrote:
> > I see this as a very generic question of changing userspace behavior or
> > not, regardless if we know how many users are affected, and from what I
> > know there are similar cases in the kernel where the response was that
> > breaking userspace is a no go - even if the intention was to be helpful
> > by having early errors.  
> 
> In general I agree that the userspace ABI has to be stable, but
> this never worked. We changed the behaviour from silently broken to
> notify userspace about a misconfiguration.
> 
> It is the question what is more annoying for the users. A bug that
> we can never fix, or changing a broken behaviour to something that
> tells you at least why it is not working.
> 
> In such a case we should gauge what's the better solution. Here
> I tend to keep it as it is.

Agreed. FWIW would be great if patch #2 started flowing towards Linus'es
tree separately if the discussion on #1 is taking longer.

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

[Kai Lueke]

Hi,
> Agreed. FWIW would be great if patch #2 started flowing towards Linus'es
> tree separately if the discussion on #1 is taking longer.

to preserve the initial goal of helping to uncover id 0 usage I think it
would be best to have the revert be accompanied by a patch that instead
creates a kernel log warning (or whatever).

Since I never did that I suggest to not wait for me.
Also, feel free to do the revert yourself with a different commit
message if mine didn't capture the things appropriately.

Regards,
Kai

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

[Jakub Kicinski]

On Wed, 2 Mar 2022 19:11:06 +0100 Kai Lueke wrote:
> > Agreed. FWIW would be great if patch #2 started flowing towards Linus'es
> > tree separately if the discussion on #1 is taking longer.  
> 
> to preserve the initial goal of helping to uncover id 0 usage I think it
> would be best to have the revert be accompanied by a patch that instead
> creates a kernel log warning (or whatever).

extack would be best, but that would mean a little bit of plumbing 
so more likely net-next material. Which would have to come after.

> Since I never did that I suggest to not wait for me.
> Also, feel free to do the revert yourself with a different commit
> message if mine didn't capture the things appropriately.

TBH I'm not 100% clear on the nature of the regression. Does Cilium
update the configuration later to make if_id be non-zero? Or the broken 
interface is not used but not being able to create it fails the whole
configuration?

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

[Steffen Klassert]

On Wed, Mar 02, 2022 at 09:33:49PM -0800, Jakub Kicinski wrote:
> On Wed, 2 Mar 2022 19:11:06 +0100 Kai Lueke wrote:
> > > Agreed. FWIW would be great if patch #2 started flowing towards Linus'es
> > > tree separately if the discussion on #1 is taking longer.  
> > 
> > to preserve the initial goal of helping to uncover id 0 usage I think it
> > would be best to have the revert be accompanied by a patch that instead
> > creates a kernel log warning (or whatever).
> 
> extack would be best, but that would mean a little bit of plumbing 
> so more likely net-next material. Which would have to come after.

We need something that a user directly sees that the configuration is
invalid. That the xfrm interface acts as a blackhole whith IF_ID 0
is one thing (revert #1).

The other thing is that IF_ID 0 for a policy/state internally means
that these are not assigned to a xfrm interface. So a policy/state
with IF_ID 0 will match flows that are not targeted to the xfrm
interface. This means that confidential stuff could be sent to the
wrong peer (revert #2).

> 
> > Since I never did that I suggest to not wait for me.
> > Also, feel free to do the revert yourself with a different commit
> > message if mine didn't capture the things appropriately.
> 
> TBH I'm not 100% clear on the nature of the regression. Does Cilium
> update the configuration later to make if_id be non-zero? Or the broken 
> interface is not used but not being able to create it fails the whole
> configuration?

As far I unerstood Pauls mail, only 68ac0f3810e7 ("xfrm: state and policy
should fail if XFRMA_IF_ID 0") caused issues in Cilium.

So maybe just revert that one and document that IF_ID 0 on a policy/state
is the same as configuring the policy/state without IF_ID.

[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 <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.35.1

[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 1/2] Revert "xfrm: interface with if_id 0 should return error"

[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