Revert "mountd: handle allocation failures in auth_unix_ip upcall"

PATCH In review J. Bruce Fields 3 participants 10 messages Nov 26, 2012 – Nov 28, 2012

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

J. Bruce Fields

author

2012-11-26 22:03 UTC #0 lore raw

PATCH 1/1 Revert "mountd: handle allocation failures in auth_unix_ip upcall"

From: "J. Bruce Fields" <[email protected]>

This reverts commit 485f7a21e1649797f29317b865cbb094c1f6a71d. The failures handled there could be any sort of name resolution failure, not just an allocation, and failing to downcall (hence leaving the client hanging) is not the correct thing to do in those cases.

Signed-off-by J. Bruce Fields <[email protected]>

---
 utils/mountd/cache.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

utils/mountd/cache.c +3 -9

index 8f14032..6710eca 100644
@@ -84,6 +84,7 @@ static void auth_unix_ip(FILE *f)
 	char ipaddr[INET6_ADDRSTRLEN];
 	char *client = NULL;
 	struct addrinfo *tmp = NULL;
+	struct addrinfo *ai = NULL;
 	if (readline(fileno(f), &lbuf, &lbuflen) != 1)
 		return;

@@ -106,16 +107,12 @@ static void auth_unix_ip(FILE *f)
 
 	/* addr is a valid, interesting address, find the domain name... */
 	if (!use_ipaddr) {
-		struct addrinfo *ai = NULL;
-
 		ai = client_resolve(tmp->ai_addr);
-		if (ai == NULL)
-			goto out;
 		client = client_compose(ai);
 		freeaddrinfo(ai);
-		if (!client)
-			goto out;
 	}
+	freeaddrinfo(tmp);
+
 	qword_print(f, "nfsd");
 	qword_print(f, ipaddr);
 	qword_printuint(f, time(0) + DEFAULT_TTL);

@@ -127,9 +124,6 @@ static void auth_unix_ip(FILE *f)
 	xlog(D_CALL, "auth_unix_ip: client %p '%s'", client, client?client: "DEFAULT");
 
 	free(client);
-out:
-	freeaddrinfo(tmp);
-
 }
 
 static void auth_unix_gid(FILE *f)

Signature · 1 line

1.7.11.7

Chuck Lever

2012-11-26 22:05 UTC #1 lore raw

Quoted 6 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>
This reverts commit 485f7a21e1649797f29317b865cbb094c1f6a71d. The failures handled there could be any sort of name resolution failure, not just an allocation, and failing to downcall (hence leaving the client hanging) is not the correct thing to do in those cases.

The problem is in the kernel, then: a downcall should be allowed to fail, IMO.

Quoted 53 lines

Signed-off-by J. Bruce Fields <[email protected]>

---
utils/mountd/cache.c | 12 +++---------

1 file changed, 3 insertions(+), 9 deletions(-)

utils/mountd/cache.c +3 -9

index 8f14032..6710eca 100644
@@ -84,6 +84,7 @@ static void auth_unix_ip(FILE *f)
 	char ipaddr[INET6_ADDRSTRLEN];
 	char *client = NULL;
 	struct addrinfo *tmp = NULL;
+	struct addrinfo *ai = NULL;
 	if (readline(fileno(f), &lbuf, &lbuflen) != 1)
 		return;

@@ -106,16 +107,12 @@ static void auth_unix_ip(FILE *f)
 
 	/* addr is a valid, interesting address, find the domain name... */
 	if (!use_ipaddr) {
-		struct addrinfo *ai = NULL;
-
 		ai = client_resolve(tmp->ai_addr);
-		if (ai == NULL)
-			goto out;
 		client = client_compose(ai);
 		freeaddrinfo(ai);
-		if (!client)
-			goto out;
 	}
+	freeaddrinfo(tmp);
+
 	qword_print(f, "nfsd");
 	qword_print(f, ipaddr);
 	qword_printuint(f, time(0) + DEFAULT_TTL);

@@ -127,9 +124,6 @@ static void auth_unix_ip(FILE *f)
 	xlog(D_CALL, "auth_unix_ip: client %p '%s'", client, client?client: "DEFAULT");
 
 	free(client);
-out:
-	freeaddrinfo(tmp);
-
 }
 
 static void auth_unix_gid(FILE *f)

Signature · 6 lines

1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

J. Bruce Fields

author

2012-11-26 22:15 UTC #2 lore raw

Quoted 11 lines · Chuck Lever

