Should exportfs/mountd cope with case-insensitive directory names.

PATCH NeilBrown 3 participants 9 messages Apr 3, 2014 – Apr 4, 2014

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

NeilBrown

author

2014-04-03 05:47 UTC #0 lore raw

Hi,
I came across an interesting issue recently.

Suppose you have a filesystem which supports case-insensitive file names (like VFAT, but in this particular case 'NSS' - Novell Storage Services).

And support you export some subdirectory (or sub-volume) using a non-canonical name.
i.e. you "mkdir /path/export", the put "/path/EXPORT" in /etc/exports and mount server:/path/EXPORT from some client.

This all works until the export cache times out and the kernel asks mountd if "/path/export" is exported. mountd says "no" and suddenly all accesses fail.

I don't think much of case-insensitive file names, but I suspect we should either make this work, it issue a warning as to why it is failing. A simple work around is to export the canonical name and use it when mounting. But if the sysadmin doesn't know they need to, they are unlikely to guess.

I don't think there is any API to test if a name is canonical, or to get the canonical name, so I cannot see any way in advance to see if this problem situation has arisen. So the only option I can think of is to fix it.

The following patch (or something much like it for an older nfs-utils) seems to do the trick.

What do people think? Is this a reasonable thing to do? Is it likely to have negative consequences that I haven't thought of?

Thanks,
NeilBrown

utils/mountd/cache.c +24 -2

index 9a1bb2767ac2..2d91db76b867 100644
@@ -377,6 +377,28 @@ static char *next_mnt(void **v, char *p)
 	return me->mnt_dir;
 }
 
+static int same_path(char *child, char *parent, int len)
+{
+	static char p[PATH_MAX];
+	struct stat sc, sp;
+	if (len <= 0)
+		len = strlen(child);
+	strncpy(p, child, len);
+	p[len] = 0;
+	if (strcmp(p, parent) == 0)
+		return 1;
+
+	if (lstat(p, &sc) != 0)
+		return 0;
+	if (lstat(parent, &sp) != 0)
+		return 0;
+	if (sc.st_dev != sp.st_dev)
+		return 0;
+	if (sc.st_ino != sp.st_ino)
+		return 0;
+	return 1;
+}
+
 static int is_subdirectory(char *child, char *parent)
 {
 	/* Check is child is strictly a subdirectory of

@@ -387,7 +409,7 @@ static int is_subdirectory(char *child, char *parent)
 	if (strcmp(parent, "/") == 0 && child[1] != 0)
 		return 1;
 
-	return (strncmp(child, parent, l) == 0 && child[l] == '/');
+	return (same_path(child, parent, l) && child[l] == '/');
 }
 
 static int path_matches(nfs_export *exp, char *path)

@@ -396,7 +418,7 @@ static int path_matches(nfs_export *exp, char *path)
 	 * exact match, or does the export have CROSSMOUNT, and path
 	 * is a descendant?
 	 */
-	return strcmp(path, exp->m_export.e_path) == 0
+	return same_path(path, exp->m_export.e_path, 0)
 		|| ((exp->m_export.e_flags & NFSEXP_CROSSMOUNT)
 		    && is_subdirectory(path, exp->m_export.e_path));
 }

J. Bruce Fields

2014-04-03 13:09 UTC #1 lore raw

Quoted 26 lines · NeilBrown

Hi,
I came across an interesting issue recently.
Suppose you have a filesystem which supports case-insensitive file names (like VFAT, but in this particular case 'NSS' - Novell Storage Services).
And support you export some subdirectory (or sub-volume) using a non-canonical name.
i.e. you "mkdir /path/export", the put "/path/EXPORT" in /etc/exports and mount server:/path/EXPORT from some client.
This all works until the export cache times out and the kernel asks mountd if "/path/export" is exported. mountd says "no" and suddenly all accesses fail.
I don't think much of case-insensitive file names, but I suspect we should either make this work, it issue a warning as to why it is failing. A simple work around is to export the canonical name and use it when mounting. But if the sysadmin doesn't know they need to, they are unlikely to guess.
I don't think there is any API to test if a name is canonical, or to get the canonical name, so I cannot see any way in advance to see if this problem situation has arisen. So the only option I can think of is to fix it.

Quoted 5 lines

The following patch (or something much like it for an older nfs-utils) seems to do the trick.
What do people think? Is this a reasonable thing to do? Is it likely to have negative consequences that I haven't thought of?

Neat-o.

