[PATCH] statd: not unlinking host files

[PATCH] statd: not unlinking host files

[Steve Dickson]

Again working with the state file code, I notice that statd was
not unlinking hosts files when the kernel sent up the 
sm_unmon messages. This following patch address the reason why... 

Comments?

steved.

commit f8536d0e210a8151900f8c68185927790239eb62
Author: Steve Dickson <steved@redhat.com>
Date:   Sat Dec 6 09:30:43 2008 -0500

    statd: not unlinking host files
    
    Statd is not unlinking host files during SM_UNMON and
    SM_UNMON_ALL calls because the given host is still on the run-time
    notify list (rtnl) and the check flag is set when xunlink() is
    called. But the next thing the caller of xunlink() does is
    remove the host from the rtnl list which means the
    unlink will never happen.
    
    So in cases where xunlink() is immediately follow by a call
    to nlist_free() (which removes the host from the list) the
    check flag to xunlink() is not set.
    
    Signed-off-by: Steve Dickson <steved@redhat.com>

diff --git a/utils/statd/monitor.c b/utils/statd/monitor.c
index a6a1d9c..7d6e4da 100644
--- a/utils/statd/monitor.c
+++ b/utils/statd/monitor.c
@@ -352,7 +352,7 @@ sm_unmon_1_svc(struct mon_id *argp, struct svc_req *rqstp)
 			/* PRC: do the HA callout: */
 			ha_callout("del-client", mon_name, my_name, -1);
 
-			xunlink(SM_DIR, clnt->dns_name, 1);
+			xunlink(SM_DIR, clnt->dns_name, 0);
 			nlist_free(&rtnl, clnt);
 
 			return (&result);
@@ -404,7 +404,7 @@ sm_unmon_all_1_svc(struct my_id *argp, struct svc_req *rqstp)
 			temp = NL_NEXT(clnt);
 			/* PRC: do the HA callout: */
 			ha_callout("del-client", mon_name, my_name, -1);
-			xunlink(SM_DIR, clnt->dns_name, 1);
+			xunlink(SM_DIR, clnt->dns_name, 0);
 			nlist_free(&rtnl, clnt);
 			++count;
 			clnt = temp;

Re: [PATCH] statd: not unlinking host files

[J. Bruce Fields]

On Sat, Dec 06, 2008 at 09:34:25AM -0500, Steve Dickson wrote:
> Again working with the state file code, I notice that statd was
> not unlinking hosts files when the kernel sent up the 
> sm_unmon messages. This following patch address the reason why... 
> 
> Comments?

Bizarre--thanks for catching that.

But it looks like these are actually the only two callers to xunlink?
In which case, we should just ditch the "check" parameter entirely and
avoid some confusion....

(I wonder how it ever got this way in the first case?)

--b.

> 
> steved.
> 
> commit f8536d0e210a8151900f8c68185927790239eb62
> Author: Steve Dickson <steved@redhat.com>
> Date:   Sat Dec 6 09:30:43 2008 -0500
> 
>     statd: not unlinking host files
>     
>     Statd is not unlinking host files during SM_UNMON and
>     SM_UNMON_ALL calls because the given host is still on the run-time
>     notify list (rtnl) and the check flag is set when xunlink() is
>     called. But the next thing the caller of xunlink() does is
>     remove the host from the rtnl list which means the
>     unlink will never happen.
>     
>     So in cases where xunlink() is immediately follow by a call
>     to nlist_free() (which removes the host from the list) the
>     check flag to xunlink() is not set.
>     
>     Signed-off-by: Steve Dickson <steved@redhat.com>
> 
> diff --git a/utils/statd/monitor.c b/utils/statd/monitor.c
> index a6a1d9c..7d6e4da 100644
> --- a/utils/statd/monitor.c
> +++ b/utils/statd/monitor.c
> @@ -352,7 +352,7 @@ sm_unmon_1_svc(struct mon_id *argp, struct svc_req *rqstp)
>  			/* PRC: do the HA callout: */
>  			ha_callout("del-client", mon_name, my_name, -1);
>  
> -			xunlink(SM_DIR, clnt->dns_name, 1);
> +			xunlink(SM_DIR, clnt->dns_name, 0);
>  			nlist_free(&rtnl, clnt);
>  
>  			return (&result);
> @@ -404,7 +404,7 @@ sm_unmon_all_1_svc(struct my_id *argp, struct svc_req *rqstp)
>  			temp = NL_NEXT(clnt);
>  			/* PRC: do the HA callout: */
>  			ha_callout("del-client", mon_name, my_name, -1);
> -			xunlink(SM_DIR, clnt->dns_name, 1);
> +			xunlink(SM_DIR, clnt->dns_name, 0);
>  			nlist_free(&rtnl, clnt);
>  			++count;
>  			clnt = temp;
> --
> 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

Re: [PATCH] statd: not unlinking host files

[Chuck Lever]

