Re: [PATCH] Revert "mountd: handle allocation failures in auth_unix_ip upcall"

[Chuck Lever <chuck.lever@oracle.com>]

On Nov 26, 2012, at 5:15 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Nov 26, 2012 at 05:05:22PM -0500, Chuck Lever wrote:
>> 
>> On Nov 26, 2012, at 5:03 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:
>> 
>>> From: "J. Bruce Fields" <bfields@redhat.com>
>>> 
>>> 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.

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

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

> 
> --b.
> 
>> 
>>> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>>> ---
>>> utils/mountd/cache.c | 12 +++---------
>>> 1 file changed, 3 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
>>> index 8f14032..6710eca 100644
>>> --- a/utils/mountd/cache.c
>>> +++ b/utils/mountd/cache.c
>>> @@ -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)
>>> -- 
>>> 1.7.11.7
>>> 
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>> --
>> Chuck Lever
>> chuck[dot]lever[at]oracle[dot]com
>> 
>> 
>> 

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