Are we adding a stat of every export in the export table where there wasn't one before? But those should typically be cached, so maybe there's no problem.

I'm not super-fond of the combining the comparison of two paths and truncating of one into the same function, though I see why it's convenient.

--b.

Quoted 57 lines

Thanks,
NeilBrown

utils/mountd/cache.c +24 -2

index 9a1bb2767ac2..2d91db76b867 100644
@@ -377,6 +377,28 @@ static char *next_mnt(void **v, char *p)
 	return me->mnt_dir;
 }
 
+static int same_path(char *child, char *parent, int len)
+{
+	static char p[PATH_MAX];
+	struct stat sc, sp;
+	if (len <= 0)
+		len = strlen(child);
+	strncpy(p, child, len);
+	p[len] = 0;
+	if (strcmp(p, parent) == 0)
+		return 1;
+
+	if (lstat(p, &sc) != 0)
+		return 0;
+	if (lstat(parent, &sp) != 0)
+		return 0;
+	if (sc.st_dev != sp.st_dev)
+		return 0;
+	if (sc.st_ino != sp.st_ino)
+		return 0;
+	return 1;
+}
+
 static int is_subdirectory(char *child, char *parent)
 {
 	/* Check is child is strictly a subdirectory of

@@ -387,7 +409,7 @@ static int is_subdirectory(char *child, char *parent)
 	if (strcmp(parent, "/") == 0 && child[1] != 0)
 		return 1;
 
-	return (strncmp(child, parent, l) == 0 && child[l] == '/');
+	return (same_path(child, parent, l) && child[l] == '/');
 }
 
 static int path_matches(nfs_export *exp, char *path)

@@ -396,7 +418,7 @@ static int path_matches(nfs_export *exp, char *path)
 	 * exact match, or does the export have CROSSMOUNT, and path
 	 * is a descendant?
 	 */
-	return strcmp(path, exp->m_export.e_path) == 0
+	return same_path(path, exp->m_export.e_path, 0)
 		|| ((exp->m_export.e_flags & NFSEXP_CROSSMOUNT)
 		    && is_subdirectory(path, exp->m_export.e_path));
 }

Boaz Harrosh

2014-04-03 16:02 UTC #2 lore raw

Quoted 59 lines · J. Bruce Fields

Quoted 26 lines · NeilBrown
Hi,
I came across an interesting issue recently.
Suppose you have a filesystem which supports case-insensitive file names (like VFAT, but in this particular case 'NSS' - Novell Storage Services).
And support you export some subdirectory (or sub-volume) using a non-canonical name.
i.e. you "mkdir /path/export", the put "/path/EXPORT" in /etc/exports and mount server:/path/EXPORT from some client.
This all works until the export cache times out and the kernel asks mountd if "/path/export" is exported. mountd says "no" and suddenly all accesses fail.
I don't think much of case-insensitive file names, but I suspect we should either make this work, it issue a warning as to why it is failing. A simple work around is to export the canonical name and use it when mounting. But if the sysadmin doesn't know they need to, they are unlikely to guess.
I don't think there is any API to test if a name is canonical, or to get the canonical name, so I cannot see any way in advance to see if this problem situation has arisen. So the only option I can think of is to fix it.
Quoted 5 lines
The following patch (or something much like it for an older nfs-utils) seems to do the trick.
What do people think? Is this a reasonable thing to do? Is it likely to have negative consequences that I haven't thought of?
Neat-o.
Are we adding a stat of every export in the export table where there wasn't one before? But those should typically be cached, so maybe there's no problem.
I'm not super-fond of the combining the comparison of two paths and truncating of one into the same function, though I see why it's convenient.
--b.
Quoted 14 lines
Thanks,
NeilBrown
utils/mountd/cache.c +0 -0
index 9a1bb2767ac2..2d91db76b867 100644
@@ -377,6 +377,28 @@ static char *next_mnt(void **v, char *p)
 	return me->mnt_dir;
 }
 

Quoted 11 lines

utils/mountd/cache.c static char *next_mnt(void **v, char *p) · line 390

Quoted 11 lines

utils/mountd/cache.c +11 -0