On Dec 8, 2008, at Dec 8, 2008, 6:33 PM, J. Bruce Fields wrote:
> On Sat, Dec 06, 2008 at 09:34:25AM -0500, Steve Dickson wrote:
>> Again working with the state file code, I notice that statd was
>> not unlinking hosts files when the kernel sent up the
>> sm_unmon messages. This following patch address the reason why...
>>
>> Comments?
>
> Bizarre--thanks for catching that.
>
> But it looks like these are actually the only two callers to xunlink?
> In which case, we should just ditch the "check" parameter entirely and
> avoid some confusion....

It looks like xunlink() doesn't check error returns properly either.   
alloca() is a convenience, but the price is a SIGSEGV if the stack  
frame can't be extended.

For a system-level daemon like rpc.statd, I would rather see a proper  
implementation of this using malloc(3) or automatic storage, and  
ensuring that sprintf doesn't overrun its buffer.  This also makes it  
possible to track the buffer allocation here using valgrind.  alloca()  
is a completely inline implementation, according to its man page.

I'm not sure why statd has it's own implementation of xstrdup and  
xmalloc here as well; support/nfs/* already has both of these.  It  
would be worth getting rid of these too.

> (I wonder how it ever got this way in the first case?)

Likely that rpc.statd was integrated into nfs-utils a long time ago  
from somewhere else that needed the switch argument.

>
>
> --b.
>
>>
>> steved.
>>
>> commit f8536d0e210a8151900f8c68185927790239eb62
>> Author: Steve Dickson <steved@redhat.com>
>> Date:   Sat Dec 6 09:30:43 2008 -0500
>>
>>    statd: not unlinking host files
>>
>>    Statd is not unlinking host files during SM_UNMON and
>>    SM_UNMON_ALL calls because the given host is still on the run-time
>>    notify list (rtnl) and the check flag is set when xunlink() is
>>    called. But the next thing the caller of xunlink() does is
>>    remove the host from the rtnl list which means the
>>    unlink will never happen.
>>
>>    So in cases where xunlink() is immediately follow by a call
>>    to nlist_free() (which removes the host from the list) the
>>    check flag to xunlink() is not set.
>>
>>    Signed-off-by: Steve Dickson <steved@redhat.com>
>>
>> diff --git a/utils/statd/monitor.c b/utils/statd/monitor.c
>> index a6a1d9c..7d6e4da 100644
>> --- a/utils/statd/monitor.c
>> +++ b/utils/statd/monitor.c
>> @@ -352,7 +352,7 @@ sm_unmon_1_svc(struct mon_id *argp, struct  
>> svc_req *rqstp)
>> 			/* PRC: do the HA callout: */
>> 			ha_callout("del-client", mon_name, my_name, -1);
>>
>> -			xunlink(SM_DIR, clnt->dns_name, 1);
>> +			xunlink(SM_DIR, clnt->dns_name, 0);
>> 			nlist_free(&rtnl, clnt);
>>
>> 			return (&result);
>> @@ -404,7 +404,7 @@ sm_unmon_all_1_svc(struct my_id *argp, struct  
>> svc_req *rqstp)
>> 			temp = NL_NEXT(clnt);
>> 			/* PRC: do the HA callout: */
>> 			ha_callout("del-client", mon_name, my_name, -1);
>> -			xunlink(SM_DIR, clnt->dns_name, 1);
>> +			xunlink(SM_DIR, clnt->dns_name, 0);
>> 			nlist_free(&rtnl, clnt);
>> 			++count;
>> 			clnt = temp;
>> --
>> 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
> --
> 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

Re: [PATCH] statd: not unlinking host files

[Steve Dickson]