Quoted 6 lines · J. Bruce Fields
From: "J. Bruce Fields" <[email protected]>
This reverts commit 485f7a21e1649797f29317b865cbb094c1f6a71d. The failures handled there could be any sort of name resolution failure, not just an allocation, and failing to downcall (hence leaving the client hanging) is not the correct thing to do in those cases.
The problem is in the kernel, then: a downcall should be allowed to fail, IMO.

In this case, after a revert, a failure here will result in the downcall passing down a client named "DEFAULT". Presumably that won't be permitted access to the export, so the client will end up getting an error.

But I may not understand your objection.

--b.

Quoted 61 lines

Quoted 54 lines

Signed-off-by J. Bruce Fields <[email protected]>

---
utils/mountd/cache.c | 12 +++---------

1 file changed, 3 insertions(+), 9 deletions(-)

utils/mountd/cache.c +3 -9

index 8f14032..6710eca 100644
@@ -84,6 +84,7 @@ static void auth_unix_ip(FILE *f)
 	char ipaddr[INET6_ADDRSTRLEN];
 	char *client = NULL;
 	struct addrinfo *tmp = NULL;
+	struct addrinfo *ai = NULL;
 	if (readline(fileno(f), &lbuf, &lbuflen) != 1)
 		return;

@@ -106,16 +107,12 @@ static void auth_unix_ip(FILE *f)
 
 	/* addr is a valid, interesting address, find the domain name... */
 	if (!use_ipaddr) {
-		struct addrinfo *ai = NULL;
-
 		ai = client_resolve(tmp->ai_addr);
-		if (ai == NULL)
-			goto out;
 		client = client_compose(ai);
 		freeaddrinfo(ai);
-		if (!client)
-			goto out;
 	}
+	freeaddrinfo(tmp);
+
 	qword_print(f, "nfsd");
 	qword_print(f, ipaddr);
 	qword_printuint(f, time(0) + DEFAULT_TTL);

@@ -127,9 +124,6 @@ static void auth_unix_ip(FILE *f)
 	xlog(D_CALL, "auth_unix_ip: client %p '%s'", client, client?client: "DEFAULT");
 
 	free(client);
-out:
-	freeaddrinfo(tmp);
-
 }
 
 static void auth_unix_gid(FILE *f)

Signature · 6 lines

1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Chuck Lever

2012-11-26 22:38 UTC #3 lore raw

Quoted 11 lines · J. Bruce Fields

Quoted 10 lines · Chuck Lever
Quoted 6 lines · J. Bruce Fields
From: "J. Bruce Fields" <[email protected]>
This reverts commit 485f7a21e1649797f29317b865cbb094c1f6a71d. The failures handled there could be any sort of name resolution failure, not just an allocation, and failing to downcall (hence leaving the client hanging) is not the correct thing to do in those cases.

Quoted 6 lines

Quoted 1 line
The problem is in the kernel, then: a downcall should be allowed to fail, IMO.
In this case, after a revert, a failure here will result in the downcall passing down a client named "DEFAULT". Presumably that won't be permitted access to the export, so the client will end up getting an error.

"A failure here" can mean either malloc() returned NULL in client_resolve() or client_compose(), or . . . ?

Quoted 1 line

But I may not understand your objection.

The main problem is I don't understand your patch description. :-)

I don't seem to have commit 485f7a21 in my nfs-utils git tree (it's helpful to include the short description for such a case).

What exactly is the problem with the current code?

client_resolve() can return a NULL in some cases. Why is it OK to pass a NULL "ai" to client_compose() ? Looks like that can result in a mountd segfault. The kernel won't get any downcall reply in that case! Is that what you are trying to fix?

WRT my original objection: In general I don't see how to make it impossible for mountd to fail. Thus the kernel needs to be better about recovering when mountd suddenly disappears.

Quoted 64 lines

--b.

Quoted 61 lines

Quoted 54 lines

Signed-off-by J. Bruce Fields <[email protected]>

---
utils/mountd/cache.c | 12 +++---------

1 file changed, 3 insertions(+), 9 deletions(-)

utils/mountd/cache.c +3 -9

index 8f14032..6710eca 100644
@@ -84,6 +84,7 @@ static void auth_unix_ip(FILE *f)
 	char ipaddr[INET6_ADDRSTRLEN];
 	char *client = NULL;
 	struct addrinfo *tmp = NULL;
+	struct addrinfo *ai = NULL;
 	if (readline(fileno(f), &lbuf, &lbuflen) != 1)
 		return;

