Re: [net] Revert "net: core: maybe return -EEXIST in __dev_alloc_name"

Hi Johannes,

From: Johannes Berg <redacted>

This reverts commit d6f295e9def0; some userspace (in the case
we noticed it's wpa_supplicant), is relying on the current
error code to determine that a fixed name interface already
exists.

Reported-by: Jouni Malinen <redacted>
Signed-off-by: Johannes Berg <redacted>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

This revert seems to have broken networking on one of my powerpc
machines, according to git bisect.

The symptom is DHCP fails and I don't get a link, I didn't dig any
further than that. I can if it's helpful.

I think the problem is that 87c320e51519 ("net: core: dev_get_valid_name
is now the same as dev_alloc_name_ns") only makes sense while
d6f295e9def0 remains in the tree.

ie. before the entire series, dev_get_valid_name() would return EEXIST,
and that was retained when 87c320e51519 was merged, but now that
d6f295e9def0 has been reverted dev_get_valid_name() is returning ENFILE.

I can get the network up again if I also revert 87c320e51519 ("net:
core: dev_get_valid_name is now the same as dev_alloc_name_ns"), or with
the gross patch below.

cheers

diff --git a/net/core/dev.c b/net/core/dev.c
index f47e96b62308..d0304461ad32 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -1146,7 +1146,11 @@ EXPORT_SYMBOL(dev_alloc_name);
 int dev_get_valid_name(struct net *net, struct net_device *dev,
 		       const char *name)
 {
-	return dev_alloc_name_ns(net, dev, name);
+	int rc = dev_alloc_name_ns(net, dev, name);
+	if (rc == -ENFILE)
+		rc = -EEXIST;
+
+	return rc;
 }
 EXPORT_SYMBOL(dev_get_valid_name);
 

Hi,

This revert seems to have broken networking on one of my powerpc
machines, according to git bisect.

Fun!

TBH, I only looked at the immediate problem we ran into, and reverted
what was causing it. I don't think we saw the follow-up problem you're
seeing.

The symptom is DHCP fails and I don't get a link, I didn't dig any
further than that. I can if it's helpful.

I think the problem is that 87c320e51519 ("net: core: dev_get_valid_name
is now the same as dev_alloc_name_ns") only makes sense while
d6f295e9def0 remains in the tree.

ie. before the entire series, dev_get_valid_name() would return EEXIST,
and that was retained when 87c320e51519 was merged, but now that
d6f295e9def0 has been reverted dev_get_valid_name() is returning ENFILE.

I can get the network up again if I also revert 87c320e51519 ("net:
core: dev_get_valid_name is now the same as dev_alloc_name_ns"), or with
the gross patch below.

Makes sense. I guess that should be reverted too then, or even your
"gross" patch applied.

johannes

On Tue, Dec 19 2017, Michael Ellerman [off-list ref] wrote:

Hi Johannes,

quoted

From: Johannes Berg <redacted>

This reverts commit d6f295e9def0; some userspace (in the case

This revert seems to have broken networking on one of my powerpc
machines, according to git bisect.

The symptom is DHCP fails and I don't get a link, I didn't dig any
further than that. I can if it's helpful.

I think the problem is that 87c320e51519 ("net: core: dev_get_valid_name
is now the same as dev_alloc_name_ns") only makes sense while
d6f295e9def0 remains in the tree.

I'm sorry about all of this, I really didn't think there would be such
consequences of changing an errno return. Indeed, d6f29 was preparation
for unifying the two functions that do the exact same thing (and how we
ever got into that situation is somewhat unclear), except for
their behaviour in the case the requested name already exists. So one of
the two interfaces had to change its return value, and as I wrote, I
thought EEXIST was the saner choice when an explicit name (no %d) had
been requested.

ie. before the entire series, dev_get_valid_name() would return EEXIST,
and that was retained when 87c320e51519 was merged, but now that
d6f295e9def0 has been reverted dev_get_valid_name() is returning ENFILE.

I can get the network up again if I also revert 87c320e51519 ("net:
core: dev_get_valid_name is now the same as dev_alloc_name_ns"), or with
the gross patch below.

I don't think changing -ENFILE to -EEXIST would be right either, since
dev_get_valid_name() used to be able to return both (-EEXIST in the case
where there's no %d, -ENFILE in the case where we end up calling
dev_alloc_name_ns()). If anything, we could do the check for the old
-EEXIST condition first, and then call dev_alloc_name_ns(). But I'm also
fine with reverting.

Again, sorry :(

Rasmus

Rasmus Villemoes [off-list ref] writes:

On Tue, Dec 19 2017, Michael Ellerman [off-list ref] wrote:

quoted

quoted

From: Johannes Berg <redacted>

This reverts commit d6f295e9def0; some userspace (in the case

This revert seems to have broken networking on one of my powerpc
machines, according to git bisect.

The symptom is DHCP fails and I don't get a link, I didn't dig any
further than that. I can if it's helpful.

I think the problem is that 87c320e51519 ("net: core: dev_get_valid_name
is now the same as dev_alloc_name_ns") only makes sense while
d6f295e9def0 remains in the tree.

I'm sorry about all of this, I really didn't think there would be such
consequences of changing an errno return. Indeed, d6f29 was preparation
for unifying the two functions that do the exact same thing (and how we
ever got into that situation is somewhat unclear), except for
their behaviour in the case the requested name already exists. So one of
the two interfaces had to change its return value, and as I wrote, I
thought EEXIST was the saner choice when an explicit name (no %d) had
been requested.

No worries.

quoted

ie. before the entire series, dev_get_valid_name() would return EEXIST,
and that was retained when 87c320e51519 was merged, but now that
d6f295e9def0 has been reverted dev_get_valid_name() is returning ENFILE.

I can get the network up again if I also revert 87c320e51519 ("net:
core: dev_get_valid_name is now the same as dev_alloc_name_ns"), or with
the gross patch below.

I don't think changing -ENFILE to -EEXIST would be right either, since
dev_get_valid_name() used to be able to return both (-EEXIST in the case
where there's no %d, -ENFILE in the case where we end up calling
dev_alloc_name_ns()). If anything, we could do the check for the old
-EEXIST condition first, and then call dev_alloc_name_ns(). But I'm also
fine with reverting.

Yeah I think a revert would be best, given it's nearly rc5.

My userspace is not exotic AFAIK, just debian something, so presumably
this will affect other people too.

cheers

From: Michael Ellerman <mpe@ellerman.id.au>
Date: Fri, 22 Dec 2017 15:22:22 +1100

quoted

On Tue, Dec 19 2017, Michael Ellerman [off-list ref] wrote:

quoted

This revert seems to have broken networking on one of my powerpc
machines, according to git bisect.

The symptom is DHCP fails and I don't get a link, I didn't dig any
further than that. I can if it's helpful.

I think the problem is that 87c320e51519 ("net: core: dev_get_valid_name
is now the same as dev_alloc_name_ns") only makes sense while
d6f295e9def0 remains in the tree.

I'm sorry about all of this, I really didn't think there would be such
consequences of changing an errno return. Indeed, d6f29 was preparation
for unifying the two functions that do the exact same thing (and how we
ever got into that situation is somewhat unclear), except for
their behaviour in the case the requested name already exists. So one of
the two interfaces had to change its return value, and as I wrote, I
thought EEXIST was the saner choice when an explicit name (no %d) had
been requested.

No worries.

quoted

quoted

ie. before the entire series, dev_get_valid_name() would return EEXIST,
and that was retained when 87c320e51519 was merged, but now that
d6f295e9def0 has been reverted dev_get_valid_name() is returning ENFILE.

I can get the network up again if I also revert 87c320e51519 ("net:
core: dev_get_valid_name is now the same as dev_alloc_name_ns"), or with
the gross patch below.

I don't think changing -ENFILE to -EEXIST would be right either, since
dev_get_valid_name() used to be able to return both (-EEXIST in the case
where there's no %d, -ENFILE in the case where we end up calling
dev_alloc_name_ns()). If anything, we could do the check for the old
-EEXIST condition first, and then call dev_alloc_name_ns(). But I'm also
fine with reverting.

Yeah I think a revert would be best, given it's nearly rc5.

My userspace is not exotic AFAIK, just debian something, so presumably
this will affect other people too.

I've just queued up the following revert, thanks!

====================

From 5047543928139184f060c8f3bccb788b3df4c1ea Mon Sep 17 00:00:00 2001

From: "David S. Miller" <davem@davemloft.net>
Date: Tue, 2 Jan 2018 11:45:07 -0500
Subject: [PATCH] Revert "net: core: dev_get_valid_name is now the same as
 dev_alloc_name_ns"

This reverts commit 87c320e51519a83c496ab7bfb4e96c8f9c001e89.

Changing the error return code in some situations turns out to
be harmful in practice.  In particular Michael Ellerman reports
that DHCP fails on his powerpc machines, and this revert gets
things working again.

Johannes Berg agrees that this revert is the best course of
action for now.

Fixes: 029b6d140550 ("Revert "net: core: maybe return -EEXIST in __dev_alloc_name"")
Reported-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/core/dev.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 01ee854454a8..0e0ba36eeac9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -1146,7 +1146,19 @@ EXPORT_SYMBOL(dev_alloc_name);
 int dev_get_valid_name(struct net *net, struct net_device *dev,
 		       const char *name)
 {
-	return dev_alloc_name_ns(net, dev, name);
+	BUG_ON(!net);
+
+	if (!dev_valid_name(name))
+		return -EINVAL;
+
+	if (strchr(name, '%'))
+		return dev_alloc_name_ns(net, dev, name);
+	else if (__dev_get_by_name(net, name))
+		return -EEXIST;
+	else if (dev->name != name)
+		strlcpy(dev->name, name, IFNAMSIZ);
+
+	return 0;
 }
 EXPORT_SYMBOL(dev_get_valid_name);
 

-- 
2.14.3

On Tue, 2018-01-02 at 11:50 -0500, David Miller wrote:

From: Michael Ellerman <mpe@ellerman.id.au>
Date: Fri, 22 Dec 2017 15:22:22 +1100

quoted

quoted

On Tue, Dec 19 2017, Michael Ellerman [off-list ref] wrote:

quoted

This revert seems to have broken networking on one of my powerpc
machines, according to git bisect.

The symptom is DHCP fails and I don't get a link, I didn't dig any
further than that. I can if it's helpful.

I think the problem is that 87c320e51519 ("net: core: dev_get_valid_name
is now the same as dev_alloc_name_ns") only makes sense while
d6f295e9def0 remains in the tree.

I'm sorry about all of this, I really didn't think there would be such
consequences of changing an errno return. Indeed, d6f29 was preparation
for unifying the two functions that do the exact same thing (and how we
ever got into that situation is somewhat unclear), except for
their behaviour in the case the requested name already exists. So one of
the two interfaces had to change its return value, and as I wrote, I
thought EEXIST was the saner choice when an explicit name (no %d) had
been requested.

No worries.

quoted

quoted

ie. before the entire series, dev_get_valid_name() would return EEXIST,
and that was retained when 87c320e51519 was merged, but now that
d6f295e9def0 has been reverted dev_get_valid_name() is returning ENFILE.

I can get the network up again if I also revert 87c320e51519 ("net:
core: dev_get_valid_name is now the same as dev_alloc_name_ns"), or with
the gross patch below.

I don't think changing -ENFILE to -EEXIST would be right either, since
dev_get_valid_name() used to be able to return both (-EEXIST in the case
where there's no %d, -ENFILE in the case where we end up calling
dev_alloc_name_ns()). If anything, we could do the check for the old
-EEXIST condition first, and then call dev_alloc_name_ns(). But I'm also
fine with reverting.

Yeah I think a revert would be best, given it's nearly rc5.

My userspace is not exotic AFAIK, just debian something, so presumably
this will affect other people too.

I've just queued up the following revert, thanks!

====================
From 5047543928139184f060c8f3bccb788b3df4c1ea Mon Sep 17 00:00:00 2001
From: "David S. Miller" <davem@davemloft.net>
Date: Tue, 2 Jan 2018 11:45:07 -0500
Subject: [PATCH] Revert "net: core: dev_get_valid_name is now the same as
 dev_alloc_name_ns"

This reverts commit 87c320e51519a83c496ab7bfb4e96c8f9c001e89.

Changing the error return code in some situations turns out to
be harmful in practice.  In particular Michael Ellerman reports
that DHCP fails on his powerpc machines, and this revert gets
things working again.

Johannes Berg agrees that this revert is the best course of
action for now.

I'm not sure my voice matters much, I merely did the first revert of
these two patches ... :)

But I agree with Michael that you can't really salvage this without the
other patch, and that one caused problems in wifi ...

Thanks :)

johannes 

David Miller [off-list ref] writes:

From: Michael Ellerman <mpe@ellerman.id.au>
Date: Fri, 22 Dec 2017 15:22:22 +1100

quoted

quoted

On Tue, Dec 19 2017, Michael Ellerman [off-list ref] wrote:

quoted

This revert seems to have broken networking on one of my powerpc
machines, according to git bisect.

The symptom is DHCP fails and I don't get a link, I didn't dig any
further than that. I can if it's helpful.

I think the problem is that 87c320e51519 ("net: core: dev_get_valid_name
is now the same as dev_alloc_name_ns") only makes sense while
d6f295e9def0 remains in the tree.

I'm sorry about all of this, I really didn't think there would be such
consequences of changing an errno return. Indeed, d6f29 was preparation
for unifying the two functions that do the exact same thing (and how we
ever got into that situation is somewhat unclear), except for
their behaviour in the case the requested name already exists. So one of
the two interfaces had to change its return value, and as I wrote, I
thought EEXIST was the saner choice when an explicit name (no %d) had
been requested.

No worries.

quoted

quoted

ie. before the entire series, dev_get_valid_name() would return EEXIST,
and that was retained when 87c320e51519 was merged, but now that
d6f295e9def0 has been reverted dev_get_valid_name() is returning ENFILE.

I can get the network up again if I also revert 87c320e51519 ("net:
core: dev_get_valid_name is now the same as dev_alloc_name_ns"), or with
the gross patch below.

I don't think changing -ENFILE to -EEXIST would be right either, since
dev_get_valid_name() used to be able to return both (-EEXIST in the case
where there's no %d, -ENFILE in the case where we end up calling
dev_alloc_name_ns()). If anything, we could do the check for the old
-EEXIST condition first, and then call dev_alloc_name_ns(). But I'm also
fine with reverting.

Yeah I think a revert would be best, given it's nearly rc5.

My userspace is not exotic AFAIK, just debian something, so presumably
this will affect other people too.

I've just queued up the following revert, thanks!

Thanks.

I don't see it in rc7, will it get to Linus sometime before the release?

cheers

From: Michael Ellerman <mpe@ellerman.id.au>
Date: Mon, 08 Jan 2018 14:26:12 +1100

I don't see it in rc7, will it get to Linus sometime before the
release?

I will do what I can to make sure it does, it is in my GIT tree.