[PATCH] Close etab file's file descriptor on stat error.

[PATCH] Close etab file's file descriptor on stat error.

[Malahal Naineni]

Also, fixed erroneously closing file descriptor 0 at init time.

Signed-off-by: Malahal Naineni <malahal@us.ibm.com>
---
 utils/mountd/auth.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
index 330cab5..894a7a5 100644
--- a/utils/mountd/auth.c
+++ b/utils/mountd/auth.c
@@ -85,7 +85,7 @@ auth_reload()
 {
 	struct stat		stb;
 	static ino_t		last_inode;
-	static int		last_fd;
+	static int		last_fd = -1;
 	static unsigned int	counter;
 	int			fd;
 
@@ -93,11 +93,22 @@ auth_reload()
 		xlog(L_FATAL, "couldn't open %s", _PATH_ETAB);
 	} else if (fstat(fd, &stb) < 0) {
 		xlog(L_FATAL, "couldn't stat %s", _PATH_ETAB);
-	} else if (stb.st_ino == last_inode) {
+		close(fd);
+	} else if (last_fd != -1 && stb.st_ino == last_inode) {
+		/* We opened the etab file before, and its inode
+		 * number hasn't changed since then.
+		 */
 		close(fd);
 		return counter;
 	} else {
-		close(last_fd);
+		/* Need to process entries from the etab file.  Close
+		 * the file descriptor from the previous open (last_fd),
+		 * and keep the current file descriptor open to prevent
+		 * the file system reusing the current inode number
+		 * (last_inode).
+		 */
+		if (last_fd != -1)
+			close(last_fd);
 		last_fd = fd;
 		last_inode = stb.st_ino;
 	}
-- 
1.8.3.1

Re: [PATCH] Close etab file's file descriptor on stat error.

[J. Bruce Fields]

On Tue, Oct 27, 2015 at 07:01:30PM -0500, Malahal Naineni wrote:
> Also, fixed erroneously closing file descriptor 0 at init time.

Thanks, that's interesting.  Did that extra close(0) have any
user-visible consequences?

> 
> Signed-off-by: Malahal Naineni <malahal@us.ibm.com>
> ---
>  utils/mountd/auth.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
> index 330cab5..894a7a5 100644
> --- a/utils/mountd/auth.c
> +++ b/utils/mountd/auth.c
> @@ -85,7 +85,7 @@ auth_reload()
>  {
>  	struct stat		stb;
>  	static ino_t		last_inode;
> -	static int		last_fd;
> +	static int		last_fd = -1;
>  	static unsigned int	counter;
>  	int			fd;
>  
> @@ -93,11 +93,22 @@ auth_reload()
>  		xlog(L_FATAL, "couldn't open %s", _PATH_ETAB);
>  	} else if (fstat(fd, &stb) < 0) {
>  		xlog(L_FATAL, "couldn't stat %s", _PATH_ETAB);
> -	} else if (stb.st_ino == last_inode) {
> +		close(fd);
> +	} else if (last_fd != -1 && stb.st_ino == last_inode) {

I think that (last_fd != -1) is actually unnecessary (basically because
last_inode is initially 0, which isn't a legal inode number), but...
it's probably clearer this way, OK.

Patch looks OK to me.--b.

> +		/* We opened the etab file before, and its inode
> +		 * number hasn't changed since then.
> +		 */
>  		close(fd);
>  		return counter;
>  	} else {
> -		close(last_fd);
> +		/* Need to process entries from the etab file.  Close
> +		 * the file descriptor from the previous open (last_fd),
> +		 * and keep the current file descriptor open to prevent
> +		 * the file system reusing the current inode number
> +		 * (last_inode).
> +		 */
> +		if (last_fd != -1)
> +			close(last_fd);
>  		last_fd = fd;
>  		last_inode = stb.st_ino;
>  	}
> -- 
> 1.8.3.1
> 
> --
> 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] Close etab file's file descriptor on stat error.

[Malahal Naineni]

I wondered that myself! gdb (of course, running in foreground) showed
0,1,2 as a pty as expected. file descriptor 3 was some socket! Maybe, I
will debug it to find out what socket that was and what issues a user
could get into!