Chuck Lever wrote:
> On Dec 8, 2008, at Dec 8, 2008, 6:33 PM, J. Bruce Fields wrote:
>> On Sat, Dec 06, 2008 at 09:34:25AM -0500, Steve Dickson wrote:
>>> Again working with the state file code, I notice that statd was
>>> not unlinking hosts files when the kernel sent up the
>>> sm_unmon messages. This following patch address the reason why...
>>>
>>> Comments?
>>
>> Bizarre--thanks for catching that.
>>
>> But it looks like these are actually the only two callers to xunlink?
>> In which case, we should just ditch the "check" parameter entirely and
>> avoid some confusion....
> 
> It looks like xunlink() doesn't check error returns properly either. 
> alloca() is a convenience, but the price is a SIGSEGV if the stack frame
> can't be extended.
> 
> For a system-level daemon like rpc.statd, I would rather see a proper
> implementation of this using malloc(3) or automatic storage, and
> ensuring that sprintf doesn't overrun its buffer.  This also makes it
> possible to track the buffer allocation here using valgrind.  alloca()
> is a completely inline implementation, according to its man page.
> 
> I'm not sure why statd has it's own implementation of xstrdup and
> xmalloc here as well; support/nfs/* already has both of these.  It would
> be worth getting rid of these too.
Added to the TODO list... 

steved.

Re: [PATCH] statd: not unlinking host files

[Steve Dickson]

J. Bruce Fields wrote:
> On Sat, Dec 06, 2008 at 09:34:25AM -0500, Steve Dickson wrote:
>> Again working with the state file code, I notice that statd was
>> not unlinking hosts files when the kernel sent up the 
>> sm_unmon messages. This following patch address the reason why... 
>>
>> Comments?
> 
> Bizarre--thanks for catching that.
> 
> But it looks like these are actually the only two callers to xunlink?
> In which case, we should just ditch the "check" parameter entirely and
> avoid some confusion....
I thought of that... I wanted to get the patch out and I wanted
to get some context from the list as to why the check was there... 
I'll clean it up...

steved.

Re: [PATCH] statd: not unlinking host files

[Steve Dickson]

This is the patch I committed....

steved.

commit bc870150cc2116584aee288d15ac2b9a2f825ff5
Author: Steve Dickson <steved@redhat.com>
Date:   Wed Dec 17 16:41:35 2008 -0500

    statd: not unlinking host files
    
    Statd is not unlinking host files during SM_UNMON and
    SM_UNMON_ALL calls because the given host is still on the run-time
    notify list (rtnl) and the check flag is set when xunlink() is
    called. But the next thing the caller of xunlink() does is
    remove the host from the rtnl list which means the
    unlink will never happen.
    
    So this patch removes the check flag from xunlink() since
    its not needed and correctly allocates and frees memory
    used by xunlink().
    
    Signed-off-by: Steve Dickson <steved@redhat.com>

diff --git a/utils/statd/misc.c b/utils/statd/misc.c
index fd201b4..7256291 100644
--- a/utils/statd/misc.c
+++ b/utils/statd/misc.c
@@ -53,23 +53,25 @@ xstrdup (const char *string)
 
 
 /*
- * Call with check=1 to verify that this host is not still on the rtnl
- * before unlinking file.
+ * Unlinking a file.
  */
 void
-xunlink (char *path, char *host, short int check)
+xunlink (char *path, char *host)
 {
-  char *tozap;
+	char *tozap;
 
-  tozap=alloca (strlen(path)+strlen(host)+2);
-  sprintf (tozap, "%s/%s", path, host);
+	tozap = malloc(strlen(path)+strlen(host)+2);
+	if (tozap == NULL) {
+		note(N_ERROR, "xunlink: malloc failed: errno %d (%s)", 
+			errno, strerror(errno));
+		return;
+	}
+	sprintf (tozap, "%s/%s", path, host);
 
-  if (!check || !nlist_gethost(rtnl, host, 0)) {
-    if (unlink (tozap) == -1)
-      note (N_ERROR, "unlink (%s): %s", tozap, strerror (errno));
-    else
-      dprintf (N_DEBUG, "Unlinked %s", tozap);
-  }
-  else
-    dprintf (N_DEBUG, "Not unlinking %s--host still monitored.", tozap);
+	if (unlink (tozap) == -1)
+		note(N_ERROR, "unlink (%s): %s", tozap, strerror (errno));
+	else
+		dprintf (N_DEBUG, "Unlinked %s", tozap);
+
+	free(tozap);
 }
diff --git a/utils/statd/monitor.c b/utils/statd/monitor.c
index a6a1d9c..24c2531 100644
--- a/utils/statd/monitor.c
+++ b/utils/statd/monitor.c
@@ -352,7 +352,7 @@ sm_unmon_1_svc(struct mon_id *argp, struct svc_req *rqstp)
 			/* PRC: do the HA callout: */
 			ha_callout("del-client", mon_name, my_name, -1);
 
-			xunlink(SM_DIR, clnt->dns_name, 1);
+			xunlink(SM_DIR, clnt->dns_name);
 			nlist_free(&rtnl, clnt);
 
 			return (&result);
@@ -404,7 +404,7 @@ sm_unmon_all_1_svc(struct my_id *argp, struct svc_req *rqstp)
 			temp = NL_NEXT(clnt);
 			/* PRC: do the HA callout: */
 			ha_callout("del-client", mon_name, my_name, -1);
-			xunlink(SM_DIR, clnt->dns_name, 1);
+			xunlink(SM_DIR, clnt->dns_name);
 			nlist_free(&rtnl, clnt);
 			++count;
 			clnt = temp;
diff --git a/utils/statd/statd.h b/utils/statd/statd.h
index 5a6e289..88ba208 100644
--- a/utils/statd/statd.h
+++ b/utils/statd/statd.h
@@ -53,7 +53,7 @@ extern int	process_notify_list(void);
 extern int	process_reply(FD_SET_TYPE *);
 extern char *	xstrdup(const char *);
 extern void *	xmalloc(size_t);
-extern void	xunlink (char *, char *, short int);
+extern void	xunlink (char *, char *);
 extern void	load_state(void);
 
 /*