2022-12-13 16:09:53

by Jeffrey Layton

Subject: [nfs-utils PATCH] Don't allow junction tests to trigger automounts

JianHong reported some strange behavior with automounts on an nfs server
without an explicit pseudoroot. When clients issued a readdir in the
pseudoroot, automounted directories that were not yet mounted would show
up even if they weren't exported, though the clients wouldn't be able to
do anything with them.

The issue was that triggering the automount on a directory would cause
the mountd upcall to time out, which would cause nfsd to include the
automounted dentry in the readdir response. Eventually, the automount
would work and report that it wasn't exported and subsequent attempts to
access the dentry would (properly) fail.

We never want mountd to trigger an automount. The kernel should do that
if it wants to use it. Change the junction checks to do an O_PATH open
and use fstatat with AT_NO_AUTOMOUNT.

Cc: Chuck Lever <[email protected]>
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2148353
Reported-by: JianHong Yin <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
support/junction/junction.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/support/junction/junction.c b/support/junction/junction.c
index 41cce261cb52..0628bb0ffffb 100644
--- a/support/junction/junction.c
+++ b/support/junction/junction.c
@@ -63,7 +63,7 @@ junction_open_path(const char *pathname, int *fd)
if (pathname == NULL || fd == NULL)
return FEDFS_ERR_INVAL;