+static int same_path(char *child, char *parent, int len)
+{
+	static char p[PATH_MAX];
+	struct stat sc, sp;
+	if (len <= 0)
+		len = strlen(child);
+	strncpy(p, child, len);
+	p[len] = 0;
+	if (strcmp(p, parent) == 0)
+		return 1;
+

Addressing Bruce concern, perhaps you would like to also stricmp the names, so to not lstat on all export entries.

+ if (stricmp(p, parent) =! 0)
+ return 0;

Quoted 8 lines

utils/mountd/cache.c static char *next_mnt(void **v, char *p) · line 398
Quoted 8 lines
utils/mountd/cache.c +8 -0
+	if (lstat(p, &sc) != 0)
+		return 0;
+	if (lstat(parent, &sp) != 0)
+		return 0;
+	if (sc.st_dev != sp.st_dev)
+		return 0;
+	if (sc.st_ino != sp.st_ino)
+		return 0;

Hard links on directories any one ? ;-)

Thanks
Boaz

Quoted 31 lines

Quoted 16 lines

utils/mountd/cache.c +4 -1

+	return 1;
+}
+
 static int is_subdirectory(char *child, char *parent)
 {
 	/* Check is child is strictly a subdirectory of

@@ -387,7 +409,7 @@ static int is_subdirectory(char *child, char *parent)
 	if (strcmp(parent, "/") == 0 && child[1] != 0)
 		return 1;
 
-	return (strncmp(child, parent, l) == 0 && child[l] == '/');
+	return (same_path(child, parent, l) && child[l] == '/');
 }
 
 static int path_matches(nfs_export *exp, char *path)

utils/mountd/cache.c static int path_matches(nfs_export *exp, char *path) · line 424

Quoted 8 lines

utils/mountd/cache.c +1 -1

@@ -396,7 +418,7 @@ static int path_matches(nfs_export *exp, char *path)
 	 * exact match, or does the export have CROSSMOUNT, and path
 	 * is a descendant?
 	 */
-	return strcmp(path, exp->m_export.e_path) == 0
+	return same_path(path, exp->m_export.e_path, 0)
 		|| ((exp->m_export.e_flags & NFSEXP_CROSSMOUNT)
 		    && is_subdirectory(path, exp->m_export.e_path));
 }

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

J. Bruce Fields

2014-04-03 17:16 UTC #3 lore raw

Quoted 10 lines · Boaz Harrosh

Quoted 9 lines · J. Bruce Fields
Quoted 8 lines · NeilBrown
utils/mountd/cache.c +0 -0
index 9a1bb2767ac2..2d91db76b867 100644
@@ -377,6 +377,28 @@ static char *next_mnt(void **v, char *p)
 	return me->mnt_dir;
 }
 

Quoted 15 lines

Quoted 11 lines
utils/mountd/cache.c static char *next_mnt(void **v, char *p) · line 390
Quoted 11 lines
utils/mountd/cache.c +11 -0
+static int same_path(char *child, char *parent, int len)
+{
+	static char p[PATH_MAX];
+	struct stat sc, sp;
+	if (len <= 0)
+		len = strlen(child);
+	strncpy(p, child, len);
+	p[len] = 0;
+	if (strcmp(p, parent) == 0)
+		return 1;
+
Addressing Bruce concern, perhaps you would like to also stricmp the names, so to not lstat on all export entries.

If we try to guess the filesystem's case rules we'll go mad.

Quoted 13 lines

(quoted diff) +2 -0
+	if (stricmp(p, parent) =! 0)
+		return 0;
Quoted 9 lines
utils/mountd/cache.c static char *next_mnt(void **v, char *p) · line 398
Quoted 9 lines
utils/mountd/cache.c +8 -0
+	if (lstat(p, &sc) != 0)
+		return 0;
+	if (lstat(parent, &sp) != 0)
+		return 0;
+	if (sc.st_dev != sp.st_dev)
+		return 0;
+	if (sc.st_ino != sp.st_ino)
+		return 0;
Hard links on directories any one ? ;-)

In the presence of bind mounts it's definitely possible for two different paths to reach the same directory.

I don't know if that's actually a problem here....

--b.

J. Bruce Fields

2014-04-03 17:24 UTC #4 lore raw

Quoted 4 lines · J. Bruce Fields

In the presence of bind mounts it's definitely possible for two different paths to reach the same directory.
I don't know if that's actually a problem here....

Without testing: if we've got the same filesystem mounted at /foo/bar and /foo/baz, but only export /foo/bar, will this change make that export show up at /foo/baz as well?

(Since we'll now consider /foo/bar and /foo/baz to match where previously they didn't?)

--b.

NeilBrown

author

2014-04-03 21:22 UTC #5 lore raw

Quoted 4 lines · On Thu, 3 Apr 2014 13:24:31 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 3 lines · J. Bruce Fields
In the presence of bind mounts it's definitely possible for two different paths to reach the same directory.

