2015-10-28 00:02:15

by Malahal Naineni

[permalink] [raw]
Subject: [PATCH] Close etab file's file descriptor on stat error.

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

Signed-off-by: Malahal Naineni <[email protected]>
---
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



2015-10-28 13:53:32

by J. Bruce Fields

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

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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2015-10-28 16:16:57

by Malahal Naineni

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

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 [[email protected]] 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 <[email protected]>
> > ---
> > 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>


2015-11-04 21:51:20

by Steve Dickson

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



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 <[email protected]>
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;
> }
>