@@ -106,16 +107,12 @@ static void auth_unix_ip(FILE *f)
 
 	/* addr is a valid, interesting address, find the domain name... */
 	if (!use_ipaddr) {
-		struct addrinfo *ai = NULL;
-
 		ai = client_resolve(tmp->ai_addr);
-		if (ai == NULL)
-			goto out;
 		client = client_compose(ai);
 		freeaddrinfo(ai);
-		if (!client)
-			goto out;
 	}
+	freeaddrinfo(tmp);
+
 	qword_print(f, "nfsd");
 	qword_print(f, ipaddr);
 	qword_printuint(f, time(0) + DEFAULT_TTL);

@@ -127,9 +124,6 @@ static void auth_unix_ip(FILE *f)
 	xlog(D_CALL, "auth_unix_ip: client %p '%s'", client, client?client: "DEFAULT");
 
 	free(client);
-out:
-	freeaddrinfo(tmp);
-
 }
 
 static void auth_unix_gid(FILE *f)

Signature · 6 lines

1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

J. Bruce Fields

author

2012-11-26 22:51 UTC #4 lore raw

Quoted 14 lines · Chuck Lever

Quoted 11 lines · J. Bruce Fields
Quoted 10 lines · Chuck Lever
Quoted 6 lines · J. Bruce Fields
From: "J. Bruce Fields" <[email protected]>
This reverts commit 485f7a21e1649797f29317b865cbb094c1f6a71d. The failures handled there could be any sort of name resolution failure, not just an allocation, and failing to downcall (hence leaving the client hanging) is not the correct thing to do in those cases.

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
The problem is in the kernel, then: a downcall should be allowed to fail, IMO.
In this case, after a revert, a failure here will result in the downcall passing down a client named "DEFAULT". Presumably that won't be permitted access to the export, so the client will end up getting an error.
"A failure here" can mean either malloc() returned NULL in client_resolve() or client_compose(), or . . . ?

Looks like it'd also fail if we couldn't map the client's ip address to a name.

Quoted 6 lines

Quoted 1 line
But I may not understand your objection.
The main problem is I don't understand your patch description. :-)
I don't seem to have commit 485f7a21 in my nfs-utils git tree (it's helpful to include the short description for such a case).

Ah, crap, sorry, looks like I reverted a local commit of that patch.... The upstream commit is bf6a4febaa78bf188896b7b5b02c46562dd08b70 (and the short description is in the subject line above).

Quoted 5 lines

What exactly is the problem with the current code?
client_resolve() can return a NULL in some cases. Why is it OK to pass a NULL "ai" to client_compose() ? Looks like that can result in a mountd segfault.

Bah, I thought I'd checked this and found it was prepared to handle that, but no:

	client_check->check_wildcard()

looks like it can oops.

OK, I'll take another look.

Quoted 5 lines

The kernel won't get any downcall reply in that
case! Is that what you are trying to fix?
WRT my original objection: In general I don't see how to make it impossible for mountd to fail.

Sure, but mountd is required for the server to function, so it's just a question of how we fail.

Quoted 2 lines

Thus the kernel needs to be better about recovering when mountd suddenly disappears.

Currently it drops and lets the client retry. I suspect that's the correct thing to do, but alternatives are welcomed.

--b.

Chuck Lever

2012-11-26 23:10 UTC #5 lore raw

Quoted 22 lines · J. Bruce Fields

Quoted 21 lines · Chuck Lever
Quoted 17 lines · J. Bruce Fields
Quoted 11 lines · Chuck Lever
Quoted 6 lines · J. Bruce Fields
From: "J. Bruce Fields" <[email protected]>
This reverts commit 485f7a21e1649797f29317b865cbb094c1f6a71d. The failures handled there could be any sort of name resolution failure, not just an allocation, and failing to downcall (hence leaving the client hanging) is not the correct thing to do in those cases.
The problem is in the kernel, then: a downcall should be allowed to fail, IMO.
In this case, after a revert, a failure here will result in the downcall passing down a client named "DEFAULT". Presumably that won't be permitted access to the export, so the client will end up getting an error.

Quoted 4 lines

Quoted 1 line
"A failure here" can mean either malloc() returned NULL in client_resolve() or client_compose(), or . . . ?
Looks like it'd also fail if we couldn't map the client's ip address to a name.

That's the common failure mode here, which is now treated just like a malloc(3) failure.

Quoted 1 line

Quoted 1 line
What exactly is the problem with the current code?

Anyway, it would help my addled brain if the problem description in the next version of this patch was more clear about what the code is doing wrong now.