Regards, Malahal.

J. Bruce Fields [bfields@fieldses.org] wrote:
> On Tue, Oct 27, 2015 at 07:01:30PM -0500, Malahal Naineni wrote:
> > Also, fixed erroneously closing file descriptor 0 at init time.
> 
> Thanks, that's interesting.  Did that extra close(0) have any
> user-visible consequences?
> 
> > 
> > Signed-off-by: Malahal Naineni <malahal@us.ibm.com>
> > ---
> >  utils/mountd/auth.c | 17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
> > index 330cab5..894a7a5 100644
> > --- a/utils/mountd/auth.c
> > +++ b/utils/mountd/auth.c
> > @@ -85,7 +85,7 @@ auth_reload()
> >  {
> >  	struct stat		stb;
> >  	static ino_t		last_inode;
> > -	static int		last_fd;
> > +	static int		last_fd = -1;
> >  	static unsigned int	counter;
> >  	int			fd;
> >  
> > @@ -93,11 +93,22 @@ auth_reload()
> >  		xlog(L_FATAL, "couldn't open %s", _PATH_ETAB);
> >  	} else if (fstat(fd, &stb) < 0) {
> >  		xlog(L_FATAL, "couldn't stat %s", _PATH_ETAB);
> > -	} else if (stb.st_ino == last_inode) {
> > +		close(fd);
> > +	} else if (last_fd != -1 && stb.st_ino == last_inode) {
> 
> I think that (last_fd != -1) is actually unnecessary (basically because
> last_inode is initially 0, which isn't a legal inode number), but...
> it's probably clearer this way, OK.
> 
> Patch looks OK to me.--b.
> 
> > +		/* We opened the etab file before, and its inode
> > +		 * number hasn't changed since then.
> > +		 */
> >  		close(fd);
> >  		return counter;
> >  	} else {
> > -		close(last_fd);
> > +		/* Need to process entries from the etab file.  Close
> > +		 * the file descriptor from the previous open (last_fd),
> > +		 * and keep the current file descriptor open to prevent
> > +		 * the file system reusing the current inode number
> > +		 * (last_inode).
> > +		 */
> > +		if (last_fd != -1)
> > +			close(last_fd);
> >  		last_fd = fd;
> >  		last_inode = stb.st_ino;
> >  	}
> > -- 
> > 1.8.3.1
> > 
> > --
> > 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] Close etab file's file descriptor on stat error.

[Steve Dickson]

On 10/27/2015 08:01 PM, Malahal Naineni wrote:
> Also, fixed erroneously closing file descriptor 0 at init time.
> 
> Signed-off-by: Malahal Naineni <malahal@us.ibm.com>
Committed... 

steved.

> ---
>  utils/mountd/auth.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
> index 330cab5..894a7a5 100644
> --- a/utils/mountd/auth.c
> +++ b/utils/mountd/auth.c
> @@ -85,7 +85,7 @@ auth_reload()
>  {
>  	struct stat		stb;
>  	static ino_t		last_inode;
> -	static int		last_fd;
> +	static int		last_fd = -1;
>  	static unsigned int	counter;
>  	int			fd;
>  
> @@ -93,11 +93,22 @@ auth_reload()
>  		xlog(L_FATAL, "couldn't open %s", _PATH_ETAB);
>  	} else if (fstat(fd, &stb) < 0) {
>  		xlog(L_FATAL, "couldn't stat %s", _PATH_ETAB);
> -	} else if (stb.st_ino == last_inode) {
> +		close(fd);
> +	} else if (last_fd != -1 && stb.st_ino == last_inode) {
> +		/* We opened the etab file before, and its inode
> +		 * number hasn't changed since then.
> +		 */
>  		close(fd);
>  		return counter;
>  	} else {
> -		close(last_fd);
> +		/* Need to process entries from the etab file.  Close
> +		 * the file descriptor from the previous open (last_fd),
> +		 * and keep the current file descriptor open to prevent
> +		 * the file system reusing the current inode number
> +		 * (last_inode).
> +		 */
> +		if (last_fd != -1)
> +			close(last_fd);
>  		last_fd = fd;
>  		last_inode = stb.st_ino;
>  	}
>