* [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"
@ 2022-03-01 13:15 kailueke
2022-03-01 13:15 ` [PATCH 2/2] Revert "xfrm: state and policy should fail if XFRMA_IF_ID 0" kailueke
2022-03-01 13:28 ` [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error" Eyal Birger
0 siblings, 2 replies; 17+ messages in thread
From: kailueke @ 2022-03-01 13:15 UTC (permalink / raw)
To: netdev, Steffen Klassert, Eyal Birger
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 2/2] Revert "xfrm: state and policy should fail if XFRMA_IF_ID 0"
2022-03-01 13:15 [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error" kailueke
@ 2022-03-01 13:15 ` kailueke
2022-03-01 13:28 ` [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error" Eyal Birger
1 sibling, 0 replies; 17+ messages in thread
From: kailueke @ 2022-03-01 13:15 UTC (permalink / raw)
To: netdev, Steffen Klassert, Eyal Birger
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"
2022-03-01 13:15 [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error" kailueke
2022-03-01 13:15 ` [PATCH 2/2] Revert "xfrm: state and policy should fail if XFRMA_IF_ID 0" kailueke
@ 2022-03-01 13:28 ` Eyal Birger
2022-03-01 14:17 ` Kai Lüke
1 sibling, 1 reply; 17+ messages in thread
From: Eyal Birger @ 2022-03-01 13:28 UTC (permalink / raw)
To: kailueke; +Cc: netdev, Steffen Klassert
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"
2022-03-01 13:28 ` [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error" Eyal Birger
@ 2022-03-01 14:17 ` Kai Lüke
2022-03-01 14:34 ` Eyal Birger
0 siblings, 1 reply; 17+ messages in thread
From: Kai Lüke @ 2022-03-01 14:17 UTC (permalink / raw)
To: Eyal Birger; +Cc: netdev, Steffen Klassert
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"
2022-03-01 14:17 ` Kai Lüke
@ 2022-03-01 14:34 ` Eyal Birger
2022-03-01 15:09 ` Paul Chaignon
0 siblings, 1 reply; 17+ messages in thread
From: Eyal Birger @ 2022-03-01 14:34 UTC (permalink / raw)
To: Kai Lüke; +Cc: netdev, Steffen Klassert
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"
2022-03-01 14:34 ` Eyal Birger
@ 2022-03-01 15:09 ` Paul Chaignon
2022-03-01 15:48 ` Kai Lüke
0 siblings, 1 reply; 17+ messages in thread
From: Paul Chaignon @ 2022-03-01 15:09 UTC (permalink / raw)
To: Eyal Birger, kailueke; +Cc: netdev, Steffen Klassert
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"
2022-03-01 15:09 ` Paul Chaignon
@ 2022-03-01 15:48 ` Kai Lüke
2022-03-01 16:10 ` Steffen Klassert
0 siblings, 1 reply; 17+ messages in thread
From: Kai Lüke @ 2022-03-01 15:48 UTC (permalink / raw)
To: Paul Chaignon, Eyal Birger; +Cc: netdev, Steffen Klassert
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"
2022-03-01 15:48 ` Kai Lüke
@ 2022-03-01 16:10 ` Steffen Klassert
2022-03-01 16:44 ` Kai Lueke
2022-03-02 16:04 ` Jakub Kicinski
0 siblings, 2 replies; 17+ messages in thread
From: Steffen Klassert @ 2022-03-01 16:10 UTC (permalink / raw)
To: Kai Lüke; +Cc: Paul Chaignon, Eyal Birger, netdev
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"
2022-03-01 16:10 ` Steffen Klassert
@ 2022-03-01 16:44 ` Kai Lueke
2022-03-02 9:27 ` Nicolas Dichtel
2022-03-02 16:04 ` Jakub Kicinski
1 sibling, 1 reply; 17+ messages in thread
From: Kai Lueke @ 2022-03-01 16:44 UTC (permalink / raw)
To: Steffen Klassert; +Cc: Paul Chaignon, Eyal Birger, netdev
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)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"
2022-03-01 16:44 ` Kai Lueke
@ 2022-03-02 9:27 ` Nicolas Dichtel
0 siblings, 0 replies; 17+ messages in thread
From: Nicolas Dichtel @ 2022-03-02 9:27 UTC (permalink / raw)
To: Kai Lueke, Steffen Klassert; +Cc: Paul Chaignon, Eyal Birger, netdev
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"
2022-03-01 16:10 ` Steffen Klassert
2022-03-01 16:44 ` Kai Lueke
@ 2022-03-02 16:04 ` Jakub Kicinski
2022-03-02 18:11 ` Kai Lueke
1 sibling, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2022-03-02 16:04 UTC (permalink / raw)
To: Steffen Klassert; +Cc: Kai Lüke, Paul Chaignon, Eyal Birger, netdev
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"
2022-03-02 16:04 ` Jakub Kicinski
@ 2022-03-02 18:11 ` Kai Lueke
2022-03-03 5:33 ` Jakub Kicinski
0 siblings, 1 reply; 17+ messages in thread
From: Kai Lueke @ 2022-03-02 18:11 UTC (permalink / raw)
To: Jakub Kicinski, Steffen Klassert; +Cc: Paul Chaignon, Eyal Birger, netdev
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"
2022-03-02 18:11 ` Kai Lueke
@ 2022-03-03 5:33 ` Jakub Kicinski
2022-03-03 7:54 ` Steffen Klassert
0 siblings, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2022-03-03 5:33 UTC (permalink / raw)
To: Kai Lueke, Paul Chaignon; +Cc: Steffen Klassert, Eyal Birger, netdev
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?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"
2022-03-03 5:33 ` Jakub Kicinski
@ 2022-03-03 7:54 ` Steffen Klassert
0 siblings, 0 replies; 17+ messages in thread
From: Steffen Klassert @ 2022-03-03 7:54 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Kai Lueke, Paul Chaignon, Eyal Birger, netdev
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"
@ 2022-02-28 18:55 Kai Lüke
0 siblings, 0 replies; 17+ messages in thread
From: Kai Lüke @ 2022-02-28 18:55 UTC (permalink / raw)
To: netdev, Steffen Klassert, Eyal Birger; +Cc: Kai Lueke
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"
@ 2022-02-28 18:51 Kai Lüke
0 siblings, 0 replies; 17+ messages in thread
From: Kai Lüke @ 2022-02-28 18:51 UTC (permalink / raw)
To: netdev, Steffen Klassert, Eyal Birger; +Cc: Kai Lueke
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error"
@ 2022-02-28 18:49 Kai Lüke
0 siblings, 0 replies; 17+ messages in thread
From: Kai Lüke @ 2022-02-28 18:49 UTC (permalink / raw)
To: netdev, Steffen Klassert, Eyal Birger; +Cc: Kai Lueke
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-03-03 7:54 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-01 13:15 [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error" kailueke
2022-03-01 13:15 ` [PATCH 2/2] Revert "xfrm: state and policy should fail if XFRMA_IF_ID 0" kailueke
2022-03-01 13:28 ` [PATCH 1/2] Revert "xfrm: interface with if_id 0 should return error" Eyal Birger
2022-03-01 14:17 ` Kai Lüke
2022-03-01 14:34 ` Eyal Birger
2022-03-01 15:09 ` Paul Chaignon
2022-03-01 15:48 ` Kai Lüke
2022-03-01 16:10 ` Steffen Klassert
2022-03-01 16:44 ` Kai Lueke
2022-03-02 9:27 ` Nicolas Dichtel
2022-03-02 16:04 ` Jakub Kicinski
2022-03-02 18:11 ` Kai Lueke
2022-03-03 5:33 ` Jakub Kicinski
2022-03-03 7:54 ` Steffen Klassert
-- strict thread matches above, loose matches on Subject: below --
2022-02-28 18:55 Kai Lüke
2022-02-28 18:51 Kai Lüke
2022-02-28 18:49 Kai Lüke
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.