Quoted 6 lines

Quoted 5 lines
The kernel won't get any downcall reply in that
case! Is that what you are trying to fix?
WRT my original objection: In general I don't see how to make it impossible for mountd to fail.

Quoted 8 lines

Sure, but mountd is required for the server to function, so it's just a question of how we fail.
Quoted 2 lines
Thus the kernel needs to be better about recovering when mountd suddenly disappears.
Currently it drops and lets the client retry. I suspect that's the correct thing to do, but alternatives are welcomed.

By "drops" do you mean the server drops the NFS request, and the client retransmits the request? That's actually pretty unfriendly behavior, IMO, since an application on a client is typically stuck at that point until that RPC gets some sort of result (after possibly several RTOs).

Maybe the server could signal an NFS error of some kind and let the client decide if it wants to retry until the server is working again, or fail that request immediately.

(Also, if NFSv4 is dependent on a mountd upcall, it is not supposed to drop a request, AFAIK).

OK, but this is a separate issue from the case you are trying to fix.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Steve Dickson

2012-11-27 14:24 UTC #6 lore raw

In reply to J. Bruce Fields

Hello,

Quoted 11 lines · J. Bruce Fields

Quoted 5 lines
What exactly is the problem with the current code?
Quoted 4 lines
client_resolve() can return a NULL in some cases. Why is it OK to pass a NULL "ai" to client_compose() ? Looks like that can result in a mountd segfault.
Bah, I thought I'd checked this and found it was prepared to handle that, but no:
	client_check->check_wildcard()
looks like it can oops.

Yeah... added client_check->check_netgroup() to this list...

Quoted 2 lines

OK, I'll take another look.

A simply check that ai is not null when calling client_compose() should work..

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
The kernel won't get any downcall reply in that
case! Is that what you are trying to fix?
WRT my original objection: In general I don't see how to make it impossible for mountd to fail.

Quoted 7 lines

Sure, but mountd is required for the server to function, so it's just a question of how we fail.
Quoted 2 lines
Quoted 2 lines
Thus the kernel needs to be better about recovering when mountd suddenly disappears.
Currently it drops and lets the client retry. I suspect that's the correct thing to do, but alternatives are welcomed.

This one things I didn't see happen... the client retrying... expected it would...

steved.

J. Bruce Fields

author

2012-11-27 21:31 UTC #7 lore raw

In reply to J. Bruce Fields

Quoted 7 lines · J. Bruce Fields

Quoted 5 lines · Chuck Lever
What exactly is the problem with the current code?
client_resolve() can return a NULL in some cases. Why is it OK to pass a NULL "ai" to client_compose() ? Looks like that can result in a mountd segfault.

Quoted 6 lines

Bah, I thought I'd checked this and found it was prepared to handle that, but no:
	client_check->check_wildcard()
looks like it can oops.

Right, so the real problem is just that we're skipping the downcall on failure instead of responding with a negative cache entry. Thus we're leaving the client hanging instead of returning an error.

So, I suppose we should do the below, based on steved's suggestion (compiled, otherwise untested).

--b.

commit dfb31d861261c8461a2dc4fb7e8823f5169a9079

Author: J. Bruce Fields <[email protected]>
Date:   Tue Nov 27 16:10:41 2012 -0500

mountd: auth_unix_ip should downcall on error to prevent hangs

Since bf6a4febaa78bf188896b7b5b02c46562dd08b70 "mountd: handle allocation failures in auth_unix_ip upcall", a failure to map the address of an incoming client to a name could result in a hang.

We should be responding with an error in the case, not just skipping the downcall and leaving everybody hanging.

Signed-off-by J. Bruce Fields <[email protected]>

utils/mountd/cache.c +4 -7

index e950ec6..c13f305 100644
@@ -109,12 +109,10 @@ static void auth_unix_ip(FILE *f)
 		struct addrinfo *ai = NULL;
 
 		ai = client_resolve(tmp->ai_addr);
-		if (ai == NULL)
-			goto out;
-		client = client_compose(ai);
-		freeaddrinfo(ai);
-		if (!client)
-			goto out;
+		if (ai) {
+			client = client_compose(ai);
+			freeaddrinfo(ai);
+		}
 	}
 	qword_print(f, "nfsd");
 	qword_print(f, ipaddr);

@@ -127,7 +125,6 @@ static void auth_unix_ip(FILE *f)
 	xlog(D_CALL, "auth_unix_ip: client %p '%s'", client, client?client: "DEFAULT");
 
 	free(client);