Quoted 8 lines

Quoted 1 line
I don't know if that's actually a problem here....
Without testing: if we've got the same filesystem mounted at /foo/bar and /foo/baz, but only export /foo/bar, will this change make that export show up at /foo/baz as well?
(Since we'll now consider /foo/bar and /foo/baz to match where previously they didn't?)

Good question. I should test, but I suspect "yes". I'm not really happy about that.

Maybe we could use name_to_handle_at(). That returns the mnt_id which is different for different bind mounts.
So if the mnt_id and the handle are the same, it is the same directory. If not, then not.

I did worry a bit about all the extra stat calls, but as you say: they are cached so it shouldn't be a big problem.

It would make sense to stat (or name_to_handle_at) the name from the kernel just once, then compare the value against everything in the export table.

We could possible store handles in the export table, but then we would need to check for changes to mountinfo (I think 'poll' can do that) and clear out the cache whenever a mount changes.

I might play with some code if I find time...

Thanks,
NeilBrown

J. Bruce Fields

2014-04-03 21:36 UTC #6 lore raw

Quoted 13 lines · NeilBrown

Quoted 10 lines · On Thu, 3 Apr 2014 13:24:31 -0400 "J. Bruce Fields" <[email protected]>…
Quoted 4 lines · J. Bruce Fields
In the presence of bind mounts it's definitely possible for two different paths to reach the same directory.
I don't know if that's actually a problem here....
Without testing: if we've got the same filesystem mounted at /foo/bar and /foo/baz, but only export /foo/bar, will this change make that export show up at /foo/baz as well?

Quoted 5 lines

Quoted 2 lines
(Since we'll now consider /foo/bar and /foo/baz to match where previously they didn't?)
Good question. I should test, but I suspect "yes". I'm not really happy about that.

Yeah.

Quoted 8 lines

Maybe we could use name_to_handle_at(). That returns the mnt_id which is different for different bind mounts.
So if the mnt_id and the handle are the same, it is the same directory. If not, then not.
I did worry a bit about all the extra stat calls, but as you say: they are cached so it shouldn't be a big problem.

Quoted 8 lines

It would make sense to stat (or name_to_handle_at) the name from the kernel just once, then compare the value against everything in the export table.
We could possible store handles in the export table, but then we would need to check for changes to mountinfo (I think 'poll' can do that) and clear out the cache whenever a mount changes.
I might play with some code if I find time...

Sounds reasonable. But also getting a bit more involved for an uncommon case (path to the root on a case-insensitive filesystem).

--b.

(BTW: I also noticed the other day that systemd is calling name_to_handle_at to get a mount id. Seems like overkill in both cases--shouldn't there be a simpler way to get just the mount id?)

NeilBrown

author

2014-04-03 22:13 UTC #7 lore raw

Quoted 3 lines · On Thu, 3 Apr 2014 17:36:10 -0400 "J. Bruce Fields" <[email protected]>…

(BTW: I also noticed the other day that systemd is calling name_to_handle_at to get a mount id. Seems like overkill in both cases--shouldn't there be a simpler way to get just the mount id?)

Yes, it is called "xstat" and is an extensible version of stat which even includes a 'query' bitmap to list which things you want.

Unfortunately it never made it to mainline. We should submit a patch to systemd to get it to use xstat instead of name_to_handle_at. That would ensure that the kernel implemented it double-quick :-)

NeilBrown

J. Bruce Fields

2014-04-04 14:34 UTC #8 lore raw

Quoted 9 lines · NeilBrown

Quoted 3 lines · On Thu, 3 Apr 2014 17:36:10 -0400 "J. Bruce Fields" <[email protected]>…
(BTW: I also noticed the other day that systemd is calling name_to_handle_at to get a mount id. Seems like overkill in both cases--shouldn't there be a simpler way to get just the mount id?)
Yes, it is called "xstat" and is an extensible version of stat which even includes a 'query' bitmap to list which things you want.

Yes.

I forget that encoding the filehandle is typically just reading inode and generation number and encoding the results in a funny way, so it's not particularly heavy-weight.

But lumping the two together does mean that your request can fail just because the filesystem doesn't support nfs exports.

Quoted 3 lines

Unfortunately it never made it to mainline. We should submit a patch to systemd to get it to use xstat instead of name_to_handle_at. That would ensure that the kernel implemented it double-quick :-)

Hm....

--b.