- tmp = open(pathname, O_DIRECTORY);
+ tmp = open(pathname, O_PATH|O_DIRECTORY);
if (tmp == -1) {
switch (errno) {
case EPERM:
@@ -93,7 +93,7 @@ junction_is_directory(int fd, const char *path)
{
struct stat stb;

- if (fstat(fd, &stb) == -1) {
+ if (fstatat(fd, "", &stb, AT_NO_AUTOMOUNT|AT_EMPTY_PATH) == -1) {
xlog(D_GENERAL, "%s: failed to stat %s: %m",
__func__, path);
return FEDFS_ERR_ACCESS;
@@ -121,7 +121,7 @@ junction_is_sticky_bit_set(int fd, const char *path)
{
struct stat stb;

- if (fstat(fd, &stb) == -1) {
+ if (fstatat(fd, "", &stb, AT_NO_AUTOMOUNT|AT_EMPTY_PATH) == -1) {
xlog(D_GENERAL, "%s: failed to stat %s: %m",
__func__, path);
return FEDFS_ERR_ACCESS;
@@ -155,7 +155,7 @@ junction_set_sticky_bit(int fd, const char *path)
{
struct stat stb;

- if (fstat(fd, &stb) == -1) {
+ if (fstatat(fd, "", &stb, AT_NO_AUTOMOUNT|AT_EMPTY_PATH) == -1) {
xlog(D_GENERAL, "%s: failed to stat %s: %m",
__func__, path);
return FEDFS_ERR_ACCESS;
@@ -393,7 +393,7 @@ junction_get_mode(const char *pathname, mode_t *mode)
if (retval != FEDFS_OK)
return retval;

- if (fstat(fd, &stb) == -1) {
+ if (fstatat(fd, "", &stb, AT_NO_AUTOMOUNT|AT_EMPTY_PATH) == -1) {
xlog(D_GENERAL, "%s: failed to stat %s: %m",
__func__, pathname);
(void)close(fd);
--
2.38.1

2022-12-13 16:32:09

by Chuck Lever

Subject: Re: [nfs-utils PATCH] Don't allow junction tests to trigger automounts

> On Dec 13, 2022, at 11:01 AM, Jeff Layton <[email protected]> wrote:
>
> JianHong reported some strange behavior with automounts on an nfs server
> without an explicit pseudoroot. When clients issued a readdir in the
> pseudoroot, automounted directories that were not yet mounted would show
> up even if they weren't exported, though the clients wouldn't be able to
> do anything with them.
>
> The issue was that triggering the automount on a directory would cause
> the mountd upcall to time out, which would cause nfsd to include the
> automounted dentry in the readdir response. Eventually, the automount
> would work and report that it wasn't exported and subsequent attempts to
> access the dentry would (properly) fail.
>
> We never want mountd to trigger an automount. The kernel should do that
> if it wants to use it. Change the junction checks to do an O_PATH open
> and use fstatat with AT_NO_AUTOMOUNT.
>
> Cc: Chuck Lever <[email protected]>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2148353

And also https://bugzilla.kernel.org/show_bug.cgi?id=216777 ?


> Reported-by: JianHong Yin <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> support/junction/junction.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/support/junction/junction.c b/support/junction/junction.c
> index 41cce261cb52..0628bb0ffffb 100644
> --- a/support/junction/junction.c
> +++ b/support/junction/junction.c
> @@ -63,7 +63,7 @@ junction_open_path(const char *pathname, int *fd)
> if (pathname == NULL || fd == NULL)
> return FEDFS_ERR_INVAL;
>
> - tmp = open(pathname, O_DIRECTORY);
> + tmp = open(pathname, O_PATH|O_DIRECTORY);
> if (tmp == -1) {
> switch (errno) {
> case EPERM:
> @@ -93,7 +93,7 @@ junction_is_directory(int fd, const char *path)
> {
> struct stat stb;
>
> - if (fstat(fd, &stb) == -1) {
> + if (fstatat(fd, "", &stb, AT_NO_AUTOMOUNT|AT_EMPTY_PATH) == -1) {
> xlog(D_GENERAL, "%s: failed to stat %s: %m",
> __func__, path);
> return FEDFS_ERR_ACCESS;
> @@ -121,7 +121,7 @@ junction_is_sticky_bit_set(int fd, const char *path)
> {
> struct stat stb;
>
> - if (fstat(fd, &stb) == -1) {
> + if (fstatat(fd, "", &stb, AT_NO_AUTOMOUNT|AT_EMPTY_PATH) == -1) {
> xlog(D_GENERAL, "%s: failed to stat %s: %m",
> __func__, path);
> return FEDFS_ERR_ACCESS;
> @@ -155,7 +155,7 @@ junction_set_sticky_bit(int fd, const char *path)
> {
> struct stat stb;
>
> - if (fstat(fd, &stb) == -1) {
> + if (fstatat(fd, "", &stb, AT_NO_AUTOMOUNT|AT_EMPTY_PATH) == -1) {
> xlog(D_GENERAL, "%s: failed to stat %s: %m",
> __func__, path);
> return FEDFS_ERR_ACCESS;
> @@ -393,7 +393,7 @@ junction_get_mode(const char *pathname, mode_t *mode)
> if (retval != FEDFS_OK)
> return retval;
>
> - if (fstat(fd, &stb) == -1) {
> + if (fstatat(fd, "", &stb, AT_NO_AUTOMOUNT|AT_EMPTY_PATH) == -1) {
> xlog(D_GENERAL, "%s: failed to stat %s: %m",
> __func__, pathname);
> (void)close(fd);
> --
> 2.38.1
>

--
Chuck Lever

2022-12-13 16:44:29

by Jeffrey Layton

Subject: Re: [nfs-utils PATCH] Don't allow junction tests to trigger automounts

On Tue, 2022-12-13 at 16:25 +0000, Chuck Lever III wrote:
>
> > On Dec 13, 2022, at 11:01 AM, Jeff Layton <[email protected]> wrote:
> >
> > JianHong reported some strange behavior with automounts on an nfs server
> > without an explicit pseudoroot. When clients issued a readdir in the
> > pseudoroot, automounted directories that were not yet mounted would show
> > up even if they weren't exported, though the clients wouldn't be able to
> > do anything with them.
> >
> > The issue was that triggering the automount on a directory would cause
> > the mountd upcall to time out, which would cause nfsd to include the
> > automounted dentry in the readdir response. Eventually, the automount
> > would work and report that it wasn't exported and subsequent attempts to
> > access the dentry would (properly) fail.
> >
> > We never want mountd to trigger an automount. The kernel should do that
> > if it wants to use it. Change the junction checks to do an O_PATH open
> > and use fstatat with AT_NO_AUTOMOUNT.
> >
> > Cc: Chuck Lever <[email protected]>
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2148353
>
> And also https://bugzilla.kernel.org/show_bug.cgi?id=216777 ?
>

Yes indeed, thanks! Note too that I suspect there may also be a kernel
error handling bug related to this.

I know that nfsd_cross_mnt returned -ETIMEDOUT, but the READDIR response
still contained the dentry. The follow-on stat call failed, but it seems
like the readdir response shouldn't have included that dentry in the
first place.

I'm still looking at that bit, but we should probably aim to fix that
too.

>
> > Reported-by: JianHong Yin <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > support/junction/junction.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/support/junction/junction.c b/support/junction/junction.c
> > index 41cce261cb52..0628bb0ffffb 100644
> > --- a/support/junction/junction.c
> > +++ b/support/junction/junction.c
> > @@ -63,7 +63,7 @@ junction_open_path(const char *pathname, int *fd)
> > if (pathname == NULL || fd == NULL)
> > return FEDFS_ERR_INVAL;
> >
> > - tmp = open(pathname, O_DIRECTORY);
> > + tmp = open(pathname, O_PATH|O_DIRECTORY);
> > if (tmp == -1) {
> > switch (errno) {
> > case EPERM:
> > @@ -93,7 +93,7 @@ junction_is_directory(int fd, const char *path)
> > {
> > struct stat stb;
> >
> > - if (fstat(fd, &stb) == -1) {
> > + if (fstatat(fd, "", &stb, AT_NO_AUTOMOUNT|AT_EMPTY_PATH) == -1) {
> > xlog(D_GENERAL, "%s: failed to stat %s: %m",
> > __func__, path);
> > return FEDFS_ERR_ACCESS;
> > @@ -121,7 +121,7 @@ junction_is_sticky_bit_set(int fd, const char *path)
> > {
> > struct stat stb;
> >
> > - if (fstat(fd, &stb) == -1) {
> > + if (fstatat(fd, "", &stb, AT_NO_AUTOMOUNT|AT_EMPTY_PATH) == -1) {
> > xlog(D_GENERAL, "%s: failed to stat %s: %m",
> > __func__, path);
> > return FEDFS_ERR_ACCESS;
> > @@ -155,7 +155,7 @@ junction_set_sticky_bit(int fd, const char *path)
> > {
> > struct stat stb;
> >
> > - if (fstat(fd, &stb) == -1) {
> > + if (fstatat(fd, "", &stb, AT_NO_AUTOMOUNT|AT_EMPTY_PATH) == -1) {
> > xlog(D_GENERAL, "%s: failed to stat %s: %m",
> > __func__, path);
> > return FEDFS_ERR_ACCESS;
> > @@ -393,7 +393,7 @@ junction_get_mode(const char *pathname, mode_t *mode)
> > if (retval != FEDFS_OK)
> > return retval;
> >
> > - if (fstat(fd, &stb) == -1) {
> > + if (fstatat(fd, "", &stb, AT_NO_AUTOMOUNT|AT_EMPTY_PATH) == -1) {
> > xlog(D_GENERAL, "%s: failed to stat %s: %m",
> > __func__, pathname);
> > (void)close(fd);
> > --
> > 2.38.1
> >
>
> --
> Chuck Lever
>
>
>

--
Jeff Layton <[email protected]>

2023-01-04 12:25:55

by Jeffrey Layton

Subject: Re: [nfs-utils PATCH] Don't allow junction tests to trigger automounts

On Tue, 2022-12-13 at 11:01 -0500, Jeff Layton wrote:
> JianHong reported some strange behavior with automounts on an nfs server
> without an explicit pseudoroot. When clients issued a readdir in the
> pseudoroot, automounted directories that were not yet mounted would show
> up even if they weren't exported, though the clients wouldn't be able to
> do anything with them.
>
> The issue was that triggering the automount on a directory would cause
> the mountd upcall to time out, which would cause nfsd to include the
> automounted dentry in the readdir response. Eventually, the automount
> would work and report that it wasn't exported and subsequent attempts to
> access the dentry would (properly) fail.
>
> We never want mountd to trigger an automount. The kernel should do that
> if it wants to use it. Change the junction checks to do an O_PATH open
> and use fstatat with AT_NO_AUTOMOUNT.
>
> Cc: Chuck Lever <[email protected]>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2148353
> Reported-by: JianHong Yin <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> support/junction/junction.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>

Ping? Steve, can you comment on (or merge) this patch?

Thanks,
--
Jeff Layton <[email protected]>

2023-01-11 15:57:18

by Steve Dickson

Subject: Re: [nfs-utils PATCH] Don't allow junction tests to trigger automounts

On 12/13/22 11:01 AM, Jeff Layton wrote:
> JianHong reported some strange behavior with automounts on an nfs server
> without an explicit pseudoroot. When clients issued a readdir in the
> pseudoroot, automounted directories that were not yet mounted would show
> up even if they weren't exported, though the clients wouldn't be able to
> do anything with them.
>
> The issue was that triggering the automount on a directory would cause
> the mountd upcall to time out, which would cause nfsd to include the
> automounted dentry in the readdir response. Eventually, the automount
> would work and report that it wasn't exported and subsequent attempts to
> access the dentry would (properly) fail.
>
> We never want mountd to trigger an automount. The kernel should do that
> if it wants to use it. Change the junction checks to do an O_PATH open
> and use fstatat with AT_NO_AUTOMOUNT.
>
> Cc: Chuck Lever <[email protected]>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2148353
> Reported-by: JianHong Yin <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
Committed... (tag: nfs-utils-2-6-3-rc6)

steved
> ---
> support/junction/junction.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/support/junction/junction.c b/support/junction/junction.c
> index 41cce261cb52..0628bb0ffffb 100644
> --- a/support/junction/junction.c
> +++ b/support/junction/junction.c
> @@ -63,7 +63,7 @@ junction_open_path(const char *pathname, int *fd)
> if (pathname == NULL || fd == NULL)
> return FEDFS_ERR_INVAL;
>
> - tmp = open(pathname, O_DIRECTORY);
> + tmp = open(pathname, O_PATH|O_DIRECTORY);
> if (tmp == -1) {
> switch (errno) {
> case EPERM:
> @@ -93,7 +93,7 @@ junction_is_directory(int fd, const char *path)
> {
> struct stat stb;
>
> - if (fstat(fd, &stb) == -1) {
> + if (fstatat(fd, "", &stb, AT_NO_AUTOMOUNT|AT_EMPTY_PATH) == -1) {
> xlog(D_GENERAL, "%s: failed to stat %s: %m",
> __func__, path);
> return FEDFS_ERR_ACCESS;
> @@ -121,7 +121,7 @@ junction_is_sticky_bit_set(int fd, const char *path)
> {
> struct stat stb;
>
> - if (fstat(fd, &stb) == -1) {
> + if (fstatat(fd, "", &stb, AT_NO_AUTOMOUNT|AT_EMPTY_PATH) == -1) {
> xlog(D_GENERAL, "%s: failed to stat %s: %m",
> __func__, path);
> return FEDFS_ERR_ACCESS;
> @@ -155,7 +155,7 @@ junction_set_sticky_bit(int fd, const char *path)
> {
> struct stat stb;
>
> - if (fstat(fd, &stb) == -1) {
> + if (fstatat(fd, "", &stb, AT_NO_AUTOMOUNT|AT_EMPTY_PATH) == -1) {
> xlog(D_GENERAL, "%s: failed to stat %s: %m",
> __func__, path);
> return FEDFS_ERR_ACCESS;
> @@ -393,7 +393,7 @@ junction_get_mode(const char *pathname, mode_t *mode)
> if (retval != FEDFS_OK)
> return retval;
>
> - if (fstat(fd, &stb) == -1) {
> + if (fstatat(fd, "", &stb, AT_NO_AUTOMOUNT|AT_EMPTY_PATH) == -1) {
> xlog(D_GENERAL, "%s: failed to stat %s: %m",
> __func__, pathname);
> (void)close(fd);