-out:
 	freeaddrinfo(tmp);
 
 }

Chuck Lever

2012-11-27 21:34 UTC #8 lore raw

Quoted 13 lines · J. Bruce Fields

Quoted 12 lines · J. Bruce Fields
Quoted 5 lines · Chuck Lever
What exactly is the problem with the current code?
client_resolve() can return a NULL in some cases. Why is it OK to pass a NULL "ai" to client_compose() ? Looks like that can result in a mountd segfault.
Bah, I thought I'd checked this and found it was prepared to handle that, but no:
	client_check->check_wildcard()

Quoted 8 lines

Quoted 1 line
looks like it can oops.
Right, so the real problem is just that we're skipping the downcall on failure instead of responding with a negative cache entry. Thus we're leaving the client hanging instead of returning an error.
So, I suppose we should do the below, based on steved's suggestion (compiled, otherwise untested).

Thanks, this seems reasonable.

Quoted 40 lines

--b.
commit dfb31d861261c8461a2dc4fb7e8823f5169a9079
Author: J. Bruce Fields <[email protected]>
Date:   Tue Nov 27 16:10:41 2012 -0500
mountd: auth_unix_ip should downcall on error to prevent hangs
Since bf6a4febaa78bf188896b7b5b02c46562dd08b70 "mountd: handle allocation failures in auth_unix_ip upcall", a failure to map the address of an incoming client to a name could result in a hang.
We should be responding with an error in the case, not just skipping the downcall and leaving everybody hanging.
Signed-off-by J. Bruce Fields <[email protected]>
utils/mountd/cache.c +4 -6
index e950ec6..c13f305 100644
@@ -109,12 +109,10 @@ static void auth_unix_ip(FILE *f)
 		struct addrinfo *ai = NULL;
 
 		ai = client_resolve(tmp->ai_addr);
-		if (ai == NULL)
-			goto out;
-		client = client_compose(ai);
-		freeaddrinfo(ai);
-		if (!client)
-			goto out;
+		if (ai) {
+			client = client_compose(ai);
+			freeaddrinfo(ai);
+		}
 	}
 	qword_print(f, "nfsd");
 	qword_print(f, ipaddr);

utils/mountd/cache.c static void auth_unix_ip(FILE *f) · line 130

Quoted 7 lines

utils/mountd/cache.c +0 -1

@@ -127,7 +125,6 @@ static void auth_unix_ip(FILE *f)
 	xlog(D_CALL, "auth_unix_ip: client %p '%s'", client, client?client: "DEFAULT");
 
 	free(client);
-out:
 	freeaddrinfo(tmp);
 
 }

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Steve Dickson

2012-11-28 14:39 UTC #9 lore raw

In reply to J. Bruce Fields

Quoted 7 lines · J. Bruce Fields

On Mon, Nov 26, 2012 at 05:51:16PM -0500, J. Bruce Fields wrote:
commit dfb31d861261c8461a2dc4fb7e8823f5169a9079
Author: J. Bruce Fields <[email protected]>
Date:   Tue Nov 27 16:10:41 2012 -0500
mountd: auth_unix_ip should downcall on error to prevent hangs

Quoted 8 lines

Since bf6a4febaa78bf188896b7b5b02c46562dd08b70 "mountd: handle allocation failures in auth_unix_ip upcall", a failure to map the address of an incoming client to a name could result in a hang.
We should be responding with an error in the case, not just skipping the downcall and leaving everybody hanging.
Signed-off-by J. Bruce Fields <[email protected]>

Committed....

steved.

Quoted 31 lines

utils/mountd/cache.c +4 -7

index e950ec6..c13f305 100644
@@ -109,12 +109,10 @@ static void auth_unix_ip(FILE *f)
 		struct addrinfo *ai = NULL;
 
 		ai = client_resolve(tmp->ai_addr);
-		if (ai == NULL)
-			goto out;
-		client = client_compose(ai);
-		freeaddrinfo(ai);
-		if (!client)
-			goto out;
+		if (ai) {
+			client = client_compose(ai);
+			freeaddrinfo(ai);
+		}
 	}
 	qword_print(f, "nfsd");
 	qword_print(f, ipaddr);

@@ -127,7 +125,6 @@ static void auth_unix_ip(FILE *f)
 	xlog(D_CALL, "auth_unix_ip: client %p '%s'", client, client?client: "DEFAULT");
 
 	free(client);
-out:
 	freeaddrinfo(tmp);
 
 }